From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHaE9-0005gz-FY for qemu-devel@nongnu.org; Mon, 27 Jun 2016 13:21:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHaE5-0008SL-BF for qemu-devel@nongnu.org; Mon, 27 Jun 2016 13:21:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44635) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHaE5-0008SE-2d for qemu-devel@nongnu.org; Mon, 27 Jun 2016 13:21:37 -0400 References: <1466152443-15660-1-git-send-email-lma@suse.com> <1466152443-15660-3-git-send-email-lma@suse.com> From: Max Reitz Message-ID: <29536906-ff41-4f6a-7601-fb039366f877@redhat.com> Date: Mon, 27 Jun 2016 19:21:34 +0200 MIME-Version: 1.0 In-Reply-To: <1466152443-15660-3-git-send-email-lma@suse.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="HEDt35Uh2gg0SOKTkd7GnXv8ItLiPCQj4" Subject: Re: [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lin Ma , kwolf@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --HEDt35Uh2gg0SOKTkd7GnXv8ItLiPCQj4 From: Max Reitz To: Lin Ma , kwolf@redhat.com, qemu-devel@nongnu.org Message-ID: <29536906-ff41-4f6a-7601-fb039366f877@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/2 V2] hmp: show all of snapshot info on every block dev in output of 'info snapshots' References: <1466152443-15660-1-git-send-email-lma@suse.com> <1466152443-15660-3-git-send-email-lma@suse.com> In-Reply-To: <1466152443-15660-3-git-send-email-lma@suse.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 17.06.2016 10:34, Lin Ma wrote: > Currently, the output of 'info snapshots' shows fully available snapsho= ts. > It's opaque, hides some snapshot information to users. It's not conveni= ent > if users want to know more about all of snapshot information on every b= lock > device via monitor. >=20 > Follow Kevin's and Max's proposals, The patch make the output more deta= iled: > (qemu) info snapshots > List of snapshots present on all disks: > ID TAG VM SIZE DATE VM CLO= CK > -- checkpoint-1 165M 2016-05-22 16:58:07 00:02:06.8= 13 >=20 > List of partial (non-loadable) snapshots on 'drive_image1': > ID TAG VM SIZE DATE VM CLO= CK > 1 snap1 0 2016-05-22 16:57:31 00:01:30.5= 67 >=20 > Signed-off-by: Lin Ma > --- > migration/savevm.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++= +++----- > 1 file changed, 83 insertions(+), 7 deletions(-) >=20 > diff --git a/migration/savevm.c b/migration/savevm.c > index 0c4e0d9..b78e37e 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2190,12 +2190,31 @@ 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; > + bool no_snapshot =3D true; > int nb_sns, i; > int total; > - int *available_snapshots; > + int *global_snapshots; > AioContext *aio_context; > =20 > + typedef struct SnapshotEntry { > + QEMUSnapshotInfo sn; > + QTAILQ_ENTRY(SnapshotEntry) next; > + } SnapshotEntry; > + > + typedef struct ImageEntry { > + const char *imagename; > + QTAILQ_ENTRY(ImageEntry) next; > + QTAILQ_HEAD(, SnapshotEntry) snapshots; > + } ImageEntry; > + > + QTAILQ_HEAD(image_list, ImageEntry) image_list =3D I think this should be just QTAILQ_HEAD(, ImageEntry). There's no need to name the type. > + QTAILQ_HEAD_INITIALIZER(image_list); > + > + ImageEntry *image_entry; > + SnapshotEntry *snapshot_entry; > + > bs =3D bdrv_all_find_vmstate_bs(); > if (!bs) { > monitor_printf(mon, "No available block device supports snapsh= ots\n"); > @@ -2212,25 +2231,65 @@ void hmp_info_snapshots(Monitor *mon, const QDi= ct *qdict) > return; > } > =20 > - if (nb_sns =3D=3D 0) { > + for (bs1 =3D bdrv_first(&it1); bs1; bs1 =3D bdrv_next(&it1)) { > + int bs1_nb_sns =3D 0; > + ImageEntry *ie; > + SnapshotEntry *se; > + AioContext *ctx =3D bdrv_get_aio_context(bs1); > + > + aio_context_acquire(ctx); > + if (bdrv_can_snapshot(bs1)) { > + sn =3D NULL; > + bs1_nb_sns =3D bdrv_snapshot_list(bs1, &sn); > + if (bs1_nb_sns > 0) { > + no_snapshot =3D false; > + ie =3D g_new0(ImageEntry, 1); > + ie->imagename =3D bdrv_get_device_name(bs1); > + QTAILQ_INIT(&ie->snapshots); > + QTAILQ_INSERT_TAIL(&image_list, ie, next); > + for (i =3D 0; i < bs1_nb_sns; i++) { > + se =3D g_new0(SnapshotEntry, 1); > + se->sn =3D sn[i]; > + QTAILQ_INSERT_TAIL(&ie->snapshots, se, next); > + } > + } > + g_free(sn); > + } > + aio_context_release(ctx); > + } > + > + if (no_snapshot) { > monitor_printf(mon, "There is no snapshot available.\n"); > return; > } > =20 > - available_snapshots =3D g_new0(int, nb_sns); > + global_snapshots =3D g_new0(int, nb_sns); > total =3D 0; > for (i =3D 0; i < nb_sns; i++) { > + SnapshotEntry *tmp; That is not a very expressive variable name. I'd call it next_sn or something like that, even if you don't actively use it. > if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) =3D=3D 0) { > - available_snapshots[total] =3D i; > + global_snapshots[total] =3D i; > total++; > + QTAILQ_FOREACH(image_entry, &image_list, next) { > + QTAILQ_FOREACH_SAFE(snapshot_entry, &image_entry->snap= shots, > + next, tmp) { > + if (!strcmp(sn_tab[i].name, snapshot_entry->sn.nam= e)) { > + QTAILQ_REMOVE(&image_entry->snapshots, snapsho= t_entry, > + next); In case a line needs to be wrapped in the middle of a function call (or macro) parameter list, the following lines should be aligned to the opening parenthesis (as you have done for bdrv_snapshot_dump() below). > + g_free(snapshot_entry); > + } > + } > + } > } > } > =20 > + 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 =3D 0; i < total; i++) { > - sn =3D &sn_tab[available_snapshots[i]]; > + sn =3D &sn_tab[global_snapshots[i]]; > /* The ID is not guaranteed to be the same on all images, = so > * overwrite it. > */ > @@ -2239,11 +2298,28 @@ void hmp_info_snapshots(Monitor *mon, const QDi= ct *qdict) > monitor_printf(mon, "\n"); > } > } else { > - monitor_printf(mon, "There is no suitable snapshot available\n= "); > + monitor_printf(mon, "None\n"); > + } > + > + QTAILQ_FOREACH(image_entry, &image_list, next) { > + if (QTAILQ_EMPTY(&image_entry->snapshots)) { > + continue; > + } > + monitor_printf(mon, "\n"); > + monitor_printf(mon, "List of partial (non-loadable) snapshots = on '%s':" > + "\n", > + image_entry->imagename); Sorry that I'm nagging about this again, but I'd again combine these consecutive monitor_printf()s to: monitor_printf(mon, "\nList of partial (non-loadable) snapshots on '%s':\n", image_entry->imagename); > + 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"); > + } > } > =20 > g_free(sn_tab); > - g_free(available_snapshots); > + g_free(global_snapshots); > =20 You're still leaking the image_list and thus all snapshot lists here. All of the entries in image_list and all of the entries in their ImageEntry.snapshots lists still need to be freed. Max > } > =20 >=20 --HEDt35Uh2gg0SOKTkd7GnXv8ItLiPCQj4 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEvBAEBCAAZBQJXcWCeEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrdBx B/9r9WNdmKFwNeAx7PCuzlml1D+6vUdnpmxKcXWVfjR3F8dUuZPRNAmN3tLqvLNQ AK5Tl96kpw6J0LYu1RH0BJ1cqtFK9r4VqWpMMDmVD+7lu09hXEy4y+wl6o40zVXW +Y/QY4a4s4pbOhw1H0FEVStsmqJsMw3BFP0S/jxi0wWv3HJfas84VdU8cZ6TiQ9Y ongBEHlKG2sAA5+PPWymt6rq1Qp26HFnDa3+uR1hiyh6HYf+xyS3CTvkz8FHN0pE jBYFr+UhYf54fZ3i1wmMAa5hqsT9Mwmw2b4GvVpg7C6krO7d3QXlkrBjz4IeTP74 gwFJNaNCd+ueVllxk9bC3Mxo =sWJx -----END PGP SIGNATURE----- --HEDt35Uh2gg0SOKTkd7GnXv8ItLiPCQj4--