[Qemu-devel] [PATCH v2 0/4] 3 easy-to-fix clang warnings, 1 error code fixed

[Qemu-devel] [PATCH v2 0/4] 3 easy-to-fix clang warnings, 1 error code fixed

[Philippe Mathieu-Daudé]

This patchset fixes three easy-to-fix unrelated clang warnings, so it was
probably better to send them separately the first time.

v2:
    - hw/usb/dev-smartcard-reader.c: The first patch was treating NULL and 0 as
      errors but those values are legal in some cases, i.e. no (virtual)
      smartcard inserted into the card reader (Gerd Hoffmann's review).
    - device_tree.c: Added Marc-André Lureau's Reviewed-by.
    - qga/commands-posix.c: Added a commit to improve failed operation, as
      suggested by Michael Roth.

Philippe Mathieu-Daudé (4):
  usb-ccid: make ccid_write_data_block() cope with null buffers
  device_tree: fix compiler warnings (clang 5)
  qga: fix compiler warnings (clang 5)
  qga: improve error handling in transfer_memory_block

 device_tree.c                 |  1 +
 hw/usb/dev-smartcard-reader.c |  5 ++++-
 qga/commands-posix.c          | 11 ++++++++---
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/4] usb-ccid: make ccid_write_data_block() cope with null buffers

[Philippe Mathieu-Daudé]

static code analyzer complain:

hw/usb/dev-smartcard-reader.c:816:5: warning: Null pointer passed as an argument to a 'nonnull' parameter
    memcpy(p->abData, data, len);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/dev-smartcard-reader.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 757b8b3f5a..49cb1829b5 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -813,7 +813,10 @@ static void ccid_write_data_block(USBCCIDState *s, uint8_t slot, uint8_t seq,
     if (p->b.bError) {
         DPRINTF(s, D_VERBOSE, "error %d\n", p->b.bError);
     }
-    memcpy(p->abData, data, len);
+    if (len) {
+        g_assert_nonnull(data);
+        memcpy(p->abData, data, len);
+    }
     ccid_reset_error_status(s);
     usb_wakeup(s->bulk, 0);
 }
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/4] device_tree: fix compiler warnings (clang 5)

[Philippe Mathieu-Daudé]

static code analyzer complain:

device_tree.c:155:18: warning: Null pointer passed as an argument to a 'nonnull' parameter
    while ((de = readdir(d)) != NULL) {
                 ^~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 device_tree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/device_tree.c b/device_tree.c
index 6e06320830..a24ddff02b 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -148,6 +148,7 @@ static void read_fstree(void *fdt, const char *dirname)
     d = opendir(dirname);
     if (!d) {
         error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
+        return;
     }
 
     while ((de = readdir(d)) != NULL) {
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/4] qga: fix compiler warnings (clang 5)

[Philippe Mathieu-Daudé]

static code analyzer complain:

qga/commands-posix.c:2127:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
        closedir(dp);
        ^~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 915df9ed90..fc45102a1e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2124,9 +2124,11 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
          * we think this VM does not support online/offline memory block,
          * any other solution?
          */
-        if (!dp && errno == ENOENT) {
-            result->response =
-                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+        if (!dp) {
+            if (errno == ENOENT) {
+                result->response =
+                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+            }
             goto out1;
         }
         closedir(dp);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 3/4] qga: fix compiler warnings (clang 5)

[Michael Roth]

Quoting Philippe Mathieu-Daudé (2017-04-07 17:20:15)
> static code analyzer complain:
> 
> qga/commands-posix.c:2127:9: warning: Null pointer passed as an argument to a 'nonnull' parameter
>         closedir(dp);
>         ^~~~~~~~~~~~
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  qga/commands-posix.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 915df9ed90..fc45102a1e 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2124,9 +2124,11 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>           * we think this VM does not support online/offline memory block,
>           * any other solution?
>           */
> -        if (!dp && errno == ENOENT) {
> -            result->response =
> -                GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
> +        if (!dp) {
> +            if (errno == ENOENT) {
> +                result->response =
> +                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
> +            }
>              goto out1;
>          }
>          closedir(dp);
> -- 
> 2.11.0
> 

[Qemu-devel] [PATCH v2 4/4] qga: improve error handling in transfer_memory_block

[Philippe Mathieu-Daudé]

Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

Michael should I use Signed-off-by instead of the Suggested-by (since it is your
code)?

 qga/commands-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index fc45102a1e..ca5a24b2c9 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2128,6 +2128,9 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
             if (errno == ENOENT) {
                 result->response =
                     GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
+            } else {
+                result->response =
+                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
             }
             goto out1;
         }
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 4/4] qga: improve error handling in transfer_memory_block

