Re: [Qemu-devel] [PATCH v6 1/5] block: Support Archipelago as a QEMU block backend

[Eric Blake <eblake@redhat.com>]

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

On 07/02/2014 08:18 AM, Chrysostomos Nanakos wrote:
> On 07/02/2014 04:59 PM, Eric Blake wrote:
>> On 06/27/2014 02:24 AM, Chrysostomos Nanakos wrote:
>>> VM Image on Archipelago volume is specified like this:
>>>
>>> file.driver=archipelago,file.volume=<volumename>[,file.mport=<mapperd_port>[,
>>>
>>> file.vport=<vlmcd_port>][,file.segment=<segment_name>]]
>>>
>>> 'archipelago' is the protocol.
>>>

>> Just a high-level glance through, and not a thorough review.
>>
>> The command line approach here looks reasonable, although it might be
>> easier to add the QAPI types first (patch 4/5) and then use that type in
>> this patch, instead of open-coding things.
> 
> If I understand well the order of the commit should change? Patch 4/5
> should be first (1/5) and then 1/5 should be 2/5 and so on?

'git rebase -i' can be used to reorder patches; my question is whether
the current patch 4/5 should be hoisted earlier into the series (either
squashed in with the current patch 1, or at least adjacent to it - but
maybe moving it to 2/5 rather than 1/5).  What I'm less sure of is
whether the auto-generated types created by declaring your structure in
the .json file will make life in this patch easier by reusing that
struct, instead of open-coding your QemuOpts.  So it is more of a
question on my end on how much code can be shared between the QemuOpts
of the command line and the QMP additions (since I haven't actually
written a block device myself).  On looking a bit closer, it looks like
you have done things in the correct order.  After all, the command line
parsing was implemented first on most other block devices, then we added
QMP blockdev-add, and its implementation is merely taking a generated
QAPI struct, and converting it into a QemuOpts data structure that can
then be fed to the pre-existing command line code.

At any rate, the important part is that each patch compiles in
isolation, and that you aren't duplicating large portions of code.

-- 
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: 604 bytes --]