Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages

[Markus Armbruster]

Alberto Garcia <address@hidden> writes:

> There are several error messages that identify a BlockDriverState by
> its device name. However those errors can be produced in nodes that
> don't have a device name associated.
>
> In those cases we should use bdrv_get_device_or_node_name() to fall
> back to the node name and produce a more meaningful message.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block.c       | 13 +++++++------
>  block/qcow.c  |  4 ++--
>  block/qcow2.c |  2 +-
>  block/qed.c   |  2 +-
>  block/vdi.c   |  2 +-
>  block/vhdx.c  |  2 +-
>  block/vmdk.c  |  4 ++--
>  block/vpc.c   |  2 +-
>  block/vvfat.c |  3 ++-
>  9 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index af284e3..655d9aa 100644
> --- a/block.c
> +++ b/block.c
> @@ -1225,7 +1225,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>      } else if (backing_hd) {
>          error_setg(&bs->backing_blocker,
>                     "device is used as backing hd of '%s'",
> -                   bdrv_get_device_name(bs));
> +                   bdrv_get_device_or_node_name(bs));
>      }
>  
>      bs->backing_hd = backing_hd;

Before:

* if @bs belongs to a BB with name N, print "backing hd of 'N'"
* else print "backing hd of ''" (not nice, but works)

After: 

* if @bs belongs to a BB with name N, print "backing hd of 'N'" (no change)
* else if @bs has a node name NN, print "backing hd of 'NN'" (improvement)
* else print a null pointer, which crashes on some systems (bug)

If you want your bdrv_get_device_or_node_name() serve as drop-in
replacement for bdrv_get_device_name(), it must not return null.

In my opinion, such a drop-in replacement can only be a stop gap.  We
need to review every error message and figure out how to update it for
node names.  We should've done it back when we added node names.

Your misuse of bdrv_get_device_or_node_name() shows misuse is likely.
As long as that's the case, it needs a function comment.

[...]