Re: [Qemu-devel] [RFC] Fixing the error failure

[Daniel P. Berrange]

On Wed, Jun 20, 2012 at 02:48:38PM -0300, Luiz Capitulino wrote:
> Yet another thread fork.
> 
> After talking with Daniel and Markus about QMP errors (which is not just about
> QMP, as this affects QEMU as whole), I've put together the proposal below.
> 
> I'll discuss three points. First, the error format and classes. Second, the
> internal API and third compatibility.
> 
> Don't be afraid about the length of this email, only the first section is long
> but it mostly contains error classes listings.
> 
> 1. Error format and classes
> 
> We can keep the same error format we have today, which is:
> 
>  { "error": { "class": json-string,
>               "data": json-object,
>               "desc": json-string }, "id": json-value }
> 
>   where 'data', 'desc' and 'id' are optional fields.

Yep, that is good.

> However, we'd change how we use 'desc' and our error classes. 'desc' would
> become a string which is filled by a printf-like function (see section 2) and

Good, using a printf-like string for desc is the big change
I wanted to see in QMP errors.

> we'd replace all error classes we have today by the following ones:

Nooo, that's going a bit to far in simplification....

>   o ParameterError: any error which involves a bad parameter. Replaces
>     InvalidParameter, InvalidParameterCombination, InvalidParameterType,
>     InvalidParameterValue, MissingParameter
> 
>   o SystemError: syscall or library errors. Replaces BufferOverrun,
>     IOError, OpenFileFailed, PermissionDenied, TooManyFiles,
>     SockConnectInprogress, SockConnectFailed, SockListenFailed,
>     SockBindFailed, SockCreateFailed.
> 
>     This error can include an optional 'os-error' field in the 'data'
>     member (see section 2).
> 
>   o QEMUError: errors that are internal to QEMU, like AmbiguousPath,
>     BadBusForDevice, BusNoHotplug, BusNotFound, CommandDisabled,
>     CommandNotFound, DuplicateId, FeatureDisabled, JSONParseError,
>     JSONParsing, KVMMissingCap, MigrationActive, MigrationNotSupported,
>     MigrationExpected, NoBusForDevice, NotSupported, PropertyValueBad,
>     PropertyValueInUse, PropertyValueNotFound, PropertyValueNotPowerOf2,
>     PropertyValueOutOfRange, ResetRequired, SetPasswdFailed, Unsupported,
>     VirtFSFeatureBlocksMigration, VNCServerFailed
> 
>   o UndefinedError: the same it's today, undefined :)

There is a balance to be struck here - previously we were tending
to invent a new error class for every conceivable error condition.
This proposal meanwhile is swinging too far to the other extreme
having only 4/5 classes. There is a balance to be had here.

It is perfectly reasonable, and indeed useful, to have distinct
errors like CommandNotFound, CommandDisabled, and so on.  What
we shouldn't do, however, is do things like invent a new error
for every possible errno value. The examples of  PropertyValueNotFound,
PropertyValueNotPowerOf2, PropertyValueOutOfRange are cases where
we invented too many codes, and in the new world we would do
something like

   PropertyValueInvalid
     msg = "Value must be a power of 2"
     msg = "Value must be in range 1-30"
     msg = "Value not found"

We have to just use prudent judgement to decide whether to use
an existing error, or create a new one.

In libvirt we have always reserved the right to change the error
code reported for particular scenarios. So, for example, alot of
our errors started out as "InternalError" (equiv to UndefinedError)
but over time we have changed some to more specialized values
eg "InvalidOperation", "ConfigNotSupported" and so on.

We should probably explicitly note that any use of "UndefinedError"
is liable to be changed in a future QEMU release.

> Now, there are two important points to be observed:
> 
>  - We check for DeviceEncrypted in hmp.c and fetch its parameters. This
>    probably indicates that we might want to create specialized classes
>    when necessary
> 
>  - I don't know where to put all the DeviceFoo errors, but they probably fit
>    in QEMUError or a new class like DeviceError
> 3. Compatibility
> 
> We have two options here:
> 
>  1. Maintain the current errors and implement the new way only for new
>     commands (includes commands merged for 1.2).
> 
>       Pros: maintains compatibility and won't require any code churn
>       Cons: we'll have two different errors schemas in use at the same
>             time and will be carrying garbage forward
> 
>  2. Do a full conversion to the new way.
> 
>       Pros: we drop bad code and avoid pollution (good for Rio+20)
>       Cons: possibly breaks compatibility and will require a lot of code
>             churn up front

Just maintain existing usage, but apply appropriate judgement for new
conversions.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|