[PATCH] Fixing coverity bug CID1311511

[PATCH] Fixing coverity bug CID1311511

[Lasya Venneti]

*This is part of my 'bite sized contribution' to Xen for the OutreachY program. 

*The change handles the return value of the function xc_dom_allocate, if the function returns NULL the function returns -1. It would not be useful to jump to err as err would check !dom for NULL. 

*Changes have been made in the build function in init-xenstore-domain.c

*I have taken these discussions for reference:
https://www.choon.net/forum/read.php?22,3805351,3805351

Signed-off: Lasya Venneti <comethalley61@gmail.com>

[PATCH] Handles the error returned by the xc_dom_allocate function

[Lasya Venneti]

---
 tools/xenstore/init-xenstore-domain.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/xenstore/init-xenstore-domain.c b/tools/xenstore/init-xenstore-domain.c
index 0d12169..d17aab5 100644
--- a/tools/xenstore/init-xenstore-domain.c
+++ b/tools/xenstore/init-xenstore-domain.c
@@ -42,6 +42,8 @@ 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)
+		return -1;
 	rv = xc_dom_kernel_file(dom, argv[1]);
 	if (rv) goto err;
 
-- 
1.9.1

Re: [PATCH] Fixing coverity bug CID1311511

[Dario Faggioli]

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

On Sun, 2015-10-25 at 15:32 +0530, Lasya Venneti wrote:
> *This is part of my 'bite sized contribution' to Xen for the
> OutreachY program. 
> 
> *The change handles the return value of the function xc_dom_allocate,
> if the function returns NULL the function returns -1. It would not be
> useful to jump to err as err would check !dom for NULL. 
> 
But then you're not closing xs_fd, is that ok? (I'm asking, because I
am not at all a xenstore expert, but, FWIW, it does not feel right to
me).

> *Changes have been made in the build function in init-xenstore
> -domain.c
> 
> *I have taken these discussions for reference:
> https://www.choon.net/forum/read.php?22,3805351,3805351
> 
> Signed-off: Lasya Venneti <comethalley61@gmail.com>
>
Most of this (except the first bullet point, perhaps), and especially
the Signed-off-by (it's 'Signed-off-by:', not 'Signed-off') tag goes in
the patch changelog.

In fact:
 - this looks like a cover letter for a patch series, but there is 
   only one patch in this case. Usually, when there is only one patch, 
   you don't need a cover letter (there are exceptions, but I don't 
   think this qualifies);
 - cover letters, no matter whether for series or single patches, do 
   not become part of the source tree, when the patch (series) is 
   committed. That is why, information about the patch 
   content/design/etc. and the tags must live in the changelog. If you 
   do like this, someone looking at `git log' wouldn't see it.

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] Fixing coverity bug CID1311511

[Lasya Venneti]

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

Hello,

I just wanted to submit this as one of the bugs, the one which George
assigned to me is still pending(it's a patch series), I will submit that
ASAP. As for this bug, if I have incorrectly handled it, can you please
point out my mistake, I will correct it and re-submit.

Thanks
Lasya V



On 26 October 2015 at 14:39, Dario Faggioli <dario.faggioli@citrix.com>
wrote:

> On Sun, 2015-10-25 at 15:32 +0530, Lasya Venneti wrote:
> > *This is part of my 'bite sized contribution' to Xen for the
> > OutreachY program.
> >
> > *The change handles the return value of the function xc_dom_allocate,
> > if the function returns NULL the function returns -1. It would not be
> > useful to jump to err as err would check !dom for NULL.
> >
> But then you're not closing xs_fd, is that ok? (I'm asking, because I
> am not at all a xenstore expert, but, FWIW, it does not feel right to
> me).
>
> > *Changes have been made in the build function in init-xenstore
> > -domain.c
> >
> > *I have taken these discussions for reference:
> > https://www.choon.net/forum/read.php?22,3805351,3805351
> >
> > Signed-off: Lasya Venneti <comethalley61@gmail.com>
> >
> Most of this (except the first bullet point, perhaps), and especially
> the Signed-off-by (it's 'Signed-off-by:', not 'Signed-off') tag goes in
> the patch changelog.
>
> In fact:
>  - this looks like a cover letter for a patch series, but there is
>    only one patch in this case. Usually, when there is only one patch,
>    you don't need a cover letter (there are exceptions, but I don't
>    think this qualifies);
>  - cover letters, no matter whether for series or single patches, do
>    not become part of the source tree, when the patch (series) is
>    committed. That is why, information about the patch
>    content/design/etc. and the tags must live in the changelog. If you
>    do like this, someone looking at `git log' wouldn't see it.
>
> 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: Type: text/html, Size: 3186 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] Fixing coverity bug CID1311511

