| Message ID | 1255453026-18637-8-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > hw/qdev.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/qdev.c b/hw/qdev.c > index 906e897..3ce48f7 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -28,6 +28,7 @@ > #include "net.h" > #include "qdev.h" > #include "sysemu.h" > +#include "qerror.h" > #include "monitor.h" > > static int qdev_hotplug = 0; > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > /* find driver */ > info = qdev_find_info(NULL, driver); > if (!info) { > - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", > - driver); > + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); > return NULL; > } > if (info->no_user) { And here we pay the price for structured error objects: reporting an error becomes more difficult. The harder you make reporting errors, the fewer errors get caught and reported. Also, the result is harder to read. If we need the structure, then it's just a price we have to pay. Do we need it? In the example above, it gets us two pieces of information in addition to the "monitor command failed" bit: the error code, which tells us the exact kind of failure, and a data object to go with the error code, in this case containing the name of the device. Now let's look at errors from the client's perspective. Clients need to classify errors to figure out how to respond to them. Something like client error (i.e. your command was stupid, and trying again will be just as stupid), permanent server error (it wasn't stupid, but I can't do it for you), transient server error (I couldn't do it for you, but trying again later could help). Some classical protocols (HTTP, FTP) provide error class (they cleverly encode it into the error code). This gives clients a chance to sanely handle errors they don't know. Even with error class figured out, clients may still be interested in the exact error code, at least in some cases. They may also be interested in a human readable description of the error, to present to their human users. Some classical protocols provide that in addition to an error code. This gives clients a chance to sanely report errors they don't know to human users. I suspect that the additional error data is the error's least interesting part most of the time. When we get QERR_QDEV_NFOUND, I figure it's usually clear what device we were trying to find. But these are just my educated guesses. I'd love to hear what folks involved with actual clients have to say. Anyone from libvirt?
On 10/14/2009 12:34 AM, Markus Armbruster wrote: > Luiz Capitulino<lcapitulino@redhat.com> writes: > >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> >> --- >> hw/qdev.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/qdev.c b/hw/qdev.c >> index 906e897..3ce48f7 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -28,6 +28,7 @@ >> #include "net.h" >> #include "qdev.h" >> #include "sysemu.h" >> +#include "qerror.h" >> #include "monitor.h" >> >> static int qdev_hotplug = 0; >> @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> /* find driver */ >> info = qdev_find_info(NULL, driver); >> if (!info) { >> - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", >> - driver); >> + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); >> return NULL; >> } >> if (info->no_user) { > > And here we pay the price for structured error objects: reporting an > error becomes more difficult. The harder you make reporting errors, the > fewer errors get caught and reported. Also, the result is harder to > read. If you count also the new code in 6/9, reporting an error definitely becomes harder. Finding a middle ground would be great, and I think that the right solution is to put as many things as possible in a single place. For example: 1) there is a lot of code that is duplicated in every occurrence of an error. One possibility to avoid this, would be to use a more printf-like syntax for qemu_object_from_va, for example one of these: .... "{ name: %s }", driver); .... "{ 'name': %s }", driver); Then you can move the first argument to the error table: + .code = QERR_QDEV_NFOUND, + .desc = "device not found", + .keys = "{ name: %s }" so that the qemu_error_structed call looks like qemu_error_structed (QERR_QDEV_NFOUND, driver); 2) do we need full flexibility of a function for printing errors? What about adding a print-from-qobject function, so that you could replace qerror_table[].user_print with a string like .formatted_print = "Device \"%{name}\" not found. Try -device" " '?' for a list." 3) I don't think this technique is used in qemu, but what about putting all errors in a header file like QEMU_ERROR(QERR_DEV_NFOUND, "device not found", whatever); Then you can use the header file to generate both the enum and the error table: typedef enum QErrorCode { #define QEMU_ERROR(code_, ...) code_, #include "qemu.def" #undef QEMU_ERROR } QErrorCode; ... static QErrorTable qerror_table[] = { #define QEMU_ERROR(code_, desc_, data_) \ { .code = (code_), .desc = (desc_), .data = (data_), #include "qemu-error.def" #undef QEMU_ERROR }; This has the disadvantage of not allowing usage of designated initializers. Paolo
On Wed, 14 Oct 2009 00:34:21 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > hw/qdev.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/qdev.c b/hw/qdev.c > > index 906e897..3ce48f7 100644 > > --- a/hw/qdev.c > > +++ b/hw/qdev.c > > @@ -28,6 +28,7 @@ > > #include "net.h" > > #include "qdev.h" > > #include "sysemu.h" > > +#include "qerror.h" > > #include "monitor.h" > > > > static int qdev_hotplug = 0; > > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > > /* find driver */ > > info = qdev_find_info(NULL, driver); > > if (!info) { > > - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", > > - driver); > > + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); > > return NULL; > > } > > if (info->no_user) { > > And here we pay the price for structured error objects: reporting an > error becomes more difficult. The harder you make reporting errors, the > fewer errors get caught and reported. Also, the result is harder to > read. > > If we need the structure, then it's just a price we have to pay. Do we > need it? Yes, if we want clients to able to determine error causes. Of course that we can decide to provide less information, but this structure has three users (monitor protocol, command-line and monitor), so adding information for one of them means adding for them all.
On Wed, 14 Oct 2009 15:29:41 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 10/14/2009 12:34 AM, Markus Armbruster wrote: > > Luiz Capitulino<lcapitulino@redhat.com> writes: > > > >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > >> --- > >> hw/qdev.c | 4 ++-- > >> 1 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/qdev.c b/hw/qdev.c > >> index 906e897..3ce48f7 100644 > >> --- a/hw/qdev.c > >> +++ b/hw/qdev.c > >> @@ -28,6 +28,7 @@ > >> #include "net.h" > >> #include "qdev.h" > >> #include "sysemu.h" > >> +#include "qerror.h" > >> #include "monitor.h" > >> > >> static int qdev_hotplug = 0; > >> @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > >> /* find driver */ > >> info = qdev_find_info(NULL, driver); > >> if (!info) { > >> - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", > >> - driver); > >> + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); > >> return NULL; > >> } > >> if (info->no_user) { > > > > And here we pay the price for structured error objects: reporting an > > error becomes more difficult. The harder you make reporting errors, the > > fewer errors get caught and reported. Also, the result is harder to > > read. > > If you count also the new code in 6/9, reporting an error definitely > becomes harder. Finding a middle ground would be great, and I think > that the right solution is to put as many things as possible in a single > place. For example: > > 1) there is a lot of code that is duplicated in every occurrence of an > error. One possibility to avoid this, would be to use a more > printf-like syntax for qemu_object_from_va, for example one of these: > > .... "{ name: %s }", driver); > .... "{ 'name': %s }", driver); > > Then you can move the first argument to the error table: > > + .code = QERR_QDEV_NFOUND, > + .desc = "device not found", > + .keys = "{ name: %s }" > > so that the qemu_error_structed call looks like > > qemu_error_structed (QERR_QDEV_NFOUND, driver); > > 2) do we need full flexibility of a function for printing errors? What > about adding a print-from-qobject function, so that you could replace > qerror_table[].user_print with a string like > > .formatted_print = "Device \"%{name}\" not found. Try -device" > " '?' for a list." I like both, but I see them as future improvements. > 3) I don't think this technique is used in qemu, but what about putting > all errors in a header file like We have qemu-monitor.hx, which is something like it. > QEMU_ERROR(QERR_DEV_NFOUND, > "device not found", > whatever); > > Then you can use the header file to generate both the enum and the error > table: > > typedef enum QErrorCode { > #define QEMU_ERROR(code_, ...) code_, > #include "qemu.def" > #undef QEMU_ERROR > } QErrorCode; > > ... > > static QErrorTable qerror_table[] = { > #define QEMU_ERROR(code_, desc_, data_) \ > { .code = (code_), .desc = (desc_), .data = (data_), > #include "qemu-error.def" > #undef QEMU_ERROR > }; > > This has the disadvantage of not allowing usage of designated initializers. Not sure if it really helps.
On Wed, Oct 14, 2009 at 12:34:21AM +0200, Markus Armbruster wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > hw/qdev.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/qdev.c b/hw/qdev.c > > index 906e897..3ce48f7 100644 > > --- a/hw/qdev.c > > +++ b/hw/qdev.c > > @@ -28,6 +28,7 @@ > > #include "net.h" > > #include "qdev.h" > > #include "sysemu.h" > > +#include "qerror.h" > > #include "monitor.h" > > > > static int qdev_hotplug = 0; > > @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > > /* find driver */ > > info = qdev_find_info(NULL, driver); > > if (!info) { > > - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", > > - driver); > > + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); > > return NULL; > > } > > if (info->no_user) { > Now let's look at errors from the client's perspective. > > Clients need to classify errors to figure out how to respond to them. > Something like client error (i.e. your command was stupid, and trying > again will be just as stupid), permanent server error (it wasn't stupid, > but I can't do it for you), transient server error (I couldn't do it for > you, but trying again later could help). > > Some classical protocols (HTTP, FTP) provide error class (they cleverly > encode it into the error code). This gives clients a chance to sanely > handle errors they don't know. > > Even with error class figured out, clients may still be interested in > the exact error code, at least in some cases. > > They may also be interested in a human readable description of the > error, to present to their human users. Some classical protocols > provide that in addition to an error code. This gives clients a chance > to sanely report errors they don't know to human users. > > I suspect that the additional error data is the error's least > interesting part most of the time. When we get QERR_QDEV_NFOUND, I > figure it's usually clear what device we were trying to find. > > But these are just my educated guesses. I'd love to hear what folks > involved with actual clients have to say. Anyone from libvirt? I think just returning error codes to the client is far too little information. I don't think we need the fully normalized structure that Luiz originally proposed with bus/dev addresses split out, but we certainly need to include a string description giving as much detail as possible. If attaching a host USB device failed, I don't want a single error code QERR_OPEN_FAILED, nor a generic message like 'could not open device', i want the exact details, eg 'could not open device: permission denied' 'could not open device: no such file or directory' 'could not open device: device or resource busy' The localization issue is somewhat of a red herring though. This string description is not something that clients would ultimately expose to the user. The client apps would likely present a more generic error message to the user, based off the error code. The error description received from QEMU will instead be written out to the logs as a record for later troubleshooting. eg if the user files a bug report, it will include the full error details, so libvirt/qemu maintainers have a better chance of figuring out what went wrong. So ultimately these error messages are for developers, not users benefit. Daniel
> I think just returning error codes to the client is far too little > information. I don't think we need the fully normalized structure > that Luiz originally proposed with bus/dev addresses split out, but > we certainly need to include a string description giving as much > detail as possible. If attaching a host USB device failed, I don't > want a single error code QERR_OPEN_FAILED, nor a generic message > like 'could not open device', i want the exact details, eg > > 'could not open device: permission denied' > 'could not open device: no such file or directory' > 'could not open device: device or resource busy' Which makes me wonder whenever it makes sense to re-use errno for the error codes instead of inventing our own QERR_* codes? Not the numbers of course because they are not standardized as far I know, but the Ename strings. If you error out because a system call failed you can pass up errno straight to the client without loosing information along the way. And for other error conditions it should be possible to find error codes describing what happened, i.e. when trying to add a device where no backend driver exists in qemu you can return ENOENT (plus freeform text message for error logging). cheers, Gerd
On Mon, Oct 19, 2009 at 12:40:08PM +0200, Gerd Hoffmann wrote: > >I think just returning error codes to the client is far too little > >information. I don't think we need the fully normalized structure > >that Luiz originally proposed with bus/dev addresses split out, but > >we certainly need to include a string description giving as much > >detail as possible. If attaching a host USB device failed, I don't > >want a single error code QERR_OPEN_FAILED, nor a generic message > >like 'could not open device', i want the exact details, eg > > > > 'could not open device: permission denied' > > 'could not open device: no such file or directory' > > 'could not open device: device or resource busy' > > Which makes me wonder whenever it makes sense to re-use errno for the > error codes instead of inventing our own QERR_* codes? Not the numbers > of course because they are not standardized as far I know, but the Ename > strings. > > If you error out because a system call failed you can pass up errno > straight to the client without loosing information along the way. And > for other error conditions it should be possible to find error codes > describing what happened, i.e. when trying to add a device where no > backend driver exists in qemu you can return ENOENT (plus freeform text > message for error logging). If applicable to the context, I think it is worthwhile including more than just the generic errno string. eg, if failed opening a disk file, then it would be fine to just send back the errno string, because the original drive_add command would already include the filename in question. If failed opening some random sysfs file relating to a PCI device that is being attached, then QEMU should back the sysfs filename because it may not be obvious from just the PCI bus/domain/slot number in the original command just which file QEMU failed to open. Daniel
On 10/19/2009 12:40 PM, Gerd Hoffmann wrote: >> 'could not open device: permission denied' 'could not open device: >> no such file or directory' 'could not open device: device or >> resource busy' > > Which makes me wonder whenever it makes sense to re-use errno for the > error codes instead of inventing our own QERR_* codes? The errno would be just one field in the error. You'd have QERR_COULD_NOT_OPEN with a string field EACCES/ENOENT/EBUSY/Ewhatever. > Not the numbers of course because they are not standardized as far I > know, but the Ename strings. No, numbers are not standardized. Paolo
Daniel P. Berrange wrote: > hink just returning error codes to the client is far too little > information. I don't think we need the fully normalized structure > that Luiz originally proposed with bus/dev addresses split out, but > we certainly need to include a string description giving as much > detail as possible. If attaching a host USB device failed, I don't > want a single error code QERR_OPEN_FAILED, nor a generic message > like 'could not open device', i want the exact details, eg > > 'could not open device: permission denied' > 'could not open device: no such file or directory' > 'could not open device: device or resource busy' > > The localization issue is somewhat of a red herring though. This string > description is not something that clients would ultimately expose to the > user. The client apps would likely present a more generic error message > to the user, based off the error code. The error description received > from QEMU will instead be written out to the logs as a record for later > troubleshooting. eg if the user files a bug report, it will include the > full error details, so libvirt/qemu maintainers have a better chance of > figuring out what went wrong. > But then advanced users end up doing silly things like analyzing libvirt logs. Why do that when your own example is obviously "could not open device: %s", strerror(errno). We should pass {"error" : {"type": "OpenFailedException", "errno": errno}}
On Mon, Oct 19, 2009 at 09:00:33AM -0500, Anthony Liguori wrote: > Daniel P. Berrange wrote: > >hink just returning error codes to the client is far too little > >information. I don't think we need the fully normalized structure > >that Luiz originally proposed with bus/dev addresses split out, but > >we certainly need to include a string description giving as much > >detail as possible. If attaching a host USB device failed, I don't > >want a single error code QERR_OPEN_FAILED, nor a generic message > >like 'could not open device', i want the exact details, eg > > > > 'could not open device: permission denied' > > 'could not open device: no such file or directory' > > 'could not open device: device or resource busy' > > > >The localization issue is somewhat of a red herring though. This string > >description is not something that clients would ultimately expose to the > >user. The client apps would likely present a more generic error message > >to the user, based off the error code. The error description received > >from QEMU will instead be written out to the logs as a record for later > >troubleshooting. eg if the user files a bug report, it will include the > >full error details, so libvirt/qemu maintainers have a better chance of > >figuring out what went wrong. > > > > But then advanced users end up doing silly things like analyzing libvirt > logs. Why do that when your own example is obviously "could not open > device: %s", strerror(errno). We should pass {"error" : {"type": > "OpenFailedException", "errno": errno}} Having a named "exception" instead of an error code is fine, but I think it is overkill to include fully-structured data fields like 'errno' instead of just a string. The libvirt application API has a insanely detailed error object allowing for passing of structured data back to apps, but I'm not aware of any application that has ever used it. They all just hook on the error code, and log/print detailed string message field. In terms of libvirt using QMP error data, there may be one or two places where we'd toggle behaviour off an error code / exception type, but any futher structured data would likely just be converted to a generic string message. So I don't see much need for anything beyond a error code or exception type to be used for behaviour decisions, and a detailed string description for logging. Daniel
Daniel P. Berrange wrote: > Having a named "exception" instead of an error code is fine, but I think > it is overkill to include fully-structured data fields like 'errno' instead > of just a string. The libvirt application API has a insanely detailed > error object allowing for passing of structured data back to apps, but > I'm not aware of any application that has ever used it. They all just > hook on the error code, and log/print detailed string message field. > Just because the current consumers are lazy doesn't mean we should restrict the API. > In terms of libvirt using QMP error data, there may be one or two > places where we'd toggle behaviour off an error code / exception type, > but any futher structured data would likely just be converted to a generic > string message. So I don't see much need for anything beyond a error code > or exception type to be used for behaviour decisions, and a detailed string > description for logging. > While it gets complicated when going through things like libvirt, if we design the python bindings for libqmp directly, these will appear as proper exception objects with members corresponding to the data. I would fully expect that users would use the structured exception data in a language like Python. For instance, when changing a cdrom iso, being able to handle EPERM and ENOENT separately is very valuable. Regards, Anthony Liguori
On Mon, Oct 19, 2009 at 10:27:51AM -0500, Anthony Liguori wrote: > Daniel P. Berrange wrote: > >Having a named "exception" instead of an error code is fine, but I think > >it is overkill to include fully-structured data fields like 'errno' instead > >of just a string. The libvirt application API has a insanely detailed > >error object allowing for passing of structured data back to apps, but > >I'm not aware of any application that has ever used it. They all just > >hook on the error code, and log/print detailed string message field. > > > > Just because the current consumers are lazy doesn't mean we should > restrict the API. > > >In terms of libvirt using QMP error data, there may be one or two > >places where we'd toggle behaviour off an error code / exception type, > >but any futher structured data would likely just be converted to a generic > >string message. So I don't see much need for anything beyond a error code > >or exception type to be used for behaviour decisions, and a detailed string > >description for logging. > > > > While it gets complicated when going through things like libvirt, if we > design the python bindings for libqmp directly, these will appear as > proper exception objects with members corresponding to the data. I would > fully expect that users would use the structured exception data in a > language like Python. For instance, when changing a cdrom iso, being > able to handle EPERM and ENOENT separately is very valuable. That would suggest that the example exception type is wrong. Instead of naming the exception after the operation that failed, named it after the cause of the failure. ie, this {"error" : {"type": "OpenFailedException", "errno": errno}} would be better as one of... {"error" : {"type": "AccessDeniedException", "desc": "unable to open disk /tmp/demo.qcow"}}, {"error" : {"type": "NoSuchFileException", "desc": "file /tmp/demo.qcow does not exist" }} ....other specific causes of failure... {"error" : {"type": "SystemException", "desc": "file /tmp/demo.qcow does: ...some other strerror()..." }} which is more like the type of exception classification you get in Java, Python. Explicit named exception types for important problems, and then one generic 'SystemException' for less commonly encountered errno's. Of course Java/Python have inheritance, so AccessDeniedException and NoSuchFileException would be inherit from SystemException anyway. It is fairly rare to find exceptions providing more fine grained detail than just the string error message. Daniel
diff --git a/hw/qdev.c b/hw/qdev.c index 906e897..3ce48f7 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -28,6 +28,7 @@ #include "net.h" #include "qdev.h" #include "sysemu.h" +#include "qerror.h" #include "monitor.h" static int qdev_hotplug = 0; @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) /* find driver */ info = qdev_find_info(NULL, driver); if (!info) { - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", - driver); + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); return NULL; } if (info->no_user) {
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- hw/qdev.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)