diff mbox

[v4,1/2] qmp event: Add QUORUM_FLUSH_ERROR

Message ID 1456218099-386-2-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Changlong Xie Feb. 23, 2016, 9:01 a.m. UTC
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block/quorum.c      |  5 +++++
 docs/qmp-events.txt | 18 ++++++++++++++++++
 qapi/event.json     | 16 ++++++++++++++++
 3 files changed, 39 insertions(+)

Comments

Changlong Xie Feb. 23, 2016, 9:07 a.m. UTC | #1
cc: Markus Armbruster <armbru@redhat.com>

On 02/23/2016 05:01 PM, Changlong Xie wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>   block/quorum.c      |  5 +++++
>   docs/qmp-events.txt | 18 ++++++++++++++++++
>   qapi/event.json     | 16 ++++++++++++++++
>   3 files changed, 39 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index f78d4cb..d3c3958 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -235,6 +235,11 @@ static void quorum_report_failure(QuorumAIOCB *acb)
>                                      acb->nb_sectors, &error_abort);
>   }
>
> +static void quorum_flush_error(char *node_name, const char *msg)
> +{
> +    qapi_event_send_quorum_flush_error(node_name, msg, &error_abort);
> +}
> +
>   static int quorum_vote_error(QuorumAIOCB *acb);
>
>   static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb)
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index b6e8937..d777873 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -340,6 +340,24 @@ Example:
>
>   Note: this event is rate-limited.
>
> +QUORUM_FLUSH_ERROR
> +-----------------
> +
> +Emitted to report flush error message of the Quorum block driver
> +
> +Data:
> +
> +- "node-name":     The graph node name of the block driver state.
> +- "error":         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 try to interpret the error string.
> +
> +Example:
> +
> +{ "event": "QUORUM_FLUSH_ERROR",
> +     "data": { "node-name": "1.raw", "error": "xxxxxx" },
> +     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
> +
>   RESET
>   -----
>
> diff --git a/qapi/event.json b/qapi/event.json
> index cfcc887..5b16706 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -358,6 +358,22 @@
>               'sector-num': 'int', 'sectors-count': 'int' } }
>
>   ##
> +# @QUORUM_FLUSH_ERROR
> +#
> +# Emitted to report flush error message of the Quorum block driver
> +#
> +# @node-name: the graph node name of the block driver state
> +#
> +# @error: 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
> +#         try to interpret the error string.
> +#
> +# Since: 2.5
> +##
> +{ 'event': 'QUORUM_FLUSH_ERROR',
> +  'data': { 'node-name': 'str', 'error': 'str'} }
> +
> +##
>   # @VSERPORT_CHANGE
>   #
>   # Emitted when the guest opens or closes a virtio-serial port.
>
Alberto Garcia Feb. 23, 2016, 12:53 p.m. UTC | #2
On Tue 23 Feb 2016 10:01:38 AM CET, Changlong Xie wrote:

> @@ -235,6 +235,11 @@ static void quorum_report_failure(QuorumAIOCB *acb)
>                                     acb->nb_sectors, &error_abort);
>  }
>  
> +static void quorum_flush_error(char *node_name, const char *msg)
> +{
> +    qapi_event_send_quorum_flush_error(node_name, msg, &error_abort);
> +}
> +

Instead of 'msg' I think you can receive an error code here and convert
it to a message using strerror(). Take a look at quorum_report_bad() to
see what I mean.

> +QUORUM_FLUSH_ERROR
> +-----------------
> +
> +Emitted to report flush error message of the Quorum block driver

Emitted to report a flush error in a Quorum file

> +
> +Data:
> +
> +- "node-name":     The graph node name of the block driver state.
> +- "error":         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 try to interpret the error string.

Can you reformat this paragraph so it takes less than 80 columns?

> +
> +Example:
> +
> +{ "event": "QUORUM_FLUSH_ERROR",
> +     "data": { "node-name": "1.raw", "error": "xxxxxx" },
> +     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }

'1.raw' is an invalid node name.

>  ##
> +# @QUORUM_FLUSH_ERROR
> +#
> +# Emitted to report flush error message of the Quorum block driver
> +#
> +# @node-name: the graph node name of the block driver state
> +#
> +# @error: 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
> +#         try to interpret the error string.

Same changes I mentioned earlier.

> +#
> +# Since: 2.5

Since 2.6

Berto
Eric Blake Feb. 23, 2016, 1:17 p.m. UTC | #3
On 02/23/2016 02:01 AM, Changlong Xie wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  block/quorum.c      |  5 +++++
>  docs/qmp-events.txt | 18 ++++++++++++++++++
>  qapi/event.json     | 16 ++++++++++++++++
>  3 files changed, 39 insertions(+)

In addition to Berto's findings:


> +++ b/docs/qmp-events.txt
> @@ -340,6 +340,24 @@ Example:
>  
>  Note: this event is rate-limited.
>  
> +QUORUM_FLUSH_ERROR
> +-----------------

Please keep the file sorted; this would fall between QUORUM_FAILURE and
QUORUM_REPORT_BAD.  Commit message should say why we need a third event,
rather than reusing either of the other two (my guess: because you don't
have a location, and don't want to modify the existing two to report a
location - but why not just use 'sector-num':0, 'sectors-count':<size of
file> to report the entire file as the location?)

