Re: [PATCH v6 03/10] block: add ability to specify list of blockdevs during snapshot

2020-10-19 Thread Eric Blake

On 10/8/20 10:49 AM, Daniel P. Berrangé wrote:

When running snapshot operations, there are various rules for which
blockdevs are included/excluded. While this provides reasonable default
behaviour, there are scenarios that are not well handled by the default
logic. Some of the conditions do not have a single correct answer.

Thus there needs to be a way for the mgmt app to provide an explicit
list of blockdevs to perform snapshots across. This can be achieved
by passing a list of node names that should be used.

Signed-off-by: Daniel P. Berrangé 
---



+static int bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
+ GList **all_bdrvs,
+ Error **errp)
+{
+g_autoptr(GList) bdrvs = NULL;
+
+if (has_devices) {
+if (!devices) {
+error_setg(errp, "At least one device is required for snapshot");
+return -1;
+}
+
+while (devices) {
+BlockDriverState *bs = bdrv_find_node(devices->value);
+if (!bs) {
+error_setg(errp, "No block device node '%s'", devices->value);
+return -1;
+}
+bdrvs = g_list_append(bdrvs, bs);
+devices = devices->next;
+}


Do we care if the user passes the same device more than once in their 
list?  (If so, a hash table might be better than g_list)


Otherwise, looks good to me.
Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org




[PATCH v6 03/10] block: add ability to specify list of blockdevs during snapshot

2020-10-08 Thread Daniel P . Berrangé
When running snapshot operations, there are various rules for which
blockdevs are included/excluded. While this provides reasonable default
behaviour, there are scenarios that are not well handled by the default
logic. Some of the conditions do not have a single correct answer.

Thus there needs to be a way for the mgmt app to provide an explicit
list of blockdevs to perform snapshots across. This can be achieved
by passing a list of node names that should be used.

Signed-off-by: Daniel P. Berrangé 
---
 block/monitor/block-hmp-cmds.c |   4 +-
 block/snapshot.c   | 180 -
 include/block/snapshot.h   |  22 ++--
 migration/savevm.c |  16 +--
 monitor/hmp-cmds.c |   2 +-
 replay/replay-debugging.c  |   4 +-
 6 files changed, 162 insertions(+), 66 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 548bb6b5e3..ebb6ae0333 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -901,7 +901,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 SnapshotEntry *snapshot_entry;
 Error *err = NULL;
 
-bs = bdrv_all_find_vmstate_bs();
+bs = bdrv_all_find_vmstate_bs(false, NULL, );
 if (!bs) {
 error_report_err(err);
 return;
@@ -953,7 +953,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 total = 0;
 for (i = 0; i < nb_sns; i++) {
 SnapshotEntry *next_sn;
-if (bdrv_all_find_snapshot(sn_tab[i].name, NULL) == 0) {
+if (bdrv_all_find_snapshot(sn_tab[i].name, false, NULL, NULL) == 0) {
 global_snapshots[total] = i;
 total++;
 QTAILQ_FOREACH(image_entry, _list, next) {
diff --git a/block/snapshot.c b/block/snapshot.c
index 50e35bb9fa..155b8aad88 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -447,6 +447,41 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState 
*bs,
 return ret;
 }
 
+
+static int bdrv_all_get_snapshot_devices(bool has_devices, strList *devices,
+ GList **all_bdrvs,
+ Error **errp)
+{
+g_autoptr(GList) bdrvs = NULL;
+
+if (has_devices) {
+if (!devices) {
+error_setg(errp, "At least one device is required for snapshot");
+return -1;
+}
+
+while (devices) {
+BlockDriverState *bs = bdrv_find_node(devices->value);
+if (!bs) {
+error_setg(errp, "No block device node '%s'", devices->value);
+return -1;
+}
+bdrvs = g_list_append(bdrvs, bs);
+devices = devices->next;
+}
+} else {
+BlockDriverState *bs;
+BdrvNextIterator it;
+for (bs = bdrv_first(); bs; bs = bdrv_next()) {
+bdrvs = g_list_append(bdrvs, bs);
+}
+}
+
+*all_bdrvs = g_steal_pointer();
+return 0;
+}
+
+
 static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
 {
 if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
@@ -462,43 +497,59 @@ static bool 
bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
  * These functions will properly handle dataplane (take aio_context_acquire
  * when appropriate for appropriate block drivers) */
 
-bool bdrv_all_can_snapshot(Error **errp)
+bool bdrv_all_can_snapshot(bool has_devices, strList *devices,
+   Error **errp)
 {
-BlockDriverState *bs;
-BdrvNextIterator it;
+g_autoptr(GList) bdrvs = NULL;
+GList *iterbdrvs;
 
-for (bs = bdrv_first(); bs; bs = bdrv_next()) {
+if (bdrv_all_get_snapshot_devices(has_devices, devices, , errp) < 0) 
{
+return false;
+}
+
+iterbdrvs = bdrvs;
+while (iterbdrvs) {
+BlockDriverState *bs = iterbdrvs->data;
 AioContext *ctx = bdrv_get_aio_context(bs);
 bool ok;
 
 aio_context_acquire(ctx);
-if (bdrv_all_snapshots_includes_bs(bs)) {
+if (devices || bdrv_all_snapshots_includes_bs(bs)) {
 ok = bdrv_can_snapshot(bs);
 }
 aio_context_release(ctx);
 if (!ok) {
 error_setg(errp, "Device '%s' is writable but does not support "
"snapshots", bdrv_get_device_or_node_name(bs));
-bdrv_next_cleanup();
 return false;
 }
+
+iterbdrvs = iterbdrvs->next;
 }
 
 return true;
 }
 
-int bdrv_all_delete_snapshot(const char *name, Error **errp)
+int bdrv_all_delete_snapshot(const char *name,
+ bool has_devices, strList *devices,
+ Error **errp)
 {
-BlockDriverState *bs;
-BdrvNextIterator it;
-QEMUSnapshotInfo sn1, *snapshot = 
+g_autoptr(GList) bdrvs = NULL;
+GList *iterbdrvs;
+
+if (bdrv_all_get_snapshot_devices(has_devices, devices, , errp) < 0) 
{
+return -1;
+}
 
-for (bs = bdrv_first();