[Qemu-devel] 答复： Re: [PATCH] Show all of snapshot info on every block device in output of 'info snapshots'

[Lin Ma]

>>> Max Reitz address@hidden> 2016/6/15 星期三 上午 1:43 >>
( mailto:address@hidden) 
......
>I have many comments, but don't worry, it's nothing that can't be fixed.
>The overall design looks good to me.
Thank you so much for reviewing the patch very carefully and gave me so many
comments. I would take most of your comments but except some of below:

......
>Nit pick: The following code will always leave an empty line after
>everything. I think that's superfluous, and it can be amended as follows
>(if you want to amend it, that is; if you really like that empty line,
>then feel free to disregard my suggestion):
>
>> +    monitor_printf(mon, "\n");
>
>Drop this.
>
>> +    QTAILQ_FOREACH(image_entry, &image_list, next) {
>> +        if (QTAILQ_EMPTY(&image_entry->snapshots)) {
>> +                continue;
>> +        }
>
>Put monitor_printf(mon, "\n"); here.
OK.

>> +        monitor_printf(mon, "List of partial (non-loadable) snapshots on 
>> '%s':",
>> +                                       image_entry->imagename);
>> +        monitor_printf(mon, "\n");
>
>(Why did you not concatenate these two strings in a single
>monitor_printf() call?)
OK.

>> +        bdrv_snapshot_dump((fprintf_function)monitor_printf, mon, NULL);
>> +        monitor_printf(mon, "\n");
>
>Drop this.
>
>> +        QTAILQ_FOREACH(snapshot_entry, &image_entry->snapshots, next) {
>
>Put monitor_printf(mon, "\n"); here.
If so, It causes the output looks like this:
-FROM:
List of partial (non-loadable) snapshots on 'drive_image1':
ID          TAG                             VM SIZE                        DATE 
         VM CLOCK
3                snapb                                   0 2016-06-16 17:37:25  
 00:00:00.000
4                snapc                                   0 2016-06-16 17:37:30  
 00:00:00.000
5                snap2                                   0 2016-06-16 17:37:34  
 00:00:00.000
(qemu)
-TO:
List of partial (non-loadable) snapshots on 'drive_image1':
ID          TAG                             VM SIZE                        DATE 
         VM CLOCK
3                snapb                                   0 2016-06-16 17:37:25  
 00:00:00.000
 
4                snapc                                   0 2016-06-16 17:37:30  
 00:00:00.000
 
5                snap2                                   0 2016-06-16 17:37:34  
 00:00:00.000
(qemu)
 
So I'll keep the code.
 
>> +                bdrv_snapshot_dump((fprintf_function)monitor_printf, mon,
>> +                                                       snapshot_entry->sn);
>> +                monitor_printf(mon, "\n");
>
>And drop this. Again, the suggestions on moving the
>monitor_printf(mon, "\n"); calls around are just suggestions, and it's
>up to you whether you want to follow them or not.
If so, It causes the output looks like this:
-FROM:
List of partial (non-loadable) snapshots on 'drive_image1':
ID          TAG                             VM SIZE                        DATE 
         VM CLOCK
3                snapb                                   0 2016-06-16 17:37:25  
 00:00:00.000
4                snapc                                   0 2016-06-16 17:37:30  
 00:00:00.000
5                snap2                                   0 2016-06-16 17:37:34  
 00:00:00.000
(qemu)
-TO:
List of partial (non-loadable) snapshots on 'drive_image1':
ID          TAG                             VM SIZE                        DATE 
         VM CLOCK
3                snapb                                   0 2016-06-16 17:37:25  
 00:00:00.0004             snapc                                   0 2016-06-16 
17:37:30   00:00:00.0005                 snap2                                  
 0 2016-06-16 17:37:34   00:00:00.000(qemu)
 
So I'll keep the code.
 
>
>> +        }
>> +    }
>> +
>
>You're leaking all entries in the image_list here, and subsequently all
>snapshots in the snapshots list of each image, and also the imagename of
>each image_list entry. The latter wouldn't occur if you made imagename a
>const char * and drop the g_strdup() when assigning is, as I have
>suggested somewhere above.
>
>>        g_free(sn_tab);
>>        g_free(available_snapshots);
>>  
>> 
OK.
 
Thanks again,
Lin