Re: [PATCH] 'dom' error handled, by raising ENOMEM if failure occurs. 'xs_fd' leak handled in success path.

[George Dunlap <george.dunlap@citrix.com>]

On 27/10/15 11:04, Lasya Venneti wrote:
> On 27 October 2015 at 16:25, Dario Faggioli <dario.faggioli@citrix.com>
> wrote:
> 
>> On Wed, 2015-10-28 at 03:49 +0530, Lasya wrote:
>>
>> The description of the patch, that you previously put in a separate
>> email, should live here.
>>
> Oh, I had thought no cover letter was needed. Hence, I didn't put in a
> description, I shall add it in v3. :)

Please also read carefully the section titled "Making good patches" in
the Submitting Xen Project Patches wiki page [1].

[1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

 -George

> 
>>
>> Also, this is the second version of a patch. That should be evident
>> from the subject line (something like "[PATCH v2]").
>>
>> And still about the subject line, it's a bit long. It must contain a
>> very quick hint at what the patch is about, then all the details go in
>> the patch description (also called changelog), i.e., right in here. :-)
>>
> The subject line was picked up from the commit message, I shall put a
> concise one this time.
> 
>>
>>> Signed-off-by: Lasya Venneti <comethalley61@gmail.com>
>>> ---
>>>  tools/xenstore/init-xenstore-domain.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/xenstore/init-xenstore-domain.c
>>> b/tools/xenstore/init-xenstore-domain.c
>>> index 0d12169..b413b09 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;
>>> +     }
>>>
>> A lot of spaces are missing...
>>
>>>       rv = xc_dom_kernel_file(dom, argv[1]);
>>           ^ ^
>>           | |
>>           -----------------------------------------------
>>                                                         |
>> Just, for instance, compare what you added with this: ---
>>
>>>       if (rv) goto err;
>>           ^
>>           |
>>           --------
>>                  |
>> And with this: ---
>>
>>> @@ -70,7 +74,8 @@ static int build(xc_interface *xch, int argc,
>>> char** argv)
>>>       if (rv) goto err;
>>>       rv = xc_domain_unpause(xch, domid);
>>>       if (rv) goto err;
>>> -
>>> +
>>>
>>
> Dario, I wasn't look at the individual spaces, just the line breaks. Will
> be careful this time.
> 
>> What happened here?
>>
>>> +     close(xs_fd);
>>>       return 0;
>>>
>> Also, Wei suggested to send two patches, one for fixing the error
>> handling for xc_dom_allocate(), and another one for stopping leaking
>> the fd on the success path.
>>
>> Regards,
>> Dario
>>
> Okay. I shall send them in as two patches.
> 
> Thank you for being so patient with this.
> 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)
>>
>>
>