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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2015-10-29 at 00:42 +0530, Lasya Venneti wrote:
> 
> On 28 October 2015 at 06:30, Dario Faggioli <
> dario.faggioli@xxxxxxxxxx> wrote:

> > > 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?
> > 

> Wei had advised me to raise ENOMEM (Out of memory). 
>
Ok, I missed / did not recall that. Wei is more knowledgeable and
authoritative than me in this area of the Xen architecture, so you're
doing the right thing listening to him.

Still, Wei, if you have five minute, I'd like your opinion on the below
reasoning on this subject.

> However,
> on your advice I checked the xc_domain_setmaxmem()  and 
> xc_domain_max_vcpus(). 

> On failure:
> ->xc_domain_setmaxmem returns a negative value. 
>
Exactly.

> function libxl__build_pre in xen/tools/libxl/libxl_dom.c returns 
> ERROR_FAIL when xc_domain_setmaxmem fails. 
> 
Err.. ok, but that is not that relevant here. That is libxl code, we
are somewhere else.

> ->xc_domain_max_vcpus returns a non zero value. 
>
Exactly again.

> It returns the same error value for libxl_build_pre function as
> above. 
> 
And irrelevant again.

> I must also add errno.h header to the file, I forgot to do that. I
> shall 
> do so in the next version.
> 
Mmm.. Why so?

> 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. 
> 
I still fail to see how ERROR_FAIL from libxl may fit in here.

Anyway (and this is what I'd appreciate Wei's opinion on), it seems to
me that this fucntion is returning:
 - 0 on success;
 - -1 on early failure;
 - whatever the various xc_dom* functions return on later failure.

It is my recollection that libxc does (and when it does not, it's a bug
:-/), on error, return -1 and puts the specific error value in the
errno variable, rather than returning an error code directly.

Checking a bunch of the xc_dom* functions that are invoked from here
confirms that.

I think that this new return path being introduced in this patch should
be consistent with the described behavior.

Lasya, does this make things more clear? Wei, Thoughts?

Regards,
Dario
-- 
<<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)

Attachment: signature.asc
Description: This is a digitally signed message part

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