| Message ID | 20210609154658.350308-3-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-blk: Implement reconnection during realize | expand |
On Wed, Jun 09, 2021 at 05:46:53PM +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. > >Specifically, in vhost-user, EPROTO is used for all errors that relate >to the connection itself, whereas other error codes are used for errors >relating to the content of the connection. This will allow us later to >automatically reconnect when the connection goes away, without ending up >in an endless loop if it's a permanent error in the configuration. > >Signed-off-by: Kevin Wolf <kwolf@redhat.com> >--- > include/hw/virtio/vhost-backend.h | 3 ++- > hw/virtio/vhost-backend.c | 2 +- > hw/virtio/vhost-user.c | 41 ++++++++++++++++--------------- > hw/virtio/vhost-vdpa.c | 2 +- > hw/virtio/vhost.c | 13 +++++----- > 5 files changed, 32 insertions(+), 29 deletions(-) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
On Wed, Jun 09, 2021 at 05:46:53PM +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. > > Specifically, in vhost-user, EPROTO is used for all errors that relate > to the connection itself, whereas other error codes are used for errors > relating to the content of the connection. This will allow us later to > automatically reconnect when the connection goes away, without ending up > in an endless loop if it's a permanent error in the configuration. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > include/hw/virtio/vhost-backend.h | 3 ++- > hw/virtio/vhost-backend.c | 2 +- > hw/virtio/vhost-user.c | 41 ++++++++++++++++--------------- > hw/virtio/vhost-vdpa.c | 2 +- > hw/virtio/vhost.c | 13 +++++----- > 5 files changed, 32 insertions(+), 29 deletions(-) > > diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h > index 8a6f8e2a7a..728ebb0ed9 100644 > --- a/include/hw/virtio/vhost-backend.h > +++ b/include/hw/virtio/vhost-backend.h > @@ -37,7 +37,8 @@ struct vhost_scsi_target; > struct vhost_iotlb_msg; > struct vhost_virtqueue; > > -typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque); > +typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque, > + Error **errp); > typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev); > typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev); > > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c > index 31b33bde37..f4f71cf58a 100644 > --- a/hw/virtio/vhost-backend.c > +++ b/hw/virtio/vhost-backend.c > @@ -30,7 +30,7 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request, > return ioctl(fd, request, arg); > } > > -static int vhost_kernel_init(struct vhost_dev *dev, void *opaque) > +static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp) > { > assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL); > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index ee57abe045..024cb201bb 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -1856,7 +1856,8 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier, > return 0; > } > > -static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > +static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, > + Error **errp) > { > uint64_t features, protocol_features, ram_slots; > struct vhost_user *u; > @@ -1871,7 +1872,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > > err = vhost_user_get_features(dev, &features); > if (err < 0) { > - return err; > + return -EPROTO; > } > > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) { > @@ -1880,7 +1881,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES, > &protocol_features); > if (err < 0) { > - return err; > + return -EPROTO; > } > > dev->protocol_features = > @@ -1891,14 +1892,14 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); > } else if (!(protocol_features & > (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) { > - error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG " > - "but backend does not support it."); > - return -1; > + error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG " > + "but backend does not support it."); > + return -EINVAL; > } > > err = vhost_user_set_protocol_features(dev, dev->protocol_features); > if (err < 0) { > - return err; > + return -EPROTO; > } > > /* query the max queues we support if backend supports Multiple Queue */ > @@ -1906,12 +1907,12 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM, > &dev->max_queues); > if (err < 0) { > - return err; > + return -EPROTO; > } > } > if (dev->num_queues && dev->max_queues < dev->num_queues) { > - error_report("The maximum number of queues supported by the " > - "backend is %" PRIu64, dev->max_queues); > + error_setg(errp, "The maximum number of queues supported by the " > + "backend is %" PRIu64, dev->max_queues); > return -EINVAL; > } > > @@ -1920,9 +1921,9 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > VHOST_USER_PROTOCOL_F_SLAVE_REQ) && > virtio_has_feature(dev->protocol_features, > VHOST_USER_PROTOCOL_F_REPLY_ACK))) { > - error_report("IOMMU support requires reply-ack and " > - "slave-req protocol features."); > - return -1; > + error_setg(errp, "IOMMU support requires reply-ack and " > + "slave-req protocol features."); > + return -EINVAL; > } > > /* get max memory regions if backend supports configurable RAM slots */ > @@ -1932,15 +1933,15 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > } else { > err = vhost_user_get_max_memslots(dev, &ram_slots); > if (err < 0) { > - return err; > + return -EPROTO; > } > > if (ram_slots < u->user->memory_slots) { > - error_report("The backend specified a max ram slots limit " > - "of %" PRIu64", when the prior validated limit was %d. " > - "This limit should never decrease.", ram_slots, > - u->user->memory_slots); > - return -1; > + error_setg(errp, "The backend specified a max ram slots limit " > + "of %" PRIu64", when the prior validated limit was " > + "%d. This limit should never decrease.", ram_slots, > + u->user->memory_slots); > + return -EINVAL; > } > > u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS); > @@ -1958,7 +1959,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) > if (dev->vq_index == 0) { > err = vhost_setup_slave_channel(dev); > if (err < 0) { > - return err; > + return -EPROTO; > } > } > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index ee51863d28..c2aadb57cb 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -273,7 +273,7 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status) > vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s); > } > > -static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque) > +static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) > { > struct vhost_vdpa *v; > uint64_t features; > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 991c67ddcd..fd13135706 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1289,9 +1289,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > VhostBackendType backend_type, uint32_t busyloop_timeout, > Error **errp) > { > + ERRP_GUARD(); > uint64_t features; > int i, r, n_initialized_vqs = 0; > - Error *local_err = NULL; > > hdev->vdev = NULL; > hdev->migration_blocker = NULL; > @@ -1299,9 +1299,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > r = vhost_set_backend_type(hdev, backend_type); > assert(r >= 0); > > - r = hdev->vhost_ops->vhost_backend_init(hdev, opaque); > + r = hdev->vhost_ops->vhost_backend_init(hdev, opaque, errp); > if (r < 0) { > - error_setg(errp, "vhost_backend_init failed"); > + if (!*errp) { > + error_setg_errno(errp, -r, "vhost_backend_init failed"); > + } > goto fail; > } > > @@ -1369,9 +1371,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > } > > if (hdev->migration_blocker != NULL) { > - r = migrate_add_blocker(hdev->migration_blocker, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > + r = migrate_add_blocker(hdev->migration_blocker, errp); > + if (*errp) { > error_free(hdev->migration_blocker); > goto fail_busyloop; > } > -- > 2.30.2 >
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index 8a6f8e2a7a..728ebb0ed9 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -37,7 +37,8 @@ struct vhost_scsi_target; struct vhost_iotlb_msg; struct vhost_virtqueue; -typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque); +typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque, + Error **errp); typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev); typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev); diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c index 31b33bde37..f4f71cf58a 100644 --- a/hw/virtio/vhost-backend.c +++ b/hw/virtio/vhost-backend.c @@ -30,7 +30,7 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request, return ioctl(fd, request, arg); } -static int vhost_kernel_init(struct vhost_dev *dev, void *opaque) +static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp) { assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL); diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index ee57abe045..024cb201bb 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1856,7 +1856,8 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier, return 0; } -static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) +static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, + Error **errp) { uint64_t features, protocol_features, ram_slots; struct vhost_user *u; @@ -1871,7 +1872,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) err = vhost_user_get_features(dev, &features); if (err < 0) { - return err; + return -EPROTO; } if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) { @@ -1880,7 +1881,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES, &protocol_features); if (err < 0) { - return err; + return -EPROTO; } dev->protocol_features = @@ -1891,14 +1892,14 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); } else if (!(protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) { - error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG " - "but backend does not support it."); - return -1; + error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG " + "but backend does not support it."); + return -EINVAL; } err = vhost_user_set_protocol_features(dev, dev->protocol_features); if (err < 0) { - return err; + return -EPROTO; } /* query the max queues we support if backend supports Multiple Queue */ @@ -1906,12 +1907,12 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM, &dev->max_queues); if (err < 0) { - return err; + return -EPROTO; } } if (dev->num_queues && dev->max_queues < dev->num_queues) { - error_report("The maximum number of queues supported by the " - "backend is %" PRIu64, dev->max_queues); + error_setg(errp, "The maximum number of queues supported by the " + "backend is %" PRIu64, dev->max_queues); return -EINVAL; } @@ -1920,9 +1921,9 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) VHOST_USER_PROTOCOL_F_SLAVE_REQ) && virtio_has_feature(dev->protocol_features, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { - error_report("IOMMU support requires reply-ack and " - "slave-req protocol features."); - return -1; + error_setg(errp, "IOMMU support requires reply-ack and " + "slave-req protocol features."); + return -EINVAL; } /* get max memory regions if backend supports configurable RAM slots */ @@ -1932,15 +1933,15 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) } else { err = vhost_user_get_max_memslots(dev, &ram_slots); if (err < 0) { - return err; + return -EPROTO; } if (ram_slots < u->user->memory_slots) { - error_report("The backend specified a max ram slots limit " - "of %" PRIu64", when the prior validated limit was %d. " - "This limit should never decrease.", ram_slots, - u->user->memory_slots); - return -1; + error_setg(errp, "The backend specified a max ram slots limit " + "of %" PRIu64", when the prior validated limit was " + "%d. This limit should never decrease.", ram_slots, + u->user->memory_slots); + return -EINVAL; } u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS); @@ -1958,7 +1959,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) if (dev->vq_index == 0) { err = vhost_setup_slave_channel(dev); if (err < 0) { - return err; + return -EPROTO; } } diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index ee51863d28..c2aadb57cb 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -273,7 +273,7 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status) vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s); } -static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque) +static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) { struct vhost_vdpa *v; uint64_t features; diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 991c67ddcd..fd13135706 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1289,9 +1289,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, VhostBackendType backend_type, uint32_t busyloop_timeout, Error **errp) { + ERRP_GUARD(); uint64_t features; int i, r, n_initialized_vqs = 0; - Error *local_err = NULL; hdev->vdev = NULL; hdev->migration_blocker = NULL; @@ -1299,9 +1299,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, r = vhost_set_backend_type(hdev, backend_type); assert(r >= 0); - r = hdev->vhost_ops->vhost_backend_init(hdev, opaque); + r = hdev->vhost_ops->vhost_backend_init(hdev, opaque, errp); if (r < 0) { - error_setg(errp, "vhost_backend_init failed"); + if (!*errp) { + error_setg_errno(errp, -r, "vhost_backend_init failed"); + } goto fail; } @@ -1369,9 +1371,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, } if (hdev->migration_blocker != NULL) { - r = migrate_add_blocker(hdev->migration_blocker, &local_err); - if (local_err) { - error_propagate(errp, local_err); + r = migrate_add_blocker(hdev->migration_blocker, errp); + if (*errp) { error_free(hdev->migration_blocker); goto fail_busyloop; }
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. Specifically, in vhost-user, EPROTO is used for all errors that relate to the connection itself, whereas other error codes are used for errors relating to the content of the connection. This will allow us later to automatically reconnect when the connection goes away, without ending up in an endless loop if it's a permanent error in the configuration. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/hw/virtio/vhost-backend.h | 3 ++- hw/virtio/vhost-backend.c | 2 +- hw/virtio/vhost-user.c | 41 ++++++++++++++++--------------- hw/virtio/vhost-vdpa.c | 2 +- hw/virtio/vhost.c | 13 +++++----- 5 files changed, 32 insertions(+), 29 deletions(-)