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

[Luiz Capitulino]

On Thu, 21 Jun 2012 13:42:19 +0100
"Daniel P. Berrange" <address@hidden> wrote:

[...]

> > 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....

[...]

> 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.

This seems a good middle ground too me, but let's elaborate it further.

I think what you're proposing boils down to three points:

 1. Allow printf-like strings for desc

 2. Avoid creating a new error class for each errno

 3. Don't create similar error classes

Item 3 seems quite obvious, but we've failed doing that and the main reason
for that is probably because desc is too strict today, so people end up
creating new errors just to have a different desc.

This means that having a printf-like string will help. I propose having
the following function for this:

 error_set_fmt(Error **errp, ErrorClass err_class, const char *fmt, ...);

Or, we could rename the current error_set() function to error_set_deprecated()
and use the above function only (renamed to error_set()).

Also, I believe this fits well with some internal improvements I'm planning
for some time now, like having qapi-errors.json (I already have a PoC for this,
but haven't posted it yet).

Lastly, regarding item 2, I think we already have consensus that returning:

 { "error": "WriteFailed", "data": { "os-error": "eio" },
   "desc": "input/output error while writing to '/tmp/forty-two'" }

Is better than returning:

 { "error": "IOError" }