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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4206 bytes --]

On 17.06.2016 10:18, Lin Ma wrote:
> 
> 
>>>> Max Reitz mreitz@redhat.com> 2016/6/15 星期三 上午 1:43 >>
> <mailto:mreitz@redhat.com> 2016/6/15 星期三 上午 1:43 >>>
> ......
>>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.

Well, the idea was to do all of the suggestions, and then these two
would counteract each other.

However, I just noticed that I was completely wrong about my nit pick
anyway. The code won't leave an empty line after printing everything, I
made a mistake there.

My suggestion instead leads to not having an end-of-line after
everything, which is definitely wrong (sorry!).

So you should probably leave all the monitor_printf(mon, "\n")
statements as they are, except the one where I asked about concatenating
it with the previous one.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]