Re: [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

>>   
>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>> +{
>> +    BlockDriverState *bs;
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i;
>> +    char buf[256];
>> +
>> +    /* find the target bs */
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>> +        return ;
>> +    }
>> +
>> +    if (!bdrv_can_snapshot(bs)) {
>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>> +        return ;
>> +    }
> 
> Rest of the function is is essentially code copied from
> do_info_snapshots_vm().  Could it be factored out?
> 
  Sure, I forgot that before. I guess a better way is adding a QMP
interface first and using common code from it.

>> +
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n",
>> +                       device, nb_sns);
>> +        return;
>> +    }
>> +
>> +    if (nb_sns == 0) {
>> +        monitor_printf(mon, "There is no snapshot available.\n");
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "Device %s:\n", device);
>> +    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>> +    for (i = 0; i < nb_sns; i++) {
>> +        sn = &sn_tab[i];
>> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
>> +    }
>> +    g_free(sn_tab);
>> +    return;
>> +}
>> +
>> +void do_info_snapshots(Monitor *mon, const QDict *qdict)
>> +{
>> +    /* Todo, there should be a layer rebuild qdict before enter this func. */
> 
> I'm not sure I get this comment.
> 
> We usually capitalize TODO for easy grepping.
> 
  My mistake, this comments should have been removed, it is used before
where qdict was not rebuilt as a new one.

>> +    const char *device = qdict_get_try_str(qdict, "device");
>> +    if (!device) {
>> +        do_info_snapshots_vm(mon);
>> +    } else {
>> +        do_info_snapshots_blk(mon, device);
>> +    }
>> +    return;
>> +}
>> +
>>   void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
>>   {
>>       qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
> 


-- 
Best Regards

Wenchao Xia