[Michael Roth]

Quoting Philippe Mathieu-Daudé (2017-04-07 17:20:16)
> Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
> 
> Michael should I use Signed-off-by instead of the Suggested-by (since it is your
> code)?

I suppose it could go either way depending on the circumstances
(assuming you've asked for / obtained their SoB obviously), but
speaking for myself here I don't think it's necessary.

> 
>  qga/commands-posix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index fc45102a1e..ca5a24b2c9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2128,6 +2128,9 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>              if (errno == ENOENT) {
>                  result->response =
>                      GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
> +            } else {
> +                result->response =
> +                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
>              }
>              goto out1;
>          }
> -- 
> 2.11.0
> 

Re: [Qemu-devel] [PATCH v2 4/4] qga: improve error handling in transfer_memory_block

[Philippe Mathieu-Daudé]

On 04/10/2017 10:20 PM, Michael Roth wrote:
> Quoting Philippe Mathieu-Daudé (2017-04-07 17:20:16)
>> Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
>> ---
>>
>> Michael should I use Signed-off-by instead of the Suggested-by (since it is your
>> code)?
>
> I suppose it could go either way depending on the circumstances
> (assuming you've asked for / obtained their SoB obviously), but
> speaking for myself here I don't think it's necessary.
>

Ok! I allowed myself to add your "Suggested-by" after reading Linux's 
process/submitting-patches.rst doc, assuming no need to ask for it since 
you answered in public ML. I'll diligently ask next time!

>>
>>  qga/commands-posix.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index fc45102a1e..ca5a24b2c9 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2128,6 +2128,9 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>>              if (errno == ENOENT) {
>>                  result->response =
>>                      GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
>> +            } else {
>> +                result->response =
>> +                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
>>              }
>>              goto out1;
>>          }
>> --
>> 2.11.0
>>
>

Re: [Qemu-devel] [PATCH v2 4/4] qga: improve error handling in transfer_memory_block

[Philippe Mathieu-Daudé]

ping?

On Mon, Apr 10, 2017 at 10:20 PM, Michael Roth
<mdroth@linux.vnet.ibm.com> wrote:
> Quoting Philippe Mathieu-Daudé (2017-04-07 17:20:16)
>> Suggested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>
>> ---
>>
>> Michael should I use Signed-off-by instead of the Suggested-by (since it is your
>> code)?
>
> I suppose it could go either way depending on the circumstances
> (assuming you've asked for / obtained their SoB obviously), but
> speaking for myself here I don't think it's necessary.
>
>>
>>  qga/commands-posix.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index fc45102a1e..ca5a24b2c9 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2128,6 +2128,9 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>>              if (errno == ENOENT) {
>>                  result->response =
>>                      GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
>> +            } else {
>> +                result->response =
>> +                    GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
>>              }
>>              goto out1;
>>          }
>> --
>> 2.11.0
>>
>

Re: [Qemu-devel] [PATCH v2 0/4] 3 easy-to-fix clang warnings, 1 error code fixed

[Michael Tokarev]

08.04.2017 01:20, Philippe Mathieu-Daudé wrote:
> This patchset fixes three easy-to-fix unrelated clang warnings, so it was
> probably better to send them separately the first time.
> 
> v2:
>     - hw/usb/dev-smartcard-reader.c: The first patch was treating NULL and 0 as
>       errors but those values are legal in some cases, i.e. no (virtual)
>       smartcard inserted into the card reader (Gerd Hoffmann's review).
>     - device_tree.c: Added Marc-André Lureau's Reviewed-by.
>     - qga/commands-posix.c: Added a commit to improve failed operation, as
>       suggested by Michael Roth.
> 
> Philippe Mathieu-Daudé (4):
>   usb-ccid: make ccid_write_data_block() cope with null buffers
>   device_tree: fix compiler warnings (clang 5)
>   qga: fix compiler warnings (clang 5)
>   qga: improve error handling in transfer_memory_block

Applied to -trivial, thanks!

/mjt