Re: [Qemu-block] [RFC PATCH 22/41] block: Add BdrvChildRole.get_link_name()

[Max Reitz]

On 13.02.2017 18:22, Kevin Wolf wrote:
> For meaningful error messages in the permission system, we want to allow
> the parent of a BdrvChild to generate some kind of human-readable
> identifier for the link represented by the BdrvChild.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c                   |  8 ++++++++
>  block/block-backend.c     | 20 ++++++++++++++++++++
>  include/block/block_int.h |  4 ++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/block.c b/block.c
> index fd06549..2116542 100644
> --- a/block.c
> +++ b/block.c
> @@ -800,6 +800,13 @@ const BdrvChildRole child_format = {
>      .drained_end     = bdrv_child_cb_drained_end,
>  };
>  
> +static char *bdrv_backing_link_name(BdrvChild *c)
> +{
> +    BlockDriverState *parent = c->opaque;
> +    return g_strdup_printf("backing file link from %s",
> +                           bdrv_get_device_or_node_name(parent));

So in this case it's really about the edge in the BDS graph.

> +}
> +
>  /*
>   * Returns the options and flags that bs->backing should get, based on the
>   * given options and flags for the parent BDS
> @@ -825,6 +832,7 @@ static void bdrv_backing_options(int *child_flags, QDict 
> *child_options,
>  }
>  
>  static const BdrvChildRole child_backing = {
> +    .get_link_name   = bdrv_backing_link_name,
>      .inherit_options = bdrv_backing_options,
>      .drained_begin   = bdrv_child_cb_drained_begin,
>      .drained_end     = bdrv_child_cb_drained_end,
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 0c23add..a314284 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -80,6 +80,7 @@ static const AIOCBInfo block_backend_aiocb_info = {
>  
>  static void drive_info_del(DriveInfo *dinfo);
>  static BlockBackend *bdrv_first_blk(BlockDriverState *bs);
> +static char *blk_get_attached_dev_id(BlockBackend *blk);
>  
>  /* All BlockBackends */
>  static QTAILQ_HEAD(, BlockBackend) block_backends =
> @@ -102,6 +103,24 @@ static void blk_root_drained_end(BdrvChild *child);
>  static void blk_root_change_media(BdrvChild *child, bool load);
>  static void blk_root_resize(BdrvChild *child);
>  
> +static char *blk_root_get_link_name(BdrvChild *child)
> +{
> +    BlockBackend *blk = child->opaque;
> +    char *dev_id;
> +
> +    if (blk->name) {
> +        return g_strdup(blk->name);

This is considering the BB to be an edge in the BDS graph. I find that a
bit weird, and the fact that it makes complete and total sense after
thinking for some time doesn't change the fact that it is weird.

> +    }
> +
> +    dev_id = blk_get_attached_dev_id(blk);
> +    if (*dev_id) {
> +        return dev_id;

But the device model OTOH is definitely not an edge in the BDS graph.

> +    } else {
> +        g_free(dev_id);
> +        return g_strdup("a block device");
> +    }
> +}
> +
>  static const char *blk_root_get_name(BdrvChild *child)
>  {
>      return blk_name(child->opaque);

[...]

Also, I'm curious what you are going to choose as a replacement if the
function pointer is not set. I have a couple of ideas, but most of them
are either not correct or would remove the need for
bdrv_backing_link_name()... Well, I'm curious. :-)

Max

signature.asc
Description: OpenPGP digital signature