From: Raphael Norwitz <raphael.norwitz@nutanix.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"qemu-block@nongnu.org" <qemu-block@nongnu.org>,
"mst@redhat.com" <mst@redhat.com>
Subject: Re: [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config()
Date: Fri, 11 Jun 2021 20:08:11 +0000 [thread overview]
Message-ID: <20210611200809.GD26687@raphael-debian-dev> (raw)
In-Reply-To: <20210609154658.350308-6-kwolf@redhat.com>
On Wed, Jun 09, 2021 at 05:46:56PM +0200, Kevin Wolf wrote:
> Instead of just returning 0/-1 and letting the caller make up a
> meaningless error message, add an Error parameter to allow reporting the
> real error and switch to 0/-errno so that different kind of errors can
> be distinguished in the caller.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Just one commmit message suggestion.
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> include/hw/virtio/vhost-backend.h | 2 +-
> include/hw/virtio/vhost.h | 4 ++--
> hw/block/vhost-user-blk.c | 9 +++++----
> hw/display/vhost-user-gpu.c | 6 ++++--
> hw/input/vhost-user-input.c | 6 ++++--
> hw/net/vhost_net.c | 2 +-
> hw/virtio/vhost-user-vsock.c | 9 +++++----
> hw/virtio/vhost-user.c | 24 ++++++++++++------------
> hw/virtio/vhost-vdpa.c | 2 +-
> hw/virtio/vhost.c | 14 +++++++++++---
> 10 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index 728ebb0ed9..8475c5a29d 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -98,7 +98,7 @@ typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *data,
> uint32_t offset, uint32_t size,
> uint32_t flags);
> typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len);
> + uint32_t config_len, Error **errp);
>
> typedef int (*vhost_crypto_create_session_op)(struct vhost_dev *dev,
> void *session_info,
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 2d7aaad67b..045d0fd9f2 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -130,8 +130,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> struct vhost_vring_file *file);
>
> int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
> -int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len);
> +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> + uint32_t config_len, Error **errp);
> int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data,
> uint32_t offset, uint32_t size, uint32_t flags);
> /* notifier callback in case vhost device config space changed
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index e9382e152a..3770f715da 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -91,11 +91,13 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> int ret;
> struct virtio_blk_config blkcfg;
> VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
> + Error *local_err = NULL;
>
> ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
> - sizeof(struct virtio_blk_config));
> + sizeof(struct virtio_blk_config),
> + &local_err);
> if (ret < 0) {
> - error_report("get config space failed");
> + error_report_err(local_err);
> return -1;
> }
>
> @@ -478,9 +480,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> assert(s->connected);
>
> ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> - sizeof(struct virtio_blk_config));
> + sizeof(struct virtio_blk_config), errp);
> if (ret < 0) {
> - error_setg(errp, "vhost-user-blk: get block config failed");
> goto vhost_err;
> }
>
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 6cdaa1c73b..389199e6ca 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -415,14 +415,16 @@ vhost_user_gpu_get_config(VirtIODevice *vdev, uint8_t *config_data)
> VirtIOGPUBase *b = VIRTIO_GPU_BASE(vdev);
> struct virtio_gpu_config *vgconfig =
> (struct virtio_gpu_config *)config_data;
> + Error *local_err = NULL;
> int ret;
>
> memset(config_data, 0, sizeof(struct virtio_gpu_config));
>
> ret = vhost_dev_get_config(&g->vhost->dev,
> - config_data, sizeof(struct virtio_gpu_config));
> + config_data, sizeof(struct virtio_gpu_config),
> + &local_err);
> if (ret) {
> - error_report("vhost-user-gpu: get device config space failed");
> + error_report_err(local_err);
> return;
> }
>
> diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
> index 63984a8ba7..273e96a7b1 100644
> --- a/hw/input/vhost-user-input.c
> +++ b/hw/input/vhost-user-input.c
> @@ -49,13 +49,15 @@ static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
> {
> VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
> + Error *local_err = NULL;
> int ret;
>
> memset(config_data, 0, vinput->cfg_size);
>
> - ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size);
> + ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size,
> + &local_err);
> if (ret) {
> - error_report("vhost-user-input: get device config space failed");
> + error_report_err(local_err);
> return;
> }
> }
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 447b119f85..10a7780a13 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -117,7 +117,7 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> int vhost_net_get_config(struct vhost_net *net, uint8_t *config,
> uint32_t config_len)
> {
> - return vhost_dev_get_config(&net->dev, config, config_len);
> + return vhost_dev_get_config(&net->dev, config, config_len, NULL);
> }
> int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
> uint32_t offset, uint32_t size, uint32_t flags)
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index b6a4a25ea1..6095ed7349 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -34,10 +34,12 @@ static void vuv_get_config(VirtIODevice *vdev, uint8_t *config)
> static int vuv_handle_config_change(struct vhost_dev *dev)
> {
> VHostUserVSock *vsock = VHOST_USER_VSOCK(dev->vdev);
> + Error *local_err = NULL;
> int ret = vhost_dev_get_config(dev, (uint8_t *)&vsock->vsockcfg,
> - sizeof(struct virtio_vsock_config));
> + sizeof(struct virtio_vsock_config),
> + &local_err);
> if (ret < 0) {
> - error_report("get config space failed");
> + error_report_err(local_err);
> return -1;
> }
>
> @@ -114,9 +116,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp)
> }
>
> ret = vhost_dev_get_config(&vvc->vhost_dev, (uint8_t *)&vsock->vsockcfg,
> - sizeof(struct virtio_vsock_config));
> + sizeof(struct virtio_vsock_config), errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "get config space failed");
> goto err_vhost_dev;
> }
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 889559d86a..1ac4a2ebec 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2117,7 +2117,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> }
>
> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len)
> + uint32_t config_len, Error **errp)
> {
> VhostUserMsg msg = {
> .hdr.request = VHOST_USER_GET_CONFIG,
> @@ -2127,32 +2127,32 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>
> if (!virtio_has_feature(dev->protocol_features,
> VHOST_USER_PROTOCOL_F_CONFIG)) {
> - return -1;
> + error_setg(errp, "VHOST_USER_PROTOCOL_F_CONFIG not supported");
> + return -EINVAL;
> }
>
> - if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
> - return -1;
> - }
> + assert(config_len <= VHOST_USER_MAX_CONFIG_SIZE);
Maybe just note that you are changing the check to an assert in the
commit message since it is a functional change?
>
> msg.payload.config.offset = 0;
> msg.payload.config.size = config_len;
> if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> - return -1;
> + return -EPROTO;
> }
>
> if (vhost_user_read(dev, &msg) < 0) {
> - return -1;
> + return -EPROTO;
> }
>
> if (msg.hdr.request != VHOST_USER_GET_CONFIG) {
> - error_report("Received unexpected msg type. Expected %d received %d",
> - VHOST_USER_GET_CONFIG, msg.hdr.request);
> - return -1;
> + error_setg(errp,
> + "Received unexpected msg type. Expected %d received %d",
> + VHOST_USER_GET_CONFIG, msg.hdr.request);
> + return -EINVAL;
> }
>
> if (msg.hdr.size != VHOST_USER_CONFIG_HDR_SIZE + config_len) {
> - error_report("Received bad msg size.");
> - return -1;
> + error_setg(errp, "Received bad msg size.");
> + return -EINVAL;
> }
>
> memcpy(config, msg.payload.config.region, config_len);
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 71897c1a01..6b6f974a83 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -451,7 +451,7 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
> }
>
> static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len)
> + uint32_t config_len, Error **errp)
> {
> struct vhost_vdpa_config *v_config;
> unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index c7f9d8bb06..1b66f1de70 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1562,15 +1562,23 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> }
>
> int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> - uint32_t config_len)
> + uint32_t config_len, Error **errp)
> {
> + ERRP_GUARD();
> + int ret;
> +
> assert(hdev->vhost_ops);
>
> if (hdev->vhost_ops->vhost_get_config) {
> - return hdev->vhost_ops->vhost_get_config(hdev, config, config_len);
> + ret = hdev->vhost_ops->vhost_get_config(hdev, config, config_len, errp);
> + if (ret < 0 && !*errp) {
> + error_setg_errno(errp, -ret, "vhost_get_config failed");
> + }
> + return ret;
> }
>
> - return -1;
> + error_setg(errp, "vhost_dev_get_config not implemented");
> + return -ENOTSUP;
> }
>
> int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data,
> --
> 2.30.2
>
next prev parent reply other threads:[~2021-06-11 20:09 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-06-09 15:46 ` [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
2021-06-10 9:05 ` Stefano Garzarella
2021-06-11 19:14 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
2021-06-10 9:07 ` Stefano Garzarella
2021-06-11 19:43 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
2021-06-10 9:09 ` Stefano Garzarella
2021-06-11 19:49 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
2021-06-10 9:11 ` Stefano Garzarella
2021-06-11 19:55 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
2021-06-10 9:16 ` Stefano Garzarella
2021-06-11 20:08 ` Raphael Norwitz [this message]
2021-06-09 15:46 ` [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
2021-06-10 9:18 ` Stefano Garzarella
2021-06-11 20:11 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 7/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-06-10 9:23 ` Stefano Garzarella
2021-06-11 20:15 ` Raphael Norwitz
2021-06-30 12:39 ` [PATCH 0/7] " Kevin Wolf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210611200809.GD26687@raphael-debian-dev \
--to=raphael.norwitz@nutanix.com \
--cc=kwolf@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).