* Re: [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-11-07 15:01 ` [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
@ 2013-11-07 20:23 ` Eric Blake
2013-11-07 21:09 ` Benoît Canet
2013-11-08 9:51 ` Kevin Wolf
2013-11-07 20:54 ` Jeff Cody
2013-11-08 8:19 ` Fam Zheng
2 siblings, 2 replies; 12+ messages in thread
From: Eric Blake @ 2013-11-07 20:23 UTC (permalink / raw)
To: Benoît Canet, qemu-devel; +Cc: kwolf, famz, armbru, stefanha
[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]
On 11/07/2013 08:01 AM, Benoît Canet wrote:
> Add the minimum of code to prepare the followings patches.
>
> If no node_name is provided to bdrv_new the bs->node_name is set to "undefined".
> This will allow to have some default string to communicate in QMP and HMP.
> This also make "undefined" a reserved string for bs->node_name.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> + /* if node name is given store it in bs and insert bs in the graph bs list
> + * note: undefined is a reserved node name
> + */
> + if (node_name &&
> + node_name[0] != '\0' &&
> + strcmp(node_name, "undefined")) {
> + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> + /* else set the bs node name to undefined for QMP and HMP */
> + } else {
> + sprintf(bs->node_name, "undefined");
strcpy() is more efficient than sprintf() with no % in the format string.
>
> +/* This function is to find a node in the bs graph */
> +BlockDriverState *bdrv_find_node(const char *node_name)
> +{
> + BlockDriverState *bs;
> +
Should this function assert that node_name is not "undefined"?
> +++ b/include/block/block_int.h
> @@ -297,11 +297,18 @@ struct BlockDriverState {
> BlockdevOnError on_read_error, on_write_error;
> bool iostatus_enabled;
> BlockDeviceIoStatus iostatus;
> +
> + /* the following member give a name to every node on the BlockDriverState
s/give/gives/
> + * graph.
> + */
> + char node_name[32];
> + QTAILQ_ENTRY(BlockDriverState) node_list;
> + /* Device name is the name associated with the "drive" the guest see */
s/see/sees/
> char device_name[32];
> + QTAILQ_ENTRY(BlockDriverState) device_list;
Maybe I missed it, but is there code that ensures there are no duplicate
node names (other than the magic "undefined")?
Seems like it's moving in the right direction, although I'm not sure
it's worth applying this until we know the qapi for working with node
names makes sense.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-11-07 20:23 ` Eric Blake
@ 2013-11-07 21:09 ` Benoît Canet
2013-11-08 9:51 ` Kevin Wolf
1 sibling, 0 replies; 12+ messages in thread
From: Benoît Canet @ 2013-11-07 21:09 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, famz, qemu-devel, stefanha, armbru
Le Thursday 07 Nov 2013 à 13:23:43 (-0700), Eric Blake a écrit :
> On 11/07/2013 08:01 AM, Benoît Canet wrote:
> > Add the minimum of code to prepare the followings patches.
> >
> > If no node_name is provided to bdrv_new the bs->node_name is set to "undefined".
> > This will allow to have some default string to communicate in QMP and HMP.
> > This also make "undefined" a reserved string for bs->node_name.
> >
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
>
> > + /* if node name is given store it in bs and insert bs in the graph bs list
> > + * note: undefined is a reserved node name
> > + */
> > + if (node_name &&
> > + node_name[0] != '\0' &&
> > + strcmp(node_name, "undefined")) {
> > + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> > + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> > + /* else set the bs node name to undefined for QMP and HMP */
> > + } else {
> > + sprintf(bs->node_name, "undefined");
>
> strcpy() is more efficient than sprintf() with no % in the format string.
>
>
> >
> > +/* This function is to find a node in the bs graph */
> > +BlockDriverState *bdrv_find_node(const char *node_name)
> > +{
> > + BlockDriverState *bs;
> > +
>
> Should this function assert that node_name is not "undefined"?
>
>
> > +++ b/include/block/block_int.h
> > @@ -297,11 +297,18 @@ struct BlockDriverState {
> > BlockdevOnError on_read_error, on_write_error;
> > bool iostatus_enabled;
> > BlockDeviceIoStatus iostatus;
> > +
> > + /* the following member give a name to every node on the BlockDriverState
>
> s/give/gives/
>
> > + * graph.
> > + */
> > + char node_name[32];
> > + QTAILQ_ENTRY(BlockDriverState) node_list;
> > + /* Device name is the name associated with the "drive" the guest see */
>
> s/see/sees/
>
> > char device_name[32];
> > + QTAILQ_ENTRY(BlockDriverState) device_list;
>
> Maybe I missed it, but is there code that ensures there are no duplicate
> node names (other than the magic "undefined")?
>
Your are right I will add some checkings.
> Seems like it's moving in the right direction, although I'm not sure
> it's worth applying this until we know the qapi for working with node
> names makes sense.
I am not seeking for fast commit of these patch, I am only trying to avoid
posting a long series at once.
Best regards
Benoît
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-11-07 20:23 ` Eric Blake
2013-11-07 21:09 ` Benoît Canet
@ 2013-11-08 9:51 ` Kevin Wolf
1 sibling, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2013-11-08 9:51 UTC (permalink / raw)
To: Eric Blake; +Cc: armbru, famz, Benoît Canet, stefanha, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]
Am 07.11.2013 um 21:23 hat Eric Blake geschrieben:
> On 11/07/2013 08:01 AM, Benoît Canet wrote:
> > Add the minimum of code to prepare the followings patches.
> >
> > If no node_name is provided to bdrv_new the bs->node_name is set to "undefined".
> > This will allow to have some default string to communicate in QMP and HMP.
> > This also make "undefined" a reserved string for bs->node_name.
> >
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
>
> > + /* if node name is given store it in bs and insert bs in the graph bs list
> > + * note: undefined is a reserved node name
> > + */
> > + if (node_name &&
> > + node_name[0] != '\0' &&
> > + strcmp(node_name, "undefined")) {
> > + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> > + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> > + /* else set the bs node name to undefined for QMP and HMP */
> > + } else {
> > + sprintf(bs->node_name, "undefined");
>
> strcpy() is more efficient than sprintf() with no % in the format string.
pstrcpy() please, even if the string is currently clearly short enough
for the buffer size.
Or well, get rid of the "undefined" string in favour of NULL or "", then
you don't even have to think about this. The magic "undefined" value is
ugly anyway.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-11-07 15:01 ` [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
2013-11-07 20:23 ` Eric Blake
@ 2013-11-07 20:54 ` Jeff Cody
2013-11-07 21:11 ` Benoît Canet
2013-11-08 8:19 ` Fam Zheng
2 siblings, 1 reply; 12+ messages in thread
From: Jeff Cody @ 2013-11-07 20:54 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, famz, qemu-devel, stefanha, armbru
On Thu, Nov 07, 2013 at 04:01:42PM +0100, Benoît Canet wrote:
> Add the minimum of code to prepare the followings patches.
>
> If no node_name is provided to bdrv_new the bs->node_name is set to "undefined".
> This will allow to have some default string to communicate in QMP and HMP.
> This also make "undefined" a reserved string for bs->node_name.
Hi Benoît,
Is it necessary to have a reserved string, or would an empty
null-terminated string be able to implicitly denote the name as
undefined?
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 70 +++++++++++++++++++++++++++++++++++------------
> block/blkverify.c | 2 +-
> block/iscsi.c | 2 +-
> block/vmdk.c | 2 +-
> block/vvfat.c | 4 +--
> blockdev.c | 8 +++---
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 3 +-
> include/block/block_int.h | 9 +++++-
> qemu-img.c | 6 ++--
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 12 files changed, 77 insertions(+), 35 deletions(-)
>
> diff --git a/block.c b/block.c
> index fd05a80..230e71a 100644
> --- a/block.c
> +++ b/block.c
> @@ -89,6 +89,9 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
> QTAILQ_HEAD_INITIALIZER(bdrv_states);
>
> +static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> + QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
> +
> static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> QLIST_HEAD_INITIALIZER(bdrv_drivers);
>
> @@ -318,14 +321,26 @@ void bdrv_register(BlockDriver *bdrv)
> }
>
> /* create a new block device (by default it is empty) */
> -BlockDriverState *bdrv_new(const char *device_name)
> +BlockDriverState *bdrv_new(const char *device_name, const char *node_name)
> {
> BlockDriverState *bs;
>
> bs = g_malloc0(sizeof(BlockDriverState));
> pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> if (device_name[0] != '\0') {
> - QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> + QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
> + }
> + /* if node name is given store it in bs and insert bs in the graph bs list
> + * note: undefined is a reserved node name
> + */
> + if (node_name &&
> + node_name[0] != '\0' &&
> + strcmp(node_name, "undefined")) {
> + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> + /* else set the bs node name to undefined for QMP and HMP */
> + } else {
> + sprintf(bs->node_name, "undefined");
> }
> bdrv_iostatus_disable(bs);
> notifier_list_init(&bs->close_notifiers);
> @@ -870,7 +885,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> options = qdict_new();
> }
>
> - bs = bdrv_new("");
> + bs = bdrv_new("", NULL);
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -992,7 +1007,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> sizeof(backing_filename));
> }
>
> - bs->backing_hd = bdrv_new("");
> + bs->backing_hd = bdrv_new("", NULL);
>
> if (bs->backing_format[0] != '\0') {
> back_drv = bdrv_find_format(bs->backing_format);
> @@ -1062,7 +1077,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> instead of opening 'filename' directly */
>
> /* if there is a backing file, use it */
> - bs1 = bdrv_new("");
> + bs1 = bdrv_new("", NULL);
> ret = bdrv_open(bs1, filename, NULL, 0, drv, &local_err);
> if (ret < 0) {
> bdrv_unref(bs1);
> @@ -1495,7 +1510,7 @@ void bdrv_close_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_close(bs);
> }
> }
> @@ -1524,7 +1539,7 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
> static bool bdrv_requests_pending_all(void)
> {
> BlockDriverState *bs;
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bdrv_requests_pending(bs)) {
> return true;
> }
> @@ -1554,7 +1569,7 @@ void bdrv_drain_all(void)
> /* FIXME: We do not have timer support here, so this is effectively
> * a busy wait.
> */
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bdrv_start_throttled_reqs(bs)) {
> busy = true;
> }
> @@ -1570,7 +1585,7 @@ void bdrv_drain_all(void)
> void bdrv_make_anon(BlockDriverState *bs)
> {
> if (bs->device_name[0] != '\0') {
> - QTAILQ_REMOVE(&bdrv_states, bs, list);
> + QTAILQ_REMOVE(&bdrv_states, bs, device_list);
> }
> bs->device_name[0] = '\0';
Do you need to do anything here to remove the BDS from your graph list?
e.g. QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list)
> }
> @@ -1626,7 +1641,12 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
> /* keep the same entry in bdrv_states */
> pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> bs_src->device_name);
> - bs_dest->list = bs_src->list;
> + bs_dest->device_list = bs_src->device_list;
> +
> + /* keep the same entry in graph_bdrv_states */
> + pstrcpy(bs_dest->node_name, sizeof(bs_dest->node_name),
> + bs_src->node_name);
> + bs_dest->node_list = bs_src->node_list;
> }
>
> /*
> @@ -1950,7 +1970,7 @@ int bdrv_commit_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bs->drv && bs->backing_hd) {
> int ret = bdrv_commit(bs);
> if (ret < 0) {
> @@ -3017,11 +3037,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
> }
> }
>
> +/* This function is to find block backend bs */
> BlockDriverState *bdrv_find(const char *name)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (!strcmp(name, bs->device_name)) {
> return bs;
> }
> @@ -3029,19 +3050,32 @@ BlockDriverState *bdrv_find(const char *name)
> return NULL;
> }
>
> +/* This function is to find a node in the bs graph */
> +BlockDriverState *bdrv_find_node(const char *node_name)
> +{
> + BlockDriverState *bs;
> +
> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> + if (!strcmp(node_name, bs->node_name)) {
> + return bs;
> + }
> + }
> + return NULL;
> +}
> +
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> return QTAILQ_FIRST(&bdrv_states);
> }
> - return QTAILQ_NEXT(bs, list);
> + return QTAILQ_NEXT(bs, device_list);
> }
>
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> it(opaque, bs);
> }
> }
> @@ -3061,7 +3095,7 @@ int bdrv_flush_all(void)
> BlockDriverState *bs;
> int result = 0;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> int ret = bdrv_flush(bs);
> if (ret < 0 && !result) {
> result = ret;
> @@ -4127,7 +4161,7 @@ void bdrv_invalidate_cache_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_invalidate_cache(bs);
> }
> }
> @@ -4136,7 +4170,7 @@ void bdrv_clear_incoming_migration_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
> }
> }
> @@ -4582,7 +4616,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> back_flags =
> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>
> - bs = bdrv_new("");
> + bs = bdrv_new("", NULL);
>
> ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> backing_drv, &local_err);
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 55819a0..674b6a5 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -155,7 +155,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> goto fail;
> }
>
> - s->test_file = bdrv_new("");
> + s->test_file = bdrv_new("", NULL);
> ret = bdrv_open(s->test_file, filename, NULL, flags, NULL, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a2a961e..5031593 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1461,7 +1461,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options,
> IscsiLun *iscsilun = NULL;
> QDict *bs_options;
>
> - bs = bdrv_new("");
> + bs = bdrv_new("", NULL);
>
> /* Read out options */
> while (options && options->name) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 32ec8b77..97801c2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1672,7 +1672,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
> return -ENOTSUP;
> }
> if (backing_file) {
> - BlockDriverState *bs = bdrv_new("");
> + BlockDriverState *bs = bdrv_new("", NULL);
> ret = bdrv_open(bs, backing_file, NULL, 0, NULL, errp);
> if (ret != 0) {
> bdrv_unref(bs);
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 3ddaa0b..a8b6011 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2935,7 +2935,7 @@ static int enable_write_target(BDRVVVFATState *s)
> goto err;
> }
>
> - s->qcow = bdrv_new("");
> + s->qcow = bdrv_new("", NULL);
>
> ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
> @@ -2951,7 +2951,7 @@ static int enable_write_target(BDRVVVFATState *s)
> unlink(s->qcow_filename);
> #endif
>
> - s->bs->backing_hd = bdrv_new("");
> + s->bs->backing_hd = bdrv_new("", NULL);
> s->bs->backing_hd->drv = &vvfat_write_target;
> s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
> *(void**)s->bs->backing_hd->opaque = s;
> diff --git a/blockdev.c b/blockdev.c
> index b260477..ac47413 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -469,7 +469,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> /* init */
> dinfo = g_malloc0(sizeof(*dinfo));
> dinfo->id = g_strdup(qemu_opts_id(opts));
> - dinfo->bdrv = bdrv_new(dinfo->id);
> + dinfo->bdrv = bdrv_new(dinfo->id, NULL);
> dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> dinfo->bdrv->read_only = ro;
> dinfo->type = type;
> @@ -1254,7 +1254,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> }
>
> /* We will manually add the backing_hd field to the bs later */
> - state->new_bs = bdrv_new("");
> + state->new_bs = bdrv_new("", NULL);
> /* TODO Inherit bs->options or only take explicit options with an
> * extended QMP command? */
> ret = bdrv_open(state->new_bs, new_image_file, NULL,
> @@ -1921,7 +1921,7 @@ void qmp_drive_backup(const char *device, const char *target,
> return;
> }
>
> - target_bs = bdrv_new("");
> + target_bs = bdrv_new("", NULL);
> ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
> if (ret < 0) {
> bdrv_unref(target_bs);
> @@ -2055,7 +2055,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> /* Mirroring takes care of copy-on-write using the source's backing
> * file.
> */
> - target_bs = bdrv_new("");
> + target_bs = bdrv_new("", NULL);
> ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> &local_err);
> if (ret < 0) {
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 098f6c6..d89e025 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -808,7 +808,7 @@ static int blk_connect(struct XenDevice *xendev)
> if (!blkdev->dinfo) {
> /* setup via xenbus -> create new block driver instance */
> xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
> - blkdev->bs = bdrv_new(blkdev->dev);
> + blkdev->bs = bdrv_new(blkdev->dev, NULL);
> if (blkdev->bs) {
> Error *local_err = NULL;
> BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
> diff --git a/include/block/block.h b/include/block/block.h
> index 3560deb..2d27bd9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -149,7 +149,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> QEMUOptionParameter *options, Error **errp);
> int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> Error **errp);
> -BlockDriverState *bdrv_new(const char *device_name);
> +BlockDriverState *bdrv_new(const char *device_name, const char *node_name);
> void bdrv_make_anon(BlockDriverState *bs);
> void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> @@ -339,6 +339,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
> void bdrv_eject(BlockDriverState *bs, bool eject_flag);
> const char *bdrv_get_format_name(BlockDriverState *bs);
> BlockDriverState *bdrv_find(const char *name);
> +BlockDriverState *bdrv_find_node(const char *node_name);
> BlockDriverState *bdrv_next(BlockDriverState *bs);
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> void *opaque);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a48731d..9e44136 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -297,11 +297,18 @@ struct BlockDriverState {
> BlockdevOnError on_read_error, on_write_error;
> bool iostatus_enabled;
> BlockDeviceIoStatus iostatus;
> +
> + /* the following member give a name to every node on the BlockDriverState
> + * graph.
> + */
> + char node_name[32];
> + QTAILQ_ENTRY(BlockDriverState) node_list;
> + /* Device name is the name associated with the "drive" the guest see */
> char device_name[32];
> + QTAILQ_ENTRY(BlockDriverState) device_list;
> HBitmap *dirty_bitmap;
> int refcnt;
> int in_use; /* users other than guest access, eg. block migration */
> - QTAILQ_ENTRY(BlockDriverState) list;
>
> QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 926f0a0..215b7b2 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -269,7 +269,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - bs = bdrv_new("image");
> + bs = bdrv_new("image", NULL);
>
> if (fmt) {
> drv = bdrv_find_format(fmt);
> @@ -2225,7 +2225,7 @@ static int img_rebase(int argc, char **argv)
> } else {
> char backing_name[1024];
>
> - bs_old_backing = bdrv_new("old_backing");
> + bs_old_backing = bdrv_new("old_backing", NULL);
> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> old_backing_drv, &local_err);
> @@ -2236,7 +2236,7 @@ static int img_rebase(int argc, char **argv)
> goto out;
> }
> if (out_baseimg[0]) {
> - bs_new_backing = bdrv_new("new_backing");
> + bs_new_backing = bdrv_new("new_backing", NULL);
> ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> new_backing_drv, &local_err);
> if (ret) {
> diff --git a/qemu-io.c b/qemu-io.c
> index 3b3340a..3e1ea88 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
> return 1;
> }
> } else {
> - qemuio_bs = bdrv_new("hda");
> + qemuio_bs = bdrv_new("hda", NULL);
>
> if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index c26c98e..35ef57c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -572,7 +572,7 @@ int main(int argc, char **argv)
> drv = NULL;
> }
>
> - bs = bdrv_new("hda");
> + bs = bdrv_new("hda", NULL);
> srcpath = argv[optind];
> ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
> if (ret < 0) {
> --
> 1.8.3.2
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-11-07 20:54 ` Jeff Cody
@ 2013-11-07 21:11 ` Benoît Canet
0 siblings, 0 replies; 12+ messages in thread
From: Benoît Canet @ 2013-11-07 21:11 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, famz, qemu-devel, stefanha, armbru
Le Thursday 07 Nov 2013 à 15:54:09 (-0500), Jeff Cody a écrit :
> On Thu, Nov 07, 2013 at 04:01:42PM +0100, Benoît Canet wrote:
> > Add the minimum of code to prepare the followings patches.
> >
> > If no node_name is provided to bdrv_new the bs->node_name is set to "undefined".
> > This will allow to have some default string to communicate in QMP and HMP.
> > This also make "undefined" a reserved string for bs->node_name.
>
> Hi Benoît,
>
> Is it necessary to have a reserved string, or would an empty
> null-terminated string be able to implicitly denote the name as
> undefined?
This would works too and just report the "undefined" logic to hmp printing code.
I'll do this.
>
> >
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> > block.c | 70 +++++++++++++++++++++++++++++++++++------------
> > block/blkverify.c | 2 +-
> > block/iscsi.c | 2 +-
> > block/vmdk.c | 2 +-
> > block/vvfat.c | 4 +--
> > blockdev.c | 8 +++---
> > hw/block/xen_disk.c | 2 +-
> > include/block/block.h | 3 +-
> > include/block/block_int.h | 9 +++++-
> > qemu-img.c | 6 ++--
> > qemu-io.c | 2 +-
> > qemu-nbd.c | 2 +-
> > 12 files changed, 77 insertions(+), 35 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index fd05a80..230e71a 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -89,6 +89,9 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> > static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
> > QTAILQ_HEAD_INITIALIZER(bdrv_states);
> >
> > +static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> > + QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
> > +
> > static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> > QLIST_HEAD_INITIALIZER(bdrv_drivers);
> >
> > @@ -318,14 +321,26 @@ void bdrv_register(BlockDriver *bdrv)
> > }
> >
> > /* create a new block device (by default it is empty) */
> > -BlockDriverState *bdrv_new(const char *device_name)
> > +BlockDriverState *bdrv_new(const char *device_name, const char *node_name)
> > {
> > BlockDriverState *bs;
> >
> > bs = g_malloc0(sizeof(BlockDriverState));
> > pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> > if (device_name[0] != '\0') {
> > - QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> > + QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
> > + }
> > + /* if node name is given store it in bs and insert bs in the graph bs list
> > + * note: undefined is a reserved node name
> > + */
> > + if (node_name &&
> > + node_name[0] != '\0' &&
> > + strcmp(node_name, "undefined")) {
> > + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> > + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> > + /* else set the bs node name to undefined for QMP and HMP */
> > + } else {
> > + sprintf(bs->node_name, "undefined");
> > }
> > bdrv_iostatus_disable(bs);
> > notifier_list_init(&bs->close_notifiers);
> > @@ -870,7 +885,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> > options = qdict_new();
> > }
> >
> > - bs = bdrv_new("");
> > + bs = bdrv_new("", NULL);
> > bs->options = options;
> > options = qdict_clone_shallow(options);
> >
> > @@ -992,7 +1007,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> > sizeof(backing_filename));
> > }
> >
> > - bs->backing_hd = bdrv_new("");
> > + bs->backing_hd = bdrv_new("", NULL);
> >
> > if (bs->backing_format[0] != '\0') {
> > back_drv = bdrv_find_format(bs->backing_format);
> > @@ -1062,7 +1077,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> > instead of opening 'filename' directly */
> >
> > /* if there is a backing file, use it */
> > - bs1 = bdrv_new("");
> > + bs1 = bdrv_new("", NULL);
> > ret = bdrv_open(bs1, filename, NULL, 0, drv, &local_err);
> > if (ret < 0) {
> > bdrv_unref(bs1);
> > @@ -1495,7 +1510,7 @@ void bdrv_close_all(void)
> > {
> > BlockDriverState *bs;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > bdrv_close(bs);
> > }
> > }
> > @@ -1524,7 +1539,7 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
> > static bool bdrv_requests_pending_all(void)
> > {
> > BlockDriverState *bs;
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > if (bdrv_requests_pending(bs)) {
> > return true;
> > }
> > @@ -1554,7 +1569,7 @@ void bdrv_drain_all(void)
> > /* FIXME: We do not have timer support here, so this is effectively
> > * a busy wait.
> > */
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > if (bdrv_start_throttled_reqs(bs)) {
> > busy = true;
> > }
> > @@ -1570,7 +1585,7 @@ void bdrv_drain_all(void)
> > void bdrv_make_anon(BlockDriverState *bs)
> > {
> > if (bs->device_name[0] != '\0') {
> > - QTAILQ_REMOVE(&bdrv_states, bs, list);
> > + QTAILQ_REMOVE(&bdrv_states, bs, device_list);
> > }
> > bs->device_name[0] = '\0';
>
> Do you need to do anything here to remove the BDS from your graph list?
> e.g. QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list)
Right.
>
> > }
> > @@ -1626,7 +1641,12 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
> > /* keep the same entry in bdrv_states */
> > pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> > bs_src->device_name);
> > - bs_dest->list = bs_src->list;
> > + bs_dest->device_list = bs_src->device_list;
> > +
> > + /* keep the same entry in graph_bdrv_states */
> > + pstrcpy(bs_dest->node_name, sizeof(bs_dest->node_name),
> > + bs_src->node_name);
> > + bs_dest->node_list = bs_src->node_list;
> > }
> >
> > /*
> > @@ -1950,7 +1970,7 @@ int bdrv_commit_all(void)
> > {
> > BlockDriverState *bs;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > if (bs->drv && bs->backing_hd) {
> > int ret = bdrv_commit(bs);
> > if (ret < 0) {
> > @@ -3017,11 +3037,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
> > }
> > }
> >
> > +/* This function is to find block backend bs */
> > BlockDriverState *bdrv_find(const char *name)
> > {
> > BlockDriverState *bs;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > if (!strcmp(name, bs->device_name)) {
> > return bs;
> > }
> > @@ -3029,19 +3050,32 @@ BlockDriverState *bdrv_find(const char *name)
> > return NULL;
> > }
> >
> > +/* This function is to find a node in the bs graph */
> > +BlockDriverState *bdrv_find_node(const char *node_name)
> > +{
> > + BlockDriverState *bs;
> > +
> > + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> > + if (!strcmp(node_name, bs->node_name)) {
> > + return bs;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > BlockDriverState *bdrv_next(BlockDriverState *bs)
> > {
> > if (!bs) {
> > return QTAILQ_FIRST(&bdrv_states);
> > }
> > - return QTAILQ_NEXT(bs, list);
> > + return QTAILQ_NEXT(bs, device_list);
> > }
> >
> > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
> > {
> > BlockDriverState *bs;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > it(opaque, bs);
> > }
> > }
> > @@ -3061,7 +3095,7 @@ int bdrv_flush_all(void)
> > BlockDriverState *bs;
> > int result = 0;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > int ret = bdrv_flush(bs);
> > if (ret < 0 && !result) {
> > result = ret;
> > @@ -4127,7 +4161,7 @@ void bdrv_invalidate_cache_all(void)
> > {
> > BlockDriverState *bs;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > bdrv_invalidate_cache(bs);
> > }
> > }
> > @@ -4136,7 +4170,7 @@ void bdrv_clear_incoming_migration_all(void)
> > {
> > BlockDriverState *bs;
> >
> > - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
> > }
> > }
> > @@ -4582,7 +4616,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> > back_flags =
> > flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> >
> > - bs = bdrv_new("");
> > + bs = bdrv_new("", NULL);
> >
> > ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> > backing_drv, &local_err);
> > diff --git a/block/blkverify.c b/block/blkverify.c
> > index 55819a0..674b6a5 100644
> > --- a/block/blkverify.c
> > +++ b/block/blkverify.c
> > @@ -155,7 +155,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> > goto fail;
> > }
> >
> > - s->test_file = bdrv_new("");
> > + s->test_file = bdrv_new("", NULL);
> > ret = bdrv_open(s->test_file, filename, NULL, flags, NULL, &local_err);
> > if (ret < 0) {
> > error_propagate(errp, local_err);
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index a2a961e..5031593 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -1461,7 +1461,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options,
> > IscsiLun *iscsilun = NULL;
> > QDict *bs_options;
> >
> > - bs = bdrv_new("");
> > + bs = bdrv_new("", NULL);
> >
> > /* Read out options */
> > while (options && options->name) {
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index 32ec8b77..97801c2 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1672,7 +1672,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
> > return -ENOTSUP;
> > }
> > if (backing_file) {
> > - BlockDriverState *bs = bdrv_new("");
> > + BlockDriverState *bs = bdrv_new("", NULL);
> > ret = bdrv_open(bs, backing_file, NULL, 0, NULL, errp);
> > if (ret != 0) {
> > bdrv_unref(bs);
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 3ddaa0b..a8b6011 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -2935,7 +2935,7 @@ static int enable_write_target(BDRVVVFATState *s)
> > goto err;
> > }
> >
> > - s->qcow = bdrv_new("");
> > + s->qcow = bdrv_new("", NULL);
> >
> > ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
> > BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
> > @@ -2951,7 +2951,7 @@ static int enable_write_target(BDRVVVFATState *s)
> > unlink(s->qcow_filename);
> > #endif
> >
> > - s->bs->backing_hd = bdrv_new("");
> > + s->bs->backing_hd = bdrv_new("", NULL);
> > s->bs->backing_hd->drv = &vvfat_write_target;
> > s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
> > *(void**)s->bs->backing_hd->opaque = s;
> > diff --git a/blockdev.c b/blockdev.c
> > index b260477..ac47413 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -469,7 +469,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> > /* init */
> > dinfo = g_malloc0(sizeof(*dinfo));
> > dinfo->id = g_strdup(qemu_opts_id(opts));
> > - dinfo->bdrv = bdrv_new(dinfo->id);
> > + dinfo->bdrv = bdrv_new(dinfo->id, NULL);
> > dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> > dinfo->bdrv->read_only = ro;
> > dinfo->type = type;
> > @@ -1254,7 +1254,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> > }
> >
> > /* We will manually add the backing_hd field to the bs later */
> > - state->new_bs = bdrv_new("");
> > + state->new_bs = bdrv_new("", NULL);
> > /* TODO Inherit bs->options or only take explicit options with an
> > * extended QMP command? */
> > ret = bdrv_open(state->new_bs, new_image_file, NULL,
> > @@ -1921,7 +1921,7 @@ void qmp_drive_backup(const char *device, const char *target,
> > return;
> > }
> >
> > - target_bs = bdrv_new("");
> > + target_bs = bdrv_new("", NULL);
> > ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
> > if (ret < 0) {
> > bdrv_unref(target_bs);
> > @@ -2055,7 +2055,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> > /* Mirroring takes care of copy-on-write using the source's backing
> > * file.
> > */
> > - target_bs = bdrv_new("");
> > + target_bs = bdrv_new("", NULL);
> > ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> > &local_err);
> > if (ret < 0) {
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 098f6c6..d89e025 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -808,7 +808,7 @@ static int blk_connect(struct XenDevice *xendev)
> > if (!blkdev->dinfo) {
> > /* setup via xenbus -> create new block driver instance */
> > xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
> > - blkdev->bs = bdrv_new(blkdev->dev);
> > + blkdev->bs = bdrv_new(blkdev->dev, NULL);
> > if (blkdev->bs) {
> > Error *local_err = NULL;
> > BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 3560deb..2d27bd9 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -149,7 +149,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> > QEMUOptionParameter *options, Error **errp);
> > int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> > Error **errp);
> > -BlockDriverState *bdrv_new(const char *device_name);
> > +BlockDriverState *bdrv_new(const char *device_name, const char *node_name);
> > void bdrv_make_anon(BlockDriverState *bs);
> > void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> > void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> > @@ -339,6 +339,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
> > void bdrv_eject(BlockDriverState *bs, bool eject_flag);
> > const char *bdrv_get_format_name(BlockDriverState *bs);
> > BlockDriverState *bdrv_find(const char *name);
> > +BlockDriverState *bdrv_find_node(const char *node_name);
> > BlockDriverState *bdrv_next(BlockDriverState *bs);
> > void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> > void *opaque);
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index a48731d..9e44136 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -297,11 +297,18 @@ struct BlockDriverState {
> > BlockdevOnError on_read_error, on_write_error;
> > bool iostatus_enabled;
> > BlockDeviceIoStatus iostatus;
> > +
> > + /* the following member give a name to every node on the BlockDriverState
> > + * graph.
> > + */
> > + char node_name[32];
> > + QTAILQ_ENTRY(BlockDriverState) node_list;
> > + /* Device name is the name associated with the "drive" the guest see */
> > char device_name[32];
> > + QTAILQ_ENTRY(BlockDriverState) device_list;
> > HBitmap *dirty_bitmap;
> > int refcnt;
> > int in_use; /* users other than guest access, eg. block migration */
> > - QTAILQ_ENTRY(BlockDriverState) list;
> >
> > QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
> >
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 926f0a0..215b7b2 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -269,7 +269,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
> > Error *local_err = NULL;
> > int ret;
> >
> > - bs = bdrv_new("image");
> > + bs = bdrv_new("image", NULL);
> >
> > if (fmt) {
> > drv = bdrv_find_format(fmt);
> > @@ -2225,7 +2225,7 @@ static int img_rebase(int argc, char **argv)
> > } else {
> > char backing_name[1024];
> >
> > - bs_old_backing = bdrv_new("old_backing");
> > + bs_old_backing = bdrv_new("old_backing", NULL);
> > bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> > ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> > old_backing_drv, &local_err);
> > @@ -2236,7 +2236,7 @@ static int img_rebase(int argc, char **argv)
> > goto out;
> > }
> > if (out_baseimg[0]) {
> > - bs_new_backing = bdrv_new("new_backing");
> > + bs_new_backing = bdrv_new("new_backing", NULL);
> > ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> > new_backing_drv, &local_err);
> > if (ret) {
> > diff --git a/qemu-io.c b/qemu-io.c
> > index 3b3340a..3e1ea88 100644
> > --- a/qemu-io.c
> > +++ b/qemu-io.c
> > @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
> > return 1;
> > }
> > } else {
> > - qemuio_bs = bdrv_new("hda");
> > + qemuio_bs = bdrv_new("hda", NULL);
> >
> > if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> > fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
> > diff --git a/qemu-nbd.c b/qemu-nbd.c
> > index c26c98e..35ef57c 100644
> > --- a/qemu-nbd.c
> > +++ b/qemu-nbd.c
> > @@ -572,7 +572,7 @@ int main(int argc, char **argv)
> > drv = NULL;
> > }
> >
> > - bs = bdrv_new("hda");
> > + bs = bdrv_new("hda", NULL);
> > srcpath = argv[optind];
> > ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
> > if (ret < 0) {
> > --
> > 1.8.3.2
> >
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph.
2013-11-07 15:01 ` [Qemu-devel] [PATCH 1/2] block: Add bs->node_name to hold the name of a bs node of the bs graph Benoît Canet
2013-11-07 20:23 ` Eric Blake
2013-11-07 20:54 ` Jeff Cody
@ 2013-11-08 8:19 ` Fam Zheng
2 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2013-11-08 8:19 UTC (permalink / raw)
To: Benoît Canet; +Cc: kwolf, qemu-devel, stefanha, armbru
On Thu, 11/07 16:01, Benoît Canet wrote:
> Add the minimum of code to prepare the followings patches.
>
> If no node_name is provided to bdrv_new the bs->node_name is set to "undefined".
> This will allow to have some default string to communicate in QMP and HMP.
> This also make "undefined" a reserved string for bs->node_name.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 70 +++++++++++++++++++++++++++++++++++------------
> block/blkverify.c | 2 +-
> block/iscsi.c | 2 +-
> block/vmdk.c | 2 +-
> block/vvfat.c | 4 +--
> blockdev.c | 8 +++---
> hw/block/xen_disk.c | 2 +-
> include/block/block.h | 3 +-
> include/block/block_int.h | 9 +++++-
> qemu-img.c | 6 ++--
> qemu-io.c | 2 +-
> qemu-nbd.c | 2 +-
> 12 files changed, 77 insertions(+), 35 deletions(-)
>
> diff --git a/block.c b/block.c
> index fd05a80..230e71a 100644
> --- a/block.c
> +++ b/block.c
> @@ -89,6 +89,9 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
> static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
> QTAILQ_HEAD_INITIALIZER(bdrv_states);
>
> +static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
> + QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
> +
> static QLIST_HEAD(, BlockDriver) bdrv_drivers =
> QLIST_HEAD_INITIALIZER(bdrv_drivers);
>
> @@ -318,14 +321,26 @@ void bdrv_register(BlockDriver *bdrv)
> }
>
> /* create a new block device (by default it is empty) */
> -BlockDriverState *bdrv_new(const char *device_name)
> +BlockDriverState *bdrv_new(const char *device_name, const char *node_name)
> {
> BlockDriverState *bs;
>
> bs = g_malloc0(sizeof(BlockDriverState));
> pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
> if (device_name[0] != '\0') {
> - QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> + QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
> + }
> + /* if node name is given store it in bs and insert bs in the graph bs list
> + * note: undefined is a reserved node name
> + */
> + if (node_name &&
> + node_name[0] != '\0' &&
> + strcmp(node_name, "undefined")) {
Either "undefined" or "" is OK for me, but I would suggest use a macro to not
to repeat it.
> + pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
> + QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> + /* else set the bs node name to undefined for QMP and HMP */
> + } else {
> + sprintf(bs->node_name, "undefined");
> }
> bdrv_iostatus_disable(bs);
> notifier_list_init(&bs->close_notifiers);
> @@ -870,7 +885,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> options = qdict_new();
> }
>
> - bs = bdrv_new("");
> + bs = bdrv_new("", NULL);
Better to unify the empty value: either both "", or both allow NULL.
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -992,7 +1007,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> sizeof(backing_filename));
> }
>
> - bs->backing_hd = bdrv_new("");
> + bs->backing_hd = bdrv_new("", NULL);
>
> if (bs->backing_format[0] != '\0') {
> back_drv = bdrv_find_format(bs->backing_format);
> @@ -1062,7 +1077,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> instead of opening 'filename' directly */
>
> /* if there is a backing file, use it */
> - bs1 = bdrv_new("");
> + bs1 = bdrv_new("", NULL);
> ret = bdrv_open(bs1, filename, NULL, 0, drv, &local_err);
> if (ret < 0) {
> bdrv_unref(bs1);
> @@ -1495,7 +1510,7 @@ void bdrv_close_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_close(bs);
> }
> }
> @@ -1524,7 +1539,7 @@ static bool bdrv_requests_pending(BlockDriverState *bs)
> static bool bdrv_requests_pending_all(void)
> {
> BlockDriverState *bs;
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bdrv_requests_pending(bs)) {
> return true;
> }
> @@ -1554,7 +1569,7 @@ void bdrv_drain_all(void)
> /* FIXME: We do not have timer support here, so this is effectively
> * a busy wait.
> */
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bdrv_start_throttled_reqs(bs)) {
> busy = true;
> }
> @@ -1570,7 +1585,7 @@ void bdrv_drain_all(void)
> void bdrv_make_anon(BlockDriverState *bs)
> {
> if (bs->device_name[0] != '\0') {
> - QTAILQ_REMOVE(&bdrv_states, bs, list);
> + QTAILQ_REMOVE(&bdrv_states, bs, device_list);
> }
> bs->device_name[0] = '\0';
> }
> @@ -1626,7 +1641,12 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
> /* keep the same entry in bdrv_states */
> pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
> bs_src->device_name);
> - bs_dest->list = bs_src->list;
> + bs_dest->device_list = bs_src->device_list;
> +
> + /* keep the same entry in graph_bdrv_states */
> + pstrcpy(bs_dest->node_name, sizeof(bs_dest->node_name),
> + bs_src->node_name);
> + bs_dest->node_list = bs_src->node_list;
> }
>
> /*
> @@ -1950,7 +1970,7 @@ int bdrv_commit_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (bs->drv && bs->backing_hd) {
> int ret = bdrv_commit(bs);
> if (ret < 0) {
> @@ -3017,11 +3037,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
> }
> }
>
> +/* This function is to find block backend bs */
> BlockDriverState *bdrv_find(const char *name)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> if (!strcmp(name, bs->device_name)) {
> return bs;
> }
> @@ -3029,19 +3050,32 @@ BlockDriverState *bdrv_find(const char *name)
> return NULL;
> }
>
> +/* This function is to find a node in the bs graph */
> +BlockDriverState *bdrv_find_node(const char *node_name)
> +{
> + BlockDriverState *bs;
> +
> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> + if (!strcmp(node_name, bs->node_name)) {
> + return bs;
> + }
> + }
> + return NULL;
> +}
> +
> BlockDriverState *bdrv_next(BlockDriverState *bs)
> {
> if (!bs) {
> return QTAILQ_FIRST(&bdrv_states);
> }
> - return QTAILQ_NEXT(bs, list);
> + return QTAILQ_NEXT(bs, device_list);
> }
>
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs), void *opaque)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> it(opaque, bs);
> }
> }
> @@ -3061,7 +3095,7 @@ int bdrv_flush_all(void)
> BlockDriverState *bs;
> int result = 0;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> int ret = bdrv_flush(bs);
> if (ret < 0 && !result) {
> result = ret;
> @@ -4127,7 +4161,7 @@ void bdrv_invalidate_cache_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bdrv_invalidate_cache(bs);
> }
> }
> @@ -4136,7 +4170,7 @@ void bdrv_clear_incoming_migration_all(void)
> {
> BlockDriverState *bs;
>
> - QTAILQ_FOREACH(bs, &bdrv_states, list) {
> + QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING);
> }
> }
> @@ -4582,7 +4616,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
> back_flags =
> flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>
> - bs = bdrv_new("");
> + bs = bdrv_new("", NULL);
>
> ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> backing_drv, &local_err);
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 55819a0..674b6a5 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -155,7 +155,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
> goto fail;
> }
>
> - s->test_file = bdrv_new("");
> + s->test_file = bdrv_new("", NULL);
> ret = bdrv_open(s->test_file, filename, NULL, flags, NULL, &local_err);
> if (ret < 0) {
> error_propagate(errp, local_err);
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a2a961e..5031593 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1461,7 +1461,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options,
> IscsiLun *iscsilun = NULL;
> QDict *bs_options;
>
> - bs = bdrv_new("");
> + bs = bdrv_new("", NULL);
>
> /* Read out options */
> while (options && options->name) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 32ec8b77..97801c2 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1672,7 +1672,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
> return -ENOTSUP;
> }
> if (backing_file) {
> - BlockDriverState *bs = bdrv_new("");
> + BlockDriverState *bs = bdrv_new("", NULL);
> ret = bdrv_open(bs, backing_file, NULL, 0, NULL, errp);
> if (ret != 0) {
> bdrv_unref(bs);
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 3ddaa0b..a8b6011 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2935,7 +2935,7 @@ static int enable_write_target(BDRVVVFATState *s)
> goto err;
> }
>
> - s->qcow = bdrv_new("");
> + s->qcow = bdrv_new("", NULL);
>
> ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
> BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
> @@ -2951,7 +2951,7 @@ static int enable_write_target(BDRVVVFATState *s)
> unlink(s->qcow_filename);
> #endif
>
> - s->bs->backing_hd = bdrv_new("");
> + s->bs->backing_hd = bdrv_new("", NULL);
> s->bs->backing_hd->drv = &vvfat_write_target;
> s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
> *(void**)s->bs->backing_hd->opaque = s;
> diff --git a/blockdev.c b/blockdev.c
> index b260477..ac47413 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -469,7 +469,7 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
> /* init */
> dinfo = g_malloc0(sizeof(*dinfo));
> dinfo->id = g_strdup(qemu_opts_id(opts));
> - dinfo->bdrv = bdrv_new(dinfo->id);
> + dinfo->bdrv = bdrv_new(dinfo->id, NULL);
> dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> dinfo->bdrv->read_only = ro;
> dinfo->type = type;
> @@ -1254,7 +1254,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
> }
>
> /* We will manually add the backing_hd field to the bs later */
> - state->new_bs = bdrv_new("");
> + state->new_bs = bdrv_new("", NULL);
> /* TODO Inherit bs->options or only take explicit options with an
> * extended QMP command? */
> ret = bdrv_open(state->new_bs, new_image_file, NULL,
> @@ -1921,7 +1921,7 @@ void qmp_drive_backup(const char *device, const char *target,
> return;
> }
>
> - target_bs = bdrv_new("");
> + target_bs = bdrv_new("", NULL);
> ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
> if (ret < 0) {
> bdrv_unref(target_bs);
> @@ -2055,7 +2055,7 @@ void qmp_drive_mirror(const char *device, const char *target,
> /* Mirroring takes care of copy-on-write using the source's backing
> * file.
> */
> - target_bs = bdrv_new("");
> + target_bs = bdrv_new("", NULL);
> ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
> &local_err);
> if (ret < 0) {
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 098f6c6..d89e025 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -808,7 +808,7 @@ static int blk_connect(struct XenDevice *xendev)
> if (!blkdev->dinfo) {
> /* setup via xenbus -> create new block driver instance */
> xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
> - blkdev->bs = bdrv_new(blkdev->dev);
> + blkdev->bs = bdrv_new(blkdev->dev, NULL);
> if (blkdev->bs) {
> Error *local_err = NULL;
> BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
> diff --git a/include/block/block.h b/include/block/block.h
> index 3560deb..2d27bd9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -149,7 +149,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> QEMUOptionParameter *options, Error **errp);
> int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> Error **errp);
> -BlockDriverState *bdrv_new(const char *device_name);
> +BlockDriverState *bdrv_new(const char *device_name, const char *node_name);
> void bdrv_make_anon(BlockDriverState *bs);
> void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
> @@ -339,6 +339,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked);
> void bdrv_eject(BlockDriverState *bs, bool eject_flag);
> const char *bdrv_get_format_name(BlockDriverState *bs);
> BlockDriverState *bdrv_find(const char *name);
> +BlockDriverState *bdrv_find_node(const char *node_name);
> BlockDriverState *bdrv_next(BlockDriverState *bs);
> void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
> void *opaque);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index a48731d..9e44136 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -297,11 +297,18 @@ struct BlockDriverState {
> BlockdevOnError on_read_error, on_write_error;
> bool iostatus_enabled;
> BlockDeviceIoStatus iostatus;
> +
> + /* the following member give a name to every node on the BlockDriverState
> + * graph.
> + */
> + char node_name[32];
Now there are two chains: node_list and device_list. Could you document their
use cases? Does a BDS exist in both lists or only one?
Thanks,
Fam
> + QTAILQ_ENTRY(BlockDriverState) node_list;
> + /* Device name is the name associated with the "drive" the guest see */
> char device_name[32];
> + QTAILQ_ENTRY(BlockDriverState) device_list;
> HBitmap *dirty_bitmap;
> int refcnt;
> int in_use; /* users other than guest access, eg. block migration */
> - QTAILQ_ENTRY(BlockDriverState) list;
>
> QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 926f0a0..215b7b2 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -269,7 +269,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
> Error *local_err = NULL;
> int ret;
>
> - bs = bdrv_new("image");
> + bs = bdrv_new("image", NULL);
>
> if (fmt) {
> drv = bdrv_find_format(fmt);
> @@ -2225,7 +2225,7 @@ static int img_rebase(int argc, char **argv)
> } else {
> char backing_name[1024];
>
> - bs_old_backing = bdrv_new("old_backing");
> + bs_old_backing = bdrv_new("old_backing", NULL);
> bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
> old_backing_drv, &local_err);
> @@ -2236,7 +2236,7 @@ static int img_rebase(int argc, char **argv)
> goto out;
> }
> if (out_baseimg[0]) {
> - bs_new_backing = bdrv_new("new_backing");
> + bs_new_backing = bdrv_new("new_backing", NULL);
> ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
> new_backing_drv, &local_err);
> if (ret) {
> diff --git a/qemu-io.c b/qemu-io.c
> index 3b3340a..3e1ea88 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -63,7 +63,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
> return 1;
> }
> } else {
> - qemuio_bs = bdrv_new("hda");
> + qemuio_bs = bdrv_new("hda", NULL);
>
> if (bdrv_open(qemuio_bs, name, opts, flags, NULL, &local_err) < 0) {
> fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index c26c98e..35ef57c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -572,7 +572,7 @@ int main(int argc, char **argv)
> drv = NULL;
> }
>
> - bs = bdrv_new("hda");
> + bs = bdrv_new("hda", NULL);
> srcpath = argv[optind];
> ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
> if (ret < 0) {
> --
> 1.8.3.2
>
^ permalink raw reply [flat|nested] 12+ messages in thread