[PATCH v3] dom variable error handled in Xenstore

[PATCH v3] dom variable error handled in Xenstore

[Lasya]

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.  

Signed-off-by: Lasya Venneti <comethalley61@gmail.com>

---
 tools/xenstore/init-xenstore-domain.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/xenstore/init-xenstore-domain.c b/tools/xenstore/init-xenstore-domain.c
index 0d12169..82ddabb 100644
--- a/tools/xenstore/init-xenstore-domain.c
+++ b/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;
+	}
 	rv = xc_dom_kernel_file(dom, argv[1]);
 	if (rv) goto err;
 
-- 
1.9.1

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2413 bytes --]

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".

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.

About the code:

> Signed-off-by: Lasya Venneti <comethalley61@gmail.com>

> 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
-- 
<<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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Lasya Venneti]

[-- Attachment #1.1: Type: text/plain, Size: 3575 bytes --]

On 28 October 2015 at 06:30, Dario Faggioli <dario.faggioli@citrix.com>
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@gmail.com>
>
> > 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.

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.

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

[-- Attachment #1.2: Type: text/html, Size: 5254 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Wei Liu]

On Thu, Oct 29, 2015 at 12:42:18AM +0530, Lasya Venneti wrote:
> On 28 October 2015 at 06:30, Dario Faggioli <dario.faggioli@citrix.com>
> 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@gmail.com>
> >
> > > 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)
> >
> >

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3111 bytes --]

On Thu, 2015-10-29 at 00:42 +0530, Lasya Venneti wrote:
> 
> On 28 October 2015 at 06:30, Dario Faggioli <
> dario.faggioli@citrix.com> 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1325 bytes --]

On Thu, 2015-10-29 at 10:07 +0000, Wei Liu wrote:
> On Thu, Oct 29, 2015 at 12:42:18AM +0530, Lasya Venneti wrote:

> > 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.
> 
Aha, I just wrote an email very similar to this... this is a typical
example of a race condition, someone should invent spinlocks for RL !!
:-D

> 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.
> 
Agreed but, I personally prefer -1, for consistency. :-)

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Lasya Venneti]

[-- Attachment #1.1: Type: text/plain, Size: 1514 bytes --]

On 29 October 2015 at 15:41, Dario Faggioli <dario.faggioli@citrix.com>
wrote:

> On Thu, 2015-10-29 at 10:07 +0000, Wei Liu wrote:
> > On Thu, Oct 29, 2015 at 12:42:18AM +0530, Lasya Venneti wrote:
>
> > > 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.
> >
> Aha, I just wrote an email very similar to this... this is a typical
> example of a race condition, someone should invent spinlocks for RL !!
> :-D
>
> > 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.
> >
> Agreed but, I personally prefer -1, for consistency. :-)
>
> So should I proceed with -1? In that case I don't need to add the header...


> 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 #1.2: Type: text/html, Size: 2362 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1118 bytes --]

On Thu, 2015-10-29 at 20:28 +0530, Lasya Venneti wrote:
> 
> 
> On 29 October 2015 at 15:41, Dario Faggioli <
> dario.faggioli@citrix.com> wrote:
> > On Thu, 2015-10-29 at 10:07 +0000, Wei Liu wrote:

> > > 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.
> > >
> > Agreed but, I personally prefer -1, for consistency. :-)
> > 
> So should I proceed with -1? In that case I don't need to add the
> header...
>  
If you're still up for this (and for the other patch, which would be
great!), yes, go for -1.

Thanks and 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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

On Fri, 2015-10-30 at 15:28 +0100, Dario Faggioli wrote:
> On Thu, 2015-10-29 at 20:28 +0530, Lasya Venneti wrote:
> > 
> > 
> > On 29 October 2015 at 15:41, Dario Faggioli <
> > dario.faggioli@citrix.com> wrote:
> > > On Thu, 2015-10-29 at 10:07 +0000, Wei Liu wrote:
> 
> > > > 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.
> > > > 
> > > Agreed but, I personally prefer -1, for consistency. :-)
> > > 
> > So should I proceed with -1? In that case I don't need to add the
> > header...
> >  
> If you're still up for this (and for the other patch, which would be
> great!), yes, go for -1.

Per my reply to v1 (still catching up on email backlog) yes, -1 is the
correct thing to use here. ENOMEM is actively wrong.

Ian.