On 28 October 2015 at 06:30, Dario Faggioli <dario.faggi...@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 <comethalle...@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)
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
