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) : ""; 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) 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); > + 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; > } > diff --git a/block/cow.c b/block/cow.c > index 6ee4833..dda1e17 100644 > --- a/block/cow.c > +++ b/block/cow.c > @@ -84,7 +84,7 @@ static int cow_open(BlockDriverState *bs, QDict *options, > int flags, > snprintf(version, sizeof(version), > "COW version %" PRIu32, cow_header.version); > error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > - bs->device_name, "cow", version); > + bdrv_get_device_name(bs), "cow", version); > ret = -ENOTSUP; > goto fail; > } > diff --git a/block/mirror.c b/block/mirror.c > index 18b18e0..829be2f 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -567,7 +567,8 @@ static void mirror_complete(BlockJob *job, Error **errp) > return; > } > if (!s->synced) { > - error_set(errp, QERR_BLOCK_JOB_NOT_READY, job->bs->device_name); > + error_set(errp, QERR_BLOCK_JOB_NOT_READY, > + bdrv_get_device_name(job->bs)); > return; > } > > diff --git a/block/qapi.c b/block/qapi.c > index 79d1e6a..cc8f711 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -272,7 +272,7 @@ void bdrv_query_info(BlockDriverState *bs, > BlockDriverState *bs0; > ImageInfo **p_image_info; > Error *local_err = NULL; > - info->device = g_strdup(bs->device_name); > + info->device = g_strdup(bdrv_get_device_name(bs)); > info->type = g_strdup("unknown"); > info->locked = bdrv_dev_is_medium_locked(bs); > info->removable = bdrv_dev_has_removable_media(bs); > @@ -327,9 +327,9 @@ static BlockStats *bdrv_query_stats(const > BlockDriverState *bs) > > s = g_malloc0(sizeof(*s)); > > - if (bs->device_name[0]) { > + if (bdrv_get_device_name(bs)[0]) { > s->has_device = true; > - s->device = g_strdup(bs->device_name); > + s->device = g_strdup(bdrv_get_device_name(bs)); > } > > s->stats = g_malloc0(sizeof(*s->stats)); > diff --git a/block/qcow.c b/block/qcow.c > index 67c237f..2d803ac 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -124,7 +124,7 @@ static int qcow_open(BlockDriverState *bs, QDict > *options, int flags, > snprintf(version, sizeof(version), "QCOW version %" PRIu32, > header.version); > error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > - bs->device_name, "qcow", version); > + bdrv_get_device_name(bs), "qcow", version); > ret = -ENOTSUP; > goto fail; > } > @@ -231,7 +231,7 @@ static int qcow_open(BlockDriverState *bs, QDict > *options, int flags, > /* Disable migration when qcow images are used */ > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > - "qcow", bs->device_name, "live migration"); > + "qcow", bdrv_get_device_name(bs), "live migration"); > migrate_add_blocker(s->migration_blocker); > > qemu_co_mutex_init(&s->lock); > diff --git a/block/qcow2.c b/block/qcow2.c > index f9e045f..0ab455e 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -203,8 +203,8 @@ static void GCC_FMT_ATTR(3, 4) > report_unsupported(BlockDriverState *bs, > vsnprintf(msg, sizeof(msg), fmt, ap); > va_end(ap); > > - error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, > "qcow2", > - msg); > + error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > + bdrv_get_device_name(bs), "qcow2", msg); > } > > static void report_unsupported_feature(BlockDriverState *bs, > diff --git a/block/qed.c b/block/qed.c > index ba395af..f20ffb3 100644 > --- a/block/qed.c > +++ b/block/qed.c > @@ -422,7 +422,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict > *options, int flags, > snprintf(buf, sizeof(buf), "%" PRIx64, > s->header.features & ~QED_FEATURE_MASK); > error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > - bs->device_name, "QED", buf); > + bdrv_get_device_name(bs), "QED", buf); > return -ENOTSUP; > } > if (!qed_is_cluster_size_valid(s->header.cluster_size)) { > diff --git a/block/quorum.c b/block/quorum.c > index 093382e..f958269 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -227,8 +227,8 @@ static void quorum_report_bad(QuorumAIOCB *acb, char > *node_name, int ret) > > static void quorum_report_failure(QuorumAIOCB *acb) > { > - const char *reference = acb->common.bs->device_name[0] ? > - acb->common.bs->device_name : > + const char *reference = bdrv_get_device_name(acb->common.bs)[0] ? > + bdrv_get_device_name(acb->common.bs) : > acb->common.bs->node_name; > > qapi_event_send_quorum_failure(reference, acb->sector_num, > diff --git a/block/vdi.c b/block/vdi.c > index 4b10aac..2e6f31c 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -490,7 +490,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, > int flags, > /* Disable migration when vdi images are used */ > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > - "vdi", bs->device_name, "live migration"); > + "vdi", bdrv_get_device_name(bs), "live migration"); > migrate_add_blocker(s->migration_blocker); > > return 0; > diff --git a/block/vhdx.c b/block/vhdx.c > index 87c99fc..8bf1af1 100644 > --- a/block/vhdx.c > +++ b/block/vhdx.c > @@ -1003,7 +1003,7 @@ static int vhdx_open(BlockDriverState *bs, QDict > *options, int flags, > /* Disable migration when VHDX images are used */ > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > - "vhdx", bs->device_name, "live migration"); > + "vhdx", bdrv_get_device_name(bs), "live migration"); > migrate_add_blocker(s->migration_blocker); > > return 0; > diff --git a/block/vmdk.c b/block/vmdk.c > index a1cb911..7bc1474 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -657,7 +657,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, > snprintf(buf, sizeof(buf), "VMDK version %" PRId32, > le32_to_cpu(header.version)); > error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, > - bs->device_name, "vmdk", buf); > + bdrv_get_device_name(bs), "vmdk", buf); > return -ENOTSUP; > } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) { > /* VMware KB 2064959 explains that version 3 added support for > @@ -939,7 +939,7 @@ static int vmdk_open(BlockDriverState *bs, QDict > *options, int flags, > /* Disable migration when VMDK images are used */ > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > - "vmdk", bs->device_name, "live migration"); > + "vmdk", bdrv_get_device_name(bs), "live migration"); > migrate_add_blocker(s->migration_blocker); > g_free(buf); > return 0; > diff --git a/block/vpc.c b/block/vpc.c > index 055efc4..7bb90d5 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -320,7 +320,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, > int flags, > /* Disable migration when VHD images are used */ > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > - "vpc", bs->device_name, "live migration"); > + "vpc", bdrv_get_device_name(bs), "live migration"); > migrate_add_blocker(s->migration_blocker); > > return 0; > diff --git a/block/vvfat.c b/block/vvfat.c > index 6c9fde0..cefe3a4 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -1182,7 +1182,7 @@ static int vvfat_open(BlockDriverState *bs, QDict > *options, int flags, > if (s->qcow) { > error_set(&s->migration_blocker, > QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED, > - "vvfat (rw)", bs->device_name, "live migration"); > + "vvfat (rw)", bdrv_get_device_name(bs), "live migration"); > migrate_add_blocker(s->migration_blocker); > } > > diff --git a/blockjob.c b/blockjob.c > index 0689fdd..3af0f6c 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -107,7 +107,8 @@ void block_job_set_speed(BlockJob *job, int64_t speed, > Error **errp) > void block_job_complete(BlockJob *job, Error **errp) > { > if (job->paused || job->cancelled || !job->driver->complete) { > - error_set(errp, QERR_BLOCK_JOB_NOT_READY, job->bs->device_name); > + error_set(errp, QERR_BLOCK_JOB_NOT_READY, > + bdrv_get_device_name(job->bs)); > return; > } > > diff --git a/include/block/block.h b/include/block/block.h > index 8cf9ea3..fb4fff9 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -203,7 +203,6 @@ BlockDriver *bdrv_find_whitelisted_format(const char > *format_name, > int bdrv_create(BlockDriver *drv, const char* filename, > QemuOpts *opts, Error **errp); > int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); > -BlockDriverState *bdrv_new_named(const char *device_name, Error **errp); > BlockDriverState *bdrv_new(void); > void bdrv_make_anon(BlockDriverState *bs); > void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); > @@ -415,7 +414,7 @@ int bdrv_set_key(BlockDriverState *bs, const char *key); > int bdrv_query_missing_keys(void); > void bdrv_iterate_format(void (*it)(void *opaque, const char *name), > void *opaque); > -const char *bdrv_get_device_name(BlockDriverState *bs); > +const char *bdrv_get_device_name(const BlockDriverState *bs); > int bdrv_get_flags(BlockDriverState *bs); > int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, > const uint8_t *buf, int nb_sectors); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index d5de08b..08ccb27 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -394,8 +394,6 @@ struct BlockDriverState { > char node_name[32]; > /* element of the list of named nodes building the graph */ > QTAILQ_ENTRY(BlockDriverState) node_list; > - /* Device name is the name associated with the "drive" the guest sees */ > - char device_name[32]; > QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; > int refcnt; > > -- > 1.9.3 > >