| Message ID | 1436938201-16766-2-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 15 Jul 2015 13:29:57 +0800 Jason Wang <jasowang@redhat.com> wrote: > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/9pfs/virtio-9p-device.c | 3 ++- > hw/block/virtio-blk.c | 3 ++- > hw/char/virtio-serial-bus.c | 3 ++- > hw/display/virtio-gpu.c | 3 ++- > hw/input/virtio-input.c | 3 ++- > hw/net/virtio-net.c | 3 ++- > hw/scsi/vhost-scsi.c | 3 ++- > hw/scsi/virtio-scsi.c | 3 ++- > hw/virtio/virtio-balloon.c | 3 ++- > hw/virtio/virtio-bus.c | 3 ++- > hw/virtio/virtio-rng.c | 2 +- > include/hw/virtio/virtio.h | 4 +++- > 12 files changed, 24 insertions(+), 12 deletions(-) > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index 3926f7e..febda76 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -54,7 +54,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > /* Get the features of the plugged device. */ > assert(vdc->get_features != NULL); > - vdev->host_features = vdc->get_features(vdev, vdev->host_features); > + vdev->host_features = vdc->get_features(vdev, vdev->host_features, > + errp); > } > > /* Reset the virtio_bus */ Don't you need to propagate the error instead of passing it through? Or am I just confused by error handling? :)
On 07/15/2015 05:01 PM, Cornelia Huck wrote: > On Wed, 15 Jul 2015 13:29:57 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/9pfs/virtio-9p-device.c | 3 ++- >> hw/block/virtio-blk.c | 3 ++- >> hw/char/virtio-serial-bus.c | 3 ++- >> hw/display/virtio-gpu.c | 3 ++- >> hw/input/virtio-input.c | 3 ++- >> hw/net/virtio-net.c | 3 ++- >> hw/scsi/vhost-scsi.c | 3 ++- >> hw/scsi/virtio-scsi.c | 3 ++- >> hw/virtio/virtio-balloon.c | 3 ++- >> hw/virtio/virtio-bus.c | 3 ++- >> hw/virtio/virtio-rng.c | 2 +- >> include/hw/virtio/virtio.h | 4 +++- >> 12 files changed, 24 insertions(+), 12 deletions(-) >> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >> index 3926f7e..febda76 100644 >> --- a/hw/virtio/virtio-bus.c >> +++ b/hw/virtio/virtio-bus.c >> @@ -54,7 +54,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >> >> /* Get the features of the plugged device. */ >> assert(vdc->get_features != NULL); >> - vdev->host_features = vdc->get_features(vdev, vdev->host_features); >> + vdev->host_features = vdc->get_features(vdev, vdev->host_features, >> + errp); >> } >> >> /* Reset the virtio_bus */ > Don't you need to propagate the error instead of passing it through? Or > am I just confused by error handling? :) > If I understand the code correctly. The caller (virtio_device_realize()) will propagate the error.
On 15/07/2015 11:01, Cornelia Huck wrote: > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > > index 3926f7e..febda76 100644 > > --- a/hw/virtio/virtio-bus.c > > +++ b/hw/virtio/virtio-bus.c > > @@ -54,7 +54,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) > > > > /* Get the features of the plugged device. */ > > assert(vdc->get_features != NULL); > > - vdev->host_features = vdc->get_features(vdev, vdev->host_features); > > + vdev->host_features = vdc->get_features(vdev, vdev->host_features, > > + errp); > > } > > > > /* Reset the virtio_bus */ > > Don't you need to propagate the error instead of passing it through? Or > am I just confused by error handling? :) Explicit propagation is only needed if you need to look at the error (because errp could be NULL). Here you don't need to do that, so passing it through is fine. Error management is not hard if you know the rules (and the rules are not hard, just important). Someone needs to dig up Markus's latest explanation and put it in docs/. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 15/07/2015 11:01, Cornelia Huck wrote: >> > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c >> > index 3926f7e..febda76 100644 >> > --- a/hw/virtio/virtio-bus.c >> > +++ b/hw/virtio/virtio-bus.c >> > @@ -54,7 +54,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) >> > >> > /* Get the features of the plugged device. */ >> > assert(vdc->get_features != NULL); >> > - vdev->host_features = vdc->get_features(vdev, vdev->host_features); >> > + vdev->host_features = vdc->get_features(vdev, vdev->host_features, >> > + errp); >> > } >> > >> > /* Reset the virtio_bus */ >> >> Don't you need to propagate the error instead of passing it through? Or >> am I just confused by error handling? :) > > Explicit propagation is only needed if you need to look at the error > (because errp could be NULL). Here you don't need to do that, so > passing it through is fine. > > Error management is not hard if you know the rules (and the rules are > not hard, just important). Someone needs to dig up Markus's latest > explanation and put it in docs/. I got a patch pending that puts it in include/qapi/error.h: [PATCH 6/7] error: Revamp interface documentation part of error: On abort, report where the error was created Intend to respin soonish to address Eric's and Laszlo's review.
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c index 3f4c9e7..93a407c 100644 --- a/hw/9pfs/virtio-9p-device.c +++ b/hw/9pfs/virtio-9p-device.c @@ -21,7 +21,8 @@ #include "virtio-9p-coth.h" #include "hw/virtio/virtio-access.h" -static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features) +static uint64_t virtio_9p_get_features(VirtIODevice *vdev, uint64_t features, + Error **errp) { virtio_add_feature(&features, VIRTIO_9P_MOUNT_TAG); return features; diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 6aefda4..4c27974 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -722,7 +722,8 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config) aio_context_release(blk_get_aio_context(s->blk)); } -static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features) +static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, + Error **errp) { VirtIOBlock *s = VIRTIO_BLK(vdev); diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c index 78c73e5..90bdc31 100644 --- a/hw/char/virtio-serial-bus.c +++ b/hw/char/virtio-serial-bus.c @@ -499,7 +499,8 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq) } } -static uint64_t get_features(VirtIODevice *vdev, uint64_t features) +static uint64_t get_features(VirtIODevice *vdev, uint64_t features, + Error **errp) { VirtIOSerial *vser; diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 990a26b..a67d927 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -89,7 +89,8 @@ static void virtio_gpu_set_config(VirtIODevice *vdev, const uint8_t *config) } } -static uint64_t virtio_gpu_get_features(VirtIODevice *vdev, uint64_t features) +static uint64_t virtio_gpu_get_features(VirtIODevice *vdev, uint64_t features, + Error **errp) { return features; } diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c index 7f5b8d6..7b25d27 100644 --- a/hw/input/virtio-input.c +++ b/hw/input/virtio-input.c @@ -166,7 +166,8 @@ static void virtio_input_set_config(VirtIODevice *vdev, virtio_notify_config(vdev); } -static uint64_t virtio_input_get_features(VirtIODevice *vdev, uint64_t f) +static uint64_t virtio_input_get_features(VirtIODevice *vdev, uint64_t f, + Error **errp) { return f; } diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index e3c2db3..a56bcab 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -438,7 +438,8 @@ static void virtio_net_set_queues(VirtIONet *n) static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue); -static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features) +static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, + Error **errp) { VirtIONet *n = VIRTIO_NET(vdev); NetClientState *nc = qemu_get_queue(n->nic); diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 52549f8..a69918b 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -153,7 +153,8 @@ static void vhost_scsi_stop(VHostSCSI *s) } static uint64_t vhost_scsi_get_features(VirtIODevice *vdev, - uint64_t features) + uint64_t features, + Error **errp) { VHostSCSI *s = VHOST_SCSI(vdev); diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index f7d3c7c..701efeb 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -629,7 +629,8 @@ static void virtio_scsi_set_config(VirtIODevice *vdev, } static uint64_t virtio_scsi_get_features(VirtIODevice *vdev, - uint64_t requested_features) + uint64_t requested_features, + Error **errp) { VirtIOSCSI *s = VIRTIO_SCSI(vdev); diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index 2990f8d..3577b7a 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -310,7 +310,8 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, trace_virtio_balloon_set_config(dev->actual, oldactual); } -static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f) +static uint64_t virtio_balloon_get_features(VirtIODevice *vdev, uint64_t f, + Error **errp) { VirtIOBalloon *dev = VIRTIO_BALLOON(vdev); f |= dev->host_features; diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index 3926f7e..febda76 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -54,7 +54,8 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp) /* Get the features of the plugged device. */ assert(vdc->get_features != NULL); - vdev->host_features = vdc->get_features(vdev, vdev->host_features); + vdev->host_features = vdc->get_features(vdev, vdev->host_features, + errp); } /* Reset the virtio_bus */ diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c index 740ed31..63f35cb 100644 --- a/hw/virtio/virtio-rng.c +++ b/hw/virtio/virtio-rng.c @@ -98,7 +98,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq) virtio_rng_process(vrng); } -static uint64_t get_features(VirtIODevice *vdev, uint64_t f) +static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp) { return f; } diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 473fb75..1cd824f 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -97,7 +97,9 @@ typedef struct VirtioDeviceClass { /* This is what a VirtioDevice must implement */ DeviceRealize realize; DeviceUnrealize unrealize; - uint64_t (*get_features)(VirtIODevice *vdev, uint64_t requested_features); + uint64_t (*get_features)(VirtIODevice *vdev, + uint64_t requested_features, + Error **errp); uint64_t (*bad_features)(VirtIODevice *vdev); void (*set_features)(VirtIODevice *vdev, uint64_t val); int (*validate_features)(VirtIODevice *vdev);
Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/9pfs/virtio-9p-device.c | 3 ++- hw/block/virtio-blk.c | 3 ++- hw/char/virtio-serial-bus.c | 3 ++- hw/display/virtio-gpu.c | 3 ++- hw/input/virtio-input.c | 3 ++- hw/net/virtio-net.c | 3 ++- hw/scsi/vhost-scsi.c | 3 ++- hw/scsi/virtio-scsi.c | 3 ++- hw/virtio/virtio-balloon.c | 3 ++- hw/virtio/virtio-bus.c | 3 ++- hw/virtio/virtio-rng.c | 2 +- include/hw/virtio/virtio.h | 4 +++- 12 files changed, 24 insertions(+), 12 deletions(-)