1. Patchew
  2. Libvirt
  3. [libvirt PATCH 0/4] Coverity fixes
  4. View patch

[libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile

Ján Tomko posted 4 patches 4 years, 2 months ago
[libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
Posted by Ján Tomko 4 years, 2 months ago 
Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
Fixes: 43820e4b8037680ec451761216750c6b139db67a
Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 tests/virpcivpdtest.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
index 284350fe29..62c51cdeb9 100644
--- a/tests/virpcivpdtest.c
+++ b/tests/virpcivpdtest.c
@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED)
     buf = g_malloc0(dataLen);
 
     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+    if (fd < 0)
+        return -1;
 
     readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, &csum);
 
@@ -482,6 +484,9 @@ testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED)
 
     dataLen = G_N_ELEMENTS(stringResExample);
     fd = virCreateAnonymousFile(stringResExample, dataLen);
+    if (fd < 0)
+        return -1;
+
     result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum, res);
     VIR_FORCE_CLOSE(fd);
 
@@ -552,6 +557,9 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED)
 
     dataLen = G_N_ELEMENTS(fullVPDExample);
     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+    if (fd < 0)
+        return -1;
+
     res = virPCIVPDParse(fd);
     VIR_FORCE_CLOSE(fd);
 
@@ -620,6 +628,9 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque G_GNUC_UNUSED)
 
     dataLen = G_N_ELEMENTS(fullVPDExample);
     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+    if (fd < 0)
+        return -1;
+
     res = virPCIVPDParse(fd);
     VIR_FORCE_CLOSE(fd);
 
@@ -670,6 +681,9 @@ testVirPCIVPDParseNoRW(const void *opaque G_GNUC_UNUSED)
 
     dataLen = G_N_ELEMENTS(fullVPDExample);
     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+    if (fd < 0)
+        return -1;
+
     res = virPCIVPDParse(fd);
     VIR_FORCE_CLOSE(fd);
 
@@ -723,6 +737,9 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED)
 
     dataLen = G_N_ELEMENTS(fullVPDExample);
     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+    if (fd < 0)
+        return -1;
+
     res = virPCIVPDParse(fd);
     VIR_FORCE_CLOSE(fd);
 
@@ -776,6 +793,9 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED)
 
     dataLen = G_N_ELEMENTS(fullVPDExample);
     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
+    if (fd < 0)
+        return -1;
+
     res = virPCIVPDParse(fd);
     VIR_FORCE_CLOSE(fd);
 
-- 
2.31.1
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
Posted by Kristina Hanicova 4 years, 2 months ago 
On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:

> Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
> Fixes: 43820e4b8037680ec451761216750c6b139db67a
> Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
>  tests/virpcivpdtest.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
> index 284350fe29..62c51cdeb9 100644
> --- a/tests/virpcivpdtest.c
> +++ b/tests/virpcivpdtest.c
> @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
> G_GNUC_UNUSED)
>      buf = g_malloc0(dataLen);
>
>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
> +    if (fd < 0)
> +        return -1;
>

I would prefer if you rewrote this before merging as:

   if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
       return -1;

(The whole patch.) I think it looks cleaner, but that's just my preference.

     readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, &csum);
