Benoît Canet <benoit.ca...@irqsave.net> 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 <arm...@redhat.com>
>> ---
>> 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;
>> }
[...]
