Re: [Qemu-devel] [PATCH v2 3/5] libvhost-user: quit when no more data received

[Jens Freimann <jfreimann@redhat.com>]

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