| Message ID | 1272486729-14771-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > We need to expose errno in QMP, for three reasons: > > 1. Some error handling functions print errno codes to the user, > while it's debatable whether this is good or not from a user > perspective, sometimes it's the best we can do because it's > what system calls and libraries return > > 2. Some events (eg. BLOCK_IO_ERROR) will be made even more > complete with errno information > > 3. It's very good for debugging > > So, we need a way to expose those codes in QMP. We can't just use > the codes themselfs because they may vary between systems. > > The best solution I can think of is to return the string > representation of the name. For example, EIO becomes "EIO". > > This is what get_errno_name() does. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qemu-error.h | 1 + > 2 files changed, 86 insertions(+), 0 deletions(-) > > diff --git a/qemu-error.c b/qemu-error.c > index 5a35e7c..7035417 100644 > --- a/qemu-error.c > +++ b/qemu-error.c > @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) > va_end(ap); > error_printf("\n"); > } > + > +/* > + * Probably only useful for QMP > + */ > +const char *get_errno_name(int err) > +{ > + switch (abs(err)) { > + case EPERM: > + return "EPERM"; > + case ENOENT: > + return "ENOENT"; [...] > + case EDOM: > + return "EDOM"; > + case ERANGE: > + return "ERANGE"; > + case ENOMEDIUM: > + return "ENOMEDIUM"; > + case ENOTSUP: > + return "ENOTSUP"; > + default: > + return "unknown"; How did you choose the codes to implement? POSIX has many more... > + } > + > + abort(); > +} > diff --git a/qemu-error.h b/qemu-error.h > index a45609f..c5e39b5 100644 > --- a/qemu-error.h > +++ b/qemu-error.h > @@ -38,4 +38,5 @@ void error_print_loc(void); > void error_set_progname(const char *argv0); > void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2))); > > +const char *get_errno_name(int err); > #endif
On 05/03/2010 08:06 AM, Markus Armbruster wrote: > Luiz Capitulino<lcapitulino@redhat.com> writes: > > >> We need to expose errno in QMP, for three reasons: >> >> 1. Some error handling functions print errno codes to the user, >> while it's debatable whether this is good or not from a user >> perspective, sometimes it's the best we can do because it's >> what system calls and libraries return >> >> 2. Some events (eg. BLOCK_IO_ERROR) will be made even more >> complete with errno information >> >> 3. It's very good for debugging >> >> So, we need a way to expose those codes in QMP. We can't just use >> the codes themselfs because they may vary between systems. >> >> The best solution I can think of is to return the string >> representation of the name. For example, EIO becomes "EIO". >> >> This is what get_errno_name() does. >> >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> >> --- >> qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qemu-error.h | 1 + >> 2 files changed, 86 insertions(+), 0 deletions(-) >> >> diff --git a/qemu-error.c b/qemu-error.c >> index 5a35e7c..7035417 100644 >> --- a/qemu-error.c >> +++ b/qemu-error.c >> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) >> va_end(ap); >> error_printf("\n"); >> } >> + >> +/* >> + * Probably only useful for QMP >> + */ >> +const char *get_errno_name(int err) >> +{ >> + switch (abs(err)) { >> + case EPERM: >> + return "EPERM"; >> + case ENOENT: >> + return "ENOENT"; >> > [...] > >> + case EDOM: >> + return "EDOM"; >> + case ERANGE: >> + return "ERANGE"; >> + case ENOMEDIUM: >> + return "ENOMEDIUM"; >> + case ENOTSUP: >> + return "ENOTSUP"; >> + default: >> + return "unknown"; >> > How did you choose the codes to implement? POSIX has many more... > Let me say another way why I think this is a bad path to go down. In generally, we could never just pass errno through down. Different host platforms are going to generate different errno values so we really need to filter and send reliable errno values so that clients don't have to have special code for when they're on Linux vs. AIX vs. Solaris. If we're white listing errno values, we should be able to trivially convert errnos to QError types via a table just like you have above. Regards, Anthony Liguori >> + } >> + >> + abort(); >> +} >> diff --git a/qemu-error.h b/qemu-error.h >> index a45609f..c5e39b5 100644 >> --- a/qemu-error.h >> +++ b/qemu-error.h >> @@ -38,4 +38,5 @@ void error_print_loc(void); >> void error_set_progname(const char *argv0); >> void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2))); >> >> +const char *get_errno_name(int err); >> #endif >> > >
On Mon, 03 May 2010 08:16:35 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 05/03/2010 08:06 AM, Markus Armbruster wrote: > > Luiz Capitulino<lcapitulino@redhat.com> writes: > > > > > >> We need to expose errno in QMP, for three reasons: > >> > >> 1. Some error handling functions print errno codes to the user, > >> while it's debatable whether this is good or not from a user > >> perspective, sometimes it's the best we can do because it's > >> what system calls and libraries return > >> > >> 2. Some events (eg. BLOCK_IO_ERROR) will be made even more > >> complete with errno information > >> > >> 3. It's very good for debugging > >> > >> So, we need a way to expose those codes in QMP. We can't just use > >> the codes themselfs because they may vary between systems. > >> > >> The best solution I can think of is to return the string > >> representation of the name. For example, EIO becomes "EIO". > >> > >> This is what get_errno_name() does. > >> > >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > >> --- > >> qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> qemu-error.h | 1 + > >> 2 files changed, 86 insertions(+), 0 deletions(-) > >> > >> diff --git a/qemu-error.c b/qemu-error.c > >> index 5a35e7c..7035417 100644 > >> --- a/qemu-error.c > >> +++ b/qemu-error.c > >> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) > >> va_end(ap); > >> error_printf("\n"); > >> } > >> + > >> +/* > >> + * Probably only useful for QMP > >> + */ > >> +const char *get_errno_name(int err) > >> +{ > >> + switch (abs(err)) { > >> + case EPERM: > >> + return "EPERM"; > >> + case ENOENT: > >> + return "ENOENT"; > >> > > [...] > > > >> + case EDOM: > >> + return "EDOM"; > >> + case ERANGE: > >> + return "ERANGE"; > >> + case ENOMEDIUM: > >> + return "ENOMEDIUM"; > >> + case ENOTSUP: > >> + return "ENOTSUP"; > >> + default: > >> + return "unknown"; > >> > > How did you choose the codes to implement? POSIX has many more... I just ran an awk script on the linux's base errno header file, my idea is just to have the common names, anything 'new' will hit the default clause and we can add it later. > Let me say another way why I think this is a bad path to go down. > > In generally, we could never just pass errno through down. Different > host platforms are going to generate different errno values so we really > need to filter and send reliable errno values so that clients don't have > to have special code for when they're on Linux vs. AIX vs. Solaris. Sorry for the potential stupid question, but what would a 'reliable' errno be? Or, what's an unreliable errno? We're not sending plain integers to the clients, so the only problem I can see is if different unices return different errnos for the same error. Is that the problem you're seeing? > If we're white listing errno values, we should be able to trivially > convert errnos to QError types via a table just like you have above. Having a direct errno -> QError mapping doesn't seem good for the current use cases. For example, do_savevm() (not merged yet) has a StateVmSaveFailed error which also has a 'reason' member, which would look like this on the wire: "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': "EIO" } }" So, the QError class says what has failed and the errno part says why it has failed. I'd be happy to implement a different solution that satisfies this basic requirement.
On 05/04/2010 08:56 AM, Luiz Capitulino wrote: > On Mon, 03 May 2010 08:16:35 -0500 > Anthony Liguori<anthony@codemonkey.ws> wrote: > > >> On 05/03/2010 08:06 AM, Markus Armbruster wrote: >> >>> Luiz Capitulino<lcapitulino@redhat.com> writes: >>> >>> >>> >>>> We need to expose errno in QMP, for three reasons: >>>> >>>> 1. Some error handling functions print errno codes to the user, >>>> while it's debatable whether this is good or not from a user >>>> perspective, sometimes it's the best we can do because it's >>>> what system calls and libraries return >>>> >>>> 2. Some events (eg. BLOCK_IO_ERROR) will be made even more >>>> complete with errno information >>>> >>>> 3. It's very good for debugging >>>> >>>> So, we need a way to expose those codes in QMP. We can't just use >>>> the codes themselfs because they may vary between systems. >>>> >>>> The best solution I can think of is to return the string >>>> representation of the name. For example, EIO becomes "EIO". >>>> >>>> This is what get_errno_name() does. >>>> >>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> >>>> --- >>>> qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> qemu-error.h | 1 + >>>> 2 files changed, 86 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/qemu-error.c b/qemu-error.c >>>> index 5a35e7c..7035417 100644 >>>> --- a/qemu-error.c >>>> +++ b/qemu-error.c >>>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) >>>> va_end(ap); >>>> error_printf("\n"); >>>> } >>>> + >>>> +/* >>>> + * Probably only useful for QMP >>>> + */ >>>> +const char *get_errno_name(int err) >>>> +{ >>>> + switch (abs(err)) { >>>> + case EPERM: >>>> + return "EPERM"; >>>> + case ENOENT: >>>> + return "ENOENT"; >>>> >>>> >>> [...] >>> >>> >>>> + case EDOM: >>>> + return "EDOM"; >>>> + case ERANGE: >>>> + return "ERANGE"; >>>> + case ENOMEDIUM: >>>> + return "ENOMEDIUM"; >>>> + case ENOTSUP: >>>> + return "ENOTSUP"; >>>> + default: >>>> + return "unknown"; >>>> >>>> >>> How did you choose the codes to implement? POSIX has many more... >>> > I just ran an awk script on the linux's base errno header file, my > idea is just to have the common names, anything 'new' will hit the > default clause and we can add it later. > > >> Let me say another way why I think this is a bad path to go down. >> >> In generally, we could never just pass errno through down. Different >> host platforms are going to generate different errno values so we really >> need to filter and send reliable errno values so that clients don't have >> to have special code for when they're on Linux vs. AIX vs. Solaris. >> > Sorry for the potential stupid question, but what would a 'reliable' > errno be? Or, what's an unreliable errno? > > We're not sending plain integers to the clients, so the only problem > I can see is if different unices return different errnos for the > same error. Is that the problem you're seeing? > Different types of platforms return different errno values for the same error type. As an example, on Linux, connect() returns EINPROGRESS when you set the socket non-blocking. On Windows, it returns EWOULDBLOCK. If you just pass through errno, then all QMP clients are going to have to know the different between QEMU on Windows and Linux and handle the errnos appropriately. >> If we're white listing errno values, we should be able to trivially >> convert errnos to QError types via a table just like you have above. >> > Having a direct errno -> QError mapping doesn't seem good for the > current use cases. > > For example, do_savevm() (not merged yet) has a StateVmSaveFailed > error which also has a 'reason' member, which would look like this > on the wire: > > "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': "EIO" } }" > > So, the QError class says what has failed and the errno part says > why it has failed. > > I'd be happy to implement a different solution that satisfies this > basic requirement. > We need to map errnos to some QMP defined error type. However, as I've said before, "reason" is usually an indicator that an error is bad. A better error would be: { 'class': 'SocketIOError': 'data' : { 'source': 'migration' }} Or something like that. You don't want to have command and then CommandFailed as the error. You want the errors to be the 'reason's for the commands failure. Regards, Anthony Liguori
On Tue, 04 May 2010 09:03:47 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 05/04/2010 08:56 AM, Luiz Capitulino wrote: > > On Mon, 03 May 2010 08:16:35 -0500 > > Anthony Liguori<anthony@codemonkey.ws> wrote: > > > > > >> On 05/03/2010 08:06 AM, Markus Armbruster wrote: > >> > >>> Luiz Capitulino<lcapitulino@redhat.com> writes: > >>> > >>> > >>> > >>>> We need to expose errno in QMP, for three reasons: > >>>> > >>>> 1. Some error handling functions print errno codes to the user, > >>>> while it's debatable whether this is good or not from a user > >>>> perspective, sometimes it's the best we can do because it's > >>>> what system calls and libraries return > >>>> > >>>> 2. Some events (eg. BLOCK_IO_ERROR) will be made even more > >>>> complete with errno information > >>>> > >>>> 3. It's very good for debugging > >>>> > >>>> So, we need a way to expose those codes in QMP. We can't just use > >>>> the codes themselfs because they may vary between systems. > >>>> > >>>> The best solution I can think of is to return the string > >>>> representation of the name. For example, EIO becomes "EIO". > >>>> > >>>> This is what get_errno_name() does. > >>>> > >>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> > >>>> --- > >>>> qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>> qemu-error.h | 1 + > >>>> 2 files changed, 86 insertions(+), 0 deletions(-) > >>>> > >>>> diff --git a/qemu-error.c b/qemu-error.c > >>>> index 5a35e7c..7035417 100644 > >>>> --- a/qemu-error.c > >>>> +++ b/qemu-error.c > >>>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) > >>>> va_end(ap); > >>>> error_printf("\n"); > >>>> } > >>>> + > >>>> +/* > >>>> + * Probably only useful for QMP > >>>> + */ > >>>> +const char *get_errno_name(int err) > >>>> +{ > >>>> + switch (abs(err)) { > >>>> + case EPERM: > >>>> + return "EPERM"; > >>>> + case ENOENT: > >>>> + return "ENOENT"; > >>>> > >>>> > >>> [...] > >>> > >>> > >>>> + case EDOM: > >>>> + return "EDOM"; > >>>> + case ERANGE: > >>>> + return "ERANGE"; > >>>> + case ENOMEDIUM: > >>>> + return "ENOMEDIUM"; > >>>> + case ENOTSUP: > >>>> + return "ENOTSUP"; > >>>> + default: > >>>> + return "unknown"; > >>>> > >>>> > >>> How did you choose the codes to implement? POSIX has many more... > >>> > > I just ran an awk script on the linux's base errno header file, my > > idea is just to have the common names, anything 'new' will hit the > > default clause and we can add it later. > > > > > >> Let me say another way why I think this is a bad path to go down. > >> > >> In generally, we could never just pass errno through down. Different > >> host platforms are going to generate different errno values so we really > >> need to filter and send reliable errno values so that clients don't have > >> to have special code for when they're on Linux vs. AIX vs. Solaris. > >> > > Sorry for the potential stupid question, but what would a 'reliable' > > errno be? Or, what's an unreliable errno? > > > > We're not sending plain integers to the clients, so the only problem > > I can see is if different unices return different errnos for the > > same error. Is that the problem you're seeing? > > > > Different types of platforms return different errno values for the same > error type. > > As an example, on Linux, connect() returns EINPROGRESS when you set the > socket non-blocking. On Windows, it returns EWOULDBLOCK. Wonderful. > If you just pass through errno, then all QMP clients are going to have > to know the different between QEMU on Windows and Linux and handle the > errnos appropriately. Right. > >> If we're white listing errno values, we should be able to trivially > >> convert errnos to QError types via a table just like you have above. > >> > > Having a direct errno -> QError mapping doesn't seem good for the > > current use cases. > > > > For example, do_savevm() (not merged yet) has a StateVmSaveFailed > > error which also has a 'reason' member, which would look like this > > on the wire: > > > > "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': "EIO" } }" > > > > So, the QError class says what has failed and the errno part says > > why it has failed. > > > > I'd be happy to implement a different solution that satisfies this > > basic requirement. > > > > We need to map errnos to some QMP defined error type. However, as I've > said before, "reason" is usually an indicator that an error is bad. > > A better error would be: > > { 'class': 'SocketIOError': 'data' : { 'source': 'migration' }} > > Or something like that. You don't want to have command and then > CommandFailed as the error. You want the errors to be the 'reason's for > the commands failure. StateVmSaveFailed is not like CommandFailed, there are five errors in do_savevm() and StateVmSaveFailed happens to be one of them. But I understand what you mean and I have considered doing something like it, one of the problems though is that I'm not sure 'source' is enough to determine where the error has happened. Consider do_savevm() again. We have three 'operations' that might fail: delete an existing snapshot, save the VM state and create the snapshot. All those operations can return -EIO as an error. So, the first question is: would you map EIO to an QError? Just like you did for SocketIOError? If so, how would you know which operation has failed? Would you put its name in source? Or have an additional 'operation' key? A related problem is not to degrade the current set of error messages we offer to the users. For do_savevm()'s 'save state' operation, current message is: "Error -7 while writing VM" With StateVmSaveFailed it becomes: "Failed to save VM state ("EIO")" Given the current implementation of QError, I'm not sure how we can have such a good error message if our QErrors are not 'operation based'.
On 05/04/2010 03:30 PM, Luiz Capitulino wrote: > > StateVmSaveFailed is not like CommandFailed, there are five errors > in do_savevm() and StateVmSaveFailed happens to be one of them. > > But I understand what you mean and I have considered doing something > like it, one of the problems though is that I'm not sure 'source' is > enough to determine where the error has happened. > > Consider do_savevm() again. We have three 'operations' that might > fail: delete an existing snapshot, save the VM state and create the > snapshot. All those operations can return -EIO as an error. > Maybe those three operations should return distinct errnos? That way, we can make more useful QErrors. > So, the first question is: would you map EIO to an QError? Just like > you did for SocketIOError? If so, how would you know which operation > has failed? Would you put its name in source? Or have an additional > 'operation' key? > > A related problem is not to degrade the current set of error messages > we offer to the users. For do_savevm()'s 'save state' operation, current > message is: > > "Error -7 while writing VM" > > With StateVmSaveFailed it becomes: > > "Failed to save VM state ("EIO")" > I don't think this is that useful to preserve because as you said, it could be one of three things. Regards, Anthony Liguori > Given the current implementation of QError, I'm not sure how we can have > such a good error message if our QErrors are not 'operation based'. >
On Tue, 04 May 2010 16:56:19 -0500 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 05/04/2010 03:30 PM, Luiz Capitulino wrote: > > > > StateVmSaveFailed is not like CommandFailed, there are five errors > > in do_savevm() and StateVmSaveFailed happens to be one of them. > > > > But I understand what you mean and I have considered doing something > > like it, one of the problems though is that I'm not sure 'source' is > > enough to determine where the error has happened. > > > > Consider do_savevm() again. We have three 'operations' that might > > fail: delete an existing snapshot, save the VM state and create the > > snapshot. All those operations can return -EIO as an error. > > > > Maybe those three operations should return distinct errnos? I don't think this is possible, as we would have to guarantee that no function called by a handler return the same errno. Taking the block layer as an example. Most block drivers handlers check if the driver really exist (-ENOMEDIUM) and if the driver supports the operation being requested (-ENOTSUP). How can we have unique errnos in this case? Also remember that we're only talking about the surface. The call chain is deep. We have almost a hundred handlers, they use functions from almost all subsystems. > That way, we can make more useful QErrors. Perhaps, the question boils down to how QErrors should be done. Today, we're doing it like this, consider handler foo(), it does the following: 1. connect somewhere 2. send some data 3. close All operations performed can fail and we want to report that. Currently, afaiu we're doing the following (Markus correct me if I'm wrong): 1. ConnectFailed 2. SendFailed 3. CloseFailed An obvious problem is that we don't say why it has failed. But this is what errno is for and I thought we could use it someway. The advantage of this approach is that, errors are high-level. It's easy to identify what went wrong and we can have very good error messages for them. Now, if I got it right, you're suggesting we should do: 1. BadFileDescriptor, Interrupted, NoPermission ... (or anything connect() may return) 2. IOError ... 3. IOError, BadFileDescriptor This makes sense, but if I'm a user (or a QMP client) I don't want this: (qemu) savevm foobar Bad file descriptor I'd prefer this instead: (qemu) savevm foobar Can't delete 'foobar': Bad file descriptor Or: (qemu) savevm foobar Can't save VM state: I/O Error > > So, the first question is: would you map EIO to an QError? Just like > > you did for SocketIOError? If so, how would you know which operation > > has failed? Would you put its name in source? Or have an additional > > 'operation' key? > > > > A related problem is not to degrade the current set of error messages > > we offer to the users. For do_savevm()'s 'save state' operation, current > > message is: > > > > "Error -7 while writing VM" > > > > With StateVmSaveFailed it becomes: > > > > "Failed to save VM state ("EIO")" > > > > I don't think this is that useful to preserve because as you said, it > could be one of three things. But as I said above, we have to say which operation has failed, otherwise it's very difficult to diagnose the problem.
Anthony Liguori <anthony@codemonkey.ws> writes: > On 05/03/2010 08:06 AM, Markus Armbruster wrote: >> Luiz Capitulino<lcapitulino@redhat.com> writes: >> >> >>> We need to expose errno in QMP, for three reasons: >>> >>> 1. Some error handling functions print errno codes to the user, >>> while it's debatable whether this is good or not from a user >>> perspective, sometimes it's the best we can do because it's >>> what system calls and libraries return >>> >>> 2. Some events (eg. BLOCK_IO_ERROR) will be made even more >>> complete with errno information >>> >>> 3. It's very good for debugging >>> >>> So, we need a way to expose those codes in QMP. We can't just use >>> the codes themselfs because they may vary between systems. >>> >>> The best solution I can think of is to return the string >>> representation of the name. For example, EIO becomes "EIO". >>> >>> This is what get_errno_name() does. >>> >>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com> >>> --- >>> qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> qemu-error.h | 1 + >>> 2 files changed, 86 insertions(+), 0 deletions(-) >>> >>> diff --git a/qemu-error.c b/qemu-error.c >>> index 5a35e7c..7035417 100644 >>> --- a/qemu-error.c >>> +++ b/qemu-error.c >>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) >>> va_end(ap); >>> error_printf("\n"); >>> } >>> + >>> +/* >>> + * Probably only useful for QMP >>> + */ >>> +const char *get_errno_name(int err) >>> +{ >>> + switch (abs(err)) { >>> + case EPERM: >>> + return "EPERM"; >>> + case ENOENT: >>> + return "ENOENT"; >>> >> [...] >> >>> + case EDOM: >>> + return "EDOM"; >>> + case ERANGE: >>> + return "ERANGE"; >>> + case ENOMEDIUM: >>> + return "ENOMEDIUM"; >>> + case ENOTSUP: >>> + return "ENOTSUP"; >>> + default: >>> + return "unknown"; >>> >> How did you choose the codes to implement? POSIX has many more... >> > > Let me say another way why I think this is a bad path to go down. > > In generally, we could never just pass errno through down. Different > host platforms are going to generate different errno values so we > really need to filter and send reliable errno values so that clients > don't have to have special code for when they're on Linux vs. AIX > vs. Solaris. Most instances are covered by POSIX. The best way to deal with systems that violate POSIX is to provide conforming replacements for the offenders. See gnulib (I just wish it wasn't joined at the hip to automake). > If we're white listing errno values, we should be able to trivially > convert errnos to QError types via a table just like you have above. Yes, but you'll get many tables. Luiz encodes the information as event name + errno for events, and as error class + errno for errors. The same errno can mean different things for different events / errors. Thus, if you flatten the two into a single error, you need in the order of one table per event / error.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Tue, 04 May 2010 16:56:19 -0500 > Anthony Liguori <anthony@codemonkey.ws> wrote: > >> On 05/04/2010 03:30 PM, Luiz Capitulino wrote: >> > >> > StateVmSaveFailed is not like CommandFailed, there are five errors >> > in do_savevm() and StateVmSaveFailed happens to be one of them. >> > >> > But I understand what you mean and I have considered doing something >> > like it, one of the problems though is that I'm not sure 'source' is >> > enough to determine where the error has happened. >> > >> > Consider do_savevm() again. We have three 'operations' that might >> > fail: delete an existing snapshot, save the VM state and create the >> > snapshot. All those operations can return -EIO as an error. >> > >> >> Maybe those three operations should return distinct errnos? > > I don't think this is possible, as we would have to guarantee that no > function called by a handler return the same errno. > > Taking the block layer as an example. Most block drivers handlers check > if the driver really exist (-ENOMEDIUM) and if the driver supports the > operation being requested (-ENOTSUP). > > How can we have unique errnos in this case? > > Also remember that we're only talking about the surface. The call > chain is deep. We have almost a hundred handlers, they use functions > from almost all subsystems. > >> That way, we can make more useful QErrors. > > Perhaps, the question boils down to how QErrors should be done. > > Today, we're doing it like this, consider handler foo(), it does the following: > > 1. connect somewhere > 2. send some data > 3. close > > All operations performed can fail and we want to report that. Currently, > afaiu we're doing the following (Markus correct me if I'm wrong): > > 1. ConnectFailed > 2. SendFailed > 3. CloseFailed > > An obvious problem is that we don't say why it has failed. But this is > what errno is for and I thought we could use it someway. The advantage > of this approach is that, errors are high-level. It's easy to identify > what went wrong and we can have very good error messages for them. > > Now, if I got it right, you're suggesting we should do: > > 1. BadFileDescriptor, Interrupted, NoPermission ... > (or anything connect() may return) > 2. IOError ... > 3. IOError, BadFileDescriptor > > This makes sense, but if I'm a user (or a QMP client) I don't want this: > > (qemu) savevm foobar > Bad file descriptor > > I'd prefer this instead: > > (qemu) savevm foobar > Can't delete 'foobar': Bad file descriptor > > Or: > > (qemu) savevm foobar > Can't save VM state: I/O Error These are of the form "what failed: how it failed". Aside: not mentioned, because it's obvious for monitor commands: where it failed. error_report() adds that information, but only when it's not obvious, see error_print_loc(). We keep these parts separate because squashing them together into a single QError class multiplies the classes for no good reason, and makes it needlessly hard to group errors. E.g. sometimes we care only about "what failed". Then a squashed class is most unwelcome, because to recognize the "what failed" part you have to know all the squashed classes, hence all possible how-it-faileds, including the ones that will be invented after your program ships. [...]
diff --git a/qemu-error.c b/qemu-error.c index 5a35e7c..7035417 100644 --- a/qemu-error.c +++ b/qemu-error.c @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...) va_end(ap); error_printf("\n"); } + +/* + * Probably only useful for QMP + */ +const char *get_errno_name(int err) +{ + switch (abs(err)) { + case EPERM: + return "EPERM"; + case ENOENT: + return "ENOENT"; + case ESRCH: + return "ESRCH"; + case EINTR: + return "EINTR"; + case EIO: + return "EIO"; + case ENXIO: + return "ENXIO"; + case E2BIG: + return "E2BIG"; + case ENOEXEC: + return "ENOEXEC"; + case EBADF: + return "EBADF"; + case ECHILD: + return "ECHILD"; + case EAGAIN: + return "EAGAIN"; + case ENOMEM: + return "ENOMEM"; + case EACCES: + return "EACCES"; + case EFAULT: + return "EFAULT"; + case ENOTBLK: + return "ENOTBLK"; + case EBUSY: + return "EBUSY"; + case EEXIST: + return "EEXIST"; + case EXDEV: + return "EXDEV"; + case ENODEV: + return "ENODEV"; + case ENOTDIR: + return "ENOTDIR"; + case EISDIR: + return "EISDIR"; + case EINVAL: + return "EINVAL"; + case ENFILE: + return "ENFILE"; + case EMFILE: + return "EMFILE"; + case ENOTTY: + return "ENOTTY"; + case ETXTBSY: + return "ETXTBSY"; + case EFBIG: + return "EFBIG"; + case ENOSPC: + return "ENOSPC"; + case ESPIPE: + return "ESPIPE"; + case EROFS: + return "EROFS"; + case EMLINK: + return "EMLINK"; + case EPIPE: + return "EPIPE"; + case EDOM: + return "EDOM"; + case ERANGE: + return "ERANGE"; + case ENOMEDIUM: + return "ENOMEDIUM"; + case ENOTSUP: + return "ENOTSUP"; + default: + return "unknown"; + } + + abort(); +} diff --git a/qemu-error.h b/qemu-error.h index a45609f..c5e39b5 100644 --- a/qemu-error.h +++ b/qemu-error.h @@ -38,4 +38,5 @@ void error_print_loc(void); void error_set_progname(const char *argv0); void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2))); +const char *get_errno_name(int err); #endif
We need to expose errno in QMP, for three reasons: 1. Some error handling functions print errno codes to the user, while it's debatable whether this is good or not from a user perspective, sometimes it's the best we can do because it's what system calls and libraries return 2. Some events (eg. BLOCK_IO_ERROR) will be made even more complete with errno information 3. It's very good for debugging So, we need a way to expose those codes in QMP. We can't just use the codes themselfs because they may vary between systems. The best solution I can think of is to return the string representation of the name. For example, EIO becomes "EIO". This is what get_errno_name() does. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- qemu-error.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu-error.h | 1 + 2 files changed, 86 insertions(+), 0 deletions(-)