| Message ID | 1456218099-386-2-git-send-email-xiecl.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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. >
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
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?
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
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.
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
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 --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.