Re: [Qemu-devel] [PATCH 08/23] block: Eliminate BlockDriverState member device_name[]

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> The Wednesday 10 Sep 2014 à 10:13:37 (+0200), Markus Armbruster wrote :
>> device_name[] is can become non-empty only in bdrv_new_named() and
>> bdrv_move_feature_fields().  The latter is used only to undo damage
>> done by bdrv_swap().  The former is called only by blk_new_with_bs().
>> Therefore, when a BlockDriverState's device_name[] is non-empty, then
>> it's owned by a BlockBackend.
>> 
>> The converse is also true, because blk_attach_bs() is called only by
>> blk_new_with_bs() so far.
>> 
>> Furthermore, blk_new_with_bs() keeps the two names equal.
>> 
>> Therefore, device_name[] is redundant.  Eliminate it.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block-migration.c         | 12 +++++----
>>  block.c                   | 63 
>> ++++++++++++++---------------------------------
>>  block/block-backend.c     | 12 ++++-----
>>  block/cow.c               |  2 +-
>>  block/mirror.c            |  3 ++-
>>  block/qapi.c              |  6 ++---
>>  block/qcow.c              |  4 +--
>>  block/qcow2.c             |  4 +--
>>  block/qed.c               |  2 +-
>>  block/quorum.c            |  4 +--
>>  block/vdi.c               |  2 +-
>>  block/vhdx.c              |  2 +-
>>  block/vmdk.c              |  4 +--
>>  block/vpc.c               |  2 +-
>>  block/vvfat.c             |  2 +-
>>  blockjob.c                |  3 ++-
>>  include/block/block.h     |  3 +--
>>  include/block/block_int.h |  2 --
>>  18 files changed, 53 insertions(+), 79 deletions(-)
>> 
>> diff --git a/block-migration.c b/block-migration.c
>> index cb3e16c..da30e93 100644
>> --- a/block-migration.c
>> +++ b/block-migration.c
>> @@ -14,7 +14,9 @@
>>   */
>>  
>>  #include "qemu-common.h"
>> -#include "block/block_int.h"
>> +#include "block/block.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/main-loop.h"
>>  #include "hw/hw.h"
>>  #include "qemu/queue.h"
>>  #include "qemu/timer.h"
>> @@ -130,9 +132,9 @@ static void blk_send(QEMUFile *f, BlkMigBlock * blk)
>>                       | flags);
>>  
>>      /* device name */
>> -    len = strlen(blk->bmds->bs->device_name);
>> +    len = strlen(bdrv_get_device_name(blk->bmds->bs));
>>      qemu_put_byte(f, len);
>> -    qemu_put_buffer(f, (uint8_t *)blk->bmds->bs->device_name, len);
>> +    qemu_put_buffer(f, (uint8_t *)bdrv_get_device_name(blk->bmds->bs), len);
>>  
>>      /* if a block is zero we need to flush here since the network
>>       * bandwidth is now a lot higher than the storage device bandwidth.
>> @@ -382,9 +384,9 @@ static void init_blk_migration(QEMUFile *f)
>>  
>>          if (bmds->shared_base) {
>>              DPRINTF("Start migration for %s with shared base image\n",
>> -                    bs->device_name);
>> +                    bdrv_get_device_name(bs));
>>          } else {
>> -            DPRINTF("Start full migration for %s\n", bs->device_name);
>> +            DPRINTF("Start full migration for %s\n", 
>> bdrv_get_device_name(bs));
>>          }
>>  
>>          QSIMPLEQ_INSERT_TAIL(&block_mig_state.bmds_list, bmds, entry);
>> diff --git a/block.c b/block.c
>> index 593d89b..61ea15d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -332,31 +332,6 @@ void bdrv_register(BlockDriver *bdrv)
>>      QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
>>  }
>>  
>> -/* create a new block device (by default it is empty) */
>> -BlockDriverState *bdrv_new_named(const char *device_name, Error **errp)
>> -{
>> -    BlockDriverState *bs;
>> -
>> -    assert(*device_name);
>> -
>> -    if (bdrv_find(device_name)) {
>> -        error_setg(errp, "Device with id '%s' already exists",
>> -                   device_name);
>> -        return NULL;
>> -    }
>> -    if (bdrv_find_node(device_name)) {
>> -        error_setg(errp, "Device with node-name '%s' already exists",
>> -                   device_name);
>> -        return NULL;
>> -    }
>> -
>> -    bs = bdrv_new();
>> -
>> -    pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
>> -
>> -    return bs;
>> -}
>> -
>>  BlockDriverState *bdrv_new(void)
>>  {
>>      BlockDriverState *bs;
>> @@ -1159,7 +1134,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'",
>> -                   bs->device_name);
>> +                   bdrv_get_device_name(bs));
>>      }
>>  
>>      bs->backing_hd = backing_hd;
>> @@ -1533,7 +1508,7 @@ int bdrv_open(BlockDriverState **pbs, const char 
>> *filename,
>>          } else {
>>              error_setg(errp, "Block format '%s' used by device '%s' doesn't 
>> "
>>                         "support the option '%s'", drv->format_name,
>> -                       bs->device_name, entry->key);
>> +                       bdrv_get_device_name(bs), entry->key);
>>          }
>>  
>>          ret = -EINVAL;
>> @@ -1740,7 +1715,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
>> BlockReopenQueue *queue,
>>      if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
>>          reopen_state->flags & BDRV_O_RDWR) {
>>          error_set(errp, QERR_DEVICE_IS_READ_ONLY,
>> -                  reopen_state->bs->device_name);
>> +                  bdrv_get_device_name(reopen_state->bs));
>>          goto error;
>>      }
>>  
>> @@ -1767,7 +1742,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
>> BlockReopenQueue *queue,
>>          /* It is currently mandatory to have a bdrv_reopen_prepare()
>>           * handler for each supported drv. */
>>          error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>> -                  drv->format_name, reopen_state->bs->device_name,
>> +                  drv->format_name, bdrv_get_device_name(reopen_state->bs),
>>                   "reopening of file");
>>          ret = -1;
>>          goto error;
>> @@ -1955,7 +1930,6 @@ void bdrv_drain_all(void)
>>     Also, NULL terminate the device_name to prevent double remove */
>>  void bdrv_make_anon(BlockDriverState *bs)
>>  {
>> -    bs->device_name[0] = '\0';
>>      if (bs->node_name[0] != '\0') {
>>          QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
>>      }
>> @@ -2008,9 +1982,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
>> *bs_dest,
>>      /* job */
>>      bs_dest->job                = bs_src->job;
>>  
>> -    pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
>> -            bs_src->device_name);
>> -
>>      memcpy(bs_dest->op_blockers, bs_src->op_blockers,
>>             sizeof(bs_dest->op_blockers));
>>  }
>> @@ -2023,7 +1994,7 @@ static void bdrv_move_feature_fields(BlockDriverState 
>> *bs_dest,
>>   * This will modify the BlockDriverState fields, and swap contents
>>   * between bs_new and bs_old. Both bs_new and bs_old are modified.
>>   *
>> - * bs_new must be nameless and not attached to a BlockBackend.
>> + * bs_new must not be attached to a BlockBackend.
>>   *
>>   * This function does not create any image files.
>>   */
>> @@ -2042,8 +2013,7 @@ void bdrv_swap(BlockDriverState *bs_new, 
>> BlockDriverState *bs_old)
>>          QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list);
>>      }
>>  
>> -    /* bs_new must be nameless and shouldn't have anything fancy enabled */
>> -    assert(bs_new->device_name[0] == '\0');
>> +    /* bs_new must be unattached and shouldn't have anything fancy enabled 
>> */
>>      assert(!bs_new->blk);
>>      assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>>      assert(bs_new->job == NULL);
>> @@ -2060,8 +2030,7 @@ void bdrv_swap(BlockDriverState *bs_new, 
>> BlockDriverState *bs_old)
>>      bdrv_move_feature_fields(bs_old, bs_new);
>>      bdrv_move_feature_fields(bs_new, &tmp);
>>  
>> -    /* bs_new must remain nameless and unattached */
>> -    assert(bs_new->device_name[0] == '\0');
>> +    /* bs_new must remain unattached */
>>      assert(!bs_new->blk);
>>  
>>      /* Check a few fields that should remain attached to the device */
>> @@ -2089,7 +2058,7 @@ void bdrv_swap(BlockDriverState *bs_new, 
>> BlockDriverState *bs_old)
>>   * This will modify the BlockDriverState fields, and swap contents
>>   * between bs_new and bs_top. Both bs_new and bs_top are modified.
>>   *
>> - * bs_new must be nameless and not attached to a BlockBackend.
>> + * bs_new must not be attached to a BlockBackend.
>>   *
>>   * This function does not create any image files.
>>   */
>> @@ -3799,7 +3768,7 @@ BlockDriverState *bdrv_find(const char *name)
>>      BlockDriverState *bs;
>>  
>>      for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
>> -        if (!strcmp(name, bs->device_name)) {
>> +        if (!strcmp(name, bdrv_get_device_name(bs))) {
>>              return bs;
>>          }
>>      }
>> @@ -3889,9 +3858,10 @@ BlockDriverState *bdrv_next(BlockDriverState *bs)
>>      return blk ? blk_bs(blk) : NULL;
>>  }
>>  
>> -const char *bdrv_get_device_name(BlockDriverState *bs)
>> +const char *bdrv_get_device_name(const BlockDriverState *bs)
>>  {
>> -    return bs->device_name;
>> +    const char *name = bs->blk ? blk_name(bs->blk) : NULL;
>> +    return name ?: "";
>>  }
>
> Why not ?
>
>     return bs->blk ? blk_name(bs->blk) : "";

