| Message ID | 1465745921-22733-1-git-send-email-lma@suse.com |
|---|---|
| State | New |
| Headers | show |
On 12.06.2016 17:38, Lin Ma wrote: > Currently, the output of 'info snapshots' shows fully available snapshots. > > In my opinion there are 2 disadvantages: > 1. It's opaque, hides some snapshot information to users. It's not convenient > if users want to know more about all of snapshot information on every block > device via monitor. > 2. It uses snapshot id to determine whether a snapshot is 'fully available', > It causes incorrect output in some scenario. > > For instance: > (qemu) info block > drive_image1 (#block113): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk0.qcow2 > (qcow2) > Cache mode: writeback > > drive_image2 (#block349): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk1.qcow2 > (qcow2) > Cache mode: writeback > (qemu) > (qemu) info snapshots > There is no snapshot available. > (qemu) > (qemu) snapshot_blkdev_internal drive_image1 snap1 > (qemu) > (qemu) info snapshots > There is no suitable snapshot available > (qemu) > (qemu) savevm checkpoint-1 > (qemu) > (qemu) info snapshots > ID TAG VM SIZE DATE VM CLOCK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 > (qemu) > > $ qemu-img snapshot -l disk0.qcow2 > Snapshot list: > ID TAG VM SIZE DATE VM CLOCK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 > 2 checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 > > $ qemu-img snapshot -l disk1.qcow2 > Snapshot list: > ID TAG VM SIZE DATE VM CLOCK > 1 checkpoint-1 0 2016-05-22 16:58:07 00:02:06.813 > > The patch uses snapshot name instead of snapshot id to determine whether a > snapshot is 'fully available', and follow Kevin's suggestion, Make the output > more detailed/accurate: > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLOCK > -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 > > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOCK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 > > Signed-off-by: Lin Ma <lma@suse.com> > --- > migration/savevm.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 74 insertions(+), 3 deletions(-) I have many comments, but don't worry, it's nothing that can't be fixed. The overall design looks good to me. > diff --git a/migration/savevm.c b/migration/savevm.c > index 6c21231..8444c62 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2153,12 +2153,28 @@ void hmp_delvm(Monitor *mon, const QDict *qdict) > void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > { > BlockDriverState *bs, *bs1; > + BdrvNextIterator it1; > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i; > + bool no_snapshot = true; > + int nb_sns, nb_sns_tmp, i; "nb_sns_tmp" is not a very expressive name. Since you only need to use it in a for () loop below, I propose moving the declaration there. [2] > int total; > int *available_snapshots; > AioContext *aio_context; > > + typedef struct SnapshotEntry { > + QEMUSnapshotInfo *sn; I strongly propose not making this a pointer. See [3] for why. > + QTAILQ_ENTRY(SnapshotEntry) next; > + } SnapshotEntry; > + > + typedef struct ImageEntry { > + char *imagename; > + QTAILQ_ENTRY(ImageEntry) next; > + QTAILQ_HEAD(, SnapshotEntry) snapshots; > + } ImageEntry; I wouldn't declare types inside of a function, but I don't think we actually have a rule against it. > + > + ImageEntry *image_entry; > + SnapshotEntry *snapshot_entry; > + > bs = bdrv_all_find_vmstate_bs(); > if (!bs) { > monitor_printf(mon, "No available block device supports snapshots\n"); > @@ -2175,7 +2191,34 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > return; > } > > - if (nb_sns == 0) { > + QTAILQ_HEAD(image_list, ImageEntry) image_list = > + QTAILQ_HEAD_INITIALIZER(image_list); qemu's coding style mandates declaration of variables at the start of a block. > + > + for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) { [2] I think it would make sense to move the declaration of nb_sns_tmp here and call it something different. Maybe "bs1_nb_sns" or "nb_sns_bs1". (On a side note: I don't like the name nb_sns in itself and would very much prefer nb_snapshots, but since this is preexisting, I won't ask you to change it.) > + AioContext *ctx = bdrv_get_aio_context(bs); Shouldn't this be bs1? > + > + aio_context_acquire(ctx); > + nb_sns_tmp = 0; > + if (bdrv_can_snapshot(bs1)) { > + nb_sns_tmp = bdrv_snapshot_list(bs1, &sn); I think sn should be initialized to NULL before this call... [1] > + if (nb_sns_tmp > 0) { > + no_snapshot = false; > + ImageEntry *ie = g_malloc0(sizeof(*ie)); First: Declaration needs to be done at the start of the block. Second: While not wrong, you may want to use g_new0(ImageEntry, 1) instead. The benefit of using g_new0() is that it will return the correct type. > + ie->imagename = g_strdup(bdrv_get_device_name(bs1)); I'm not sure why you're using g_strdup() here. Wouldn't it suffice to make imagename a const char * and drop the g_strdup()? > + QTAILQ_INIT(&ie->snapshots); > + QTAILQ_INSERT_TAIL(&image_list, ie, next); > + int x; First: This needs to be declared at the start of this block. Second: I don't particularly like "x" as the name of this variable. Normally, iterator variables are named i, j, k, .... Since "i" is taken already, I'd propose using "j" for this one. Third: Since "i" is not used here at all, you could actually just use "i" for the following loop. This is what I prefer. > + for (x = 0; x < nb_sns_tmp; x++) { > + SnapshotEntry *se = g_malloc0(sizeof(*se)); Same here, I'd propose using g_new0(SnapshotEntry, 1). > + se->sn = &sn[x]; (Note that if you followed my proposal above of making se->sn not a pointer, this should be se->sn = sn[x].) > + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next); > + } > + } [1] ...and then sn needs to be freed here (using g_free()). (This is why it needs to be initialized to NULL, so g_free() will work regardless of whether bdrv_snapshot_list() wrote something to sn.) > + } > + aio_context_release(ctx); > + } > + > + if (no_snapshot) { > monitor_printf(mon, "There is no snapshot available.\n"); > return; > } > @@ -2183,17 +2226,28 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > available_snapshots = g_new0(int, nb_sns); It might make sense to rename this variable. It's currently named this way because everything that's not entered into this array is an "unavailable" snapshot, and will not be displayed. You are changing this behavior, and this array will simply contain all the snapshot indices that are globally available on all BDSs. I think renaming it to "global_snapshots" may make sense. But since this is a preexisting name, it's up to you whether you want to rename it or not. > total = 0; > for (i = 0; i < nb_sns; i++) { > - if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) { > + if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { I think this should be in an independent patch because this is an independent fix. > available_snapshots[total] = i; > total++; > } > } > > + monitor_printf(mon, "List of snapshots present on all disks:\n"); > + > if (total > 0) { > bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); > monitor_printf(mon, "\n"); > for (i = 0; i < total; i++) { > sn = &sn_tab[available_snapshots[i]]; > + QTAILQ_FOREACH(image_entry, &image_list, next) { > + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { This must be QTAILQ_FOREACH_SAFE(). > + if (!strcmp(sn->name, snapshot_entry->sn->name)) { > + QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry, > + next); [3] You are leaking snapshot_entry->sn here. It's rather difficult to avoid this if you want to keep that field a pointer. Therefore, I proposed not making it a pointer. Also, you are leaking snapshot_entry itself here. Easy fix, g_free() it and use QTAILQ_FOREACH_SAFE(). > + } > + } > + } I think this should be done in the for () loop above (the "for (i < 0; i < nb_sns; i++)" loop). This is because I think we should separate code for outputting information and code for gathering/filtering this information. The code added here falls in the latter category, while the existing code here is just for outputting what we found. > + pstrcpy(sn->id_str, sizeof(sn->id_str), "--"); This pstrcpy() warrants a comment, like "The ID is not guaranteed to be the same on all images, so overwrite it". Note that this pstrcpy() call addition should be in the same patch as the s/\.id_str/.name/ in the bdrv_all_find_snapshot() call above. > bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); > monitor_printf(mon, "\n"); > } > @@ -2201,6 +2255,23 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "There is no suitable snapshot available\n"); This message should now be rephrased to something like simply "(none)\n", because the new "List of snapshots present on all disks:" headline will fully explain what that means. > } Nit pick: The following code will always leave an empty line after everything. I think that's superfluous, and it can be amended as follows (if you want to amend it, that is; if you really like that empty line, then feel free to disregard my suggestion): > + monitor_printf(mon, "\n"); Drop this. > + QTAILQ_FOREACH(image_entry, &image_list, next) { > + if (QTAILQ_EMPTY(&image_entry->snapshots)) { > + continue; > + } Put monitor_printf(mon, "\n"); here. > + monitor_printf(mon, "List of partial (non-loadable) snapshots on '%s':", > + image_entry->imagename); > + monitor_printf(mon, "\n"); (Why did you not concatenate these two strings in a single monitor_printf() call?) > + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); > + monitor_printf(mon, "\n"); Drop this. > + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { Put monitor_printf(mon, "\n"); here. > + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, > + snapshot_entry->sn); > + monitor_printf(mon, "\n"); And drop this. Again, the suggestions on moving the monitor_printf(mon, "\n"); calls around are just suggestions, and it's up to you whether you want to follow them or not. > + } > + } > + You're leaking all entries in the image_list here, and subsequently all snapshots in the snapshots list of each image, and also the imagename of each image_list entry. The latter wouldn't occur if you made imagename a const char * and drop the g_strdup() when assigning is, as I have suggested somewhere above. > g_free(sn_tab); > g_free(available_snapshots); > > Despite of the many comments I had, the overall design looks good to me. Max
>>> Max Reitz mreitz@redhat.com> 2016/6/15 星期三 上午 1:43 >> ( mailto:mreitz@redhat.com) ...... >I have many comments, but don't worry, it's nothing that can't be fixed. >The overall design looks good to me. Thank you so much for reviewing the patch very carefully and gave me so many comments. I would take most of your comments but except some of below: ...... >Nit pick: The following code will always leave an empty line after >everything. I think that's superfluous, and it can be amended as follows >(if you want to amend it, that is; if you really like that empty line, >then feel free to disregard my suggestion): > >> + monitor_printf(mon, "\n"); > >Drop this. > >> + QTAILQ_FOREACH(image_entry, &image_list, next) { >> + if (QTAILQ_EMPTY(&image_entry->snapshots)) { >> + continue; >> + } > >Put monitor_printf(mon, "\n"); here. OK. >> + monitor_printf(mon, "List of partial (non-loadable) snapshots on '%s':", >> + image_entry->imagename); >> + monitor_printf(mon, "\n"); > >(Why did you not concatenate these two strings in a single >monitor_printf() call?) OK. >> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); >> + monitor_printf(mon, "\n"); > >Drop this. > >> + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { > >Put monitor_printf(mon, "\n"); here. If so, It causes the output looks like this: -FROM: List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 3 snapb 0 2016-06-16 17:37:25 00:00:00.000 4 snapc 0 2016-06-16 17:37:30 00:00:00.000 5 snap2 0 2016-06-16 17:37:34 00:00:00.000 (qemu) -TO: List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 3 snapb 0 2016-06-16 17:37:25 00:00:00.000 4 snapc 0 2016-06-16 17:37:30 00:00:00.000 5 snap2 0 2016-06-16 17:37:34 00:00:00.000 (qemu) So I'll keep the code. >> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, >> + snapshot_entry->sn); >> + monitor_printf(mon, "\n"); > >And drop this. Again, the suggestions on moving the >monitor_printf(mon, "\n"); calls around are just suggestions, and it's >up to you whether you want to follow them or not. If so, It causes the output looks like this: -FROM: List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 3 snapb 0 2016-06-16 17:37:25 00:00:00.000 4 snapc 0 2016-06-16 17:37:30 00:00:00.000 5 snap2 0 2016-06-16 17:37:34 00:00:00.000 (qemu) -TO: List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 3 snapb 0 2016-06-16 17:37:25 00:00:00.0004 snapc 0 2016-06-16 17:37:30 00:00:00.0005 snap2 0 2016-06-16 17:37:34 00:00:00.000(qemu) So I'll keep the code. > >> + } >> + } >> + > >You're leaking all entries in the image_list here, and subsequently all >snapshots in the snapshots list of each image, and also the imagename of >each image_list entry. The latter wouldn't occur if you made imagename a >const char * and drop the g_strdup() when assigning is, as I have >suggested somewhere above. > >> g_free(sn_tab); >> g_free(available_snapshots); >> >> OK. Thanks again, Lin
On 17.06.2016 10:18, Lin Ma wrote: > > >>>> Max Reitz mreitz@redhat.com> 2016/6/15 星期三 上午 1:43 >> > <mailto:mreitz@redhat.com> 2016/6/15 星期三 上午 1:43 >>> > ...... >>I have many comments, but don't worry, it's nothing that can't be fixed. >>The overall design looks good to me. > Thank you so much for reviewing the patch very carefully and gave me so many > comments. I would take most of your comments but except some of below: > > ...... >>Nit pick: The following code will always leave an empty line after >>everything. I think that's superfluous, and it can be amended as follows >>(if you want to amend it, that is; if you really like that empty line, >>then feel free to disregard my suggestion): >> >>> + monitor_printf(mon, "\n"); >> >>Drop this. >> >>> + QTAILQ_FOREACH(image_entry, &image_list, next) { >>> + if (QTAILQ_EMPTY(&image_entry->snapshots)) { >>> + continue; >>> + } >> >>Put monitor_printf(mon, "\n"); here. > OK. > >>> + monitor_printf(mon, "List of partial (non-loadable) > snapshots on '%s':", >>> + image_entry->imagename); >>> + monitor_printf(mon, "\n"); >> >>(Why did you not concatenate these two strings in a single >>monitor_printf() call?) > OK. > >>> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); >>> + monitor_printf(mon, "\n"); >> >>Drop this. >> >>> + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { >> >>Put monitor_printf(mon, "\n"); here. > If so, It causes the output looks like this: > -FROM: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOCK > 3 snapb 0 2016-06-16 17:37:25 00:00:00.000 > 4 snapc 0 2016-06-16 17:37:30 00:00:00.000 > 5 snap2 0 2016-06-16 17:37:34 00:00:00.000 > (qemu) > -TO: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOCK > 3 snapb 0 2016-06-16 17:37:25 00:00:00.000 > > 4 snapc 0 2016-06-16 17:37:30 00:00:00.000 > > 5 snap2 0 2016-06-16 17:37:34 00:00:00.000 > (qemu) > > So I'll keep the code. > >>> + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, >>> + snapshot_entry->sn); >>> + monitor_printf(mon, "\n"); >> >>And drop this. Again, the suggestions on moving the >>monitor_printf(mon, "\n"); calls around are just suggestions, and it's >>up to you whether you want to follow them or not. > If so, It causes the output looks like this: > -FROM: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOCK > 3 snapb 0 2016-06-16 17:37:25 00:00:00.000 > 4 snapc 0 2016-06-16 17:37:30 00:00:00.000 > 5 snap2 0 2016-06-16 17:37:34 00:00:00.000 > (qemu) > -TO: > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLOCK > 3 snapb 0 2016-06-16 17:37:25 00:00:00.0004 snapc 0 2016-06-16 17:37:30 00:00:00.0005 snap2 0 2016-06-16 17:37:34 > 00:00:00.000(qemu) > > So I'll keep the code. Well, the idea was to do all of the suggestions, and then these two would counteract each other. However, I just noticed that I was completely wrong about my nit pick anyway. The code won't leave an empty line after printing everything, I made a mistake there. My suggestion instead leads to not having an end-of-line after everything, which is definitely wrong (sorry!). So you should probably leave all the monitor_printf(mon, "\n") statements as they are, except the one where I asked about concatenating it with the previous one. Max
diff --git a/migration/savevm.c b/migration/savevm.c index 6c21231..8444c62 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2153,12 +2153,28 @@ void hmp_delvm(Monitor *mon, const QDict *qdict) void hmp_info_snapshots(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; + BdrvNextIterator it1; QEMUSnapshotInfo *sn_tab, *sn; - int nb_sns, i; + bool no_snapshot = true; + int nb_sns, nb_sns_tmp, i; int total; int *available_snapshots; AioContext *aio_context; + typedef struct SnapshotEntry { + QEMUSnapshotInfo *sn; + QTAILQ_ENTRY(SnapshotEntry) next; + } SnapshotEntry; + + typedef struct ImageEntry { + char *imagename; + QTAILQ_ENTRY(ImageEntry) next; + QTAILQ_HEAD(, SnapshotEntry) snapshots; + } ImageEntry; + + ImageEntry *image_entry; + SnapshotEntry *snapshot_entry; + bs = bdrv_all_find_vmstate_bs(); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); @@ -2175,7 +2191,34 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) return; } - if (nb_sns == 0) { + QTAILQ_HEAD(image_list, ImageEntry) image_list = + QTAILQ_HEAD_INITIALIZER(image_list); + + for (bs1 = bdrv_first(&it1); bs1; bs1 = bdrv_next(&it1)) { + AioContext *ctx = bdrv_get_aio_context(bs); + + aio_context_acquire(ctx); + nb_sns_tmp = 0; + if (bdrv_can_snapshot(bs1)) { + nb_sns_tmp = bdrv_snapshot_list(bs1, &sn); + if (nb_sns_tmp > 0) { + no_snapshot = false; + ImageEntry *ie = g_malloc0(sizeof(*ie)); + ie->imagename = g_strdup(bdrv_get_device_name(bs1)); + QTAILQ_INIT(&ie->snapshots); + QTAILQ_INSERT_TAIL(&image_list, ie, next); + int x; + for (x = 0; x < nb_sns_tmp; x++) { + SnapshotEntry *se = g_malloc0(sizeof(*se)); + se->sn = &sn[x]; + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next); + } + } + } + aio_context_release(ctx); + } + + if (no_snapshot) { monitor_printf(mon, "There is no snapshot available.\n"); return; } @@ -2183,17 +2226,28 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) available_snapshots = g_new0(int, nb_sns); total = 0; for (i = 0; i < nb_sns; i++) { - if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) { + if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) { available_snapshots[total] = i; total++; } } + monitor_printf(mon, "List of snapshots present on all disks:\n"); + if (total > 0) { bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); monitor_printf(mon, "\n"); for (i = 0; i < total; i++) { sn = &sn_tab[available_snapshots[i]]; + QTAILQ_FOREACH(image_entry, &image_list, next) { + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { + if (!strcmp(sn->name, snapshot_entry->sn->name)) { + QTAILQ_REMOVE(&image_entry->snapshots, snapshot_entry, + next); + } + } + } + pstrcpy(sn->id_str, sizeof(sn->id_str), "--"); bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, sn); monitor_printf(mon, "\n"); } @@ -2201,6 +2255,23 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) monitor_printf(mon, "There is no suitable snapshot available\n"); } + monitor_printf(mon, "\n"); + QTAILQ_FOREACH(image_entry, &image_list, next) { + if (QTAILQ_EMPTY(&image_entry->snapshots)) { + continue; + } + monitor_printf(mon, "List of partial (non-loadable) snapshots on '%s':", + image_entry->imagename); + monitor_printf(mon, "\n"); + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL); + monitor_printf(mon, "\n"); + QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) { + bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, + snapshot_entry->sn); + monitor_printf(mon, "\n"); + } + } + g_free(sn_tab); g_free(available_snapshots);
Currently, the output of 'info snapshots' shows fully available snapshots. In my opinion there are 2 disadvantages: 1. It's opaque, hides some snapshot information to users. It's not convenient if users want to know more about all of snapshot information on every block device via monitor. 2. It uses snapshot id to determine whether a snapshot is 'fully available', It causes incorrect output in some scenario. For instance: (qemu) info block drive_image1 (#block113): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk0.qcow2 (qcow2) Cache mode: writeback drive_image2 (#block349): /opt/vms/SLES12-SP1-JeOS-x86_64-GM/disk1.qcow2 (qcow2) Cache mode: writeback (qemu) (qemu) info snapshots There is no snapshot available. (qemu) (qemu) snapshot_blkdev_internal drive_image1 snap1 (qemu) (qemu) info snapshots There is no suitable snapshot available (qemu) (qemu) savevm checkpoint-1 (qemu) (qemu) info snapshots ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 (qemu) $ qemu-img snapshot -l disk0.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 2 checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 $ qemu-img snapshot -l disk1.qcow2 Snapshot list: ID TAG VM SIZE DATE VM CLOCK 1 checkpoint-1 0 2016-05-22 16:58:07 00:02:06.813 The patch uses snapshot name instead of snapshot id to determine whether a snapshot is 'fully available', and follow Kevin's suggestion, Make the output more detailed/accurate: (qemu) info snapshots List of snapshots present on all disks: ID TAG VM SIZE DATE VM CLOCK -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.813 List of partial (non-loadable) snapshots on 'drive_image1': ID TAG VM SIZE DATE VM CLOCK 1 snap1 0 2016-05-22 16:57:31 00:01:30.567 Signed-off-by: Lin Ma <lma@suse.com> --- migration/savevm.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 74 insertions(+), 3 deletions(-)