[Qemu-devel] [PATCH] 9pfs: fix v9fs_lock error case

[Qemu-devel] [PATCH] 9pfs: fix v9fs_lock error case

[Paolo Bonzini]

In this case, we are marshaling an error status instead of the errno value.
Reorganize the out and out_nofid labels to look like all the other cases.
Coverity reports this because the "err = -ENOENT" and "err = -EINVAL"
assignments above are dead, overwritten by the call to pdu_marshal.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/9pfs/9p.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 99e9472..d028eca 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3045,14 +3045,15 @@ static void coroutine_fn v9fs_lock(void *opaque)
         goto out;
     }
     status = P9_LOCK_SUCCESS;
-out:
-    put_fid(pdu, fidp);
-out_nofid:
     err = pdu_marshal(pdu, offset, "b", status);
-    if (err > 0) {
-        err += offset;
+    if (err < 0) {
+        goto out;
     }
+    err += offset;
     trace_v9fs_lock_return(pdu->tag, pdu->id, status);
+out:
+    put_fid(pdu, fidp);
+out_nofid:
     pdu_complete(pdu, err);
     v9fs_string_free(&flock.client_id);
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH] 9pfs: fix v9fs_lock error case

[Greg Kurz]

On Thu, 26 Jan 2017 11:07:05 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> In this case, we are marshaling an error status instead of the errno value.
> Reorganize the out and out_nofid labels to look like all the other cases.
> Coverity reports this because the "err = -ENOENT" and "err = -EINVAL"
> assignments above are dead, overwritten by the call to pdu_marshal.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

There was a confusion indeed: if the server fails it should report it to the
client with an RERROR message.

Responding an RLOCK message with a P9_LOCK_ERROR status only makes sense
when actually implementing locking (i.e. calling flock() on the backend),
which isn't the case in QEMU as stated in the comment above v9fs_lock(). 
We should hence always report a P9_LOCK_SUCCESS status when responding
an RLOCK message.

Just to make it clear, I've modified your patch to open code this and
pushed it to https://github.com/gkurz/qemu/commits/9p-next .

BTW, I've registered to https://scan.coverity.com/projects/qemu as
Peter suggested on IRC. I'll have a look at the other 9pfs issues.

Cheers.

--
Greg

>  hw/9pfs/9p.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 99e9472..d028eca 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3045,14 +3045,15 @@ static void coroutine_fn v9fs_lock(void *opaque)
>          goto out;
>      }
>      status = P9_LOCK_SUCCESS;
> -out:
> -    put_fid(pdu, fidp);
> -out_nofid:
>      err = pdu_marshal(pdu, offset, "b", status);
> -    if (err > 0) {
> -        err += offset;
> +    if (err < 0) {
> +        goto out;
>      }
> +    err += offset;
>      trace_v9fs_lock_return(pdu->tag, pdu->id, status);
> +out:
> +    put_fid(pdu, fidp);
> +out_nofid:
>      pdu_complete(pdu, err);
>      v9fs_string_free(&flock.client_id);
>  }

Re: [Qemu-devel] [PATCH] 9pfs: fix v9fs_lock error case

[Paolo Bonzini]

On 26/01/2017 12:54, Greg Kurz wrote:
> On Thu, 26 Jan 2017 11:07:05 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> In this case, we are marshaling an error status instead of the errno value.
>> Reorganize the out and out_nofid labels to look like all the other cases.
>> Coverity reports this because the "err = -ENOENT" and "err = -EINVAL"
>> assignments above are dead, overwritten by the call to pdu_marshal.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
> 
> There was a confusion indeed: if the server fails it should report it to the
> client with an RERROR message.
> 
> Responding an RLOCK message with a P9_LOCK_ERROR status only makes sense
> when actually implementing locking (i.e. calling flock() on the backend),
> which isn't the case in QEMU as stated in the comment above v9fs_lock(). 
> We should hence always report a P9_LOCK_SUCCESS status when responding
> an RLOCK message.

Which my patch does in a very roundabout way: the first assignment to
status is now dead, and the pdu_marshal("b") always uses P9_LOCK_SUCCESS.

> Just to make it clear, I've modified your patch to open code this and
> pushed it to https://github.com/gkurz/qemu/commits/9p-next .

Much, better, thanks.

Paolo

> BTW, I've registered to https://scan.coverity.com/projects/qemu as
> Peter suggested on IRC. I'll have a look at the other 9pfs issues.
> 
> Cheers.
> 
> --
> Greg
> 
>>  hw/9pfs/9p.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index 99e9472..d028eca 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -3045,14 +3045,15 @@ static void coroutine_fn v9fs_lock(void *opaque)
>>          goto out;
>>      }
>>      status = P9_LOCK_SUCCESS;
>> -out:
>> -    put_fid(pdu, fidp);
>> -out_nofid:
>>      err = pdu_marshal(pdu, offset, "b", status);
>> -    if (err > 0) {
>> -        err += offset;
>> +    if (err < 0) {
>> +        goto out;
>>      }
>> +    err += offset;
>>      trace_v9fs_lock_return(pdu->tag, pdu->id, status);
>> +out:
>> +    put_fid(pdu, fidp);
>> +out_nofid:
>>      pdu_complete(pdu, err);
>>      v9fs_string_free(&flock.client_id);
>>  }
>