On Sun, Jun 29, 2014 at 07:59:38PM -0400, Luke Shumaker wrote: > At Sun, 29 Jun 2014 12:31:13 +0100, > Djalal Harouni wrote: > > On Sat, Jun 28, 2014 at 12:09:56PM -0400, Luke Shumaker wrote: > > > This is accomplished by having wait_for_container() return a positive > > > error > > > code when we would like that error code to make its way to the user. This > > > is at odds with the CODING_STYLE rule that error codes should be returned > > > as negative values. > > This is not odd, we already do that! > > > > When I did convert to wait_for_container(), I remember I checked all > > calls to wait_for_terminate() to see what others do, and there was the: > > src/shared/util.c:wait_for_terminate_and_warn() > > > > Which also returns the positive 'status.si_status' code to caller, and it > > is used in all places... > > I missed that wait_for_terminate_and_warn() also did that! > > With that in consideration, shouldn't the check of the return value > from wait_for_terminate_and_warn() at src/quotacheck/quotacheck.c:110 > be '== 0' instead of '>= 0'? Yes,
I'm not familiar with quotacheck, a quick man-page check didn't show anything about return values, so you are right to ask, I would say this is a bug! especially if execv() fails, then we should return EXIT_FAILURE You could send a patch for it too, thanks in advance! > > So if you are able to send a v2 of this patch, here are some comments: > > I'm putting one together now; thanks for the feedback! > > > > + * Return values: > > > + * < 0 : The container was terminated by a signal, or failed for an > > > + * unknown reason. No change is made to the container argument. > > wait_for_container() failed to get the state of the container, the > > container was terminated by a signal or failed for an unknown reason. > > You mean wait_for_terminate()? No, I mean wait_for_container(), wait_for_terminate() is abstracted here, the doc should reflect the definition of the function, or you can just remove it and say we failed to get the state of container... > > A minor thing: > > > > in wait_for_container() could you add please a log_warning() if > > wait_for_terminate() fails, as it is done in > > wait_for_terminate_and_warn(). > > Yeah, I'll add it as a separate commit. > > > Thank you for the report and the patch! > > Thank you for taking the time to review it! You are welcome! when one of the committers have enough time, he will probably take the patches. Thanks! > Happy hacking, > ~ Luke Shumaker -- Djalal Harouni http://opendz.org _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