>
> @@ -482,6 +484,9 @@ testVirPCIVPDParseVPDStringResource(const void *opaque
> G_GNUC_UNUSED)
>
>      dataLen = G_N_ELEMENTS(stringResExample);
>      fd = virCreateAnonymousFile(stringResExample, dataLen);
> +    if (fd < 0)
> +        return -1;
> +
>      result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum,
> res);
>      VIR_FORCE_CLOSE(fd);
>
> @@ -552,6 +557,9 @@ testVirPCIVPDParseFullVPD(const void *opaque
> G_GNUC_UNUSED)
>
>      dataLen = G_N_ELEMENTS(fullVPDExample);
>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
> +    if (fd < 0)
> +        return -1;
> +
>      res = virPCIVPDParse(fd);
>      VIR_FORCE_CLOSE(fd);
>
> @@ -620,6 +628,9 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque
> G_GNUC_UNUSED)
>
>      dataLen = G_N_ELEMENTS(fullVPDExample);
>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
> +    if (fd < 0)
> +        return -1;
> +
>      res = virPCIVPDParse(fd);
>      VIR_FORCE_CLOSE(fd);
>
> @@ -670,6 +681,9 @@ testVirPCIVPDParseNoRW(const void *opaque
> G_GNUC_UNUSED)
>
>      dataLen = G_N_ELEMENTS(fullVPDExample);
>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
> +    if (fd < 0)
> +        return -1;
> +
>      res = virPCIVPDParse(fd);
>      VIR_FORCE_CLOSE(fd);
>
> @@ -723,6 +737,9 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const
> void *opaque G_GNUC_UNUSED)
>
>      dataLen = G_N_ELEMENTS(fullVPDExample);
>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
> +    if (fd < 0)
> +        return -1;
> +
>      res = virPCIVPDParse(fd);
>      VIR_FORCE_CLOSE(fd);
>
> @@ -776,6 +793,9 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void
> *opaque G_GNUC_UNUSED)
>
>      dataLen = G_N_ELEMENTS(fullVPDExample);
>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
> +    if (fd < 0)
> +        return -1;
> +
>      res = virPCIVPDParse(fd);
>      VIR_FORCE_CLOSE(fd);
>
> --
> 2.31.1
>
>

Kristína
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
Posted by Martin Kletzander 4 years, 2 months ago 
On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
>On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
>
>> Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
>> Fixes: 43820e4b8037680ec451761216750c6b139db67a
>> Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
>> Signed-off-by: Ján Tomko <jtomko@redhat.com>
>> ---
>>  tests/virpcivpdtest.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
>> index 284350fe29..62c51cdeb9 100644
>> --- a/tests/virpcivpdtest.c
>> +++ b/tests/virpcivpdtest.c
>> @@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
>> G_GNUC_UNUSED)
>>      buf = g_malloc0(dataLen);
>>
>>      fd = virCreateAnonymousFile(fullVPDExample, dataLen);
>> +    if (fd < 0)
>> +        return -1;
>>
>
>I would prefer if you rewrote this before merging as:
>
>   if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
>       return -1;
>
>(The whole patch.) I think it looks cleaner, but that's just my preference.
>

It might look cleaner for some until you get to the point of having more
parentheses there and we already had some issues with that.  I, for one,
prefer an extra line or even an extra variable in some cases, but
similarly to you it is only my preference and we do not have a coding
style for this, unfortunately.  We even have your preferred style used
in the libvirt coding style page [0].  And guess what, we have that
wrong!  Look:

     while ((rc = waitpid(pid, &st, 0) == -1) &&
            errno == EINTR); // ok
     while ((rc = waitpid(pid, &st, 0) == -1) &&
            errno == EINTR) { // Better
         /* nothing */
     }

I would hope that proves my point, but because we had this discussion a
couple of times already I know there are lot of libvirt developers
(probably in the majority) who prefer cramming as much into that one
line, so I guess we won't achieve a consensus on this one ;)

Just my €.02 ;)

Have a nice day,
Martin
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
Posted by Ján Tomko 4 years, 2 months ago 
On a Tuesday in 2021, Martin Kletzander wrote:
>On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
>>On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
>>
>>>Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
>>>Fixes: 43820e4b8037680ec451761216750c6b139db67a
>>>Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
>>>Signed-off-by: Ján Tomko <jtomko@redhat.com>
>>>---
>>> tests/virpcivpdtest.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>>diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
>>>index 284350fe29..62c51cdeb9 100644
>>>--- a/tests/virpcivpdtest.c
>>>+++ b/tests/virpcivpdtest.c
>>>@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
>>>G_GNUC_UNUSED)
>>>     buf = g_malloc0(dataLen);
>>>
>>>     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
>>>+    if (fd < 0)
>>>+        return -1;
>>>
>>
>>I would prefer if you rewrote this before merging as:
>>
>>  if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
>>      return -1;
>>
>>(The whole patch.) I think it looks cleaner, but that's just my preference.
>>
>
>It might look cleaner for some until you get to the point of having more
>parentheses there and we already had some issues with that.  I, for one,
>prefer an extra line or even an extra variable in some cases, but
>similarly to you it is only my preference and we do not have a coding
>style for this, unfortunately.  We even have your preferred style used
>in the libvirt coding style page [0].  And guess what, we have that

