Re: [Xen-devel] [PATCH v3] dom variable error handled in Xenstore

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Oct 29, 2015 at 12:42:18AM +0530, Lasya Venneti wrote:
> On 28 October 2015 at 06:30, Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> wrote:
> 
> > On Wed, 2015-10-28 at 05:42 +0530, Lasya wrote:
> > > xc_dom_allocate function in build function in init-xenstore-domain.c
> > > returns NULL on failure.
> > > In that case, variable rv is set to ENOMEM and directed to failure
> > > path err.
> > >
> > >
> > So, about the subject line:
> >  - we want tags (as in "tag: does some stuff"), in order to help people
> >    figure out quickly (and/or filter in their mail readers) what
> >    component of Xen the patch is about. In your case, a suitable tag
> >    would be "tools/xenstore:", or even just "xenstore:";
> >  - it should hint at and summarize very very briefly what is being
> >    done. In this case, for instance, something like this:
> >    "xenstore: check for domain allocation errors".
> >
> > Dario, I will trim down the content, and send another in.
> 
> > About the actual changelog:
> >  - wrap lines a bit more, ideally around 70 characters per line. The
> >    point being, it should display well in things like `git log', which
> >    typically indents it a bit;
> >  - it should describe what the patch does, at a higher abstraction
> >    level (e.g., why the patch is necessary, why a particular approach
> >    has been taken, etc.). What you have up here, can pretty much all
> >    be inferred already reading the code. So, for instance, something
> >    like this:
> >    "When building xenstore domain, failure at allocating the memory
> >     for it was not handled. Fix that"
> >  - since you are (I think) fixing an issue identified by Coverity,
> >    you should mention the Coverity ID in the changelog somehow as well.
> >
> I shall mention the coverity ID this time , and will keep this is mind.
> As I didn't have knowledge about the code base, I used the variable
> names. I will definitely explain the bug in conceptual terms this time.
> 
> >
> > About the code:
> >
> > > Signed-off-by: Lasya Venneti <comethalley61@xxxxxxxxx>
> >
> > > diff --git a/tools/xenstore/init-xenstore-domain.c
> > > @@ -42,6 +42,10 @@ static int build(xc_interface *xch, int argc,
> > > char** argv)
> > >       snprintf(cmdline, 512, "--event %d --internal-db", rv);
> > >
> > >       dom = xc_dom_allocate(xch, cmdline, NULL);
> > > +     if (dom == NULL) {
> > > +             rv = ENOMEM;
> > > +             goto err;
> > > +     }
> > >
> > On what basis did you decide that ENOMEM was a good return value?
> >
> > I mean, have you checked what kind of value / error code is being
> > returned in the other cases (e.g., , xc_domain_setmaxmem(),
> > xc_domain_max_vcpus(), etc), if something goes wrong?
> >
> > Thanks and Regards,
> > Dario
> >
> Wei had advised me to raise ENOMEM (Out of memory). However,
> on your advice I checked the xc_domain_setmaxmem()  and
> xc_domain_max_vcpus().
> On failure:
> ->xc_domain_setmaxmem returns a negative value.
> function libxl__build_pre in xen/tools/libxl/libxl_dom.c returns
> ERROR_FAIL when xc_domain_setmaxmem fails.
> 
> ->xc_domain_max_vcpus returns a non zero value.
> It returns the same error value for libxl_build_pre function as
> above.
> 
> I must also add errno.h header to the file, I forgot to do that. I shall
> do so in the next version.
> 

Other xc functions that issue hypercall (that is, you can trace the
call chain to do_xen_hypercall) end up calling ioctl in both Linux and
NetBSD, and they have the same behaviour to return -1 on error and set
errno to appropriate *Xen* errno.

As for xc_dom_allocate, the only failure path at the moment is malloc
failure, which would be appropriate to use ENOMEM to represent.

However if it causes too many faffs, you can just set rv to -1 and
return to caller. I think the main point is to handle the error, either
-1 or ENOMEM is fine by me.

> Request your comments, should I go with ENOMEM as we are finding
> (I think) 'amount' of dom memory allocation, and on failure it returns
> NULL, or with the generic ERROR_FAIL.
> 

Don't return ERROR_FAIL in build() under any circumstance -- it's the
library's error number which is somewhat meaningless to build()'s
caller.

Wei.

> Regards
> Lasya V
> 
> 
> --
> > <<This happens because I choose it to happen!>> (Raistlin Majere)
> > -----------------------------------------------------------------
> > Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> >
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel