On Fri, Jun 05, 2020 at 07:35:37AM +0800, Coiby Xu wrote:
> +static void coroutine_fn vu_block_virtio_process_req(void *opaque)
> +{
> + struct req_data *data = opaque;
> + VuServer *server = data->server;
> + VuVirtq *vq = data->vq;
> + VuVirtqElement *elem = data->elem;
> + uint32_t type;
> + VuBlockReq *req;
> +
> + VuBlockDev *vdev_blk = get_vu_block_device_by_server(server);
> + BlockBackend *backend = vdev_blk->backend;
> +
> + struct iovec *in_iov = elem->in_sg;
> + struct iovec *out_iov = elem->out_sg;
> + unsigned in_num = elem->in_num;
> + unsigned out_num = elem->out_num;
> + /* refer to hw/block/virtio_blk.c */
> + if (elem->out_num < 1 || elem->in_num < 1) {
> + error_report("virtio-blk request missing headers");
> + free(elem);
> + return;
> + }
> +
> + req = g_new0(VuBlockReq, 1);
elem was allocated with enough space for VuBlockReq. Can this allocation
be eliminated?
typedef struct VuBlockReq {
- VuVirtqElement *elem;
+ VuVirtqElement elem;
int64_t sector_num;
size_t size;
struct virtio_blk_inhdr *in;
struct virtio_blk_outhdr out;
VuServer *server;
struct VuVirtq *vq;
} VuBlockReq;
req = vu_queue_pop(vu_dev, vq, sizeof(*req));
> + req->server = server;
> + req->vq = vq;
> + req->elem = elem;
> +
> + if (unlikely(iov_to_buf(out_iov, out_num, 0, &req->out,
> + sizeof(req->out)) != sizeof(req->out))) {
> + error_report("virtio-blk request outhdr too short");
> + goto err;
> + }
> +
> + iov_discard_front(&out_iov, &out_num, sizeof(req->out));
> +
> + if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
> + error_report("virtio-blk request inhdr too short");
> + goto err;
> + }
> +
> + /* We always touch the last byte, so just see how big in_iov is. */
> + req->in = (void *)in_iov[in_num - 1].iov_base
> + + in_iov[in_num - 1].iov_len
> + - sizeof(struct virtio_blk_inhdr);
> + iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
> +
> +
> + type = le32toh(req->out.type);
This implementation assumes the request is always little-endian. This is
true for VIRTIO 1.0+ but not for older versions. Please check that
VIRTIO_F_VERSION_1 has been set.
In QEMU code the le32_to_cpu(), le64_to_cpu(), etc are common used
instead of le32toh(), etc.
> + switch (type & ~VIRTIO_BLK_T_BARRIER) {
> + case VIRTIO_BLK_T_IN:
> + case VIRTIO_BLK_T_OUT: {
> + ssize_t ret = 0;
> + bool is_write = type & VIRTIO_BLK_T_OUT;
> + req->sector_num = le64toh(req->out.sector);
> +
> + int64_t offset = req->sector_num * vdev_blk->blk_size;
> + QEMUIOVector *qiov = g_new0(QEMUIOVector, 1);
This can be allocated on the stack:
QEMUIOVector qiov;
> +static void vhost_user_blk_server_free(VuBlockDev *vu_block_device)
> +{
I'm unsure why this is a separate from vu_block_free(). Neither of these
functions actually free VuBlockDev, so the name could be changed to
vhost_user_blk_server_stop().
> + if (!vu_block_device) {
> + return;
> + }
> + vhost_user_server_stop(&vu_block_device->vu_server);
> + vu_block_free(vu_block_device);
> +
> +}
> +
> +/*
> + * A exported drive can serve multiple multiple clients simutateously,
> + * thus no need to export the same drive twice.
This comment is outdated. Only one client is served at a time.
> +static void vhost_user_blk_server_start(VuBlockDev *vu_block_device,
> + Error **errp)
> +{
> +
> + const char *name = vu_block_device->node_name;
> + SocketAddress *addr = vu_block_device->addr;
> + char *unix_socket = vu_block_device->addr->u.q_unix.path;
> +
> + if (vu_block_dev_find(name)) {
> + error_setg(errp, "Vhost-user-blk server with node-name '%s' "
> + "has already been started",
> + name);
> + return;
> + }
I think blk_new() permissions should prevent multiple writers. Having
multiple readers would be okay. Therefore this check can be removed.
> +
> + if (vu_block_dev_find_by_unix_socket(unix_socket)) {
> + error_setg(errp, "Vhost-user-blk server with with socket_path '%s' "
> + "has already been started", unix_socket);
> + return;
> + }
Is it a problem if the same path is reused? I don't see an issue if the
user creates a vhost-user-blk server, connects a client, unlinks the
UNIX domain socket, and creates a new vhost-user-blk server with the
same path. It might be a little confusing but if the user wants to do
it, I don't see a reason to stop them.
> +
> + if (!vu_block_init(vu_block_device, errp)) {
> + return;
> + }
> +
> +
> + AioContext *ctx = bdrv_get_aio_context(blk_bs(vu_block_device->backend));
> +
> + if (!vhost_user_server_start(VHOST_USER_BLK_MAX_QUEUES, addr, ctx,
> + &vu_block_device->vu_server,
> + NULL, &vu_block_iface,
> + errp)) {
In the previous patch I mentioned that calling g_free(server) is
probably unexpected and here is an example of why that can be a problem.
vu_server is a struct field, not an independent heap-allocated object,
so calling g_free(server) will result in undefined behavior (freeing an
object that was not allocated with g_new()).
> + goto error;
> + }
> +
> + QTAILQ_INSERT_TAIL(&vu_block_devs, vu_block_device, next);
> + blk_add_aio_context_notifier(vu_block_device->backend, blk_aio_attached,
> + blk_aio_detach, vu_block_device);
> + return;
> +
> + error:
> + vu_block_free(vu_block_device);
> +}
> +
> +static void vu_set_node_name(Object *obj, const char *value, Error **errp)
> +{
> + VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> +
> + if (vus->node_name) {
> + error_setg(errp, "evdev property already set");
> + return;
> + }
Setting it twice is okay, we just need to g_free(vus->node_name).
> +
> + vus->node_name = g_strdup(value);
> +}
> +
> +static char *vu_get_node_name(Object *obj, Error **errp)
> +{
> + VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> + return g_strdup(vus->node_name);
> +}
> +
> +
> +static void vu_set_unix_socket(Object *obj, const char *value,
> + Error **errp)
> +{
> + VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> +
> + if (vus->addr) {
> + error_setg(errp, "unix_socket property already set");
> + return;
> + }
Setting it twice is okay, we just need to
g_free(vus->addr->u.q_unix.path) and g_free(vus->addr).
> +static void vu_set_blk_size(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> +
> + Error *local_err = NULL;
> + uint32_t value;
> +
> + visit_type_uint32(v, name, &value, &local_err);
> + if (local_err) {
> + goto out;
> + }
> + if (value != BDRV_SECTOR_SIZE && value != 4096) {
> + error_setg(&local_err,
> + "Property '%s.%s' can only take value 512 or 4096",
> + object_get_typename(obj), name);
> + goto out;
> + }
Please see hw/core/qdev-properties.c:set_blocksize() for input
validation checks (min=512, max=32768, must be a power of 2). This code
can be moved into a common utility function so that both
hw/core/qdev-properties.c and vhost-user-blk-server.c can use it.
> +
> + vus->blk_size = value;
> +
> +out:
> + error_propagate(errp, local_err);
> + vus->blk_size = value;
> +}
> +
> +
> +static void vhost_user_blk_server_instance_finalize(Object *obj)
> +{
> + VuBlockDev *vub = VHOST_USER_BLK_SERVER(obj);
> +
> + vhost_user_blk_server_free(vub);
> +}
> +
> +static void vhost_user_blk_server_complete(UserCreatable *obj, Error **errp)
> +{
> + Error *local_error = NULL;
> + VuBlockDev *vub = VHOST_USER_BLK_SERVER(obj);
> +
> + vhost_user_blk_server_start(vub, &local_error);
After this call succeeds the properties should become read-only
("writable", "node-name", "unix-socket", etc) to prevent modification at
runtime.
I think the easiest way to do that is by keeping a bool field in
VuBlockDev that the property setter functions can check.
> +
> + if (local_error) {
> + error_propagate(errp, local_error);
> + return;
> + }
> +}
> +
> +static void vhost_user_blk_server_class_init(ObjectClass *klass,
> + void *class_data)
> +{
> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
> + ucc->complete = vhost_user_blk_server_complete;
> +
> + object_class_property_add_bool(klass, "writable",
> + vu_get_block_writable,
> + vu_set_block_writable);
> +
> + object_class_property_add_str(klass, "node-name",
> + vu_get_node_name,
> + vu_set_node_name);
> +
> + object_class_property_add_str(klass, "unix-socket",
> + vu_get_unix_socket,
> + vu_set_unix_socket);
> +
> + object_class_property_add(klass, "blk-size", "uint32",
> + vu_get_blk_size, vu_set_blk_size,
> + NULL, NULL);
include/hw/block/block.h:DEFINE_BLOCK_PROPERTIES_BASE calls this
property "logical_block_size". Please use the same name for consistency.
signature.asc
Description: PGP signature
