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 <kw...@redhat.com>
Just one commmit message suggestion. Reviewed-by: Raphael Norwitz <raphael.norw...@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 >