On Wed, Jul 28, 2010 at 12:38 PM, Markus Armbruster <arm...@redhat.com> wrote:
> Miguel Di Ciurcio Filho <miguel.fi...@gmail.com> 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
>> devices.
>
> 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().
>
>> ---
>> savevm.c | 65
>> ++++++++++++++++++++++++++++++++++++++++++-------------------
>> 1 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)
>>
>> void do_info_snapshots(Monitor *mon)
>> {
>> - BlockDriverState *bs, *bs1;
>> - QEMUSnapshotInfo *sn_tab, *sn;
>> - int nb_sns, i;
>> + BlockDriverState *bs_vm_state, *bs;
>> + QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
>> + int nb_sns, i, ret, available;
>> + int total;
>> + int *available_snapshots;
>> char buf[256];
>>
>> - bs = bdrv_snapshots();
>> - if (!bs) {
>> + bs_vm_state = bdrv_snapshots();
>> + if (!bs_vm_state) {
>> monitor_printf(mon, "No available block device supports
>> snapshots\n");
>> return;
>> }
>> - monitor_printf(mon, "Snapshot devices:");
>> - bs1 = NULL;
>> - while ((bs1 = bdrv_next(bs1))) {
>> - if (bdrv_can_snapshot(bs1)) {
>> - if (bs == bs1)
>> - monitor_printf(mon, " %s", bdrv_get_device_name(bs1));
>> - }
>> - }
>> - monitor_printf(mon, "\n");
>>
>> - nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> + nb_sns = bdrv_snapshot_list(bs_vm_state, &sn_tab);
>> if (nb_sns < 0) {
>> monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
>> return;
>> + } else if (nb_sns == 0) {
>> + monitor_printf(mon, "There is no snapshot available.\n");
>> }
>
> This changes output for the "no snapshots available" case from the empty
> table
>
> ID TAG VM SIZE DATE VM CLOCK
>
> to
>
> There 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.
>> - monitor_printf(mon, "Snapshot list (from %s):\n",
>> - bdrv_get_device_name(bs));
>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>> - for(i = 0; i < nb_sns; i++) {
>> +
>> + available_snapshots = 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.
>> + total = 0;
>> + for (i = 0; i < nb_sns; i++) {
>> sn = &sn_tab[i];
>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf),
>> sn));
>> + available = 1;
>> + bs = NULL;
>> +
>> + while ((bs = bdrv_next(bs))) {
>> + if (bdrv_can_snapshot(bs) && bs != bs_vm_state) {
>> + ret = bdrv_snapshot_find(bs, sn_info, sn->id_str);
>> + if (ret < 0) {
>> + available = 0;
>> + break;
>> + }
>> + }
>> + }
>> +
>> + if (available) {
>> + available_snapshots[total] = i;
>> + total++;
>> + }
>> }
>> +
>> + if (total > 0) {
>> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf),
>> NULL));
>> + for (i = 0; i < total; i++) {
>> + sn = &sn_tab[available_snapshots[i]];
>> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>> sizeof(buf), sn));
>> + }
>> +
>> + qemu_free(available_snapshots);
>> +
>> + } else {
>> + monitor_printf(mon, "There is no suitable snapshot available 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)
