From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36612)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <cnanakos@admin.grnet.gr>) id 1X2LNl-000671-My
	for qemu-devel@nongnu.org; Wed, 02 Jul 2014 10:19:39 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <cnanakos@admin.grnet.gr>) id 1X2LNf-0005lE-8o
	for qemu-devel@nongnu.org; Wed, 02 Jul 2014 10:19:33 -0400
Received: from averel.grnet-hq.admin.grnet.gr ([195.251.29.3]:8138)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <cnanakos@admin.grnet.gr>) id 1X2LNf-0005l9-0p
	for qemu-devel@nongnu.org; Wed, 02 Jul 2014 10:19:27 -0400
Message-ID: <53B414B3.3060601@grnet.gr>
Date: Wed, 02 Jul 2014 17:18:27 +0300
From: Chrysostomos Nanakos <cnanakos@grnet.gr>
MIME-Version: 1.0
References: <1403857452-23768-1-git-send-email-cnanakos@grnet.gr>
	<1403857452-23768-2-git-send-email-cnanakos@grnet.gr>
	<53B41025.8030002@redhat.com>
In-Reply-To: <53B41025.8030002@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v6 1/5] block: Support Archipelago as a
	QEMU block backend
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com, stefanha@redhat.com

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