Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Luiz Capitulino]

On Tue, 04 May 2010 09:03:47 -0500
Anthony Liguori <address@hidden> wrote:

> On 05/04/2010 08:56 AM, Luiz Capitulino wrote:
> > On Mon, 03 May 2010 08:16:35 -0500
> > Anthony Liguori<address@hidden>  wrote:
> >
> >    
> >> On 05/03/2010 08:06 AM, Markus Armbruster wrote:
> >>      
> >>> Luiz Capitulino<address@hidden>   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<address@hidden>
> >>>> ---
> >>>>    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'.