Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Changlong Xie 

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


.

Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Changlong Xie 

On 02/25/2016 09:12 AM, Eric Blake wrote:

On 02/24/2016 05:50 PM, Wen Congyang wrote:


+- "type":  Quorum operation type (json-string, optional)


I don't think 'type' needs to be optional, after all.  Just always
output it.


If we output read/write type, old libvirt will ignore the read/write error 
events?


The QMP protocol already documents that ALL clients MUST ignore
unexpected output fields.  Any client that is unprepared for new fields
appearing in the dictionary is buggy.  Libvirt will be just fine if you
output a new "type":"read".


Ok, i'll make "type" mandatory.

Thanks
-Xie

Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Eric Blake 
On 02/24/2016 05:50 PM, Wen Congyang wrote:

>>> +- "type":  Quorum operation type (json-string, optional)
>>
>> I don't think 'type' needs to be optional, after all.  Just always
>> output it.
> 
> If we output read/write type, old libvirt will ignore the read/write error 
> events?

The QMP protocol already documents that ALL clients MUST ignore
unexpected output fields.  Any client that is unprepared for new fields
appearing in the dictionary is buggy.  Libvirt will be just fine if you
output a new "type":"read".

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Wen Congyang 
On 02/25/2016 12:59 AM, Eric Blake wrote:
> On 02/24/2016 03:11 AM, Changlong Xie wrote:
>> Introduce QuorumOpType, and make QUORUM_REPORT_BAD compatible
>> with it.
>>
>> Cc: Dr. David Alan Gilbert 
>> Cc: Wen Congyang 
>> Signed-off-by: Wen Congyang 
>> Signed-off-by: Changlong Xie 
>> ---
> 
>> +++ b/docs/qmp-events.txt
>> @@ -307,6 +307,7 @@ Emitted to report a corruption of a Quorum file.
>>  
>>  Data:
>>  
>> +- "type":  Quorum operation type (json-string, optional)
> 
> I don't think 'type' needs to be optional, after all.  Just always
> output it.

If we output read/write type, old libvirt will ignore the read/write error 
events?

Thanks
Wen Congyang

> 
>>  - "error": Error message (json-string, optional)
>> Only present on failure.  This field contains a 
>> human-readable
>> error message.  There are no semantics other than that 
>> the
>> @@ -318,10 +319,17 @@ Data:
>>  
>>  Example:
>>  
>> +Read/Write operation:
>>  { "event": "QUORUM_REPORT_BAD",
>>   "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 
>> 5 },
>>   "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
> 
> and this example would then show "type":"read"
>

Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Eric Blake 
On 02/24/2016 03:11 AM, Changlong Xie wrote:
> Introduce QuorumOpType, and make QUORUM_REPORT_BAD compatible
> with it.
> 
> Cc: Dr. David Alan Gilbert 
> Cc: Wen Congyang 
> Signed-off-by: Wen Congyang 
> Signed-off-by: Changlong Xie 
> ---

> +++ b/docs/qmp-events.txt
> @@ -307,6 +307,7 @@ Emitted to report a corruption of a Quorum file.
>  
>  Data:
>  
> +- "type":  Quorum operation type (json-string, optional)

I don't think 'type' needs to be optional, after all.  Just always
output it.

>  - "error": Error message (json-string, optional)
> Only present on failure.  This field contains a 
> human-readable
> error message.  There are no semantics other than that the
> @@ -318,10 +319,17 @@ Data:
>  
>  Example:
>  
> +Read/Write operation:
>  { "event": "QUORUM_REPORT_BAD",
>   "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 
> 5 },
>   "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }

and this example would then show "type":"read"

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Eric Blake 
On 02/24/2016 05:35 AM, Alberto Garcia wrote:

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

In fact, 'type' does not need to be optional; always outputting it makes
more sense for new clients, and doesn't hurt old clients.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Alberto Garcia 
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.

Berto

[Qemu-devel] [PATCH v5 2/3] qmp event: Refactor QUORUM_REPORT_BAD

2016-02-24 Thread Changlong Xie 
Introduce QuorumOpType, and make QUORUM_REPORT_BAD compatible
with it.

