Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error

[Markus Armbruster <armbru@xxxxxxxxxx>]

Daniel P. Berrangé <berrange@xxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.