[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] 答复: Re: [PATCH] Show all of snapshot info on every block d
From: |
Lin Ma |
Subject: |
[Qemu-devel] 答复: Re: [PATCH] Show all of snapshot info on every block device in output of 'info snapshots' |
Date: |
Fri, 17 Jun 2016 02:18:35 -0600 |
>>> 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