Message ID | 20170808203900.7661-4-jfreimann@redhat.com |
---|---|
State | New |
Headers | show |
Hi On Tue, Aug 8, 2017 at 10:52 PM Jens Freimann <jfreimann@redhat.com> wrote: > > From: Jens Freimann <jfreiman@redhat.com> > > End processing of messages when VHOST_USER_NONE > is received. > > Without this we run into a vubr_panic() call and get > "PANIC: Unhandled request: 0" > > Signed-off-by: Jens Freimann <jfreiman@redhat.com> > --- > contrib/libvhost-user/libvhost-user.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > index 9efb9dac0e..35fa0c5e56 100644 > --- a/contrib/libvhost-user/libvhost-user.c > +++ b/contrib/libvhost-user/libvhost-user.c > @@ -161,7 +161,7 @@ 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) { > vu_panic(dev, "Error while recvmsg: %s", strerror(errno)); > return false; > } > @@ -806,6 +806,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: > + break; I am afraid this isn't working. vu_message_read() returns true/success, vu_process_message() returns false/no-reply, so vu_dispatch() will return success, and the caller has no clear way to know that the socket got disconnected. For me the vu_panic() was quite more appropriate here. What problem did this patch exactly solve? > > default: > vmsg_close_fds(vmsg); > vu_panic(dev, "Unhandled request: %d", vmsg->request); > -- > 2.13.3 > >
On Tue, Sep 19, 2017 at 04:46:24PM +0000, Marc-Andr?? Lureau wrote: >Hi > >On Tue, Aug 8, 2017 at 10:52 PM Jens Freimann <jfreimann@redhat.com> wrote: >> >> From: Jens Freimann <jfreiman@redhat.com> >> >> End processing of messages when VHOST_USER_NONE >> is received. >> >> Without this we run into a vubr_panic() call and get >> "PANIC: Unhandled request: 0" >> >> Signed-off-by: Jens Freimann <jfreiman@redhat.com> >> --- >> contrib/libvhost-user/libvhost-user.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c >> index 9efb9dac0e..35fa0c5e56 100644 >> --- a/contrib/libvhost-user/libvhost-user.c >> +++ b/contrib/libvhost-user/libvhost-user.c >> @@ -161,7 +161,7 @@ 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) { >> vu_panic(dev, "Error while recvmsg: %s", strerror(errno)); >> return false; >> } >> @@ -806,6 +806,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: >> + break; > > >I am afraid this isn't working. vu_message_read() returns >true/success, vu_process_message() returns false/no-reply, so >vu_dispatch() will return success, and the caller has no clear way to >know that the socket got disconnected. For me the vu_panic() was quite >more appropriate here. > >What problem did this patch exactly solve? The problem was that a VhostUserMsg of size 0 is considered an error. But recvmsg() can return 0. When I ran my pxe testcase using vhost-user-bridge I ran into vu_panic() because of this. This worked because VHOST_USER_NONE is defined as 0. Instead of doing this we could just allow a vmsg size of zero and not tread it as an error? regards, Jens
On Wed, Sep 20, 2017 at 5:09 PM, Jens Freimann <jfreimann@redhat.com> wrote: > On Tue, Sep 19, 2017 at 04:46:24PM +0000, Marc-Andr?? Lureau wrote: >> >> Hi >> >> On Tue, Aug 8, 2017 at 10:52 PM Jens Freimann <jfreimann@redhat.com> >> wrote: >>> >>> >>> From: Jens Freimann <jfreiman@redhat.com> >>> >>> End processing of messages when VHOST_USER_NONE >>> is received. >>> >>> Without this we run into a vubr_panic() call and get >>> "PANIC: Unhandled request: 0" >>> >>> Signed-off-by: Jens Freimann <jfreiman@redhat.com> >>> --- >>> contrib/libvhost-user/libvhost-user.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/contrib/libvhost-user/libvhost-user.c >>> b/contrib/libvhost-user/libvhost-user.c >>> index 9efb9dac0e..35fa0c5e56 100644 >>> --- a/contrib/libvhost-user/libvhost-user.c >>> +++ b/contrib/libvhost-user/libvhost-user.c >>> @@ -161,7 +161,7 @@ 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) { >>> vu_panic(dev, "Error while recvmsg: %s", strerror(errno)); >>> return false; >>> } >>> @@ -806,6 +806,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: >>> + break; >> >> >> >> I am afraid this isn't working. vu_message_read() returns >> true/success, vu_process_message() returns false/no-reply, so >> vu_dispatch() will return success, and the caller has no clear way to >> know that the socket got disconnected. For me the vu_panic() was quite >> more appropriate here. >> >> What problem did this patch exactly solve? > > > The problem was that a VhostUserMsg of size 0 is considered an > error. But recvmsg() can return 0. When I ran my pxe When did recvmsg() return 0? It should only be called after a poll IN/ERR, in case of data it should always return != 0, and if disconnected, it returns 0. > testcase using vhost-user-bridge I ran into vu_panic() because of this. > This worked because VHOST_USER_NONE is defined as 0. Instead of > doing this we could just allow a vmsg size of zero and not tread it > as an error? We want to treat disconnect as a panic condition imho, that the library user is free to implement in different way (abort() clean exit, reconnect etc). Please explain your use case and how you ran into recvmsg() = 0 and what you expect to happen at this point.
On Wed, Sep 20, 2017 at 04:14:33PM +0000, Marc-Andr?? Lureau wrote: >On Wed, Sep 20, 2017 at 5:09 PM, Jens Freimann <jfreimann@redhat.com> wrote: >> On Tue, Sep 19, 2017 at 04:46:24PM +0000, Marc-Andr?? Lureau wrote: >>> >>> Hi >>> >>> On Tue, Aug 8, 2017 at 10:52 PM Jens Freimann <jfreimann@redhat.com> >>> wrote: >>>> >>>> >>>> From: Jens Freimann <jfreiman@redhat.com> >>>> >>>> End processing of messages when VHOST_USER_NONE >>>> is received. >>>> >>>> Without this we run into a vubr_panic() call and get >>>> "PANIC: Unhandled request: 0" >>>> >>>> Signed-off-by: Jens Freimann <jfreiman@redhat.com> >>>> --- >>>> contrib/libvhost-user/libvhost-user.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/contrib/libvhost-user/libvhost-user.c >>>> b/contrib/libvhost-user/libvhost-user.c >>>> index 9efb9dac0e..35fa0c5e56 100644 >>>> --- a/contrib/libvhost-user/libvhost-user.c >>>> +++ b/contrib/libvhost-user/libvhost-user.c >>>> @@ -161,7 +161,7 @@ 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) { >>>> vu_panic(dev, "Error while recvmsg: %s", strerror(errno)); >>>> return false; >>>> } >>>> @@ -806,6 +806,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: >>>> + break; >>> >>> >>> >>> I am afraid this isn't working. vu_message_read() returns >>> true/success, vu_process_message() returns false/no-reply, so >>> vu_dispatch() will return success, and the caller has no clear way to >>> know that the socket got disconnected. For me the vu_panic() was quite >>> more appropriate here. >>> >>> What problem did this patch exactly solve? >> >> >> The problem was that a VhostUserMsg of size 0 is considered an >> error. But recvmsg() can return 0. When I ran my pxe > >When did recvmsg() return 0? It should only be called after a poll >IN/ERR, in case of data it should always return != 0, and if >disconnected, it returns 0. My usecase is that my testcase starts a pxeboot data transfer that goes through vhost-user-bridge. When the transfer is done the socket is disconnected and recvmsg() returns 0. I want vhost-user-bridge to end gracefully, without spitting out an error message. Is that reasonable? > >> testcase using vhost-user-bridge I ran into vu_panic() because of this. >> This worked because VHOST_USER_NONE is defined as 0. Instead of >> doing this we could just allow a vmsg size of zero and not tread it >> as an error? > >We want to treat disconnect as a panic condition imho, that the >library user is free to implement in different way (abort() clean >exit, reconnect etc). Ok, that wasn't obvious to me. Thanks for clarifying! So I was "fixing" the wrong part. On a disconnect vubr_panic() in vhost-user-bridge.c is called and is supposed to do the right thing. Currently it will print an error message "PANIC: Unhandled request: 0" in my use case. I could ignore that or check for exactly this string and suppress the error message. Both seems a bit ugly to me... > >Please explain your use case and how you ran into recvmsg() = 0 and >what you expect to happen at this point. Hope this helps! Looks like we can revert this patch. I'll send a patch to do this. regards, Jens
On Thu, Sep 21, 2017 at 01:31:37PM +0000, Jens Freimann wrote: >On Wed, Sep 20, 2017 at 04:14:33PM +0000, Marc-Andr?? Lureau wrote: >>On Wed, Sep 20, 2017 at 5:09 PM, Jens Freimann <jfreimann@redhat.com> wrote: >>>On Tue, Sep 19, 2017 at 04:46:24PM +0000, Marc-Andr?? Lureau wrote: >>>>On Tue, Aug 8, 2017 at 10:52 PM Jens Freimann <jfreimann@redhat.com> >>>>wrote: >>We want to treat disconnect as a panic condition imho, that the >>library user is free to implement in different way (abort() clean >>exit, reconnect etc). > >Ok, that wasn't obvious to me. Thanks for clarifying! So I was >"fixing" the wrong part. On a disconnect vubr_panic() in >vhost-user-bridge.c is called and is supposed to do the right thing. > >Currently it will print an error message "PANIC: Unhandled request: Actually it is "Error while recvmsg" because in vu_message_read() vu_panic is called if rc from recvmsg is <= 0. Followed by "Error while dispatching" printed by vhost-user-bridge. regards, Jens
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index 9efb9dac0e..35fa0c5e56 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -161,7 +161,7 @@ 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) { vu_panic(dev, "Error while recvmsg: %s", strerror(errno)); return false; } @@ -806,6 +806,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: + break; default: vmsg_close_fds(vmsg); vu_panic(dev, "Unhandled request: %d", vmsg->request);