Re: [Qemu-devel] [PATCH V7 08/14] qmp: add interface query-images.

Re: [Qemu-devel] [PATCH V7 08/14] qmp: add interface query-images.

[Wenchao Xia]

>>>    
>>>    ##
>>> +# @query-images:
>>> +#
>>> +# Get a list of DeviceImageInfo for all virtual block devices.
>>
>> # Get block device image information
>>
>    OK.
> 
>>> +#
>>> +# @device: #optional the name of the device to get image info from. If not
>>> +#          specified, all block devices will be queried
>>> +# @backing: #optional true to show information on backing images, false or
>>> +#          omitted to show just the top image of a block device
>>
>> I'm not sure these flags are necessary.
>>
>    These flag seems more flex to me. Always querying all info require
> caller do a filter operation on what he got, but in many times caller
> may be interested on only one device.
>
  Coding for next version, I still feel flag may bring flexbility.
Markus, are u OK with this?


>>> +#
>>> +# Returns: a list of @DeviceImageInfo describing each virtual block device
>>> +#
>>> +# Since: 1.5
>>> +##
>>> +{ 'command': 'query-images',
>>> +  'data': { '*device': 'str', '*backing': 'bool' },
>>> +  'returns': ['DeviceImageInfo'] }
>>> +



-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c

[Wenchao Xia]

于 2013-3-1 9:51, Wenchao Xia 写道:
> 于 2013-2-28 0:22, Kevin Wolf 写道:
>> Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>   block.c               |   24 ++++++++++++++++++++++++
>>>>   include/block/block.h |    2 ++
>>>>   savevm.c              |   22 ----------------------
>>>>   3 files changed, 26 insertions(+), 22 deletions(-)
>>>
>>> Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
>>> like it in block.c any better.  block.c is already unwieldy, and I'd
>>> like us to try limiting it to core block stuff.
>>>
>>> Kevin, Stefan, any ideas?
>>
>> Take some more snapshot related functions from block.c and move them to
>> block/snapshot.c?
>>
>> Kevin
>>
> Hi, Stefan
>    Do you also agree about adding new file block/snapshot.c?
>
>
Hi,
   I am going to add include/block/snapshot.h and block/snapshot.c,
all internal snapshot function in block.c will be moved there.
external backing file related function will not be touched and
remain in block.c. This consume many effort and once done it will
be hard to revert back to form an incremental commit in this
serials, so before coding, please give your opinion if u don't agree
about it.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c

[Stefan Hajnoczi]

On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
> >Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
> >>Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> >>
> >>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>---
> >>>  block.c               |   24 ++++++++++++++++++++++++
> >>>  include/block/block.h |    2 ++
> >>>  savevm.c              |   22 ----------------------
> >>>  3 files changed, 26 insertions(+), 22 deletions(-)
> >>
> >>Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
> >>like it in block.c any better.  block.c is already unwieldy, and I'd
> >>like us to try limiting it to core block stuff.
> >>
> >>Kevin, Stefan, any ideas?
> >
> >Take some more snapshot related functions from block.c and move them to
> >block/snapshot.c?
> >
> >Kevin
> >
> Hi, Stefan
>   Do you also agree about adding new file block/snapshot.c?

Yes, I agree.

Stefan

Re: [Qemu-devel] [PATCH V7 00/14] add qmp/hmp interfaces for internal snapshot info

[Stefan Hajnoczi]

On Fri, Mar 01, 2013 at 09:47:29AM +0800, Wenchao Xia wrote:
> 于 2013-2-28 23:57, Stefan Hajnoczi 写道:
> >On Tue, Feb 26, 2013 at 06:40:14PM +0800, Wenchao Xia wrote:
> >>   This serial of patches does two things: merge some info code
> >>in qemu-img, and add following interfaces:
> >>1) qmp: query-images
> >>2) qmp: query-snapshots
> >>3) hmp: show internal snapshot info on a single block device
> >>   These patches follows the rule that use qmp to retieve information,
> >>hmp layer just do a translation from qmp object it got, so almost
> >>every hmp interface may have a correlated qmp interface.
> >>   To make code graceful, snapshot retrieving code in qemu and qemu-img
> >>are merged into block.c, and some function name was adjusted to make it
> >>tips better. Now it works as:
> >>
> >>    qemu          qemu-img
> >>
> >>dump_monitor    dump_stdout
> >>      |--------------|
> >>             |
> >>            qmp
> >>             |
> >>           block
> >
> >This summary does not explain how the new commands relate to query-block
> >or why they are necessary.
> >
> >It is also useful to include QMP examples or the QMP docs in the cover
> >letter so reviewers know what you are trying to achieve.
> >
>   OK, a better explaination would be added in the cover-letter.

Thanks.  When reviewers don't know your goal at the start of their
review, they may object to things that make sense at the end of the
series because they do not have enough context yet.  A series where the
goal is unclear also risks a higher number of review-fix cycles because
the reviewers haven't bought into the idea yet.

It is in everyone's interest to explain the goal in the cover letter.
Then the patches get reviewed more smoothly and merged faster.

This is a general thought I wanted to share and I struggle with it
myself sometimes when I submit patch series.

Stefan

Re: [Qemu-devel] [PATCH V7 00/14] add qmp/hmp interfaces for internal snapshot info

[Eric Blake]

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

