vhost-user-bridge: fix iov_restore_front() warning

[Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning

Posted by Marc-André Lureau 8 years, 5 months ago

  CC      tests/vhost-user-bridge.o
/home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning: variables 'front' and 'iov' used in loop condition not modified in loop body [-Wfor-loop-analysis]
    for (cur = front; front != iov; cur++) {
                      ^~~~~    ~~~
1 warning generated.

Fix the loop, document the function, and fix some related assert().

In practice, the loop bug was harmless because the front sg buffer is
enough to discard/restore the header size.

Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-bridge.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 8618c20d53..1e5b5ca3da 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
     free(elem);
 }
 
+
+/* this function reverse the effect of iov_discard_front() it must be
+ * called with 'front' being the original struct iovec and 'bytes'
+ * being the number of bytes you shaved off
+ */
 static void
 iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
 {
     struct iovec *cur;
 
-    for (cur = front; front != iov; cur++) {
+    for (cur = front; cur != iov; cur++) {
+        assert(bytes >= cur->iov_len);
         bytes -= cur->iov_len;
     }
 
@@ -302,7 +308,8 @@ vubr_backend_recv_cb(int sock, void *ctx)
             }
             iov_from_buf(sg, elem->in_num, 0, &hdr, sizeof hdr);
             total += hdrlen;
-            assert(iov_discard_front(&sg, &num, hdrlen) == hdrlen);
+            ret = iov_discard_front(&sg, &num, hdrlen);
+            assert(ret == hdrlen);
         }
 
         struct msghdr msg = {
-- 
2.13.0.91.g00982b8dd

Re: [Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning

Posted by Dr. David Alan Gilbert 8 years, 5 months ago

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
>   CC      tests/vhost-user-bridge.o
> /home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning: variables 'front' and 'iov' used in loop condition not modified in loop body [-Wfor-loop-analysis]
>     for (cur = front; front != iov; cur++) {
>                       ^~~~~    ~~~
> 1 warning generated.
> 
> Fix the loop, document the function, and fix some related assert().
> 
> In practice, the loop bug was harmless because the front sg buffer is
> enough to discard/restore the header size.
> 
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tests/vhost-user-bridge.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 8618c20d53..1e5b5ca3da 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
>      free(elem);
>  }
>  
> +
> +/* this function reverse the effect of iov_discard_front() it must be
> + * called with 'front' being the original struct iovec and 'bytes'
> + * being the number of bytes you shaved off
> + */
>  static void
>  iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
>  {
>      struct iovec *cur;
>  
> -    for (cur = front; front != iov; cur++) {
> +    for (cur = front; cur != iov; cur++) {
> +        assert(bytes >= cur->iov_len);
>          bytes -= cur->iov_len;
>      }
>  
> @@ -302,7 +308,8 @@ vubr_backend_recv_cb(int sock, void *ctx)
>              }
>              iov_from_buf(sg, elem->in_num, 0, &hdr, sizeof hdr);
>              total += hdrlen;
> -            assert(iov_discard_front(&sg, &num, hdrlen) == hdrlen);
> +            ret = iov_discard_front(&sg, &num, hdrlen);
> +            assert(ret == hdrlen);
>          }
>  
>          struct msghdr msg = {
> -- 
> 2.13.0.91.g00982b8dd
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning

Posted by Jens Freimann 8 years, 5 months ago

On Fri, Jun 02, 2017 at 12:15:19PM +0400, Marc-André Lureau wrote:
>   CC      tests/vhost-user-bridge.o
> /home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning: variables 'front' and 'iov' used in loop condition not modified in loop body [-Wfor-loop-analysis]
>     for (cur = front; front != iov; cur++) {
>                       ^~~~~    ~~~
> 1 warning generated.
> 
> Fix the loop, document the function, and fix some related assert().
> 
> In practice, the loop bug was harmless because the front sg buffer is
> enough to discard/restore the header size.
> 
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/vhost-user-bridge.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 8618c20d53..1e5b5ca3da 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
>      free(elem);
>  }
>  
> +
> +/* this function reverse the effect of iov_discard_front() it must be
> + * called with 'front' being the original struct iovec and 'bytes'
> + * being the number of bytes you shaved off
> + */
>  static void
>  iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
>  {
>      struct iovec *cur;
>  
> -    for (cur = front; front != iov; cur++) {
> +    for (cur = front; cur != iov; cur++) {
> +        assert(bytes >= cur->iov_len);
>          bytes -= cur->iov_len;

This fixes a problem I saw when trying to pxe-boot
with vhost-user and vhost-user-bridge. It was running in an endless
loop here. Fixed with this patch.  

Tested-by: Jens Freimann <jfreiman@redhat.com>

regards,
Jens

Re: [Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning

Posted by Marc-André Lureau 8 years, 5 months ago

Hi, Michael, could you pick this patch?

thanks

On Fri, Jun 2, 2017 at 12:26 PM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

>   CC      tests/vhost-user-bridge.o
> /home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning:
> variables 'front' and 'iov' used in loop condition not modified in loop
> body [-Wfor-loop-analysis]
>     for (cur = front; front != iov; cur++) {
>                       ^~~~~    ~~~
> 1 warning generated.
>
> Fix the loop, document the function, and fix some related assert().
>
> In practice, the loop bug was harmless because the front sg buffer is
> enough to discard/restore the header size.
>
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/vhost-user-bridge.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 8618c20d53..1e5b5ca3da 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
>      free(elem);
>  }
>
> +
> +/* this function reverse the effect of iov_discard_front() it must be
> + * called with 'front' being the original struct iovec and 'bytes'
> + * being the number of bytes you shaved off
> + */
>  static void
>  iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
>  {
>      struct iovec *cur;
>
> -    for (cur = front; front != iov; cur++) {
> +    for (cur = front; cur != iov; cur++) {
> +        assert(bytes >= cur->iov_len);
>          bytes -= cur->iov_len;
>      }
>
> @@ -302,7 +308,8 @@ vubr_backend_recv_cb(int sock, void *ctx)
>              }
>              iov_from_buf(sg, elem->in_num, 0, &hdr, sizeof hdr);
>              total += hdrlen;
> -            assert(iov_discard_front(&sg, &num, hdrlen) == hdrlen);
> +            ret = iov_discard_front(&sg, &num, hdrlen);
> +            assert(ret == hdrlen);
>          }
>
>          struct msghdr msg = {
> --
> 2.13.0.91.g00982b8dd
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning

Posted by Michael S. Tsirkin 8 years, 5 months ago

On Wed, Jun 07, 2017 at 07:00:46PM +0000, Marc-André Lureau wrote:
> Hi, Michael, could you pick this patch?
> 
> thanks

OK. Pls cc me on patches you want me to review.

> On Fri, Jun 2, 2017 at 12:26 PM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
> 
> >   CC      tests/vhost-user-bridge.o
> > /home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning:
> > variables 'front' and 'iov' used in loop condition not modified in loop
> > body [-Wfor-loop-analysis]
> >     for (cur = front; front != iov; cur++) {
> >                       ^~~~~    ~~~
> > 1 warning generated.
> >
> > Fix the loop, document the function, and fix some related assert().
> >
> > In practice, the loop bug was harmless because the front sg buffer is
> > enough to discard/restore the header size.
> >
> > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/vhost-user-bridge.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> > index 8618c20d53..1e5b5ca3da 100644
> > --- a/tests/vhost-user-bridge.c
> > +++ b/tests/vhost-user-bridge.c
> > @@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
> >      free(elem);
> >  }
> >
> > +
> > +/* this function reverse the effect of iov_discard_front() it must be
> > + * called with 'front' being the original struct iovec and 'bytes'
> > + * being the number of bytes you shaved off
> > + */
> >  static void
> >  iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
> >  {
> >      struct iovec *cur;
> >
> > -    for (cur = front; front != iov; cur++) {
> > +    for (cur = front; cur != iov; cur++) {
> > +        assert(bytes >= cur->iov_len);
> >          bytes -= cur->iov_len;
> >      }
> >
> > @@ -302,7 +308,8 @@ vubr_backend_recv_cb(int sock, void *ctx)
> >              }
> >              iov_from_buf(sg, elem->in_num, 0, &hdr, sizeof hdr);
> >              total += hdrlen;
> > -            assert(iov_discard_front(&sg, &num, hdrlen) == hdrlen);
> > +            ret = iov_discard_front(&sg, &num, hdrlen);
> > +            assert(ret == hdrlen);
> >          }
> >
> >          struct msghdr msg = {
> > --
> > 2.13.0.91.g00982b8dd
> >
> >
> > --
> Marc-André Lureau