Re: [Qemu-block] [Qemu-devel] [PATCH 1/3] block: Use bdrv_lookup_bs() instead of bdrv_find_node()

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> This is a precursor to making bdrv_find_node() static, and internal
> to block.c
>
> To find a BlockDriverState interface, it can be done via blk_by_name(),
> bdrv_find_node(), and bdrv_lookup_bs().  The latter can take the place
> of the other two, in the instances where we are only concerned with
> the BlockDriverState.
>
> There is no benefit in calling bdrv_find_node() directly.  This patch
> replaces all calls to bdrv_find_node() outside of block.c with
> bdrv_lookup_bs().
>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/block-backend.c   | 2 +-
>  block/mirror.c          | 2 +-
>  block/write-threshold.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 2256551..7026a3f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -67,7 +67,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
>          error_setg(errp, "Device with id '%s' already exists", name);
>          return NULL;
>      }
> -    if (bdrv_find_node(name)) {
> +    if (bdrv_lookup_bs(NULL, name, NULL)) {
>          error_setg(errp,
>                     "Device name '%s' conflicts with an existing node name",
>                     name);

Here, you ignore bdrv_lookup_bs() errors because we actually succeed on
bdrv_lookup_bs() error.  Good.

> diff --git a/block/mirror.c b/block/mirror.c
> index 7e43511..cb3c765 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -644,7 +644,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
>      if (s->replaces) {
>          AioContext *replace_aio_context;
>  
> -        s->to_replace = bdrv_find_node(s->replaces);
> +        s->to_replace = bdrv_lookup_bs(NULL, s->replaces, NULL);
>          if (!s->to_replace) {
>              error_setg(errp, "Node name '%s' not found", s->replaces);
>              return;

Here, you ignore its errors because the caller sets a better one.
However, the callers is better only because bdrv_lookup_bs()'s sucks:
"Cannot find device= or node_name=FOO".  Follow-up patch to improve that
error and use it here?

> diff --git a/block/write-threshold.c b/block/write-threshold.c
> index a53c1f5..908fa7f 100644
> --- a/block/write-threshold.c
> +++ b/block/write-threshold.c
> @@ -110,7 +110,7 @@ void qmp_block_set_write_threshold(const char *node_name,
>      BlockDriverState *bs;
>      AioContext *aio_context;
>  
> -    bs = bdrv_find_node(node_name);
> +    bs = bdrv_lookup_bs(NULL, node_name, NULL);
>      if (!bs) {
>          error_setg(errp, "Device '%s' not found", node_name);
>          return;

Likewise.