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

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

[Dario Faggioli]

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

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.

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

> 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;
> -
> +	
>
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
-- 
<<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] 'dom' error handled, by raising ENOMEM if failure occurs. 'xs_fd' leak handled in success path.

[Lasya Venneti]

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

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

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

[-- Attachment #1.2: Type: text/html, Size: 4496 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] 'dom' error handled, by raising ENOMEM if failure occurs. 'xs_fd' leak handled in success path.

[George Dunlap]

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

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

[Lasya Venneti]

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

On 27 October 2015 at 16:37, George Dunlap <george.dunlap@citrix.com> wrote:

> 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
>
I will definitely go through this thoroughly. Thank you for the resource.

Regards
Lasya V

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

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

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

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

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

[Lasya]

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;
+	}
 	rv = xc_dom_kernel_file(dom, argv[1]);
 	if (rv) goto err;
 
@@ -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;
-
+	
+	close(xs_fd);
 	return 0;
 
 err:
-- 
1.9.1