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

[Dario Faggioli <dario.faggioli@citrix.com>]

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