davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

Change certain visorbus API functions to return ints instead of booleans #96

Closed ghost closed 7 years ago

ghost commented 7 years ago

KanBoard-1287

ghost commented 7 years ago

Branch: githubissue-96-staging-next-TurningWrenches Commit(s): 33916d53da0d29c5bb1906f368e003619d173eac 82b8c7f15ef62f73ce4c3f54449b314baa294416 7d239a1922c46fa217c7f3c07c77009e9d030cb2 checkpatch-cleared: Yes. T710-1 verified: Yes.

ghost commented 7 years ago

@davidker Ready for inspection.

ghost commented 7 years ago

@davidker pointed out that the three patches are not independently functional. As a result, the first pach was modified to return "!rc" from visorchannel_signalremove() and visorchannel_signalinsert() to compensate for the fact that their internal functions now return integer status codes rather than boolean success flags. The "!rc" enables non-zero integer status codes (i.e. errors) to be treated as 0 (or false), and 0 codes (i.e. success) to be treated as 1 (or true).

What follows is the updated summary: Branch: githubissue-96-staging-next-TurningWrenches Commit(s): e84040135af608d693188a11fb684e8c932ca6b9 514a4c1bb40f715d3e6f5056368e04b2adc4f579 e87e74a7341634c99604c6925423910bc42cdfee checkpatch-cleared: Yes. T710-1 verified: Yes.

ghost commented 7 years ago

Upon further reflection, the usage of "== 0" to check for the success of visorchannel_signalremove() and visorchannel_signalinsert() invocations may not fit best into the conventions of the existing code. This latest series of commits trades the "== 0" syntax in favor of using if(!func()).

What follows is the updated summary: Branch: githubissue-96-staging-next-TurningWrenches Commit(s): e84040135af608d693188a11fb684e8c932ca6b9 9f3ef9546b75f6ea58c3b1bdeb52164a9333a568 d03f81206869814c10d151fe9dc908c086d787be checkpatch-cleared: Yes. T710-1 verified: Yes.

selltc commented 7 years ago

Hmmm... yeah, typically the community Linux guys do NOT like "if (x == 0)" checks, but to me this is totally irrational. They would be more-accepting of "if (x == EOK)", but there is NO common define for "EOK".

Is there any way to change the callers so they check for error instead of success, so as to structure things like:

if (func())
    goto err;

?

davidker commented 7 years ago

Yes, but I think that is going to be part of the patch series that overhauls the error handling. This series simple replaces the bool with errors. Hopefully with that change it will make the overhaul of our error handling easier to tackle.

selltc commented 7 years ago

Fair enough.

davidker commented 7 years ago

I plan on making that explicitly clear in the cover letter when I send the patches up.

davidker commented 7 years ago

Dave, looking at the files, in the first one I see you setting it to !rc, but I don't see you resetting it in 9f3ef95 or d03f812 when you switch the logic of the other two functions, shouldn't you also switch the logic of the signalinsert? What am I missing here.

David

davidker commented 7 years ago

Nevermind, I see the changes in github. Need to figure out why they aren't in my repo, might not have done a proper fetch.

davidker commented 7 years ago

In staging next. 264f7b8ac3ec3e7a38affd8140da30f8720b5946