* [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState
@ 2013-11-07 15:01 Benoît Canet
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
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Benoît Canet @ 2013-11-07 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, stefanha
The following series introduce a new file.node-name property in order to be
able to give a name to each BlockDriverState of the graph.
It also define "undefined" as a special value for node-name; a value that will be
used to indicate to the management that it can not manipulate a node because it
was not named.
After this patchset is merged I would like to take care of presenting the graph
to the management. (HMP &&/|| QMP)
Eric: Do you have some ideas on this topic ?
Best regards
Benoît
Benoît Canet (2):
block: Add bs->node_name to hold the name of a bs node of the bs
graph.
block: Allow the user to define "node-name" option.
block.c | 72 +++++++++++++++++++++++++++++++++++------------
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, 79 insertions(+), 35 deletions(-)
--
1.8.3.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [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 0/2] Giving names to graph's BlockDriverState Benoît Canet
@ 2013-11-07 15:01 ` Benoît Canet
2013-11-07 20:23 ` Eric Blake
` (2 more replies)
2013-11-07 15:01 ` [Qemu-devel] [PATCH 2/2] block: Allow the user to define "node-name" option Benoît Canet
2013-11-07 20:34 ` [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState Eric Blake
2 siblings, 3 replies; 12+ messages in thread
From: Benoît Canet @ 2013-11-07 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, stefanha
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")) {
+ 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';
}
@@ -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 related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/2] block: Allow the user to define "node-name" option.
2013-11-07 15:01 [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState Benoît Canet
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 15:01 ` Benoît Canet
2013-11-07 20:31 ` Eric Blake
2013-11-08 9:55 ` Kevin Wolf
2013-11-07 20:34 ` [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState Eric Blake
2 siblings, 2 replies; 12+ messages in thread
From: Benoît Canet @ 2013-11-07 15:01 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, Benoît Canet, armbru, stefanha
As node-name is a separate name space as device-name we can enable it's
definition right now: nobody will use it so no harm involved.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
block.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 230e71a..132981f 100644
--- a/block.c
+++ b/block.c
@@ -885,7 +885,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
options = qdict_new();
}
- bs = bdrv_new("", NULL);
+ bs = bdrv_new("", qdict_get_try_str(options, "node-name"));
+ qdict_del(options, "node-name");
bs->options = options;
options = qdict_clone_shallow(options);
@@ -1007,7 +1008,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
sizeof(backing_filename));
}
- bs->backing_hd = bdrv_new("", NULL);
+ bs->backing_hd = bdrv_new("", qdict_get_try_str(options, "node-name"));
+ qdict_del(options, "node-name");
if (bs->backing_format[0] != '\0') {
back_drv = bdrv_find_format(bs->backing_format);
--
1.8.3.2
^ permalink raw reply related [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 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 2/2] block: Allow the user to define "node-name" option.
2013-11-07 15:01 ` [Qemu-devel] [PATCH 2/2] block: Allow the user to define "node-name" option Benoît Canet
@ 2013-11-07 20:31 ` Eric Blake
2013-11-08 9:55 ` Kevin Wolf
1 sibling, 0 replies; 12+ messages in thread
From: Eric Blake @ 2013-11-07 20:31 UTC (permalink / raw)
To: Benoît Canet, qemu-devel; +Cc: kwolf, famz, armbru, stefanha
[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]
On 11/07/2013 08:01 AM, Benoît Canet wrote:
> As node-name is a separate name space as device-name we can enable it's
s/space as/space from/
s/it's/its/
> definition right now: nobody will use it so no harm involved.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
> block.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
Shouldn't blockdev-add have a QMP counterpart for setting node name
during device hotplug? Also, is there a QMP command for inspecting node
names yet? This feels like a write-only interface if we don't have more
usage of it in place.
Don't get me wrong - I think we definitely want this, but in the context
of a bigger series rather than by itself.
>
> diff --git a/block.c b/block.c
> index 230e71a..132981f 100644
> --- a/block.c
> +++ b/block.c
> @@ -885,7 +885,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
> options = qdict_new();
> }
>
> - bs = bdrv_new("", NULL);
> + bs = bdrv_new("", qdict_get_try_str(options, "node-name"));
> + qdict_del(options, "node-name");
> bs->options = options;
> options = qdict_clone_shallow(options);
>
> @@ -1007,7 +1008,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
> sizeof(backing_filename));
> }
>
> - bs->backing_hd = bdrv_new("", NULL);
> + bs->backing_hd = bdrv_new("", qdict_get_try_str(options, "node-name"));
> + qdict_del(options, "node-name");
>
> if (bs->backing_format[0] != '\0') {
> back_drv = bdrv_find_format(bs->backing_format);
>
--
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 0/2] Giving names to graph's BlockDriverState
2013-11-07 15:01 [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState Benoît Canet
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 15:01 ` [Qemu-devel] [PATCH 2/2] block: Allow the user to define "node-name" option Benoît Canet
@ 2013-11-07 20:34 ` Eric Blake
2 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2013-11-07 20:34 UTC (permalink / raw)
To: Benoît Canet, qemu-devel; +Cc: kwolf, famz, armbru, stefanha
[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]
On 11/07/2013 08:01 AM, Benoît Canet wrote:
> The following series introduce a new file.node-name property in order to be
> able to give a name to each BlockDriverState of the graph.
>
> It also define "undefined" as a special value for node-name; a value that will be
> used to indicate to the management that it can not manipulate a node because it
> was not named.
Is it necessary to use the actual string "undefined" to mean unnamed, or
can we be careful and use NULL for unnamed, and all other pointers as
the user-provided name? Either way, we have to special-case code to
check for the sentinel, but a NULL check is faster than a strcmp(), plus
it feels slightly nicer to not consume a value out of the user's namespace.
>
> After this patchset is merged I would like to take care of presenting the graph
> to the management. (HMP &&/|| QMP)
>
> Eric: Do you have some ideas on this topic ?
Overall, I'm looking forward to getting this into qemu 1.8; I'm still
waiting to see what else you propose for QMP interfaces.
--
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 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: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: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
* 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 2/2] block: Allow the user to define "node-name" option.
2013-11-07 15:01 ` [Qemu-devel] [PATCH 2/2] block: Allow the user to define "node-name" option Benoît Canet
2013-11-07 20:31 ` Eric Blake
@ 2013-11-08 9:55 ` Kevin Wolf
1 sibling, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2013-11-08 9:55 UTC (permalink / raw)
To: Benoît Canet; +Cc: famz, qemu-devel, stefanha, armbru
Am 07.11.2013 um 16:01 hat Benoît Canet geschrieben:
> As node-name is a separate name space as device-name we can enable it's
> definition right now: nobody will use it so no harm involved.
>
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
As Eric already said, we need to update the QAPI schema for the new
field.
We already have an 'id' field for each BlockDriverState, which is
currently used as the device_name on the top level, and forbidden on
other devices. What is our long-term plan with this?
Should 'id' be for every node that must get a BlockBackend on top, and
'node-name' just for any node? We're mixing BDS and BB configuration
here. It may be okay to do that, especially as long as BBs don't have
more than just device_name to be configured, but we should at least be
aware that we're doing it.
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-08 9:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-07 15:01 [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState Benoît Canet
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-07 21:11 ` Benoît Canet
2013-11-08 8:19 ` Fam Zheng
2013-11-07 15:01 ` [Qemu-devel] [PATCH 2/2] block: Allow the user to define "node-name" option Benoît Canet
2013-11-07 20:31 ` Eric Blake
2013-11-08 9:55 ` Kevin Wolf
2013-11-07 20:34 ` [Qemu-devel] [PATCH 0/2] Giving names to graph's BlockDriverState Eric Blake
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).