| Message ID | 20170126100705.6005-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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); > }
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); >> } >
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); }
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(-)