[systemd-devel] [PATCH] nspawn: When exiting with an error, make the error code meaningful.

[Djalal Harouni]

On Mon, Jun 30, 2014 at 01:54:57AM +0100, Djalal Harouni wrote:
> 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...
Ok, I saw you just sent the patches, never mind!

Thanks!

-- 
Djalal Harouni
http://opendz.org