Cc: Dr. David Alan Gilbert 
Cc: Wen Congyang 
Signed-off-by: Wen Congyang 
Signed-off-by: Changlong Xie 
---
 block/quorum.c  | 28 +++-
 docs/qmp-events.txt |  8 
 qapi/block.json | 16 
 qapi/event.json |  4 +++-
 4 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..03d68c1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -215,14 +215,29 @@ static QuorumAIOCB *quorum_aio_get(BDRVQuorumState *s,
 return acb;
 }
 
-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;
+default:
+/* should never happen */
+abort();
+}
 }
 
 static void quorum_report_failure(QuorumAIOCB *acb)
@@ -282,6 +297,7 @@ static void quorum_aio_cb(void *opaque, int ret)
 QuorumChildRequest *sacb = opaque;
 QuorumAIOCB *acb = sacb->parent;
 BDRVQuorumState *s = acb->common.bs->opaque;
+QuorumOpType type;
 bool rewrite = false;
 
 if (acb->is_read && s->read_pattern == QUORUM_READ_PATTERN_FIFO) {
@@ -300,12 +316,13 @@ static void quorum_aio_cb(void *opaque, int ret)
 return;
 }
 
+type = acb->is_read ? QUORUM_OP_TYPE_READ : QUORUM_OP_TYPE_WRITE;
 sacb->ret = ret;
 acb->count++;
 if (ret == 0) {
 acb->success_count++;
 } else {
-quorum_report_bad(acb, sacb->aiocb->bs->node_name, ret);
+quorum_report_bad(type, acb, sacb->aiocb->bs->node_name, ret);
 }
 assert(acb->count <= s->num_children);
 assert(acb->success_count <= s->num_children);
@@ -338,7 +355,8 @@ static void quorum_report_bad_versions(BDRVQuorumState *s,
 continue;
 }
 QLIST_FOREACH(item, &version->items, next) {
-quorum_report_bad(acb, s->children[item->index]->bs->node_name, 0);
+quorum_report_bad(QUORUM_OP_TYPE_READ, acb,
+  s->children[item->index]->bs->node_name, 0);
 }
 }
 }
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index f96e120..0a082db 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -307,6 +307,7 @@ Emitted to report a corruption of a Quorum file.
 
 Data:
 
+- "type":  Quorum operation type (json-string, optional)
 - "error": Error message (json-string, optional)
Only present on failure.  This field contains a 
human-readable
error message.  There are no semantics other than that the
@@ -318,10 +319,17 @@ Data:
 
 Example:
 
+Read/Write operation:
 { "event": "QUORUM_REPORT_BAD",
  "data": { "node-name": "node0", "sector-num": 345435, "sectors-count": 5 
},
  "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 
+Flush operation:
+{ "event": "QUORUM_REPORT_BAD",
+ "data": { "node-name": "node0", "sectors-count": 0, "sector-num": 0,
+ "type": "flush", "error": "Broken pipe" },
+ "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }
+
 Note: this event is rate-limited.
 
 RESET
diff --git a/qapi/block.json b/qapi/block.json
index 58e6b30..937337d 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -196,3 +196,19 @@
 ##
 { 'event': 'DEVICE_TRAY_MOVED',
   'data': { 'device': 'str', 'tray-open': 'bool' } }
+
+##
+# @QuorumOpType
+#
+# An enumeration of the quorum operation types
+#
+# @read: read operation
+#
+# @write: write operation
+#
+# @flush: flush operation
+#
+# Since: 2.6
+##
+{ 'enum': 'QuorumOpType',
+  'data': [ 'read', 'write', 'flush' ] }
diff --git a/qapi/event.json b/qapi/event.json
index 390fd45..a5db99a 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -325,6 +325,8 @@
 #
 # Emitted to report a corruption of a Quorum file
 #
+# @type: #optional, quorum operation type (Since 2.6)
+#
 # @error: #optional, error message. Only present on failure. This field
 # contains a human-readable error message. There are no semantics other
 # than that the block layer reported an error and clients should not
@@ -339,7 +341,7 @@
 # Since: 2.0
 ##
 { 'event