[Lasya Venneti]

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

Thank you for pointing out the changelog errors to me, I will definitely
keep those in mind and be careful next time.

Thanks
Lasya V

On 26 October 2015 at 14:48, Lasya Venneti <comethalley61@gmail.com> wrote:

> Hello,
>
> I just wanted to submit this as one of the bugs, the one which George
> assigned to me is still pending(it's a patch series), I will submit that
> ASAP. As for this bug, if I have incorrectly handled it, can you please
> point out my mistake, I will correct it and re-submit.
>
> Thanks
> Lasya V
>
>
>
> On 26 October 2015 at 14:39, Dario Faggioli <dario.faggioli@citrix.com>
> wrote:
>
>> On Sun, 2015-10-25 at 15:32 +0530, Lasya Venneti wrote:
>> > *This is part of my 'bite sized contribution' to Xen for the
>> > OutreachY program.
>> >
>> > *The change handles the return value of the function xc_dom_allocate,
>> > if the function returns NULL the function returns -1. It would not be
>> > useful to jump to err as err would check !dom for NULL.
>> >
>> But then you're not closing xs_fd, is that ok? (I'm asking, because I
>> am not at all a xenstore expert, but, FWIW, it does not feel right to
>> me).
>>
>> > *Changes have been made in the build function in init-xenstore
>> > -domain.c
>> >
>> > *I have taken these discussions for reference:
>> > https://www.choon.net/forum/read.php?22,3805351,3805351
>> >
>> > Signed-off: Lasya Venneti <comethalley61@gmail.com>
>> >
>> Most of this (except the first bullet point, perhaps), and especially
>> the Signed-off-by (it's 'Signed-off-by:', not 'Signed-off') tag goes in
>> the patch changelog.
>>
>> In fact:
>>  - this looks like a cover letter for a patch series, but there is
>>    only one patch in this case. Usually, when there is only one patch,
>>    you don't need a cover letter (there are exceptions, but I don't
>>    think this qualifies);
>>  - cover letters, no matter whether for series or single patches, do
>>    not become part of the source tree, when the patch (series) is
>>    committed. That is why, information about the patch
>>    content/design/etc. and the tags must live in the changelog. If you
>>    do like this, someone looking at `git log' wouldn't see it.
>>
>> 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: Type: text/html, Size: 3765 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] Handles the error returned by the xc_dom_allocate function

[Wei Liu]

Aside from what Dario said.

On Sun, Oct 25, 2015 at 03:32:24PM +0530, Lasya Venneti wrote:
> ---
>  tools/xenstore/init-xenstore-domain.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/xenstore/init-xenstore-domain.c b/tools/xenstore/init-xenstore-domain.c
> index 0d12169..d17aab5 100644
> --- a/tools/xenstore/init-xenstore-domain.c
> +++ b/tools/xenstore/init-xenstore-domain.c
> @@ -42,6 +42,8 @@ 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)

Coding style is wrong. It should be

        if (dom == NULL)

Note the whitespaces.


> +		return -1;

And, please set rv to a proper error code (presumably ENOMEM) and use
goto err, otherwise you're leaking xs_fd.

BTW I notice that xs_fd is leaked in success path. You can submit
another path for it if you feel keen enough.

Wei.

>  	rv = xc_dom_kernel_file(dom, argv[1]);
>  	if (rv) goto err;
>  
> -- 
> 1.9.1

Re: [PATCH] Handles the error returned by the xc_dom_allocate function

[Ian Campbell]

On Mon, 2015-10-26 at 10:40 +0000, Wei Liu wrote:

> > +		return -1;
> 
> And, please set rv to a proper error code (presumably ENOMEM)

Please don't, xc_* functions should return -1 or NULL and set errno on
failure. (libxc error reporting is a bit of a mess, but this is the
intention).

So the caller here can assume that errno has already been set to something
appropriate (probably ENOMEM) and it is fine to translate the NULL into a 
-1 since the overall function returns an int not a pointer.

>  and use goto err, otherwise you're leaking xs_fd.

This is good advise and implies that rv should be set to -1 before the
goto.