On 03/04/2013 06:10 AM, Stefan Hajnoczi wrote:
>>> It is also useful to include QMP examples or the QMP docs in the cover
>>> letter so reviewers know what you are trying to achieve.
>>>
>>   OK, a better explaination would be added in the cover-letter.
> 
> Thanks.  When reviewers don't know your goal at the start of their
> review, they may object to things that make sense at the end of the
> series because they do not have enough context yet.  A series where the
> goal is unclear also risks a higher number of review-fix cycles because
> the reviewers haven't bought into the idea yet.
> 
> It is in everyone's interest to explain the goal in the cover letter.
> Then the patches get reviewed more smoothly and merged faster.

This is good advice - so good that I added it to the wiki:
http://wiki.qemu.org/Contribute/SubmitAPatch

> 
> This is a general thought I wanted to share and I struggle with it
> myself sometimes when I submit patch series.

Me too - but the nice thing about open source is that we can all learn
best practices from one another and become better at it :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c

[Wenchao Xia]

于 2013-3-4 21:02, Stefan Hajnoczi 写道:
> On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
>>> Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>>
>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>>   block.c               |   24 ++++++++++++++++++++++++
>>>>>   include/block/block.h |    2 ++
>>>>>   savevm.c              |   22 ----------------------
>>>>>   3 files changed, 26 insertions(+), 22 deletions(-)
>>>>
>>>> Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
>>>> like it in block.c any better.  block.c is already unwieldy, and I'd
>>>> like us to try limiting it to core block stuff.
>>>>
>>>> Kevin, Stefan, any ideas?
>>>
>>> Take some more snapshot related functions from block.c and move them to
>>> block/snapshot.c?
>>>
>>> Kevin
>>>
>> Hi, Stefan
>>    Do you also agree about adding new file block/snapshot.c?
>
> Yes, I agree.
>
> Stefan
>
great. Also there are some other functions such as image info
collecting, do you think new file block/misc.c is suitable
to store those functions there?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c

[Stefan Hajnoczi]

On Tue, Mar 05, 2013 at 03:11:48PM +0800, Wenchao Xia wrote:
> 于 2013-3-4 21:02, Stefan Hajnoczi 写道:
> >On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
> >>>Am 27.02.2013 um 17:04 hat Markus Armbruster geschrieben:
> >>>>Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> >>>>
> >>>>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>>Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>>>---
> >>>>>  block.c               |   24 ++++++++++++++++++++++++
> >>>>>  include/block/block.h |    2 ++
> >>>>>  savevm.c              |   22 ----------------------
> >>>>>  3 files changed, 26 insertions(+), 22 deletions(-)
> >>>>
> >>>>Perhaps savevm.c isn't the best home for snapshot stuff, but I don't
> >>>>like it in block.c any better.  block.c is already unwieldy, and I'd
> >>>>like us to try limiting it to core block stuff.
> >>>>
> >>>>Kevin, Stefan, any ideas?
> >>>
> >>>Take some more snapshot related functions from block.c and move them to
> >>>block/snapshot.c?
> >>>
> >>>Kevin
> >>>
> >>Hi, Stefan
> >>   Do you also agree about adding new file block/snapshot.c?
> >
> >Yes, I agree.
> >
> >Stefan
> >
> great. Also there are some other functions such as image info
> collecting, do you think new file block/misc.c is suitable
> to store those functions there?

As discussed on IRC, it's fine by me.  If a better way to organize these
functions becomes clear in the future they can be moved.

Stefan

Re: [Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c

[Kevin Wolf]

Am 05.03.2013 um 10:21 hat Stefan Hajnoczi geschrieben:
> On Tue, Mar 05, 2013 at 03:11:48PM +0800, Wenchao Xia wrote:
> > 于 2013-3-4 21:02, Stefan Hajnoczi 写道:
> > >On Fri, Mar 01, 2013 at 09:51:53AM +0800, Wenchao Xia wrote: > 于 2013-2-28 0:22, Kevin Wolf 写道:
> > >>Hi, Stefan
> > >>   Do you also agree about adding new file block/snapshot.c?
> > >
> > >Yes, I agree.
> > >
> > >Stefan
> > >
> > great. Also there are some other functions such as image info
> > collecting, do you think new file block/misc.c is suitable
> > to store those functions there?
> 
> As discussed on IRC, it's fine by me.  If a better way to organize these
> functions becomes clear in the future they can be moved.

As also discussed on IRC, I'm not excited by having a block/misc.c. We
already have something for "everything block related that doesn't fit
elsewhere" and it's block.c. I couldn't tell if a function belong into
block.c or block/misc.c.

I suggested having a file that concentrates all function related to
QAPI, the monitor and JSON (including the qemu-img JSON output) and call
it something like block/qapi.c (I'm open for better suggestions). This
would at least make it clear if a function should be in there or not.

Kevin

Re: [Qemu-devel] [PATCH V7 09/14] block: move bdrv_snapshot_find() to block.c

[Paolo Bonzini]

Il 05/03/2013 11:08, Kevin Wolf ha scritto:
>> > 
>> > As discussed on IRC, it's fine by me.  If a better way to organize these
>> > functions becomes clear in the future they can be moved.
> As also discussed on IRC, I'm not excited by having a block/misc.c. We
> already have something for "everything block related that doesn't fit
> elsewhere" and it's block.c. I couldn't tell if a function belong into
> block.c or block/misc.c.
> 
> I suggested having a file that concentrates all function related to
> QAPI, the monitor and JSON (including the qemu-img JSON output) and call
> it something like block/qapi.c (I'm open for better suggestions). This
> would at least make it clear if a function should be in there or not.

I agree with Kevin.

Paolo