Message ID | 20200716123704.6557-3-f4bug@amsat.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block/block-backend: Let blk_attach_dev() provide helpful error | expand |
On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote: > Let blk_attach_dev() take an Error* object to return helpful > information. Adapt the callers. > > $ qemu-system-arm -M n800 > qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card' > blk 'sd0' is already attached by device 'omap2-mmc' > Drive 'sd0' is already in use because it has been automatically connected to another device > (do you need 'if=none' in the drive options?) > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()") > --- > include/sysemu/block-backend.h | 2 +- > block/block-backend.c | 11 ++++++++++- > hw/block/fdc.c | 4 +--- > hw/block/swim.c | 4 +--- > hw/block/xen-block.c | 5 +++-- > hw/core/qdev-properties-system.c | 16 +++++++++------- > hw/ide/qdev.c | 4 +--- > hw/scsi/scsi-disk.c | 4 +--- > 8 files changed, 27 insertions(+), 23 deletions(-) > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index 8203d7f6f9..118fbad0b4 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk); > void blk_iostatus_disable(BlockBackend *blk); > void blk_iostatus_reset(BlockBackend *blk); > void blk_iostatus_set_err(BlockBackend *blk, int error); > -int blk_attach_dev(BlockBackend *blk, DeviceState *dev); > +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp); > void blk_detach_dev(BlockBackend *blk, DeviceState *dev); > DeviceState *blk_get_attached_dev(BlockBackend *blk); > char *blk_get_attached_dev_id(BlockBackend *blk); > diff --git a/block/block-backend.c b/block/block-backend.c > index 63ff940ef9..b7be0a4619 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) > > /* > * Attach device model @dev to @blk. > + * > + * @blk: Block backend > + * @dev: Device to attach the block backend to > + * @errp: pointer to NULL initialized error object > + * > * Return 0 on success, -EBUSY when a device model is attached already. > */ > -int blk_attach_dev(BlockBackend *blk, DeviceState *dev) > +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp) > { > trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev))); > if (blk->dev) { > + error_setg(errp, "cannot attach blk '%s' to device '%s'", > + blk_name(blk), object_get_typename(OBJECT(dev))); > + error_append_hint(errp, "blk '%s' is already attached by device '%s'\n", > + blk_name(blk), object_get_typename(OBJECT(blk->dev))); I would have a preference for expanding the main error message and not using a hint. Any hint is completely thrown away when using QMP :-( Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote: >> Let blk_attach_dev() take an Error* object to return helpful >> information. Adapt the callers. >> >> $ qemu-system-arm -M n800 >> qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card' >> blk 'sd0' is already attached by device 'omap2-mmc' >> Drive 'sd0' is already in use because it has been automatically connected to another device >> (do you need 'if=none' in the drive options?) >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()") >> --- >> include/sysemu/block-backend.h | 2 +- >> block/block-backend.c | 11 ++++++++++- >> hw/block/fdc.c | 4 +--- >> hw/block/swim.c | 4 +--- >> hw/block/xen-block.c | 5 +++-- >> hw/core/qdev-properties-system.c | 16 +++++++++------- >> hw/ide/qdev.c | 4 +--- >> hw/scsi/scsi-disk.c | 4 +--- >> 8 files changed, 27 insertions(+), 23 deletions(-) >> >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h >> index 8203d7f6f9..118fbad0b4 100644 >> --- a/include/sysemu/block-backend.h >> +++ b/include/sysemu/block-backend.h >> @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk); >> void blk_iostatus_disable(BlockBackend *blk); >> void blk_iostatus_reset(BlockBackend *blk); >> void blk_iostatus_set_err(BlockBackend *blk, int error); >> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev); >> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp); >> void blk_detach_dev(BlockBackend *blk, DeviceState *dev); >> DeviceState *blk_get_attached_dev(BlockBackend *blk); >> char *blk_get_attached_dev_id(BlockBackend *blk); >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 63ff940ef9..b7be0a4619 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) >> >> /* >> * Attach device model @dev to @blk. >> + * >> + * @blk: Block backend >> + * @dev: Device to attach the block backend to >> + * @errp: pointer to NULL initialized error object >> + * >> * Return 0 on success, -EBUSY when a device model is attached already. >> */ >> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev) >> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp) >> { >> trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev))); >> if (blk->dev) { >> + error_setg(errp, "cannot attach blk '%s' to device '%s'", >> + blk_name(blk), object_get_typename(OBJECT(dev))); >> + error_append_hint(errp, "blk '%s' is already attached by device '%s'\n", >> + blk_name(blk), object_get_typename(OBJECT(blk->dev))); > > I would have a preference for expanding the main error message and not > using a hint. Any hint is completely thrown away when using QMP :-( Hints work best in cases like error message hint suggesting things to try to fix it, in CLI syntax error message rejecting a configuration value hint listing possible values, in CLI syntax Why "in CLI syntax"? Well, we need to use *some* syntax to be useful. Hints have always been phrased for the CLI, simply because the hint feature grew out of my disgust over the loss of lovingly written CLI hints in the conversion to Error. Hints in CLI syntax would be misleading QMP. We never extended QMP to transport hints. Hints may tempt you in a case like error message that is painfully long, because it really tries hard to explain, which is laudable in theory, but sadly illegible in practice; also, interminable sentences, meh, this is an error message, not a Joyce novel to get something like terse error message Explanation that is rather long, because it really tries hard to explain. It can be multiple sentences, and lines are wrapped at a reasonable length. Comes out okay in the CLI, but the explanation is lost in QMP. I don't have a good solution to offer for errors that genuinely need explaining.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 8203d7f6f9..118fbad0b4 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk); void blk_iostatus_disable(BlockBackend *blk); void blk_iostatus_reset(BlockBackend *blk); void blk_iostatus_set_err(BlockBackend *blk, int error); -int blk_attach_dev(BlockBackend *blk, DeviceState *dev); +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp); void blk_detach_dev(BlockBackend *blk, DeviceState *dev); DeviceState *blk_get_attached_dev(BlockBackend *blk); char *blk_get_attached_dev_id(BlockBackend *blk); diff --git a/block/block-backend.c b/block/block-backend.c index 63ff940ef9..b7be0a4619 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) /* * Attach device model @dev to @blk. + * + * @blk: Block backend + * @dev: Device to attach the block backend to + * @errp: pointer to NULL initialized error object + * * Return 0 on success, -EBUSY when a device model is attached already. */ -int blk_attach_dev(BlockBackend *blk, DeviceState *dev) +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp) { trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev))); if (blk->dev) { + error_setg(errp, "cannot attach blk '%s' to device '%s'", + blk_name(blk), object_get_typename(OBJECT(dev))); + error_append_hint(errp, "blk '%s' is already attached by device '%s'\n", + blk_name(blk), object_get_typename(OBJECT(blk->dev))); return -EBUSY; } diff --git a/hw/block/fdc.c b/hw/block/fdc.c index e9ed3eef45..bc39244152 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -519,7 +519,6 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp) FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus); FDrive *drive; bool read_only; - int ret; if (dev->unit == -1) { for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) { @@ -545,8 +544,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp) if (!dev->conf.blk) { /* Anonymous BlockBackend for an empty drive */ dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); - ret = blk_attach_dev(dev->conf.blk, qdev); - assert(ret == 0); + blk_attach_dev(dev->conf.blk, qdev, &error_abort); /* Don't take write permissions on an empty drive to allow attaching a * read-only node later */ diff --git a/hw/block/swim.c b/hw/block/swim.c index 74f56e8f46..2f1ecd0bb2 100644 --- a/hw/block/swim.c +++ b/hw/block/swim.c @@ -159,7 +159,6 @@ static void swim_drive_realize(DeviceState *qdev, Error **errp) SWIMDrive *dev = SWIM_DRIVE(qdev); SWIMBus *bus = SWIM_BUS(qdev->parent_bus); FDrive *drive; - int ret; if (dev->unit == -1) { for (dev->unit = 0; dev->unit < SWIM_MAX_FD; dev->unit++) { @@ -185,8 +184,7 @@ static void swim_drive_realize(DeviceState *qdev, Error **errp) if (!dev->conf.blk) { /* Anonymous BlockBackend for an empty drive */ dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); - ret = blk_attach_dev(dev->conf.blk, qdev); - assert(ret == 0); + blk_attach_dev(dev->conf.blk, qdev, &error_abort); } if (!blkconf_blocksizes(&dev->conf, errp)) { diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index 8a7a3f5452..eb99757faf 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -609,14 +609,15 @@ static void xen_cdrom_realize(XenBlockDevice *blockdev, Error **errp) blockdev->device_type = "cdrom"; if (!conf->blk) { + Error *local_err = NULL; int rc; /* Set up an empty drive */ conf->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); - rc = blk_attach_dev(conf->blk, DEVICE(blockdev)); + rc = blk_attach_dev(conf->blk, DEVICE(blockdev), &local_err); if (!rc) { - error_setg_errno(errp, -rc, "failed to create drive"); + error_propagate_prepend(errp, local_err, "failed to create drive"); return; } } diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 3e4f16fc21..e3a757b1c3 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -136,17 +136,19 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name, object_get_typename(OBJECT(dev)), prop->name, str); goto fail; } - if (blk_attach_dev(blk, dev) < 0) { + if (blk_attach_dev(blk, dev, errp) < 0) { DriveInfo *dinfo = blk_legacy_dinfo(blk); if (dinfo && dinfo->type != IF_NONE) { - error_setg(errp, "Drive '%s' is already in use because " - "it has been automatically connected to another " - "device (did you need 'if=none' in the drive options?)", - str); + error_append_hint(errp, + "Drive '%s' is already in use because it has " + "been automatically connected to another device\n" + "(do you need 'if=none' in the drive options?)\n", + str); } else { - error_setg(errp, "Drive '%s' is already in use by another device", - str); + error_append_hint(errp, + "Drive '%s' is already in use by another device", + str); } goto fail; } diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 27ff1f7f66..9a02eb87f4 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -165,7 +165,6 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp) { IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus); IDEState *s = bus->ifs + dev->unit; - int ret; if (!dev->conf.blk) { if (kind != IDE_CD) { @@ -174,8 +173,7 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp) } else { /* Anonymous BlockBackend for an empty drive */ dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); - ret = blk_attach_dev(dev->conf.blk, &dev->qdev); - assert(ret == 0); + blk_attach_dev(dev->conf.blk, &dev->qdev, &error_abort); } } diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 8ce68a9dd6..92350642c7 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2451,14 +2451,12 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); AioContext *ctx; - int ret; if (!dev->conf.blk) { /* Anonymous BlockBackend for an empty drive. As we put it into * dev->conf, qdev takes care of detaching on unplug. */ dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); - ret = blk_attach_dev(dev->conf.blk, &dev->qdev); - assert(ret == 0); + blk_attach_dev(dev->conf.blk, &dev->qdev, &error_abort); } ctx = blk_get_aio_context(dev->conf.blk);
Let blk_attach_dev() take an Error* object to return helpful information. Adapt the callers. $ qemu-system-arm -M n800 qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card' blk 'sd0' is already attached by device 'omap2-mmc' Drive 'sd0' is already in use because it has been automatically connected to another device (do you need 'if=none' in the drive options?) Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()") --- include/sysemu/block-backend.h | 2 +- block/block-backend.c | 11 ++++++++++- hw/block/fdc.c | 4 +--- hw/block/swim.c | 4 +--- hw/block/xen-block.c | 5 +++-- hw/core/qdev-properties-system.c | 16 +++++++++------- hw/ide/qdev.c | 4 +--- hw/scsi/scsi-disk.c | 4 +--- 8 files changed, 27 insertions(+), 23 deletions(-)