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

[Chrysostomos Nanakos <cnanakos@grnet.gr>]

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.
>>
>> 'mport' is the port number on which mapperd is listening. This is optional
>> and if not specified, QEMU will make Archipelago to use the default port.
>>
>> 'vport' is the port number on which vlmcd is listening. This is optional
>> and if not specified, QEMU will make Archipelago to use the default port.
>>
>> 'segment' is the name of the shared memory segment Archipelago stack is using.
>> This is optional and if not specified, QEMU will make Archipelago to use the
>> default value, 'archipelago'.
>>
>> Examples:
>>
>> file.driver=archipelago,file.volume=my_vm_volume
>> file.driver=archipelago,file.volume=my_vm_volume,file.mport=123
>> file.driver=archipelago,file.volume=my_vm_volume,file.mport=123,
>> file.vport=1234
>> file.driver=archipelago,file.volume=my_vm_volume,file.mport=123,
>> file.vport=1234,file.segment=my_segment
>>
>> Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr>
>> ---
> 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?

>
>> +++ b/block/archipelago.c
>> @@ -0,0 +1,819 @@
>> +/*
>> + * QEMU Block driver for Archipelago
>> + *
>> + * Copyright 2014 GRNET S.A. All rights reserved.
> Is it still legally open source if you reserve all rights, or does this
> statement contradict the rest of your header?  (Not that you are the
> first person to attempt this; the phrase "All rights reserved" appears
> in a number of other files in qemu.git, including the mis-spelled
> disas/libvixl/LICENCE)

This is an error. I apologize. I will remove "All rights reserved" and 
let the rest as is in the next patch series. Do you agree?

>
>> +
>> +            switch (reqdata->op) {
>> +            case ARCHIP_OP_READ:
>> +                    data = xseg_get_data(s->xseg, req);
>> +                    segreq = reqdata->segreq;
> Coding style - indent by 4 spaces, not 8.
>
>
>> +
>> +static int __archipelago_submit_request(BDRVArchipelagoState *s,
> Please don't name internal functions with leading __ - that namespace is
> reserved for the compiler and libc.
>