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

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.

> +++ 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)

> +
> +            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.

-- 
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 --]