I got a SIGSEGV trying to access [0], so I'll go with the other
suggestion.

>wrong!  Look:
>
>    while ((rc = waitpid(pid, &st, 0) == -1) &&
>           errno == EINTR); // ok
>    while ((rc = waitpid(pid, &st, 0) == -1) &&
>           errno == EINTR) { // Better
>        /* nothing */
>    }
>
>I would hope that proves my point, but because we had this discussion a
>couple of times already I know there are lot of libvirt developers
>(probably in the majority) who prefer cramming as much into that one
>line, so I guess we won't achieve a consensus on this one ;)
>
>Just my €.02 ;)

Thank you for your 0,51 Kč ;)

Jano

>
>Have a nice day,
>Martin
Re: [libvirt PATCH 4/4] tests: pcivpdtest: check return value of virCreateAnonymousFile
Posted by Martin Kletzander 4 years, 2 months ago 
On Tue, Nov 23, 2021 at 04:49:52PM +0100, Ján Tomko wrote:
>On a Tuesday in 2021, Martin Kletzander wrote:
>>On Tue, Nov 23, 2021 at 03:40:46PM +0100, Kristina Hanicova wrote:
>>>On Tue, Nov 23, 2021 at 3:20 PM Ján Tomko <jtomko@redhat.com> wrote:
>>>
>>>>Fixes: 59c1bc3a0e25e6d725db41990f11e0b53137115d
>>>>Fixes: 43820e4b8037680ec451761216750c6b139db67a
>>>>Fixes: 600f580d623ae4077ddeb6c7cb24f8a315a7c73b
>>>>Signed-off-by: Ján Tomko <jtomko@redhat.com>
>>>>---
>>>> tests/virpcivpdtest.c | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>>
>>>>diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c
>>>>index 284350fe29..62c51cdeb9 100644
>>>>--- a/tests/virpcivpdtest.c
>>>>+++ b/tests/virpcivpdtest.c
>>>>@@ -446,6 +446,8 @@ testVirPCIVPDReadVPDBytes(const void *opaque
>>>>G_GNUC_UNUSED)
>>>>     buf = g_malloc0(dataLen);
>>>>
>>>>     fd = virCreateAnonymousFile(fullVPDExample, dataLen);
>>>>+    if (fd < 0)
>>>>+        return -1;
>>>>
>>>
>>>I would prefer if you rewrote this before merging as:
>>>
>>>  if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0)
>>>      return -1;
>>>
>>>(The whole patch.) I think it looks cleaner, but that's just my preference.
>>>
>>
>>It might look cleaner for some until you get to the point of having more
>>parentheses there and we already had some issues with that.  I, for one,
>>prefer an extra line or even an extra variable in some cases, but
>>similarly to you it is only my preference and we do not have a coding
>>style for this, unfortunately.  We even have your preferred style used
>>in the libvirt coding style page [0].  And guess what, we have that
>
>I got a SIGSEGV trying to access [0], so I'll go with the other
>suggestion.
>

lol, it was a lazy reference, only gets resolved when someone wants to
access it, I guess you do not have userfaultd support (although one
might argue that I am the living proof of userfault-duh in this case).

[0] https://libvirt.org/coding-style.html#semicolons

>>wrong!  Look:
>>
>>    while ((rc = waitpid(pid, &st, 0) == -1) &&
>>           errno == EINTR); // ok
>>    while ((rc = waitpid(pid, &st, 0) == -1) &&
>>           errno == EINTR) { // Better
>>        /* nothing */
>>    }
>>
>>I would hope that proves my point, but because we had this discussion a
>>couple of times already I know there are lot of libvirt developers
>>(probably in the majority) who prefer cramming as much into that one
>>line, so I guess we won't achieve a consensus on this one ;)
>>
>>Just my €.02 ;)
>
>Thank you for your 0,51 Kč ;)
>
>Jano
>
>>
>>Have a nice day,
>>Martin
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.