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
signature.asc
Description: OpenPGP digital signature
