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

[Lasya Venneti <comethalley61@xxxxxxxxx>]

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.

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)

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