| Message ID | 1448976530-15984-9-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 12/01 21:28, Peter Xu wrote: > One new QMP event DUMP_COMPLETED is added. When a dump finishes, one > DUMP_COMPLETED event will occur to notify the user. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > docs/qmp-events.txt | 16 ++++++++++++++++ > dump.c | 11 +++++------ > qapi-schema.json | 3 ++- > qapi/event.json | 13 +++++++++++++ > qmp-commands.hx | 5 +++-- > util/error.c | 6 +++++- > 6 files changed, 44 insertions(+), 10 deletions(-) > > diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt > index d2f1ce4..1f79588 100644 > --- a/docs/qmp-events.txt > +++ b/docs/qmp-events.txt > @@ -220,6 +220,22 @@ Data: > }, > "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > +DUMP_COMPLETED > +-------------- > + > +Emitted when the guest has finished one memory dump. > + > +Data: > + > +- "error": Error message when dump failed. This is only a > + human-readable string provided when dump failed. It should not be > + parsed in any way (json-string, optional) > + > +Example: > + > +{ "event": "DUMP_COMPLETED", > + "data": {} } > + > GUEST_PANICKED > -------------- > > diff --git a/dump.c b/dump.c > index c86bc2d..5b040b7 100644 > --- a/dump.c > +++ b/dump.c > @@ -25,6 +25,7 @@ > #include "qapi/error.h" > #include "qapi/qmp/qerror.h" > #include "qmp-commands.h" > +#include "qapi-event.h" > > #include <zlib.h> > #ifdef CONFIG_LZO > @@ -1612,6 +1613,9 @@ static void dump_process(DumpState *s, Error **errp) > s->status = DUMP_STATUS_COMPLETED; > } > > + /* send DUMP_COMPLETED message (unconditionally) */ > + qapi_event_send_dump_completed(!!(*errp), error_get_pretty(*errp), > + &error_abort); > dump_cleanup(s); > } > > @@ -1619,13 +1623,8 @@ static void *dump_thread(void *data) > { > Error *err = NULL; > DumpState *s = (DumpState *)data; > - > dump_process(s, &err); > - > - if (err) { > - /* TODO: notify user the error */ > - error_free(err); > - } > + error_free(err); > return NULL; > } > > diff --git a/qapi-schema.json b/qapi-schema.json > index 691a130..f0d3c4a 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2116,7 +2116,8 @@ > # is the fd's name. > # > # @detach: #optional if true, QMP will return immediately rather than > -# waiting for the dump to finish. (since 2.6). > +# waiting for the dump to finish. A DUMP_COMPLETED event will > +# occur at the end. (since 2.6). > # > # @begin: #optional if specified, the starting physical address. > # > diff --git a/qapi/event.json b/qapi/event.json > index f0cef01..9b7f714 100644 > --- a/qapi/event.json > +++ b/qapi/event.json > @@ -356,3 +356,16 @@ > ## > { 'event': 'MEM_UNPLUG_ERROR', > 'data': { 'device': 'str', 'msg': 'str' } } > + > +## > +# @DUMP_COMPLETED > +# > +# Emitted when background dump has completed > +# > +# @error: #optional human-readable error string that provides > +# hint on why dump failed. Please explicitly mention that successful dump emits DUMP_COMPLETED without error, and failed dump emits DUMP_COMPLETED that has an error str. > +# > +# Since: 2.6 > +## > +{ 'event': 'DUMP_COMPLETED' , > + 'data': { '*error': 'str' } } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 6b51585..7b6f915 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -857,8 +857,9 @@ Arguments: > - "paging": do paging to get guest's memory mapping (json-bool) > - "protocol": destination file(started with "file:") or destination file > descriptor (started with "fd:") (json-string) > -- "detach": if specified, command will return immediately, without waiting > - for the dump to finish (json-bool) > +- "detach": if specified, command will return immediately rather than waiting > + for the dump completion. A DUMP_COMPLETED event will occur at > + the end. (json-bool) > - "begin": the starting physical address. It's optional, and should be specified > with length together (json-int) > - "length": the memory size, in bytes. It's optional, and should be specified > diff --git a/util/error.c b/util/error.c > index 80c89a2..645b9af 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err) > > const char *error_get_pretty(Error *err) > { > - return err->msg; > + if (err) { > + return err->msg; > + } else { > + return NULL; > + } This change belongs to a separate patch, if any. But personally I don't like it, because it doesn't work very well when error_get_pretty is used in printf-like function parameters: Error *err = NULL; error_report("error: %s", error_get_pretty(err)); will print "error: (null)" which is ugly, in which case the caller need to check the pointer anyway. And that is the dominant use case for error_get_pretty in the code base. IMO the caller can always do this: err ? error_get_pretty(err) : NULL in place of your proposed error_get_pretty(err) So maybe leave this and change dump_process like above? Or if you insist, make this hunk a separate patch please. Fam > } > > void error_report_err(Error *err) > -- > 2.4.3 >
On Wed, Dec 02, 2015 at 09:11:31AM +0800, Fam Zheng wrote: > On Tue, 12/01 21:28, Peter Xu wrote: > > + > > +## > > +# @DUMP_COMPLETED > > +# > > +# Emitted when background dump has completed > > +# > > +# @error: #optional human-readable error string that provides > > +# hint on why dump failed. > > Please explicitly mention that successful dump emits DUMP_COMPLETED without > error, and failed dump emits DUMP_COMPLETED that has an error str. Ok. I can add more words to describe it. Maybe something like: # @error: #optional human-readable error string that provides # hint on why dump failed. Only presents on failure. The # user should not try to interpret the error string. How do you think about this one? IMHO, the "#optional" and the name "error" itself is clear enough though, to show that it will be there only if error happens. > > > const char *error_get_pretty(Error *err) > > { > > - return err->msg; > > + if (err) { > > + return err->msg; > > + } else { > > + return NULL; > > + } > > This change belongs to a separate patch, if any. But personally I don't like > it, because it doesn't work very well when error_get_pretty is used in > printf-like function parameters: > > Error *err = NULL; > error_report("error: %s", error_get_pretty(err)); > > will print "error: (null)" which is ugly, in which case the caller need to > check the pointer anyway. And that is the dominant use case for > error_get_pretty in the code base. > > IMO the caller can always do this: > > err ? error_get_pretty(err) : NULL > > in place of your proposed > > error_get_pretty(err) > > So maybe leave this and change dump_process like above? Or if you insist, make > this hunk a separate patch please. I think both should work as long as the modification is backward compatible and without performance drop (at least, it is safer). Whatever, since this is the first patch from me, I'd like to take your advice to avoid modifying shared codes. :) Thanks. Peter > > Fam > > > } > > > > void error_report_err(Error *err) > > -- > > 2.4.3 > >
On Wed, 12/02 16:20, Peter Xu wrote: > On Wed, Dec 02, 2015 at 09:11:31AM +0800, Fam Zheng wrote: > > On Tue, 12/01 21:28, Peter Xu wrote: > > > + > > > +## > > > +# @DUMP_COMPLETED > > > +# > > > +# Emitted when background dump has completed > > > +# > > > +# @error: #optional human-readable error string that provides > > > +# hint on why dump failed. > > > > Please explicitly mention that successful dump emits DUMP_COMPLETED without > > error, and failed dump emits DUMP_COMPLETED that has an error str. > > Ok. I can add more words to describe it. Maybe something like: > > # @error: #optional human-readable error string that provides > # hint on why dump failed. Only presents on failure. The > # user should not try to interpret the error string. > > How do you think about this one? That looks ok.
On 12/01/2015 06:11 PM, Fam Zheng wrote: > On Tue, 12/01 21:28, Peter Xu wrote: >> One new QMP event DUMP_COMPLETED is added. When a dump finishes, one >> DUMP_COMPLETED event will occur to notify the user. >> >> Signed-off-by: Peter Xu <peterx@redhat.com> >> --- >> +++ b/qapi/event.json >> @@ -356,3 +356,16 @@ >> ## >> { 'event': 'MEM_UNPLUG_ERROR', >> 'data': { 'device': 'str', 'msg': 'str' } } >> + >> +## >> +# @DUMP_COMPLETED >> +# >> +# Emitted when background dump has completed >> +# >> +# @error: #optional human-readable error string that provides >> +# hint on why dump failed. > > Please explicitly mention that successful dump emits DUMP_COMPLETED without > error, and failed dump emits DUMP_COMPLETED that has an error str. In fact, I wonder if it would also be worth having a 'status':'DumpStatus' field, which records the final status of the dump (either 'completed' or 'failed'), and which is always present. >> +++ b/util/error.c >> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err) >> >> const char *error_get_pretty(Error *err) >> { >> - return err->msg; >> + if (err) { >> + return err->msg; >> + } else { >> + return NULL; >> + } > > This change belongs to a separate patch, if any. Indeed. When I was musing about the idea, I was not expecting you to actually implement it, so much as questioning whether it is a worthwhile idea. But as it impacts more than just your series, it definitely needs to be a separate patch, if at all. > But personally I don't like > it, because it doesn't work very well when error_get_pretty is used in > printf-like function parameters: > > Error *err = NULL; > error_report("error: %s", error_get_pretty(err)); > > will print "error: (null)" which is ugly, Or even segfault. glibc is nice for printing "(null)", but the behavior is undefined by POSIX and other libc aren't as nice as glibc. And that was not a consequence I thought about when first raising the question of whether it was even worth changing the contract of error_get_pretty().
On Wed, Dec 02, 2015 at 07:45:52AM -0700, Eric Blake wrote: > On 12/01/2015 06:11 PM, Fam Zheng wrote: > > Please explicitly mention that successful dump emits DUMP_COMPLETED without > > error, and failed dump emits DUMP_COMPLETED that has an error str. > > In fact, I wonder if it would also be worth having a > 'status':'DumpStatus' field, which records the final status of the dump > (either 'completed' or 'failed'), and which is always present. Will the raw memory total size useful in any way? I am totally ok to add this, just failed to find a way for user to use it besides calculating finished work during dump... :( > > > >> +++ b/util/error.c > >> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err) > >> > >> const char *error_get_pretty(Error *err) > >> { > >> - return err->msg; > >> + if (err) { > >> + return err->msg; > >> + } else { > >> + return NULL; > >> + } > > > > This change belongs to a separate patch, if any. > > Indeed. When I was musing about the idea, I was not expecting you to > actually implement it, so much as questioning whether it is a worthwhile > idea. But as it impacts more than just your series, it definitely needs > to be a separate patch, if at all. Sorry for the misunderstanding. I will make sure to use a new patch when needed next time. For this change, I will revert it and take Fam's latter suggestion to avoid modifying error.c. Thanks. Peter > > > But personally I don't like > > it, because it doesn't work very well when error_get_pretty is used in > > printf-like function parameters: > > > > Error *err = NULL; > > error_report("error: %s", error_get_pretty(err)); > > > > will print "error: (null)" which is ugly, > > Or even segfault. glibc is nice for printing "(null)", but the behavior > is undefined by POSIX and other libc aren't as nice as glibc. And that > was not a consequence I thought about when first raising the question of > whether it was even worth changing the contract of error_get_pretty(). > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 12/02/2015 08:21 AM, Peter Xu wrote: > On Wed, Dec 02, 2015 at 07:45:52AM -0700, Eric Blake wrote: >> On 12/01/2015 06:11 PM, Fam Zheng wrote: >>> Please explicitly mention that successful dump emits DUMP_COMPLETED without >>> error, and failed dump emits DUMP_COMPLETED that has an error str. >> >> In fact, I wonder if it would also be worth having a >> 'status':'DumpStatus' field, which records the final status of the dump >> (either 'completed' or 'failed'), and which is always present. > > Will the raw memory total size useful in any way? I am totally ok to > add this, just failed to find a way for user to use it besides > calculating finished work during dump... :( Good idea. You never know if it will be helpful, but the information is basically free to provide and doesn't seem like too much of a maintenance burden to promise to always include the total. And in the case of an error, knowing the final values of complete/total might also be useful to see how far things got before failure (for example, if it failed because of ENOSPACE, knowing how much was complete may give an idea of how much additional space should be added before retrying).
On Wed, Dec 02, 2015 at 09:01:16AM -0700, Eric Blake wrote: > On 12/02/2015 08:21 AM, Peter Xu wrote: > > Will the raw memory total size useful in any way? I am totally ok to > > add this, just failed to find a way for user to use it besides > > calculating finished work during dump... :( > > Good idea. You never know if it will be helpful, but the information is > basically free to provide and doesn't seem like too much of a > maintenance burden to promise to always include the total. And in the > case of an error, knowing the final values of complete/total might also > be useful to see how far things got before failure (for example, if it > failed because of ENOSPACE, knowing how much was complete may give an > idea of how much additional space should be added before retrying). Yes, it's more meaningful when it fails. And maybe you are right, it's free to provide it. :) I can add it in v5. One thing to mention is that, since the written_byte field is only for raw memory size, which means (e.g., for kdump-zlib), the number could first goes to 70% of total very quickly in less than a second (possibly due to zero pages, so actually very little data is written to disk), then it will use another ten seconds to finish the rest 30% (which contains most of the data of the final dump file). So the number would still help little even with ENOSPACE. When user sees a 70% of "written" when failed, it will not mean "we need extra of 30% more spaces", it actually means "we need 100% more", but the user would never figure out the real situation from the number only. :( Thanks! Peter > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt index d2f1ce4..1f79588 100644 --- a/docs/qmp-events.txt +++ b/docs/qmp-events.txt @@ -220,6 +220,22 @@ Data: }, "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } +DUMP_COMPLETED +-------------- + +Emitted when the guest has finished one memory dump. + +Data: + +- "error": Error message when dump failed. This is only a + human-readable string provided when dump failed. It should not be + parsed in any way (json-string, optional) + +Example: + +{ "event": "DUMP_COMPLETED", + "data": {} } + GUEST_PANICKED -------------- diff --git a/dump.c b/dump.c index c86bc2d..5b040b7 100644 --- a/dump.c +++ b/dump.c @@ -25,6 +25,7 @@ #include "qapi/error.h" #include "qapi/qmp/qerror.h" #include "qmp-commands.h" +#include "qapi-event.h" #include <zlib.h> #ifdef CONFIG_LZO @@ -1612,6 +1613,9 @@ static void dump_process(DumpState *s, Error **errp) s->status = DUMP_STATUS_COMPLETED; } + /* send DUMP_COMPLETED message (unconditionally) */ + qapi_event_send_dump_completed(!!(*errp), error_get_pretty(*errp), + &error_abort); dump_cleanup(s); } @@ -1619,13 +1623,8 @@ static void *dump_thread(void *data) { Error *err = NULL; DumpState *s = (DumpState *)data; - dump_process(s, &err); - - if (err) { - /* TODO: notify user the error */ - error_free(err); - } + error_free(err); return NULL; } diff --git a/qapi-schema.json b/qapi-schema.json index 691a130..f0d3c4a 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2116,7 +2116,8 @@ # is the fd's name. # # @detach: #optional if true, QMP will return immediately rather than -# waiting for the dump to finish. (since 2.6). +# waiting for the dump to finish. A DUMP_COMPLETED event will +# occur at the end. (since 2.6). # # @begin: #optional if specified, the starting physical address. # diff --git a/qapi/event.json b/qapi/event.json index f0cef01..9b7f714 100644 --- a/qapi/event.json +++ b/qapi/event.json @@ -356,3 +356,16 @@ ## { 'event': 'MEM_UNPLUG_ERROR', 'data': { 'device': 'str', 'msg': 'str' } } + +## +# @DUMP_COMPLETED +# +# Emitted when background dump has completed +# +# @error: #optional human-readable error string that provides +# hint on why dump failed. +# +# Since: 2.6 +## +{ 'event': 'DUMP_COMPLETED' , + 'data': { '*error': 'str' } } diff --git a/qmp-commands.hx b/qmp-commands.hx index 6b51585..7b6f915 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -857,8 +857,9 @@ Arguments: - "paging": do paging to get guest's memory mapping (json-bool) - "protocol": destination file(started with "file:") or destination file descriptor (started with "fd:") (json-string) -- "detach": if specified, command will return immediately, without waiting - for the dump to finish (json-bool) +- "detach": if specified, command will return immediately rather than waiting + for the dump completion. A DUMP_COMPLETED event will occur at + the end. (json-bool) - "begin": the starting physical address. It's optional, and should be specified with length together (json-int) - "length": the memory size, in bytes. It's optional, and should be specified diff --git a/util/error.c b/util/error.c index 80c89a2..645b9af 100644 --- a/util/error.c +++ b/util/error.c @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err) const char *error_get_pretty(Error *err) { - return err->msg; + if (err) { + return err->msg; + } else { + return NULL; + } } void error_report_err(Error *err)
One new QMP event DUMP_COMPLETED is added. When a dump finishes, one DUMP_COMPLETED event will occur to notify the user. Signed-off-by: Peter Xu <peterx@redhat.com> --- docs/qmp-events.txt | 16 ++++++++++++++++ dump.c | 11 +++++------ qapi-schema.json | 3 ++- qapi/event.json | 13 +++++++++++++ qmp-commands.hx | 5 +++-- util/error.c | 6 +++++- 6 files changed, 44 insertions(+), 10 deletions(-)