Re: [PATCH v5 2/5] generic vhost user server

[Stefan Hajnoczi]

On Mon, Mar 09, 2020 at 06:03:39PM +0800, Coiby Xu wrote:
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 6b38b67cf1..d207b5f981 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -37,6 +37,9 @@ util-obj-y += readline.o
>  util-obj-y += rcu.o
>  util-obj-$(CONFIG_MEMBARRIER) += sys_membarrier.o
>  util-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
> +ifdef CONFIG_LINUX
> +util-obj-y += vhost-user-server.o
> +endif

Please use the more common form (which is also more concise):

util-obj-$(CONFIG_LINUX) += vhost-user-server.o

> +static bool coroutine_fn
> +vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
> +{
> +    struct iovec iov = {
> +        .iov_base = (char *)vmsg,
> +        .iov_len = VHOST_USER_HDR_SIZE,
> +    };
> +    int rc, read_bytes = 0;
> +    /*
> +     * Store fds/nfds returned from qio_channel_readv_full into
> +     * temporary variables.
> +     *
> +     * VhostUserMsg is a packed structure, gcc will complain about passing
> +     * pointer to a packed structure member if we pass &VhostUserMsg.fd_num
> +     * and &VhostUserMsg.fds directly when calling qio_channel_readv_full,
> +     * thus two temporary variables nfds and fds are used here.
> +     */
> +    size_t nfds = 0, nfds_t = 0;
> +    int *fds = NULL, *fds_t = NULL;
> +    VuClientInfo *client = container_of(vu_dev, VuClientInfo, vu_dev);
> +    QIOChannel *ioc = client->ioc;
> +
> +    Error *local_err = NULL;
> +    assert(qemu_in_coroutine());
> +    do {
> +        /*
> +         * qio_channel_readv_full may have short reads, keeping calling it
> +         * until getting VHOST_USER_HDR_SIZE or 0 bytes in total
> +         */
> +        rc = qio_channel_readv_full(ioc, &iov, 1, &fds_t, &nfds_t, 
> &local_err);
> +        if (rc < 0) {
> +            if (rc == QIO_CHANNEL_ERR_BLOCK) {
> +                qio_channel_yield(ioc, G_IO_IN);
> +                continue;
> +            } else {
> +                error_report_err(local_err);
> +                return false;
> +            }
> +        }
> +        read_bytes += rc;
> +        fds = g_renew(int, fds_t, nfds + nfds_t);
> +        memcpy(fds + nfds, fds_t, nfds_t);
> +        nfds += nfds_t;
> +        if (read_bytes == VHOST_USER_HDR_SIZE || rc == 0) {
> +            break;
> +        }

Does this loop work?  I can't see where iov_base and iov_len are
updated, so I guess the beginning of vmsg would be overwritten instead
of concatenating each readv iteration.

> +    } while (true);
> +    assert(nfds <= VHOST_MEMORY_MAX_NREGIONS);

The client is untrusted.  Please handle errors explicitly and do not
use assertion failures that can be triggered by the client.

> diff --git a/util/vhost-user-server.h b/util/vhost-user-server.h
> new file mode 100644
> index 0000000000..1a6b5e4ea9
> --- /dev/null
> +++ b/util/vhost-user-server.h
> @@ -0,0 +1,57 @@

Missing license header and #ifndef VHOST_USER_SERVER_H.

> +#include "contrib/libvhost-user/libvhost-user.h"
> +#include "io/channel-socket.h"
> +#include "io/channel-file.h"
> +#include "io/net-listener.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "standard-headers/linux/virtio_blk.h"
> +
> +typedef struct VuClientInfo VuClientInfo;
> +
> +typedef struct VuServer {
> +    QIONetListener *listener;
> +    AioContext *ctx;
> +    QTAILQ_HEAD(, VuClientInfo) clients;

The convention is that only one client can be connected to a vhost-user
UNIX domain at any given time.

Multiple devices can be exported by creating multiple vhost-user UNIX
domain sockets.

Therefore it seems unnecessary to have a list of connected clients.
Have I misunderstood something?

> +    void (*device_panic_notifier)(struct VuClientInfo *client) ;
> +    int max_queues;
> +    const VuDevIface *vu_iface;
> +    /*
> +     * @ptr_in_device: VuServer pointer memory location in vhost-user device
> +     * struct, so later container_of can be used to get device destruct
> +     */
> +    void *ptr_in_device;

Is this an opaque pointer that the device implementation will use?  If
so, please call it "opaque" and remove the comment.  "opaque" is the
standard name that is used and there is no need to explain it.

> +    bool close;

This field is unused.  Please drop it and introduce it in the patch that
needs it.

> +} VuServer;
> +
> +typedef struct KickInfo {
> +    VuDev *vu_dev;
> +    int fd; /*kick fd*/
> +    long index; /*queue index*/
> +    vu_watch_cb cb;
> +} KickInfo;
> +
> +struct VuClientInfo {

This could be inlined into VuServer since there should only be one
client connected to a vhost-user UNIX domain socket at any given time.

signature.asc
Description: PGP signature