diff mbox

[RFC,3/3] libvhost-user: quit when no more data received

Message ID 20170712094149.23069-4-jfreimann@redhat.com
State New
Headers show

Commit Message

Jens Freimann July 12, 2017, 9:41 a.m. UTC 
From: Jens Freimann <jfreiman@redhat.com>

When recvmsg() returns a message size of zero and
errno is ENOENT end processing of vhost-user messages.

Without this we run into a vubr_panic() call and get
     PANIC: Error while recvmsg: No such file or directory
     Error while dispatching.

Add a switch "quit" to the vhost user device and set true to stop
processing messages.

Signed-off-by: Jens Freimann <jfreiman@redhat.com>
---
 contrib/libvhost-user/libvhost-user.c | 12 +++++++++++-
 contrib/libvhost-user/libvhost-user.h |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Maxime Coquelin July 12, 2017, 4:05 p.m. UTC | #1 
On 07/12/2017 11:41 AM, Jens Freimann wrote:
> From: Jens Freimann<jfreiman@redhat.com>
> 
> When recvmsg() returns a message size of zero and
> errno is ENOENT end processing of vhost-user messages.
> 
> Without this we run into a vubr_panic() call and get
>       PANIC: Error while recvmsg: No such file or directory
>       Error while dispatching.
> 
> Add a switch "quit" to the vhost user device and set true to stop
> processing messages.
> 
> Signed-off-by: Jens Freimann<jfreiman@redhat.com>
> ---
>   contrib/libvhost-user/libvhost-user.c | 12 +++++++++++-
>   contrib/libvhost-user/libvhost-user.h |  1 +
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index 9efb9da..5538859 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -161,7 +161,10 @@ vu_message_read(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
>           rc = recvmsg(conn_fd, &msg, 0);
>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>   
> -    if (rc <= 0) {
> +    if (rc == 0 && (errno == ENOENT)) {
> +        vmsg->size = 0;
> +        dev->quit = true;
> +    } else if (rc < 0) {
>           vu_panic(dev, "Error while recvmsg: %s", strerror(errno));
>           return false;
>       }
> @@ -755,6 +758,10 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
>       DPRINT("Flags:   0x%x\n", vmsg->flags);
>       DPRINT("Size:    %d\n", vmsg->size);
>   
> +    if (dev->quit) {
> +        return true;
> +    }
> +

You may want to return false here, as true means a reply is requested?

Maxime
Marc-André Lureau July 13, 2017, 4:01 p.m. UTC | #2 
Hi

On Wed, Jul 12, 2017 at 1:47 PM Jens Freimann <jfreimann@redhat.com> wrote:

> From: Jens Freimann <jfreiman@redhat.com>
>
> When recvmsg() returns a message size of zero and
> errno is ENOENT end processing of vhost-user messages.
>
> Without this we run into a vubr_panic() call and get
>      PANIC: Error while recvmsg: No such file or directory
>      Error while dispatching.
>
>
How do you get ENOENT on recvmsg()?

Add a switch "quit" to the vhost user device and set true to stop
> processing messages.
>
> Signed-off-by: Jens Freimann <jfreiman@redhat.com>
> ---
>  contrib/libvhost-user/libvhost-user.c | 12 +++++++++++-
>  contrib/libvhost-user/libvhost-user.h |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c
> b/contrib/libvhost-user/libvhost-user.c
> index 9efb9da..5538859 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -161,7 +161,10 @@ vu_message_read(VuDev *dev, int conn_fd, VhostUserMsg
> *vmsg)
>          rc = recvmsg(conn_fd, &msg, 0);
>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>
> -    if (rc <= 0) {
> +    if (rc == 0 && (errno == ENOENT)) {
> +        vmsg->size = 0;
> +        dev->quit = true;
> +    } else if (rc < 0) {
>          vu_panic(dev, "Error while recvmsg: %s", strerror(errno));
>          return false;
>      }
> @@ -755,6 +758,10 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
>      DPRINT("Flags:   0x%x\n", vmsg->flags);
>      DPRINT("Size:    %d\n", vmsg->size);
>
> +    if (dev->quit) {
> +        return true;
> +    }
>

Make it false, as no reply is expected then


> +
>      if (vmsg->fd_num) {
>          int i;
>          DPRINT("Fds:");
> @@ -822,6 +829,9 @@ vu_dispatch(VuDev *dev)
>      bool success = false;
>
>      if (!vu_message_read(dev, dev->sock, &vmsg)) {
> +        if (vmsg.size == 0) {
> +            success = true;
> +        }
>

There might be better ways to indicate a disconnection than modifying and
checking vmsg.size. Perhaps dev->quit alone is enough?

         goto end;
>      }
>
> diff --git a/contrib/libvhost-user/libvhost-user.h
> b/contrib/libvhost-user/libvhost-user.h
> index 53ef222..c02215a 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -217,6 +217,7 @@ struct VuDev {
>      uint64_t features;
>      uint64_t protocol_features;
>      bool broken;
> +    bool quit;
>

I also think you could re-use broken in this case.


>
>      /* @set_watch: add or update the given fd to the watch set,
>       * call cb when condition is met */
> --
> 2.9.4
>
>
> --
Marc-André Lureau
Jens Freimann July 14, 2017, 10:12 a.m. UTC | #3 
On Thu, Jul 13, 2017 at 04:01:34PM +0000, Marc-André Lureau wrote:
>Hi
>
>On Wed, Jul 12, 2017 at 1:47 PM Jens Freimann <jfreimann@redhat.com> wrote:
>
>> From: Jens Freimann <jfreiman@redhat.com>
>>
>> When recvmsg() returns a message size of zero and
>> errno is ENOENT end processing of vhost-user messages.

The man page says that zero-length messages indicate a orderly
shutdown for stream sockets, but for datagram sockets they are
permitted as normal messages. So technically quitting when receiving  
a zero-length packet is not correct here.

I found I can replace this entire patch with this hunk:

@@ -812,6 +812,8 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
                return vu_get_queue_num_exec(dev, vmsg);
        case VHOST_USER_SET_VRING_ENABLE:
                return vu_set_vring_enable_exec(dev, vmsg);
+       case VHOST_USER_NONE:
+               return true;
        default:
                vmsg_close_fds(vmsg);
                vu_panic(dev, "Unhandled
                        request: %d",
                        vmsg->request);

This works, because vmsg is initialzed to all zeros. When we read a
message of length zero, vmsg->request is 0, which is VHOST_USER_NONE.

Would this be acceptable?

>>
>> Without this we run into a vubr_panic() call and get
>>      PANIC: Error while recvmsg: No such file or directory
>>      Error while dispatching.
>>
>>
>How do you get ENOENT on recvmsg()?

I don't. I looked at errno when the return code of recvmsg() was 0,
but errno is only set in case of an error. So it is has no meaning
regarding recvmsg(). 

>Add a switch "quit" to the vhost user device and set true to stop
>> processing messages.
>>
>> Signed-off-by: Jens Freimann <jfreiman@redhat.com>
>> ---
>>  contrib/libvhost-user/libvhost-user.c | 12 +++++++++++-
>>  contrib/libvhost-user/libvhost-user.h |  1 +
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/libvhost-user/libvhost-user.c
>> b/contrib/libvhost-user/libvhost-user.c
>> index 9efb9da..5538859 100644
>> --- a/contrib/libvhost-user/libvhost-user.c
>> +++ b/contrib/libvhost-user/libvhost-user.c
>> @@ -161,7 +161,10 @@ vu_message_read(VuDev *dev, int conn_fd, VhostUserMsg
>> *vmsg)
>>          rc = recvmsg(conn_fd, &msg, 0);
>>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>
>> -    if (rc <= 0) {
>> +    if (rc == 0 && (errno == ENOENT)) {
>> +        vmsg->size = 0;
>> +        dev->quit = true;
>> +    } else if (rc < 0) {
>>          vu_panic(dev, "Error while recvmsg: %s", strerror(errno));
>>          return false;
>>      }
>> @@ -755,6 +758,10 @@ vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
>>      DPRINT("Flags:   0x%x\n", vmsg->flags);
>>      DPRINT("Size:    %d\n", vmsg->size);
>>
>> +    if (dev->quit) {
>> +        return true;
>> +    }
>>
>
>Make it false, as no reply is expected then

ok
>
>
>> +
>>      if (vmsg->fd_num) {
>>          int i;
>>          DPRINT("Fds:");
>> @@ -822,6 +829,9 @@ vu_dispatch(VuDev *dev)
>>      bool success = false;
>>
>>      if (!vu_message_read(dev, dev->sock, &vmsg)) {
>> +        if (vmsg.size == 0) {
>> +            success = true;
>> +        }
>>
>
>There might be better ways to indicate a disconnection than modifying and
>checking vmsg.size. Perhaps dev->quit alone is enough?

yes, it is enough. I'll change it.
>
>         goto end;
>>      }
>>
>> diff --git a/contrib/libvhost-user/libvhost-user.h
>> b/contrib/libvhost-user/libvhost-user.h
>> index 53ef222..c02215a 100644
>> --- a/contrib/libvhost-user/libvhost-user.h
>> +++ b/contrib/libvhost-user/libvhost-user.h
>> @@ -217,6 +217,7 @@ struct VuDev {
>>      uint64_t features;
>>      uint64_t protocol_features;
>>      bool broken;
>> +    bool quit;
>>
>
>I also think you could re-use broken in this case.

I could use broken, it just felt wrong because in this case the device
is not broken, we just want to quit receiving messages because there is
no more data to be received. But if this is not enough to justify a
new variable, I'm fine with using broken.

Thanks for the review!

regards,
Jens 
>
>>
>>      /* @set_watch: add or update the given fd to the watch set,
>>       * call cb when condition is met */
>> --
>> 2.9.4
>>
>>
>> --
>Marc-André Lureau
diff mbox

Patch

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 9efb9da..5538859 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -161,7 +161,10 @@  vu_message_read(VuDev *dev, int conn_fd, VhostUserMsg *vmsg)
         rc = recvmsg(conn_fd, &msg, 0);
     } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
 
-    if (rc <= 0) {
+    if (rc == 0 && (errno == ENOENT)) {
+        vmsg->size = 0;
+        dev->quit = true;
+    } else if (rc < 0) {
         vu_panic(dev, "Error while recvmsg: %s", strerror(errno));
         return false;
     }
@@ -755,6 +758,10 @@  vu_process_message(VuDev *dev, VhostUserMsg *vmsg)
     DPRINT("Flags:   0x%x\n", vmsg->flags);
     DPRINT("Size:    %d\n", vmsg->size);
 
+    if (dev->quit) {
+        return true;
+    }
+
     if (vmsg->fd_num) {
         int i;
         DPRINT("Fds:");
@@ -822,6 +829,9 @@  vu_dispatch(VuDev *dev)
     bool success = false;
 
     if (!vu_message_read(dev, dev->sock, &vmsg)) {
+        if (vmsg.size == 0) {
+            success = true;
+        }
         goto end;
     }
 
diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
index 53ef222..c02215a 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -217,6 +217,7 @@  struct VuDev {
     uint64_t features;
     uint64_t protocol_features;
     bool broken;
+    bool quit;
 
     /* @set_watch: add or update the given fd to the watch set,
      * call cb when condition is met */