| Message ID | 20210511213736.281016-8-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: Few cleanups in virtio_send_data_iov() | expand |
On 5/11/21 4:37 PM, Vivek Goyal wrote: > There is no reason to set it in label "err". We should be able to set > it right after sending reply. It is easier to read. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > tools/virtiofsd/fuse_virtio.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > index aa53808ef9..b1767dd5b9 100644 > --- a/tools/virtiofsd/fuse_virtio.c > +++ b/tools/virtiofsd/fuse_virtio.c > @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, > vu_queue_notify(dev, q); > pthread_mutex_unlock(&qi->vq_lock); > vu_dispatch_unlock(qi->virtio_dev); > + req->reply_sent = true; > > err: Just a really minor comment: after all these changes, I would venture that "out" is a more appropriate label name than "err" at this point. > - if (ret == 0) { > - req->reply_sent = true; > - } > - > return ret; > } > >
On Thu, May 13, 2021 at 03:50:13PM -0500, Connor Kuehl wrote: > On 5/11/21 4:37 PM, Vivek Goyal wrote: > > There is no reason to set it in label "err". We should be able to set > > it right after sending reply. It is easier to read. > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > --- > > tools/virtiofsd/fuse_virtio.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > > index aa53808ef9..b1767dd5b9 100644 > > --- a/tools/virtiofsd/fuse_virtio.c > > +++ b/tools/virtiofsd/fuse_virtio.c > > @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, > > vu_queue_notify(dev, q); > > pthread_mutex_unlock(&qi->vq_lock); > > vu_dispatch_unlock(qi->virtio_dev); > > + req->reply_sent = true; > > > > err: > > Just a really minor comment: after all these changes, I would venture > that "out" is a more appropriate label name than "err" at this point. May be. This path is used both by error path as well as success path. Just that value of "ret" changes. I am not particular about it. So I will change this to "out". Thanks Vivek > > > - if (ret == 0) { > > - req->reply_sent = true; > > - } > > - > > return ret; > > } > > > > >
* Vivek Goyal (vgoyal@redhat.com) wrote: > There is no reason to set it in label "err". We should be able to set > it right after sending reply. It is easier to read. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > tools/virtiofsd/fuse_virtio.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > index aa53808ef9..b1767dd5b9 100644 > --- a/tools/virtiofsd/fuse_virtio.c > +++ b/tools/virtiofsd/fuse_virtio.c > @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, > vu_queue_notify(dev, q); > pthread_mutex_unlock(&qi->vq_lock); > vu_dispatch_unlock(qi->virtio_dev); > + req->reply_sent = true; > > err: > - if (ret == 0) { > - req->reply_sent = true; > - } > - > return ret; > } > > -- > 2.25.4 >
* Vivek Goyal (vgoyal@redhat.com) wrote: > On Thu, May 13, 2021 at 03:50:13PM -0500, Connor Kuehl wrote: > > On 5/11/21 4:37 PM, Vivek Goyal wrote: > > > There is no reason to set it in label "err". We should be able to set > > > it right after sending reply. It is easier to read. > > > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > --- > > > tools/virtiofsd/fuse_virtio.c | 5 +---- > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > > > index aa53808ef9..b1767dd5b9 100644 > > > --- a/tools/virtiofsd/fuse_virtio.c > > > +++ b/tools/virtiofsd/fuse_virtio.c > > > @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, > > > vu_queue_notify(dev, q); > > > pthread_mutex_unlock(&qi->vq_lock); > > > vu_dispatch_unlock(qi->virtio_dev); > > > + req->reply_sent = true; > > > > > > err: > > > > Just a really minor comment: after all these changes, I would venture > > that "out" is a more appropriate label name than "err" at this point. > > May be. This path is used both by error path as well as success path. Just > that value of "ret" changes. I am not particular about it. So I will > change this to "out". Well, if it only does 'return ret' we can get rid of the label and just do return's at the places that did the goto's. Dave > Thanks > Vivek > > > > > - if (ret == 0) { > > > - req->reply_sent = true; > > > - } > > > - > > > return ret; > > > } > > > > > > > > > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index aa53808ef9..b1767dd5b9 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -446,12 +446,9 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch, vu_queue_notify(dev, q); pthread_mutex_unlock(&qi->vq_lock); vu_dispatch_unlock(qi->virtio_dev); + req->reply_sent = true; err: - if (ret == 0) { - req->reply_sent = true; - } - return ret; }
There is no reason to set it in label "err". We should be able to set it right after sending reply. It is easier to read. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- tools/virtiofsd/fuse_virtio.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)