From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56900 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oe9dJ-0002sZ-5B for qemu-devel@nongnu.org; Wed, 28 Jul 2010 12:37:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oe9dH-00036j-Rx for qemu-devel@nongnu.org; Wed, 28 Jul 2010 12:37:29 -0400 Received: from mail-wy0-f173.google.com ([74.125.82.173]:62147) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oe9dH-00036F-I5 for qemu-devel@nongnu.org; Wed, 28 Jul 2010 12:37:27 -0400 Received: by wyi11 with SMTP id 11so4648734wyi.4 for ; Wed, 28 Jul 2010 09:37:25 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1280242448-20775-1-git-send-email-miguel.filho@gmail.com> Date: Wed, 28 Jul 2010 13:37:25 -0300 Message-ID: Subject: Re: [Qemu-devel] [PATCH] monitor: make 'info snapshots' show only fully available snapshots From: Miguel Di Ciurcio Filho Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com On Wed, Jul 28, 2010 at 12:38 PM, Markus Armbruster wro= te: > Miguel Di Ciurcio Filho writes: > >> The output generated by 'info snapshots' shows only snapshots that exist= on the >> block device that saves the VM state. This output can cause an user to >> erroneously try to load an snapshot that is not available on all block d= evices. > > What happens when you try that? > I've sent a patch that will protect that from happening [1]. With that patch, the VM stays stopped, without it the VM keeps running with a failed bdrv_snapshot_goto(). > >> --- >> =A0savevm.c | =A0 65 ++++++++++++++++++++++++++++++++++++++++++---------= ---------- >> =A01 files changed, 45 insertions(+), 20 deletions(-) >> >> diff --git a/savevm.c b/savevm.c >> index 7a1de3c..be83878 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -1997,37 +1997,62 @@ void do_delvm(Monitor *mon, const QDict *qdict) >> >> =A0void do_info_snapshots(Monitor *mon) >> =A0{ >> - =A0 =A0BlockDriverState *bs, *bs1; >> - =A0 =A0QEMUSnapshotInfo *sn_tab, *sn; >> - =A0 =A0int nb_sns, i; >> + =A0 =A0BlockDriverState *bs_vm_state, *bs; >> + =A0 =A0QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info =3D &s; >> + =A0 =A0int nb_sns, i, ret, available; >> + =A0 =A0int total; >> + =A0 =A0int *available_snapshots; >> =A0 =A0 =A0char buf[256]; >> >> - =A0 =A0bs =3D bdrv_snapshots(); >> - =A0 =A0if (!bs) { >> + =A0 =A0bs_vm_state =3D bdrv_snapshots(); >> + =A0 =A0if (!bs_vm_state) { >> =A0 =A0 =A0 =A0 =A0monitor_printf(mon, "No available block device suppor= ts snapshots\n"); >> =A0 =A0 =A0 =A0 =A0return; >> =A0 =A0 =A0} >> - =A0 =A0monitor_printf(mon, "Snapshot devices:"); >> - =A0 =A0bs1 =3D NULL; >> - =A0 =A0while ((bs1 =3D bdrv_next(bs1))) { >> - =A0 =A0 =A0 =A0if (bdrv_can_snapshot(bs1)) { >> - =A0 =A0 =A0 =A0 =A0 =A0if (bs =3D=3D bs1) >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0monitor_printf(mon, " %s", bdrv_get_dev= ice_name(bs1)); >> - =A0 =A0 =A0 =A0} >> - =A0 =A0} >> - =A0 =A0monitor_printf(mon, "\n"); >> >> - =A0 =A0nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); >> + =A0 =A0nb_sns =3D bdrv_snapshot_list(bs_vm_state, &sn_tab); >> =A0 =A0 =A0if (nb_sns < 0) { >> =A0 =A0 =A0 =A0 =A0monitor_printf(mon, "bdrv_snapshot_list: error %d\n",= nb_sns); >> =A0 =A0 =A0 =A0 =A0return; >> + =A0 =A0} else if (nb_sns =3D=3D 0) { >> + =A0 =A0 =A0 =A0monitor_printf(mon, "There is no snapshot available.\n"= ); >> =A0 =A0 =A0} > > This changes output for the "no snapshots available" case from the empty > table > > =A0 =A0ID =A0 =A0 =A0 =A0TAG =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 VM SIZE =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0DATE =A0 =A0 =A0 VM CLOCK > > to > > =A0 =A0There is no snapshot available. > > I'd prefer that as separate patch, if at all. I think a clear message saying "there is nothing" is better than an empty table. I'm already changing the output to something more reasonable, so. > > Nitpick: I don't like "return; else". > Yeah, kinda ugly. I will fix it. >> - =A0 =A0monitor_printf(mon, "Snapshot list (from %s):\n", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdrv_get_device_name(bs)); >> - =A0 =A0monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf)= , NULL)); >> - =A0 =A0for(i =3D 0; i < nb_sns; i++) { >> + >> + =A0 =A0available_snapshots =3D qemu_mallocz(sizeof(int) * nb_sns); > > This can die due to the nonsensical semantics of qemu_mallocz(0). > Will fix that, so this code will be reached only if nb_sns > 0 and qemu_mallocz(0) will never be executed. >> + =A0 =A0total =3D 0; >> + =A0 =A0for (i =3D 0; i < nb_sns; i++) { >> =A0 =A0 =A0 =A0 =A0sn =3D &sn_tab[i]; >> - =A0 =A0 =A0 =A0monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, siz= eof(buf), sn)); >> + =A0 =A0 =A0 =A0available =3D 1; >> + =A0 =A0 =A0 =A0bs =3D NULL; >> + >> + =A0 =A0 =A0 =A0while ((bs =3D bdrv_next(bs))) { >> + =A0 =A0 =A0 =A0 =A0 =A0if (bdrv_can_snapshot(bs) && bs !=3D bs_vm_stat= e) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D bdrv_snapshot_find(bs, sn_info,= sn->id_str); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret < 0) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0available =3D 0; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} >> + =A0 =A0 =A0 =A0 =A0 =A0} >> + =A0 =A0 =A0 =A0} >> + >> + =A0 =A0 =A0 =A0if (available) { >> + =A0 =A0 =A0 =A0 =A0 =A0available_snapshots[total] =3D i; >> + =A0 =A0 =A0 =A0 =A0 =A0total++; >> + =A0 =A0 =A0 =A0} >> =A0 =A0 =A0} >> + >> + =A0 =A0if (total > 0) { >> + =A0 =A0 =A0 =A0monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, siz= eof(buf), NULL)); >> + =A0 =A0 =A0 =A0for (i =3D 0; i < total; i++) { >> + =A0 =A0 =A0 =A0 =A0 =A0sn =3D &sn_tab[available_snapshots[i]]; >> + =A0 =A0 =A0 =A0 =A0 =A0monitor_printf(mon, "%s\n", bdrv_snapshot_dump(= buf, sizeof(buf), sn)); >> + =A0 =A0 =A0 =A0} >> + >> + =A0 =A0 =A0 =A0qemu_free(available_snapshots); >> + >> + =A0 =A0} else { >> + =A0 =A0 =A0 =A0monitor_printf(mon, "There is no suitable snapshot avai= lable to be loaded.\n"); > > Where is available_snapshots freed when control flows through this point? > Oops. I will fix that too. Thanks for the feedback. Regards, Miguel [1] http://lists.gnu.org/archive/html/qemu-devel/2010-07/msg01065.html (Kevin has applied it to this block branch)