Re: [Qemu-devel] [PATCH] monitor: make 'info snapshots' show only fully available snapshots

[Miguel Di Ciurcio Filho]

On Wed, Jul 28, 2010 at 12:38 PM, Markus Armbruster <address@hidden> wrote:
> Miguel Di Ciurcio Filho <address@hidden> 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)