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

[Chrysostomos Nanakos]

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 <address@hidden>
> > ---
> 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.