Re: [Qemu-devel] [PATCH 11/18] vhost-user: add shutdown support

[Yuanhan Liu]

Hi Marc,

First of all, sorry again for late response!

Last time I tried with your first version, I found few issues related
with reconnect, mainly on the acked_feautres lost. While checking your
new code, I found that you've already solved that, which is great.

So, I tried harder this time, your patches work great, except that I
found few nits.

On Fri, Apr 01, 2016 at 01:16:21PM +0200, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
...
> +Slave message types
> +-------------------
> +
> + * VHOST_USER_SLAVE_SHUTDOWN:
> +      Id: 1
> +      Master payload: N/A
> +      Slave payload: u64
> +
> +      Request the master to shutdown the slave. A 0 reply is for
> +      success, in which case the slave may close all connections
> +      immediately and quit.

Assume we are using ovs + dpdk here, that we could have two
vhost-user connections. While ovs tries to initiate a restart,
it might unregister the two connections one by one. In such
case, two VHOST_USER_SLAVE_SHUTDOWN request will be sent,
and two replies will get. Therefore, I don't think it's a
proper ask here to let the backend implementation to do quit
here.


>  
>      switch (msg.request) {
> +    case VHOST_USER_SLAVE_SHUTDOWN: {
> +        uint64_t success = 1; /* 0 is for success */
> +        if (dev->stop) {
> +            dev->stop(dev);
> +            success = 0;
> +        }
> +        msg.payload.u64 = success;
> +        msg.size = sizeof(msg.payload.u64);
> +        size = send(u->slave_fd, &msg, VHOST_USER_HDR_SIZE + msg.size, 0);
> +        if (size != VHOST_USER_HDR_SIZE + msg.size) {
> +            error_report("Failed to write reply.");
> +        }
> +        break;

You might want to remove the slave_fd from watch list? We
might also need to close slave_fd here, assuming that we
will no longer use it when VHOST_USER_SLAVE_SHUTDOWN is
received?

I'm asking because I found a seg fault issue sometimes,
due to opaque is NULL.


        --yliu