Sold!

> or 
>
>     if (bs->blk) {
>         return blk_name(bs->blk);
>     }
>
>     return "";
>
> Would it fail to do the job ?
>
> Also we could have made sure that bdrv_get_device_name return either
> a non empty string or NULL.
>
> (We know blk_name will return a non empty string it's asserted)

In current master, bdrv_get_device_name(bs) returns an empty string if
and only if bs is in bdrv_states.

PATCH 06 switches from bdrv_states to blk_backends without changing the
value of bdrv_get_device_name().

Likewise, this patch switches from BDS member device_name to BB member
name without changing the value of bdrv_get_device_name().

That's good, because changing the value would involve checking all the
callers, and this series taxes certainly myself and probably my
reviewers enough already, so I'd rather not do that right now.

> This would need to change a few test all around the code but the final
> code logic would be less convoluted:
> -convert NULL to "" then test for not ""
> would turn into
> -return NULL test for not NULL
>
>>  
>>  int bdrv_get_flags(BlockDriverState *bs)
>> @@ -5253,13 +5223,15 @@ int bdrv_media_changed(BlockDriverState *bs)
>>  void bdrv_eject(BlockDriverState *bs, bool eject_flag)
>>  {
>>      BlockDriver *drv = bs->drv;
>> +    const char *device_name;
>>  
>>      if (drv && drv->bdrv_eject) {
>>          drv->bdrv_eject(bs, eject_flag);
>>      }
>>  
>> -    if (bs->device_name[0] != '\0') {
>> -        qapi_event_send_device_tray_moved(bdrv_get_device_name(bs),
>
>> +    device_name = bdrv_get_device_name(bs);
>> +        qapi_event_send_device_tray_moved(device_name,
>>                                            eject_flag, &error_abort);
>>      }
>>  }
>> @@ -5469,7 +5441,8 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, 
>> BlockOpType op, Error **errp)
>>          blocker = QLIST_FIRST(&bs->op_blockers[op]);
>>          if (errp) {
>>              error_setg(errp, "Device '%s' is busy: %s",
>> -                       bs->device_name, error_get_pretty(blocker->reason));
>> +                       bdrv_get_device_name(bs),
>> +                       error_get_pretty(blocker->reason));
>>          }
>>          return true;
>>      }
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index c0876fa..2f10d6a 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -67,17 +67,17 @@ BlockBackend *blk_new_with_bs(const char *name, Error 
>> **errp)
>>      BlockBackend *blk;
>>      BlockDriverState *bs;
>>  
>> +    if (bdrv_find_node(name)) {
>> +        error_setg(errp, "Device with node-name '%s' already exists", name);
>
> Maybe the message written by the person who contributed the initial code (me) 
> mislead you.
> I think the intent is good but the wording is wrong.
> node-name is associated with a regular BDS. A device cannot have a node name.
>
> Maybe something in the vein of:
>         error_setg(errp, "Device name '%s' would conflict with the node-name 
> of an existing block driver state", name);

I moved the message from block.c.  Comes from Kevin's commit f2d953e.  I
like your wording better.  Kevin, would you mind me changing it in the
same patch, or do you want a separate one?

>> +        return NULL;
>> +    }
>> +
>>      blk = blk_new(name, errp);
>>      if (!blk) {
>>          return NULL;
>>      }
>>  
>> -    bs = bdrv_new_named(name, errp);
>> -    if (!bs) {
>> -        blk_unref(blk);
>> -        return NULL;
>> -    }
>> -
>> +    bs = bdrv_new();
>>      blk_attach_bs(blk, bs);
>>      return blk;
>>  }
[...]