Length of ---- separator should match text above it (you were off by one).

Is this event rate-limited? Should it be?
Alberto Garcia Feb. 23, 2016, 1:24 p.m. UTC | #4
On Tue 23 Feb 2016 02:17:23 PM CET, Eric Blake wrote:

> Commit message should say why we need a third event, rather than
> reusing either of the other two (my guess: because you don't have a
> location, and don't want to modify the existing two to report a
> location - but why not just use 'sector-num':0, 'sectors-count':<size
> of file> to report the entire file as the location?)

I would also be fine with that solution.

Berto
Eric Blake Feb. 23, 2016, 1:45 p.m. UTC | #5
On 02/23/2016 06:24 AM, Alberto Garcia wrote:
> On Tue 23 Feb 2016 02:17:23 PM CET, Eric Blake wrote:
> 
>> Commit message should say why we need a third event, rather than
>> reusing either of the other two (my guess: because you don't have a
>> location, and don't want to modify the existing two to report a
>> location - but why not just use 'sector-num':0, 'sectors-count':<size
>> of file> to report the entire file as the location?)
> 
> I would also be fine with that solution.

I would also be fine if we added an optional enum member to the existing
event that said which operation failed ('read', 'write', 'flush') -
adding optional output members is safe, while converting existing
mandatory output members to optional may confuse existing clients.
Alberto Garcia Feb. 23, 2016, 2:05 p.m. UTC | #6
On Tue 23 Feb 2016 02:45:49 PM CET, Eric Blake wrote:

>>> Commit message should say why we need a third event, rather than
>>> reusing either of the other two (my guess: because you don't have a
>>> location, and don't want to modify the existing two to report a
>>> location - but why not just use 'sector-num':0,
>>> 'sectors-count':<size of file> to report the entire file as the
>>> location?)
>> 
>> I would also be fine with that solution.
>
> I would also be fine if we added an optional enum member to the
> existing event that said which operation failed ('read', 'write',
> 'flush') - adding optional output members is safe, while converting
> existing mandatory output members to optional may confuse existing
> clients.

That might actually be the best option :-)

Berto
Changlong Xie Feb. 24, 2016, 2:49 a.m. UTC | #7
On 02/23/2016 10:05 PM, Alberto Garcia wrote:
> On Tue 23 Feb 2016 02:45:49 PM CET, Eric Blake wrote:
>
>>>> Commit message should say why we need a third event, rather than
>>>> reusing either of the other two (my guess: because you don't have a
>>>> location, and don't want to modify the existing two to report a
>>>> location - but why not just use 'sector-num':0,
>>>> 'sectors-count':<size of file> to report the entire file as the
>>>> location?)
>>>
>>> I would also be fine with that solution.
>>
>> I would also be fine if we added an optional enum member to the
>> existing event that said which operation failed ('read', 'write',
>> 'flush') - adding optional output members is safe, while converting
>> existing mandatory output members to optional may confuse existing
>> clients.
>

Hi Berto & Eric

Thanks for all your comments. Surely, this is the best option to me too
:-), will fix it in next version.

Thanks
	-Xie

> That might actually be the best option :-)
>
> Berto
>
>
> .
>
diff mbox

Patch

diff --git a/block/quorum.c b/block/quorum.c
index f78d4cb..d3c3958 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -235,6 +235,11 @@  static void quorum_report_failure(QuorumAIOCB *acb)
                                    acb->nb_sectors, &error_abort);
 }
 
+static void quorum_flush_error(char *node_name, const char *msg)
+{
+    qapi_event_send_quorum_flush_error(node_name, msg, &error_abort);
+}
+
 static int quorum_vote_error(QuorumAIOCB *acb);
 
 static bool quorum_has_too_much_io_failed(QuorumAIOCB *acb)
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index b6e8937..d777873 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -340,6 +340,24 @@  Example:
 
 Note: this event is rate-limited.
 
+QUORUM_FLUSH_ERROR
+-----------------
+
+Emitted to report flush error message of the Quorum block driver
+
+Data:
+
+- "node-name":     The graph node name of the block driver state.
+- "error":         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 try to interpret the error string.
+
+Example:
+
+{ "event": "QUORUM_FLUSH_ERROR",
+     "data": { "node-name": "1.raw", "error": "xxxxxx" },
+     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
+
 RESET
 -----
 
diff --git a/qapi/event.json b/qapi/event.json
index cfcc887..5b16706 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -358,6 +358,22 @@ 
             'sector-num': 'int', 'sectors-count': 'int' } }
 
 ##
+# @QUORUM_FLUSH_ERROR
+#
+# Emitted to report flush error message of the Quorum block driver
+#
+# @node-name: the graph node name of the block driver state
+#
+# @error: 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
+#         try to interpret the error string.
+#
+# Since: 2.5
+##
+{ 'event': 'QUORUM_FLUSH_ERROR',
+  'data': { 'node-name': 'str', 'error': 'str'} }
+
+##
 # @VSERPORT_CHANGE
 #
 # Emitted when the guest opens or closes a virtio-serial port.