[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] vhost-user-blk: introduce a vhost-user-blk
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] vhost-user-blk: introduce a vhost-user-blk sample application |
Date: |
Wed, 26 Jul 2017 12:03:50 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Thu, Jul 27, 2017 at 10:00:51AM +0800, Changpeng Liu wrote:
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c
> b/contrib/vhost-user-blk/vhost-user-blk.c
> new file mode 100644
> index 0000000..00826f5
> --- /dev/null
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -0,0 +1,695 @@
> +/*
> + * vhost-user-blk sample application
> + *
> + * Copyright IBM, Corp. 2007
> + * Copyright (c) 2016 Nutanix Inc. All rights reserved.
> + * Copyright (c) 2017 Intel Corporation. All rights reserved.
> + *
> + * Author:
> + * Anthony Liguori <address@hidden>
> + * Felipe Franciosi <address@hidden>
> + * Changpeng Liu <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 only.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/virtio/virtio-blk.h"
> +#include "contrib/libvhost-user/libvhost-user.h"
> +
> +#include <glib.h>
> +
> +/* Small compat shim from glib 2.32 */
> +#ifndef G_SOURCE_CONTINUE
> +#define G_SOURCE_CONTINUE TRUE
> +#endif
> +#ifndef G_SOURCE_REMOVE
> +#define G_SOURCE_REMOVE FALSE
> +#endif
> +
> +/* 1 IO queue with 128 entries */
> +#define VIRTIO_BLK_QUEUE_NUM 128
This is unused, please drop it.
> +/* And this is the final byte of request*/
> +#define VIRTIO_BLK_S_OK 0
> +#define VIRTIO_BLK_S_IOERR 1
> +#define VIRTIO_BLK_S_UNSUPP 2
> +
> +struct vhost_blk_dev {
Please follow QEMU coding style.
> +static int vu_virtio_blk_process_req(struct vhost_blk_dev *vdev_blk,
> + VuVirtq *vq)
> +{
> + VuVirtqElement *elem;
> + uint32_t type;
> + unsigned in_num;
> + unsigned out_num;
> + struct vhost_blk_request *req;
> +
> + elem = vu_queue_pop(&vdev_blk->vu_dev, vq, sizeof(VuVirtqElement));
> + if (!elem) {
> + return -1;
> + }
> +
> + /* refer virtio_blk.c */
> + if (elem->out_num < 1 || elem->in_num < 1) {
> + fprintf(stderr, "Invalid descriptor\n");
> + return -1;
elem is leaked.
> + }
> +
> + req = calloc(1, sizeof(*req));
Can you make VuVirtqElement the first field of req to avoid the calloc()
call? This would also fix the elem memory leaks below.
> + assert(req);
> + req->vdev_blk = vdev_blk;
> + req->vq = vq;
> + req->elem = elem;
> +
> + in_num = elem->in_num;
> + out_num = elem->out_num;
> +
> + if (elem->out_sg[0].iov_len < sizeof(struct virtio_blk_outhdr)) {
This violates the virtio specification. Please see 2.4.4 Message Framing.
The device cannot assume any particular framing (iovec layout).
> + fprintf(stderr, "Invalid outhdr size\n");
> + free(req);
> + return -1;
> + }
> + req->out = (struct virtio_blk_outhdr *)elem->out_sg[0].iov_base;
> + out_num--;
> +
> + if (elem->in_sg[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
> + fprintf(stderr, "Invalid inhdr size\n");
> + free(req);
> + return -1;
> + }
> + req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base;
> + in_num--;
> +
> + type = le32_to_cpu(req->out->type);
Is this a VIRTIO 1.0-only device? I'm not sure le32_to_cpu() is correct
for all VIRTIO versions and all guest/host architectures.
> + switch (type & ~(VIRTIO_BLK_T_OUT | VIRTIO_BLK_T_BARRIER)) {
> + case VIRTIO_BLK_T_IN: {
> + ssize_t ret = 0;
> + bool is_write = type & VIRTIO_BLK_T_OUT;
> + req->sector_num = le64_to_cpu(req->out->sector);
> + if (is_write) {
> + assert(out_num != 0);
The guest must not be able to SIGABRT the process. Please implement
real error handling.
> + ret = vu_blk_writev(req, &elem->out_sg[1], out_num);
> + } else {
> + assert(in_num != 0);
> + ret = vu_blk_readv(req, &elem->in_sg[0], in_num);
> + }
> + if (ret >= 0) {
> + req->in->status = VIRTIO_BLK_S_OK;
> + } else {
> + req->in->status = VIRTIO_BLK_S_IOERR;
> + }
> + vu_blk_req_complete(req);
> + break;
> + }
> + case VIRTIO_BLK_T_FLUSH: {
> + vu_blk_flush(req);
> + req->in->status = VIRTIO_BLK_S_OK;
> + vu_blk_req_complete(req);
> + break;
> + }
> + case VIRTIO_BLK_T_GET_ID: {
> + size_t size = MIN(vu_blk_iov_size(&elem->in_sg[0], in_num),
> + VIRTIO_BLK_ID_BYTES);
> + snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk");
> + req->in->status = VIRTIO_BLK_S_OK;
> + req->size = elem->in_sg[0].iov_len;
> + vu_blk_req_complete(req);
> + break;
> + }
> + default: {
> + req->in->status = VIRTIO_BLK_S_UNSUPP;
> + vu_blk_req_complete(req);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void vu_blk_process_vq(VuDev *vu_dev, int idx)
> +{
> + struct vhost_blk_dev *vdev_blk;
> + VuVirtq *vq;
> + int ret;
> +
> + if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) {
> + fprintf(stderr, "VQ Index out of range: %d\n", idx);
> + vu_blk_panic_cb(vu_dev, NULL);
> + return;
> + }
> +
> +
> + vdev_blk = (struct vhost_blk_dev *)((uintptr_t)vu_dev -
> + offsetof(struct vhost_blk_dev, vu_dev));
qemu/osdep.h includes compiler.h, so container_of() should be available.
Several other places in the patch duplicate this.
signature.asc
Description: PGP signature
- [Qemu-devel] [PATCH 0/2] Introduce a new vhost-user-blk device and sample application, Changpeng Liu, 2017/07/25
- [Qemu-devel] [PATCH 2/2] vhost-user-blk: introduce a vhost-user-blk sample application, Changpeng Liu, 2017/07/25
- Re: [Qemu-devel] [PATCH 2/2] vhost-user-blk: introduce a vhost-user-blk sample application,
Stefan Hajnoczi <=
- [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Changpeng Liu, 2017/07/25
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/07/26
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/07/26
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/07/27
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/07/27
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/07/28
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/07/28
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Stefan Hajnoczi, 2017/07/31
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Paolo Bonzini, 2017/07/31
- Re: [Qemu-devel] [PATCH 1/2] vhost-user-blk: introduce a new vhost-user-blk host device, Liu, Changpeng, 2017/07/31