From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51443)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <xiecl.fnst@cn.fujitsu.com>) id 1aYltS-000413-RA
	for qemu-devel@nongnu.org; Wed, 24 Feb 2016 21:43:07 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <xiecl.fnst@cn.fujitsu.com>) id 1aYltP-0006OH-JJ
	for qemu-devel@nongnu.org; Wed, 24 Feb 2016 21:43:06 -0500
Received: from [59.151.112.132] (port=47396 helo=heian.cn.fujitsu.com)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <xiecl.fnst@cn.fujitsu.com>) id 1aYltP-0006FX-2X
	for qemu-devel@nongnu.org; Wed, 24 Feb 2016 21:43:03 -0500
Message-ID: <56CE6A81.70606@cn.fujitsu.com>
Date: Thu, 25 Feb 2016 10:44:17 +0800
From: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
MIME-Version: 1.0
References: <1456308715-15465-1-git-send-email-xiecl.fnst@cn.fujitsu.com>
	<1456308715-15465-3-git-send-email-xiecl.fnst@cn.fujitsu.com>
	<w5137si1e1d.fsf@maestria.local.igalia.com>
In-Reply-To: <w5137si1e1d.fsf@maestria.local.igalia.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor
	QUORUM_REPORT_BAD
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: Alberto Garcia <berto@igalia.com>, qemu devel <qemu-devel@nongnu.org>, Eric Blake <eblake@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>, Markus Armbruster <armbru@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

On 02/24/2016 08:35 PM, Alberto Garcia wrote:
> On Wed 24 Feb 2016 11:11:54 AM CET, Changlong Xie wrote:
>> -static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
>> +static void quorum_report_bad(QuorumOpType type, QuorumAIOCB *acb,
>> +                              char *node_name, int ret)
>>   {
>>       const char *msg = NULL;
>>       if (ret < 0) {
>>           msg = strerror(-ret);
>>       }
>> -    qapi_event_send_quorum_report_bad(!!msg, msg, node_name,
>> -                                      acb->sector_num, acb->nb_sectors, &error_abort);
>> +
>> +    switch (type) {
>> +    case QUORUM_OP_TYPE_READ:
>> +    case QUORUM_OP_TYPE_WRITE:
>> +        qapi_event_send_quorum_report_bad(false, 0, !!msg, msg, node_name,
>> +                                          acb->sector_num, acb->nb_sectors,
>> +                                          &error_abort);
>> +        break;
>> +    case QUORUM_OP_TYPE_FLUSH:
>> +        qapi_event_send_quorum_report_bad(true, type, !!msg, msg, node_name,
>> +                                          0, 0, &error_abort);
>> +        break;
>
> A few comments:
>
>    - Why don't you set the 'type' field in read and write operations? You
>      defined all three values but you are only using 'flush' here.
>
>    - For the case of flush you could set sectors-count to the total size
>      of the BlockDriverState as Eric suggested (bdrv_nb_sectors(bs)).
>      Setting both to 0 could confuse clients that are not ready to deal
>      with flush failures.
>
>    - Since the QuorumAIOCB parameter is not used in the flush case, you
>      could replace it from the function prototype and use sector_num and
>      nb_sectors instead. That way you can also omit the switch.
>

Surely, all your suggestions are helpful. Also i will add "Reviewed-by" 
in patch 1/3, 3/3 in next version.

Thanks
	-Xie

> Berto
>
>
> .
>