* [PATCH-for-5.2 v2 0/2] block/block-backend: Let blk_attach_dev() provide helpful error
@ 2020-07-16 12:37 Philippe Mathieu-Daudé
2020-07-16 12:37 ` [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev() Philippe Mathieu-Daudé
2020-07-16 12:37 ` [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé
0 siblings, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-16 12:37 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Stefano Stabellini,
Daniel P. Berrangé, Eduardo Habkost, qemu-block,
Paul Durrant, Laurent Vivier, Max Reitz, Paolo Bonzini,
Anthony Perard, xen-devel, John Snow, Philippe Mathieu-Daudé
A pair of patches which helps me debug an issue with block
drive already attached.
Suggestions to correctly/better use the Error API welcome, in
particular in qdev-properties-system::set_drive_helper().
Since v1:
- Rebased after 668f62ec62 ("error: Eliminate error_propagate()")
Philippe Mathieu-Daudé (2):
block/block-backend: Trace blk_attach_dev()
block/block-backend: Let blk_attach_dev() provide helpful error
include/sysemu/block-backend.h | 2 +-
block/block-backend.c | 12 +++++++++++-
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 +---
block/trace-events | 1 +
9 files changed, 29 insertions(+), 23 deletions(-)
--
2.21.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev() 2020-07-16 12:37 [PATCH-for-5.2 v2 0/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé @ 2020-07-16 12:37 ` Philippe Mathieu-Daudé 2020-07-16 12:42 ` Philippe Mathieu-Daudé 2020-07-16 12:37 ` [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé 1 sibling, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-16 12:37 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Fam Zheng, Kevin Wolf, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, qemu-block, Paul Durrant, Laurent Vivier, Max Reitz, Paolo Bonzini, Anthony Perard, xen-devel, John Snow, Philippe Mathieu-Daudé Add a trace event to follow devices attaching block drives: $ qemu-system-arm -M n800 -trace blk_\* 9513@1594040428.738162:blk_attach_dev attaching blk 'sd0' to device 'omap2-mmc' 9513@1594040428.738189:blk_attach_dev attaching blk 'sd0' to device 'sd-card' qemu-system-arm: sd_init failed: blk 'sd0' already attached by device 'sd-card' Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- block/block-backend.c | 1 + block/trace-events | 1 + 2 files changed, 2 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index 0bf0188133..63ff940ef9 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -888,6 +888,7 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) */ int blk_attach_dev(BlockBackend *blk, DeviceState *dev) { + trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev))); if (blk->dev) { return -EBUSY; } diff --git a/block/trace-events b/block/trace-events index dbe76a7613..aa641c2034 100644 --- a/block/trace-events +++ b/block/trace-events @@ -9,6 +9,7 @@ blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p" blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p" +blk_attach_dev(const char *blk_name, const char *dev_name) "attaching blk '%s' to device '%s'" # io.c bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x" -- 2.21.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev() 2020-07-16 12:37 ` [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev() Philippe Mathieu-Daudé @ 2020-07-16 12:42 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-16 12:42 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Fam Zheng, Kevin Wolf, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, qemu-block, Paul Durrant, Laurent Vivier, Max Reitz, xen-devel, Anthony Perard, Paolo Bonzini, John Snow On 7/16/20 2:37 PM, Philippe Mathieu-Daudé wrote: > Add a trace event to follow devices attaching block drives: > > $ qemu-system-arm -M n800 -trace blk_\* > 9513@1594040428.738162:blk_attach_dev attaching blk 'sd0' to device 'omap2-mmc' > 9513@1594040428.738189:blk_attach_dev attaching blk 'sd0' to device 'sd-card' > qemu-system-arm: sd_init failed: blk 'sd0' already attached by device 'sd-card' Oops, the current error is: qemu-system-arm: sd_init failed: Drive 'sd0' is already in use by another device > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > block/block-backend.c | 1 + > block/trace-events | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 0bf0188133..63ff940ef9 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -888,6 +888,7 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm) > */ > int blk_attach_dev(BlockBackend *blk, DeviceState *dev) > { > + trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev))); > if (blk->dev) { > return -EBUSY; > } > diff --git a/block/trace-events b/block/trace-events > index dbe76a7613..aa641c2034 100644 > --- a/block/trace-events > +++ b/block/trace-events > @@ -9,6 +9,7 @@ blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags > blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x" > blk_root_attach(void *child, void *blk, void *bs) "child %p blk %p bs %p" > blk_root_detach(void *child, void *blk, void *bs) "child %p blk %p bs %p" > +blk_attach_dev(const char *blk_name, const char *dev_name) "attaching blk '%s' to device '%s'" > > # io.c > bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x" > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error 2020-07-16 12:37 [PATCH-for-5.2 v2 0/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé 2020-07-16 12:37 ` [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev() Philippe Mathieu-Daudé @ 2020-07-16 12:37 ` Philippe Mathieu-Daudé 2020-07-16 13:04 ` Daniel P. Berrangé 1 sibling, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2020-07-16 12:37 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Fam Zheng, Kevin Wolf, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, qemu-block, Paul Durrant, Laurent Vivier, Max Reitz, Paolo Bonzini, Anthony Perard, xen-devel, John Snow, Philippe Mathieu-Daudé 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))); 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); -- 2.21.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error 2020-07-16 12:37 ` [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé @ 2020-07-16 13:04 ` Daniel P. Berrangé 2020-07-17 5:32 ` Markus Armbruster 0 siblings, 1 reply; 6+ messages in thread From: Daniel P. Berrangé @ 2020-07-16 13:04 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Fam Zheng, Kevin Wolf, Stefano Stabellini, Eduardo Habkost, qemu-block, Paul Durrant, Markus Armbruster, qemu-devel, Paolo Bonzini, Anthony Perard, xen-devel, Max Reitz, John Snow, Laurent Vivier 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 -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error 2020-07-16 13:04 ` Daniel P. Berrangé @ 2020-07-17 5:32 ` Markus Armbruster 0 siblings, 0 replies; 6+ messages in thread From: Markus Armbruster @ 2020-07-17 5:32 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Fam Zheng, Kevin Wolf, Stefano Stabellini, Eduardo Habkost, qemu-block, Paul Durrant, Philippe Mathieu-Daudé, qemu-devel, xen-devel, Anthony Perard, Paolo Bonzini, Max Reitz, John Snow, Laurent Vivier 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-17 5:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-16 12:37 [PATCH-for-5.2 v2 0/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé 2020-07-16 12:37 ` [PATCH-for-5.2 v2 1/2] block/block-backend: Trace blk_attach_dev() Philippe Mathieu-Daudé 2020-07-16 12:42 ` Philippe Mathieu-Daudé 2020-07-16 12:37 ` [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error Philippe Mathieu-Daudé 2020-07-16 13:04 ` Daniel P. Berrangé 2020-07-17 5:32 ` Markus Armbruster
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).