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 73d93eb5ce..8028141534 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2122,9 +2122,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

Quoting Philippe Mathieu-Daudé (2017-03-22 15:48:44)
> 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 73d93eb5ce..8028141534 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2122,9 +2122,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;
>          }

I think this should be:

    if (!dp) {
        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;
    }

otherwise we might set result->error_code while still indicating
success for the operation. That wasn't handled properly before your
patch either, it's just more apparent now.

>          closedir(dp);
> -- 
> 2.11.0
> 

Hi Michael,

On 03/22/2017 06:22 PM, Michael Roth wrote:
> Quoting Philippe Mathieu-Daudé (2017-03-22 15:48:44)
>> 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 73d93eb5ce..8028141534 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2122,9 +2122,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;
>>          }
>
> I think this should be:
>
>     if (!dp) {
>         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;
>     }
>
> otherwise we might set result->error_code while still indicating
> success for the operation. That wasn't handled properly before your
> patch either, it's just more apparent now.

Indeed, I'll resend it in 2 patches It seems easier to me to understand.

>
>>          closedir(dp);
>> --
>> 2.11.0
>>
>
>