[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] 答复: Re: [PATCH] Show all of snapshot info on every bl
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] 答复: Re: [PATCH] Show all of snapshot info on every block device in output of 'info snapshots' |
Date: |
Sat, 18 Jun 2016 14:49:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 17.06.2016 10:18, Lin Ma wrote:
>
>
>>>> Max Reitz address@hidden> 2016/6/15 星期三 上午 1:43 >>
> <mailto:address@hidden> 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
signature.asc
Description: OpenPGP digital signature