Questionable aspects of QEMU Error's design

Questionable aspects of QEMU Error's design

[Markus Armbruster]

QEMU's Error was patterned after GLib's GError.  Differences include:

* &error_fatal, &error_abort for convenience

* Error can optionally store hints

* Pointlessly different names: error_prepend() vs. g_error_prefix() and
  so forth *shrug*

* Propagating errors

  Thanks to Vladimir, we'll soon have "auto propagation", which is less
  verbose and less error-prone.

* Accumulating errors

  error_propagate() permits it, g_propagate_error() does not[*].

  I believe this feature is used rarely.  Perhaps we'd be better off
  without it.  The problem is identifying its uses.  If I remember
  correctly, Vladimir struggled with that for his "auto propagation"
  work.

  Perhaps "auto propagation" will reduce the number of manual
  error_propagate() to the point where we can identify accumulations.
  Removing the feature would become feasible then.

* Distinguishing different errors

  Where Error has ErrorClass, GError has Gquark domain, gint code.  Use
  of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
  discouraged.  When we need callers to distinguish errors, we return
  suitable error codes separately.

* Return value conventions

  Common: non-void functions return a distinct error value on failure
  when such a value can be defined.  Patterns:

  - Functions returning non-null pointers on success return null pointer
    on failure.

  - Functions returning non-negative integers on success return a
    negative error code on failure.

  Different: GLib discourages void functions, because these lead to
  awkward error checking code.  We have tons of them, and tons of
  awkward error checking code:

    Error *err = NULL;
    frobnicate(arg, &err);
    if (err) {
        ... recover ...
        error_propagate(errp, err);
    }

  instead of

    if (!frobnicate(arg, errp))
        ... recover ...
    }

  Can also lead to pointless creation of Error objects.

  I consider this a design mistake.  Can we still fix it?  We have more
  than 2000 void functions taking an Error ** parameter...

  Transforming code that receives and checks for errors with Coccinelle
  shouldn't be hard.  Transforming code that returns errors seems more
  difficult.  We need to transform explicit and implicit return to
  either return true or return false, depending on what we did to the
  @errp parameter on the way to the return.  Hmm.


[*] According to documentation; the code merely calls g_warning() then,
in typical GLib fashion.

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

01.04.2020 12:02, Markus Armbruster wrote:
> QEMU's Error was patterned after GLib's GError.  Differences include:
> 
> * &error_fatal, &error_abort for convenience
> 
> * Error can optionally store hints
> 
> * Pointlessly different names: error_prepend() vs. g_error_prefix() and
>    so forth *shrug*
> 
> * Propagating errors
> 
>    Thanks to Vladimir, we'll soon have "auto propagation", which is less
>    verbose and less error-prone.
> 
> * Accumulating errors
> 
>    error_propagate() permits it, g_propagate_error() does not[*].
> 
>    I believe this feature is used rarely.  Perhaps we'd be better off
>    without it.  The problem is identifying its uses.  If I remember
>    correctly, Vladimir struggled with that for his "auto propagation"
>    work.
> 
>    Perhaps "auto propagation" will reduce the number of manual
>    error_propagate() to the point where we can identify accumulations.
>    Removing the feature would become feasible then.
> 
> * Distinguishing different errors
> 
>    Where Error has ErrorClass, GError has Gquark domain, gint code.  Use
>    of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
>    discouraged.  When we need callers to distinguish errors, we return
>    suitable error codes separately.
> 
> * Return value conventions
> 
>    Common: non-void functions return a distinct error value on failure
>    when such a value can be defined.  Patterns:
> 
>    - Functions returning non-null pointers on success return null pointer
>      on failure.
> 
>    - Functions returning non-negative integers on success return a
>      negative error code on failure.
> 
>    Different: GLib discourages void functions, because these lead to
>    awkward error checking code.  We have tons of them, and tons of
>    awkward error checking code:
> 
>      Error *err = NULL;
>      frobnicate(arg, &err);
>      if (err) {
>          ... recover ...
>          error_propagate(errp, err);
>      }
> 
>    instead of
> 
>      if (!frobnicate(arg, errp))
>          ... recover ...
>      }
> 
>    Can also lead to pointless creation of Error objects.
> 
>    I consider this a design mistake.  Can we still fix it?  We have more
>    than 2000 void functions taking an Error ** parameter...
> 
>    Transforming code that receives and checks for errors with Coccinelle
>    shouldn't be hard.  Transforming code that returns errors seems more
>    difficult.  We need to transform explicit and implicit return to
>    either return true or return false, depending on what we did to the
>    @errp parameter on the way to the return.  Hmm.
> 
> 
> [*] According to documentation; the code merely calls g_warning() then,
> in typical GLib fashion.
> 


Side question:

Can we somehow implement a possibility to reliably identify file and line number
where error is set by error message?

It's where debug of error-bugs always starts: try to imagine which parts of the error
message are "%s", and how to grep for it in the code, keeping in mind also,
that error massage may be split into several lines..

Put file:line into each error? Seems too noisy for users.. A lot of errors are not
bugs: use do something wrong and see the error, and understands what he is doing
wrong.. It's not usual practice to print file:line into each message for user.


But what if we do some kind of mapping file:line <-> error code, so user will see
something like:


    Error 12345: Device drive-scsi0-0-0-0 is not found

....

Hmm, maybe, just add one more argument to error_setg:

error_setg(errp, 12345, "Device %s is not found", device_name);

- it's enough grep-able.


-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

01.04.2020 15:10, Vladimir Sementsov-Ogievskiy wrote:
> 01.04.2020 12:02, Markus Armbruster wrote:
>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>
>> * &error_fatal, &error_abort for convenience
>>
>> * Error can optionally store hints
>>
>> * Pointlessly different names: error_prepend() vs. g_error_prefix() and
>>    so forth *shrug*
>>
>> * Propagating errors
>>
>>    Thanks to Vladimir, we'll soon have "auto propagation", which is less
>>    verbose and less error-prone.
>>
>> * Accumulating errors
>>
>>    error_propagate() permits it, g_propagate_error() does not[*].
>>
>>    I believe this feature is used rarely.  Perhaps we'd be better off
>>    without it.  The problem is identifying its uses.  If I remember
>>    correctly, Vladimir struggled with that for his "auto propagation"
>>    work.
>>
>>    Perhaps "auto propagation" will reduce the number of manual
>>    error_propagate() to the point where we can identify accumulations.
>>    Removing the feature would become feasible then.
>>
>> * Distinguishing different errors
>>
>>    Where Error has ErrorClass, GError has Gquark domain, gint code.  Use
>>    of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
>>    discouraged.  When we need callers to distinguish errors, we return
>>    suitable error codes separately.
>>
>> * Return value conventions
>>
>>    Common: non-void functions return a distinct error value on failure
>>    when such a value can be defined.  Patterns:
>>
>>    - Functions returning non-null pointers on success return null pointer
>>      on failure.
>>
>>    - Functions returning non-negative integers on success return a
>>      negative error code on failure.
>>
>>    Different: GLib discourages void functions, because these lead to
>>    awkward error checking code.  We have tons of them, and tons of
>>    awkward error checking code:
>>
>>      Error *err = NULL;
>>      frobnicate(arg, &err);
>>      if (err) {
>>          ... recover ...
>>          error_propagate(errp, err);
>>      }
>>
>>    instead of
>>
>>      if (!frobnicate(arg, errp))
>>          ... recover ...
>>      }
>>
>>    Can also lead to pointless creation of Error objects.
>>
>>    I consider this a design mistake.  Can we still fix it?  We have more
>>    than 2000 void functions taking an Error ** parameter...
>>
>>    Transforming code that receives and checks for errors with Coccinelle
>>    shouldn't be hard.  Transforming code that returns errors seems more
>>    difficult.  We need to transform explicit and implicit return to
>>    either return true or return false, depending on what we did to the
>>    @errp parameter on the way to the return.  Hmm.
>>
>>
>> [*] According to documentation; the code merely calls g_warning() then,
>> in typical GLib fashion.
>>
> 
> 
> Side question:
> 
> Can we somehow implement a possibility to reliably identify file and line number
> where error is set by error message?
> 
> It's where debug of error-bugs always starts: try to imagine which parts of the error
> message are "%s", and how to grep for it in the code, keeping in mind also,
> that error massage may be split into several lines..
> 
> Put file:line into each error? Seems too noisy for users.. A lot of errors are not
> bugs: use do something wrong and see the error, and understands what he is doing
> wrong.. It's not usual practice to print file:line into each message for user.
> 
> 
> But what if we do some kind of mapping file:line <-> error code, so user will see
> something like:
> 
> 
>     Error 12345: Device drive-scsi0-0-0-0 is not found
> 
> ....
> 
> Hmm, maybe, just add one more argument to error_setg:
> 
> error_setg(errp, 12345, "Device %s is not found", device_name);
> 
> - it's enough grep-able.
> 


Or, maybe, go the hard way, add a script, which smartly search for error message in code-base,
correctly handling "%s" and multi-line messages.. And add to checkpatch a check
that new added error messages doesn't match to any of existing.

Hmm. I think, I'll try to do something like this.



-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Daniel P. Berrangé]

On Wed, Apr 01, 2020 at 11:02:11AM +0200, Markus Armbruster wrote:
> QEMU's Error was patterned after GLib's GError.  Differences include:
> 
> * &error_fatal, &error_abort for convenience

I think this doesn't really need to exist, and is an artifact
of the later point "return values" where we commonly make methds
return void.  If we adopted a non-void return value, then these
are no longer so compelling.

Consider if we didn't have &error_fatal right now, then we would
need to

   Error *local_err = NULL;
   qemu_boot_set(boot_once, &local_err)
   if (*local_err)
      abort();

This is tedious, so we invented &error_abort to make our lives
better

   qemu_boot_set(boot_once, &error_abort)


If we had a "bool" return value though, we would probably have just
ended up doing:

   assert(qemu_boot_set(boot_once, NULL));

or

   if (!qemu_boot_set(boot_once, NULL))
       abort()

and would never have invented &error_fatal.

> * Distinguishing different errors
> 
>   Where Error has ErrorClass, GError has Gquark domain, gint code.  Use
>   of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
>   discouraged.  When we need callers to distinguish errors, we return
>   suitable error codes separately.

The GQuark is just a static string, and in most cases this ends up being
defined per-file, or sometimes per functional group. So essentially you
can consider it to approximately a source file in most cases. The code
is a constant of some arbitrary type that is generally considered to be
scoped within the context of the GQuark domain.

> * Return value conventions
> 
>   Common: non-void functions return a distinct error value on failure
>   when such a value can be defined.  Patterns:
> 
>   - Functions returning non-null pointers on success return null pointer
>     on failure.
> 
>   - Functions returning non-negative integers on success return a
>     negative error code on failure.
> 
>   Different: GLib discourages void functions, because these lead to
>   awkward error checking code.  We have tons of them, and tons of
>   awkward error checking code:
> 
>     Error *err = NULL;
>     frobnicate(arg, &err);
>     if (err) {
>         ... recover ...
>         error_propagate(errp, err);
>     }

Yeah, I really dislike this verbose style...

> 
>   instead of
> 
>     if (!frobnicate(arg, errp))
>         ... recover ...
>     }

...so I've followed this style for any code I've written in QEMU
where possible.

> 
>   Can also lead to pointless creation of Error objects.
> 
>   I consider this a design mistake.  Can we still fix it?  We have more
>   than 2000 void functions taking an Error ** parameter...

Even if we don't do full conversion, we can at least encourage the
simpler style - previously reviewers have told me to rewrite code
to use the more verbose style, which I resisted. So at the very
least setting the expectations for preferred style is useful.

>   Transforming code that receives and checks for errors with Coccinelle
>   shouldn't be hard.  Transforming code that returns errors seems more
>   difficult.  We need to transform explicit and implicit return to
>   either return true or return false, depending on what we did to the
>   @errp parameter on the way to the return.  Hmm.

Even if we only converted methods which are currently void, that
would be a notable benefit I think.

It is a shame we didn't just use GError from the start, but I guess
its probably too late to consider changing that now.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

01.04.2020 15:44, Daniel P. Berrangé wrote:
> On Wed, Apr 01, 2020 at 11:02:11AM +0200, Markus Armbruster wrote:
>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>
>> * &error_fatal, &error_abort for convenience
> 
> I think this doesn't really need to exist, and is an artifact
> of the later point "return values" where we commonly make methds
> return void.  If we adopted a non-void return value, then these
> are no longer so compelling.
> 
> Consider if we didn't have &error_fatal right now, then we would
> need to
> 
>     Error *local_err = NULL;
>     qemu_boot_set(boot_once, &local_err)
>     if (*local_err)
>        abort();
> 
> This is tedious, so we invented &error_abort to make our lives
> better
> 
>     qemu_boot_set(boot_once, &error_abort)
> 
> 
> If we had a "bool" return value though, we would probably have just
> ended up doing:
> 
>     assert(qemu_boot_set(boot_once, NULL));

But error_abort is better: it crashes where error fired and backtrace
in core file is backtrace of and error.

> 
> or
> 
>     if (!qemu_boot_set(boot_once, NULL))
>         abort()
> 
> and would never have invented &error_fatal.
> 
>> * Distinguishing different errors
>>
>>    Where Error has ErrorClass, GError has Gquark domain, gint code.  Use
>>    of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
>>    discouraged.  When we need callers to distinguish errors, we return
>>    suitable error codes separately.
> 
> The GQuark is just a static string, and in most cases this ends up being
> defined per-file, or sometimes per functional group. So essentially you
> can consider it to approximately a source file in most cases. The code
> is a constant of some arbitrary type that is generally considered to be
> scoped within the context of the GQuark domain.
> 
>> * Return value conventions
>>
>>    Common: non-void functions return a distinct error value on failure
>>    when such a value can be defined.  Patterns:
>>
>>    - Functions returning non-null pointers on success return null pointer
>>      on failure.
>>
>>    - Functions returning non-negative integers on success return a
>>      negative error code on failure.
>>
>>    Different: GLib discourages void functions, because these lead to
>>    awkward error checking code.  We have tons of them, and tons of
>>    awkward error checking code:
>>
>>      Error *err = NULL;
>>      frobnicate(arg, &err);
>>      if (err) {
>>          ... recover ...
>>          error_propagate(errp, err);
>>      }
> 
> Yeah, I really dislike this verbose style...
> 
>>
>>    instead of
>>
>>      if (!frobnicate(arg, errp))
>>          ... recover ...
>>      }
> 
> ...so I've followed this style for any code I've written in QEMU
> where possible.
> 
>>
>>    Can also lead to pointless creation of Error objects.
>>
>>    I consider this a design mistake.  Can we still fix it?  We have more
>>    than 2000 void functions taking an Error ** parameter...
> 
> Even if we don't do full conversion, we can at least encourage the
> simpler style - previously reviewers have told me to rewrite code
> to use the more verbose style, which I resisted. So at the very
> least setting the expectations for preferred style is useful.
> 
>>    Transforming code that receives and checks for errors with Coccinelle
>>    shouldn't be hard.  Transforming code that returns errors seems more
>>    difficult.  We need to transform explicit and implicit return to
>>    either return true or return false, depending on what we did to the
>>    @errp parameter on the way to the return.  Hmm.
> 
> Even if we only converted methods which are currently void, that
> would be a notable benefit I think.
> 
> It is a shame we didn't just use GError from the start, but I guess
> its probably too late to consider changing that now.
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Alex Bennée]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 01.04.2020 12:02, Markus Armbruster wrote:
>> QEMU's Error was patterned after GLib's GError.  Differences include:
>> * &error_fatal, &error_abort for convenience
>> * Error can optionally store hints
>> * Pointlessly different names: error_prepend() vs. g_error_prefix()
>> and
>>    so forth *shrug*
>> * Propagating errors
>>    Thanks to Vladimir, we'll soon have "auto propagation", which is
>> less
>>    verbose and less error-prone.
>> * Accumulating errors
>>    error_propagate() permits it, g_propagate_error() does not[*].
>>    I believe this feature is used rarely.  Perhaps we'd be better
>> off
>>    without it.  The problem is identifying its uses.  If I remember
>>    correctly, Vladimir struggled with that for his "auto propagation"
>>    work.
>>    Perhaps "auto propagation" will reduce the number of manual
>>    error_propagate() to the point where we can identify accumulations.
>>    Removing the feature would become feasible then.
>> * Distinguishing different errors
>>    Where Error has ErrorClass, GError has Gquark domain, gint code.
>> Use
>>    of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
>>    discouraged.  When we need callers to distinguish errors, we return
>>    suitable error codes separately.
>> * Return value conventions
>>    Common: non-void functions return a distinct error value on
>> failure
>>    when such a value can be defined.  Patterns:
>>    - Functions returning non-null pointers on success return null
>> pointer
>>      on failure.
>>    - Functions returning non-negative integers on success return a
>>      negative error code on failure.
>>    Different: GLib discourages void functions, because these lead to
>>    awkward error checking code.  We have tons of them, and tons of
>>    awkward error checking code:
>>      Error *err = NULL;
>>      frobnicate(arg, &err);
>>      if (err) {
>>          ... recover ...
>>          error_propagate(errp, err);
>>      }
>>    instead of
>>      if (!frobnicate(arg, errp))
>>          ... recover ...
>>      }
>>    Can also lead to pointless creation of Error objects.
>>    I consider this a design mistake.  Can we still fix it?  We have
>> more
>>    than 2000 void functions taking an Error ** parameter...
>>    Transforming code that receives and checks for errors with
>> Coccinelle
>>    shouldn't be hard.  Transforming code that returns errors seems more
>>    difficult.  We need to transform explicit and implicit return to
>>    either return true or return false, depending on what we did to the
>>    @errp parameter on the way to the return.  Hmm.
>> 
>> [*] According to documentation; the code merely calls g_warning() then,
>> in typical GLib fashion.
>> 
>
>
> Side question:
>
> Can we somehow implement a possibility to reliably identify file and line number
> where error is set by error message?
>
> It's where debug of error-bugs always starts: try to imagine which parts of the error
> message are "%s", and how to grep for it in the code, keeping in mind also,
> that error massage may be split into several lines..
>
> Put file:line into each error? Seems too noisy for users.. A lot of errors are not
> bugs: use do something wrong and see the error, and understands what he is doing
> wrong.. It's not usual practice to print file:line into each message
> for user.

I tend to use __func__ for these things as the result is usually easily
grep-able.


>
>
> But what if we do some kind of mapping file:line <-> error code, so user will see
> something like:
>
>
>    Error 12345: Device drive-scsi0-0-0-0 is not found
>
> ....
>
> Hmm, maybe, just add one more argument to error_setg:
>
> error_setg(errp, 12345, "Device %s is not found", device_name);
>
> - it's enough grep-able.


-- 
Alex Bennée

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Side question:
>
> Can we somehow implement a possibility to reliably identify file and line number
> where error is set by error message?
>
> It's where debug of error-bugs always starts: try to imagine which parts of the error
> message are "%s", and how to grep for it in the code, keeping in mind also,
> that error massage may be split into several lines..
>
> Put file:line into each error? Seems too noisy for users.. A lot of errors are not
> bugs: use do something wrong and see the error, and understands what he is doing
> wrong.. It's not usual practice to print file:line into each message for user.
>
>
> But what if we do some kind of mapping file:line <-> error code, so user will see
> something like:
>
>
>    Error 12345: Device drive-scsi0-0-0-0 is not found
>
> ....
>
> Hmm, maybe, just add one more argument to error_setg:
>
> error_setg(errp, 12345, "Device %s is not found", device_name);
>
> - it's enough grep-able.

error_setg() already records source file and line number in the Error
object, so that error_handle_fatal(&error_abort, err) can report them.

Making the programmer pick and pass an error ID at every call site is
onerous.  More so when the error ID should be globally unique.

With GLib's domain, code, the code needs only be unique within the
domain, but you still have to define globally unique domains.
Differently onerous.

We could have -msg,debug=on make error_report_err() report the Error
object's source file and line number.  Doesn't help for the many direct
uses of error_report().  To cover those, we'd have to turn
error_report() into a macro, similar to how error_setg() works.

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Apr 01, 2020 at 11:02:11AM +0200, Markus Armbruster wrote:
>> QEMU's Error was patterned after GLib's GError.  Differences include:
>> 
>> * &error_fatal, &error_abort for convenience
>
> I think this doesn't really need to exist, and is an artifact
> of the later point "return values" where we commonly make methds
> return void.  If we adopted a non-void return value, then these
> are no longer so compelling.
>
> Consider if we didn't have &error_fatal right now, then we would
> need to
>
>    Error *local_err = NULL;
>    qemu_boot_set(boot_once, &local_err)
>    if (*local_err)
>       abort();
>
> This is tedious, so we invented &error_abort to make our lives
> better
>
>    qemu_boot_set(boot_once, &error_abort)
>
>
> If we had a "bool" return value though, we would probably have just
> ended up doing:
>
>    assert(qemu_boot_set(boot_once, NULL));

This assumes !defined(NDEBUG).

> or
>
>    if (!qemu_boot_set(boot_once, NULL))
>        abort()
>
> and would never have invented &error_fatal.

Yes, the readability improvement of &error_abort over this is only
marginal.

However, &error_abort also results in more useful stack backtraces, as
Vladimir already pointed out.

Our use of error_propagate() sabotages this advantage.  Vladimir's auto
propagation work stops that.

>> * Distinguishing different errors
>> 
>>   Where Error has ErrorClass, GError has Gquark domain, gint code.  Use
>>   of ErrorClass other than ERROR_CLASS_GENERIC_ERROR is strongly
>>   discouraged.  When we need callers to distinguish errors, we return
>>   suitable error codes separately.
>
> The GQuark is just a static string, and in most cases this ends up being
> defined per-file, or sometimes per functional group. So essentially you
> can consider it to approximately a source file in most cases. The code
> is a constant of some arbitrary type that is generally considered to be
> scoped within the context of the GQuark domain.
>
>> * Return value conventions
>> 
>>   Common: non-void functions return a distinct error value on failure
>>   when such a value can be defined.  Patterns:
>> 
>>   - Functions returning non-null pointers on success return null pointer
>>     on failure.
>> 
>>   - Functions returning non-negative integers on success return a
>>     negative error code on failure.
>> 
>>   Different: GLib discourages void functions, because these lead to
>>   awkward error checking code.  We have tons of them, and tons of
>>   awkward error checking code:
>> 
>>     Error *err = NULL;
>>     frobnicate(arg, &err);
>>     if (err) {
>>         ... recover ...
>>         error_propagate(errp, err);
>>     }
>
> Yeah, I really dislike this verbose style...
>
>> 
>>   instead of
>> 
>>     if (!frobnicate(arg, errp))
>>         ... recover ...
>>     }
>
> ...so I've followed this style for any code I've written in QEMU
> where possible.
>
>> 
>>   Can also lead to pointless creation of Error objects.
>> 
>>   I consider this a design mistake.  Can we still fix it?  We have more
>>   than 2000 void functions taking an Error ** parameter...
>
> Even if we don't do full conversion, we can at least encourage the
> simpler style - previously reviewers have told me to rewrite code
> to use the more verbose style, which I resisted. So at the very
> least setting the expectations for preferred style is useful.

It's a matter of patching the big comment in error.h.

Of course, the non-preferred style will still be copied from bad
examples until we get rid of them.

>>   Transforming code that receives and checks for errors with Coccinelle
>>   shouldn't be hard.  Transforming code that returns errors seems more
>>   difficult.  We need to transform explicit and implicit return to
>>   either return true or return false, depending on what we did to the
>>   @errp parameter on the way to the return.  Hmm.
>
> Even if we only converted methods which are currently void, that
> would be a notable benefit I think.

Manual conversion of a modest set of frequently used functions with
automated conversion of its calls should be feasible.

For more, I believe we need to figure out how to automatically transform
code that returns errors.

> It is a shame we didn't just use GError from the start, but I guess
> its probably too late to consider changing that now.

If I remember correctly, error.h predates our adoption of GLib.  Water
under the bridge now.

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Side question:
>>
>> Can we somehow implement a possibility to reliably identify file and line number
>> where error is set by error message?
>>
>> It's where debug of error-bugs always starts: try to imagine which parts of the error
>> message are "%s", and how to grep for it in the code, keeping in mind also,
>> that error massage may be split into several lines..
>>
>> Put file:line into each error? Seems too noisy for users.. A lot of errors are not
>> bugs: use do something wrong and see the error, and understands what he is doing
>> wrong.. It's not usual practice to print file:line into each message
>> for user.
>
> I tend to use __func__ for these things as the result is usually easily
> grep-able.

Putting __func__ in error messages makes them both greppable and ugly.

Re: Questionable aspects of QEMU Error's design

[Peter Maydell]

On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>
> QEMU's Error was patterned after GLib's GError.  Differences include:

From my POV the major problem with Error as we have it today
is that it makes the simple process of writing code like
device realize functions horrifically boilerplate heavy;
for instance this is from hw/arm/armsse.c:

        object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                 "memory", &err);
        if (err) {
            error_propagate(errp, err);
            return;
        }
        object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
        if (err) {
            error_propagate(errp, err);
            return;
        }
        object_property_set_bool(cpuobj, true, "realized", &err);
        if (err) {
            error_propagate(errp, err);
            return;
        }

16 lines of code just to set 2 properties on an object
and realize it. It's a lot of boilerplate and as
a result we frequently get it wrong or take shortcuts
(eg forgetting the error-handling entirely, calling
error_propagate just once for a whole sequence of
calls, taking the lazy approach and using err_abort
or err_fatal when we ought really to be propagating
an error, etc). I haven't looked at 'auto propagation'
yet, hopefully it will help?

thanks
-- PMM

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

01.04.2020 23:15, Peter Maydell wrote:
> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> QEMU's Error was patterned after GLib's GError.  Differences include:
> 
>  From my POV the major problem with Error as we have it today
> is that it makes the simple process of writing code like
> device realize functions horrifically boilerplate heavy;
> for instance this is from hw/arm/armsse.c:
> 
>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                   "memory", &err);
>          if (err) {
>              error_propagate(errp, err);
>              return;
>          }
>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>          if (err) {
>              error_propagate(errp, err);
>              return;
>          }
>          object_property_set_bool(cpuobj, true, "realized", &err);
>          if (err) {
>              error_propagate(errp, err);
>              return;
>          }
> 
> 16 lines of code just to set 2 properties on an object
> and realize it. It's a lot of boilerplate and as
> a result we frequently get it wrong or take shortcuts
> (eg forgetting the error-handling entirely, calling
> error_propagate just once for a whole sequence of
> calls, taking the lazy approach and using err_abort
> or err_fatal when we ought really to be propagating
> an error, etc). I haven't looked at 'auto propagation'
> yet, hopefully it will help?

Yes, after it the code above will look like this:

... some_func(..., errp)
{
     ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error *err" definition

...
           object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                    "memory", errp);
           if (*errp) {
               return;
           }
           object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
           if (*errp) {
               return;
           }
           object_property_set_bool(cpuobj, true, "realized", errp);
           if (*errp) {
               return;
           }
...
}

  - propagation is automatic, errp is used directly and may be safely dereferenced.



-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> QEMU's Error was patterned after GLib's GError.  Differences include:
>
> From my POV the major problem with Error as we have it today
> is that it makes the simple process of writing code like
> device realize functions horrifically boilerplate heavy;
> for instance this is from hw/arm/armsse.c:
>
>         object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                  "memory", &err);
>         if (err) {
>             error_propagate(errp, err);
>             return;
>         }
>         object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>         if (err) {
>             error_propagate(errp, err);
>             return;
>         }
>         object_property_set_bool(cpuobj, true, "realized", &err);
>         if (err) {
>             error_propagate(errp, err);
>             return;
>         }
>
> 16 lines of code just to set 2 properties on an object
> and realize it. It's a lot of boilerplate and as
> a result we frequently get it wrong or take shortcuts
> (eg forgetting the error-handling entirely, calling
> error_propagate just once for a whole sequence of
> calls, taking the lazy approach and using err_abort
> or err_fatal when we ought really to be propagating
> an error, etc). I haven't looked at 'auto propagation'
> yet, hopefully it will help?

With that, you can have

        object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                 "memory", errp);
        if (*errp) {
            return;
        }
        object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
        if (*errp) {
            return;
        }
        object_property_set_bool(cpuobj, true, "realized", errp);
        if (*errp) {
            return;
        }

but you have to add

        ERRP_AUTO_PROPAGATE();

right at the beginning of the function.

It's a small improvement.  A bigger one is

        if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                     "memory", errp)) {
            return;
        }
        if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
            return;
        }
        if (object_property_set_bool(cpuobj, true, "realized", errp)) {
            return;
        }

This is item "Return value conventions" in the message you replied to.

Elsewhere in this thread, I discussed the difficulties of automating the
conversion to this style.  I think I know how to automate converting the
calls to use the bool return value, but converting the functions to
return it looks hard.  We could do that manually for a modest set of
frequently used functions.  object.h would top my list.

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

02.04.2020 8:54, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>
>>  From my POV the major problem with Error as we have it today
>> is that it makes the simple process of writing code like
>> device realize functions horrifically boilerplate heavy;
>> for instance this is from hw/arm/armsse.c:
>>
>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                   "memory", &err);
>>          if (err) {
>>              error_propagate(errp, err);
>>              return;
>>          }
>>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>>          if (err) {
>>              error_propagate(errp, err);
>>              return;
>>          }
>>          object_property_set_bool(cpuobj, true, "realized", &err);
>>          if (err) {
>>              error_propagate(errp, err);
>>              return;
>>          }
>>
>> 16 lines of code just to set 2 properties on an object
>> and realize it. It's a lot of boilerplate and as
>> a result we frequently get it wrong or take shortcuts
>> (eg forgetting the error-handling entirely, calling
>> error_propagate just once for a whole sequence of
>> calls, taking the lazy approach and using err_abort
>> or err_fatal when we ought really to be propagating
>> an error, etc). I haven't looked at 'auto propagation'
>> yet, hopefully it will help?
> 
> With that, you can have
> 
>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                   "memory", errp);
>          if (*errp) {
>              return;
>          }
>          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>          if (*errp) {
>              return;
>          }
>          object_property_set_bool(cpuobj, true, "realized", errp);
>          if (*errp) {
>              return;
>          }
> 
> but you have to add
> 
>          ERRP_AUTO_PROPAGATE();
> 
> right at the beginning of the function.
> 
> It's a small improvement.  A bigger one is
> 
>          if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                       "memory", errp)) {
>              return;

return false; you mean, assuming we converting the caller too...

>          }
>          if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
>              return;
>          }
>          if (object_property_set_bool(cpuobj, true, "realized", errp)) {
>              return;
>          }

Hmm..

Somehow, in general, especially with long function names and long parameter lists I prefer

ret = func(..);
if (ret < 0) {
     return ret;
}

notation to moving the entire function call into if condition.. Extra level of parentheses complicates the view of the code.

But, yes, may be for boolean functions it looks a bit strange:

ok = func(..);
if (!ok) {
     return false;
}


> 
> This is item "Return value conventions" in the message you replied to.
> 
> Elsewhere in this thread, I discussed the difficulties of automating the
> conversion to this style.  I think I know how to automate converting the
> calls to use the bool return value, but converting the functions to
> return it looks hard.  We could do that manually for a modest set of
> frequently used functions.  object.h would top my list.
> 

Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.

Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.

-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Peter Maydell]

On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
> Somehow, in general, especially with long function names and long parameter lists I prefer
>
> ret = func(..);
> if (ret < 0) {
>      return ret;
> }

Personally I prefer the other approach -- this one has an extra line
in the source and
needs an extra local variable.

> Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
>
> Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.

When would we want to return an errno? I thought the whole point of the
Error* was that that was where information about the error was returned.
If all your callsites are just going to do "if (ret < 0) { ... } then having
the functions pick an errno value to return is just extra work.

thanks
-- PMM

Re: Questionable aspects of QEMU Error's design

[Daniel P. Berrangé]

On Thu, Apr 02, 2020 at 07:54:11AM +0200, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> QEMU's Error was patterned after GLib's GError.  Differences include:
> >
> > From my POV the major problem with Error as we have it today
> > is that it makes the simple process of writing code like
> > device realize functions horrifically boilerplate heavy;
> > for instance this is from hw/arm/armsse.c:
> >
> >         object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
> >                                  "memory", &err);
> >         if (err) {
> >             error_propagate(errp, err);
> >             return;
> >         }
> >         object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
> >         if (err) {
> >             error_propagate(errp, err);
> >             return;
> >         }
> >         object_property_set_bool(cpuobj, true, "realized", &err);
> >         if (err) {
> >             error_propagate(errp, err);
> >             return;
> >         }
> >
> > 16 lines of code just to set 2 properties on an object
> > and realize it. It's a lot of boilerplate and as
> > a result we frequently get it wrong or take shortcuts
> > (eg forgetting the error-handling entirely, calling
> > error_propagate just once for a whole sequence of
> > calls, taking the lazy approach and using err_abort
> > or err_fatal when we ought really to be propagating
> > an error, etc). I haven't looked at 'auto propagation'
> > yet, hopefully it will help?
> 
> With that, you can have
> 
>         object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                  "memory", errp);
>         if (*errp) {
>             return;
>         }
>         object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>         if (*errp) {
>             return;
>         }
>         object_property_set_bool(cpuobj, true, "realized", errp);
>         if (*errp) {
>             return;
>         }
> 
> but you have to add
> 
>         ERRP_AUTO_PROPAGATE();
> 
> right at the beginning of the function.
> 
> It's a small improvement.  A bigger one is
> 
>         if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                      "memory", errp)) {
>             return;
>         }
>         if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
>             return;
>         }
>         if (object_property_set_bool(cpuobj, true, "realized", errp)) {
>             return;
>         }
> 
> This is item "Return value conventions" in the message you replied to.

Even better, we can then string the checks together

        if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                      "memory", errp) ||
            object_property_set_link(cpuobj, OBJECT(s), "idau", errp) ||
            object_property_set_bool(cpuobj, true, "realized", errp)) {
             return;
        }
 
Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Questionable aspects of QEMU Error's design

[Daniel P. Berrangé]

On Thu, Apr 02, 2020 at 08:11:11AM +0000, Peter Maydell wrote:
> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
> > Somehow, in general, especially with long function names and long parameter lists I prefer
> >
> > ret = func(..);
> > if (ret < 0) {
> >      return ret;
> > }
> 
> Personally I prefer the other approach -- this one has an extra line
> in the source and
> needs an extra local variable.
> 
> > Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
> >
> > Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.
> 
> When would we want to return an errno? I thought the whole point of the
> Error* was that that was where information about the error was returned.
> If all your callsites are just going to do "if (ret < 0) { ... } then having
> the functions pick an errno value to return is just extra work.

IMHO errno should only be returned if the callers are likely to have 
a genuine need to take action based on the errno in a way that isn't
possible from the Error alone. I expect very few scenarios need that.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>> Somehow, in general, especially with long function names and long parameter lists I prefer
>>
>> ret = func(..);
>> if (ret < 0) {
>>      return ret;
>> }
>
> Personally I prefer the other approach -- this one has an extra line
> in the source and
> needs an extra local variable.

Me too, except when func(...) is so long that

    if (func(...) < 0) {

becomes illegible like

    if (func(... yadda, yadda,
             yadda, ...) < 0) {

Then an extra variable can improve things.

>> Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
>>
>> Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.
>
> When would we want to return an errno? I thought the whole point of the
> Error* was that that was where information about the error was returned.
> If all your callsites are just going to do "if (ret < 0) { ... } then having
> the functions pick an errno value to return is just extra work.

0/-1 vs. true/false is a matter of convention.  Lacking convention, it's
a matter of taste.

0/-1 vs. 0/-errno depends on the function and its callers.  When -errno
enables callers to distinguish failures in a sane and simple way, use
it.  When -errno feels "natural", I'd say feel free to use it even when
all existing callers only check < 0.

When you return non-null/null or true/false on success/error, neglecting
to document that in a function contract can perhaps be tolerated; you're
using the return type the common, obvious way.  But when you return 0/-1
or 0/-errno, please spell it out.  I've seen too many "Operation not
permitted" that were actually -1 mistaken for -EPERM.  Also too many
functions that return -1 for some failures and -errno for others.

Re: Questionable aspects of QEMU Error's design

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Apr 02, 2020 at 07:54:11AM +0200, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>> > On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> QEMU's Error was patterned after GLib's GError.  Differences include:
>> >
>> > From my POV the major problem with Error as we have it today
>> > is that it makes the simple process of writing code like
>> > device realize functions horrifically boilerplate heavy;
>> > for instance this is from hw/arm/armsse.c:
>> >
>> >         object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>> >                                  "memory", &err);
>> >         if (err) {
>> >             error_propagate(errp, err);
>> >             return;
>> >         }
>> >         object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>> >         if (err) {
>> >             error_propagate(errp, err);
>> >             return;
>> >         }
>> >         object_property_set_bool(cpuobj, true, "realized", &err);
>> >         if (err) {
>> >             error_propagate(errp, err);
>> >             return;
>> >         }
>> >
>> > 16 lines of code just to set 2 properties on an object
>> > and realize it. It's a lot of boilerplate and as
>> > a result we frequently get it wrong or take shortcuts
>> > (eg forgetting the error-handling entirely, calling
>> > error_propagate just once for a whole sequence of
>> > calls, taking the lazy approach and using err_abort
>> > or err_fatal when we ought really to be propagating
>> > an error, etc). I haven't looked at 'auto propagation'
>> > yet, hopefully it will help?
>> 
>> With that, you can have
>> 
>>         object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                  "memory", errp);
>>         if (*errp) {
>>             return;
>>         }
>>         object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>>         if (*errp) {
>>             return;
>>         }
>>         object_property_set_bool(cpuobj, true, "realized", errp);
>>         if (*errp) {
>>             return;
>>         }
>> 
>> but you have to add
>> 
>>         ERRP_AUTO_PROPAGATE();
>> 
>> right at the beginning of the function.
>> 
>> It's a small improvement.  A bigger one is
>> 
>>         if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                      "memory", errp)) {
>>             return;
>>         }
>>         if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
>>             return;
>>         }
>>         if (object_property_set_bool(cpuobj, true, "realized", errp)) {
>>             return;
>>         }
>> 
>> This is item "Return value conventions" in the message you replied to.
>
> Even better, we can then string the checks together
>
>         if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                       "memory", errp) ||
>             object_property_set_link(cpuobj, OBJECT(s), "idau", errp) ||
>             object_property_set_bool(cpuobj, true, "realized", errp)) {
>              return;
>         }

You know at this point I wonder if we can't come up with some data table
that describes all these object interactions and a helper function that
processes it and tells us if it worked or not?

We are essentially just filling out an data structure anyway with all
this stuff.

>  
> Regards,
> Daniel


-- 
Alex Bennée

Re: Questionable aspects of QEMU Error's design

[BALATON Zoltan]

On Thu, 2 Apr 2020, Vladimir Sementsov-Ogievskiy wrote:
> 01.04.2020 23:15, Peter Maydell wrote:
>> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>
>>  From my POV the major problem with Error as we have it today
>> is that it makes the simple process of writing code like
>> device realize functions horrifically boilerplate heavy;
>> for instance this is from hw/arm/armsse.c:
>>
>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                   "memory", &err);
>>          if (err) {
>>              error_propagate(errp, err);
>>              return;
>>          }
>>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>>          if (err) {
>>              error_propagate(errp, err);
>>              return;
>>          }
>>          object_property_set_bool(cpuobj, true, "realized", &err);
>>          if (err) {
>>              error_propagate(errp, err);
>>              return;
>>          }
>> 
>> 16 lines of code just to set 2 properties on an object
>> and realize it. It's a lot of boilerplate and as
>> a result we frequently get it wrong or take shortcuts
>> (eg forgetting the error-handling entirely, calling
>> error_propagate just once for a whole sequence of
>> calls, taking the lazy approach and using err_abort
>> or err_fatal when we ought really to be propagating
>> an error, etc). I haven't looked at 'auto propagation'
>> yet, hopefully it will help?
>
> Yes, after it the code above will look like this:
>
> ... some_func(..., errp)
> {
>    ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error 
> *err" definition
>
> ...
>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                   "memory", errp);
>          if (*errp) {
>              return;
>          }
>          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>          if (*errp) {
>              return;
>          }
>          object_property_set_bool(cpuobj, true, "realized", errp);
>          if (*errp) {
>              return;
>          }
> ...
> }
>
> - propagation is automatic, errp is used directly and may be safely 
> dereferenced.

Not much better. Could it be something like:

     ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                       "memory", errp));
     ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
     ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));

Regards,
BALATON Zoltan

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

02.04.2020 12:36, BALATON Zoltan wrote:
> On Thu, 2 Apr 2020, Vladimir Sementsov-Ogievskiy wrote:
>> 01.04.2020 23:15, Peter Maydell wrote:
>>> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>>
>>>  From my POV the major problem with Error as we have it today
>>> is that it makes the simple process of writing code like
>>> device realize functions horrifically boilerplate heavy;
>>> for instance this is from hw/arm/armsse.c:
>>>
>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>                                   "memory", &err);
>>>          if (err) {
>>>              error_propagate(errp, err);
>>>              return;
>>>          }
>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>>>          if (err) {
>>>              error_propagate(errp, err);
>>>              return;
>>>          }
>>>          object_property_set_bool(cpuobj, true, "realized", &err);
>>>          if (err) {
>>>              error_propagate(errp, err);
>>>              return;
>>>          }
>>>
>>> 16 lines of code just to set 2 properties on an object
>>> and realize it. It's a lot of boilerplate and as
>>> a result we frequently get it wrong or take shortcuts
>>> (eg forgetting the error-handling entirely, calling
>>> error_propagate just once for a whole sequence of
>>> calls, taking the lazy approach and using err_abort
>>> or err_fatal when we ought really to be propagating
>>> an error, etc). I haven't looked at 'auto propagation'
>>> yet, hopefully it will help?
>>
>> Yes, after it the code above will look like this:
>>
>> ... some_func(..., errp)
>> {
>>    ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error *err" definition
>>
>> ...
>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                   "memory", errp);
>>          if (*errp) {
>>              return;
>>          }
>>          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>>          if (*errp) {
>>              return;
>>          }
>>          object_property_set_bool(cpuobj, true, "realized", errp);
>>          if (*errp) {
>>              return;
>>          }
>> ...
>> }
>>
>> - propagation is automatic, errp is used directly and may be safely dereferenced.
> 
> Not much better. Could it be something like:

Actually, much better, as it solves some real problems around error propagation.

> 
>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                        "memory", errp));
>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
>      ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));
> 

and turn all

ret = func(...);
if (ret < 0) {
     return ret;
}

into

FAIL_RET(func(...))

?

Not a problem to make such macro.. But I think it's a bad idea to turn all the code
into sequence of macro invocations. It's hard to debug and follow.


-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Eric Blake]

On 4/2/20 12:54 AM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:

> It's a small improvement.  A bigger one is
> 
>          if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                       "memory", errp)) {
>              return;
>          }
>          if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
>              return;
>          }
>          if (object_property_set_bool(cpuobj, true, "realized", errp)) {
>              return;
>          }

Or even:

if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
                                       "memory", errp) ||
     object_property_set_link(cpuobj, OBJECT(s), "idau", errp) ||
     object_property_set_bool(cpuobj, true, "realized", errp)) {
         return;
}

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 02.04.2020 12:36, BALATON Zoltan wrote:
>> On Thu, 2 Apr 2020, Vladimir Sementsov-Ogievskiy wrote:
>>> 01.04.2020 23:15, Peter Maydell wrote:
>>>> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>>>
>>>>  From my POV the major problem with Error as we have it today
>>>> is that it makes the simple process of writing code like
>>>> device realize functions horrifically boilerplate heavy;
>>>> for instance this is from hw/arm/armsse.c:
>>>>
>>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>>                                   "memory", &err);
>>>>          if (err) {
>>>>              error_propagate(errp, err);
>>>>              return;
>>>>          }
>>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>>>>          if (err) {
>>>>              error_propagate(errp, err);
>>>>              return;
>>>>          }
>>>>          object_property_set_bool(cpuobj, true, "realized", &err);
>>>>          if (err) {
>>>>              error_propagate(errp, err);
>>>>              return;
>>>>          }
>>>>
>>>> 16 lines of code just to set 2 properties on an object
>>>> and realize it. It's a lot of boilerplate and as
>>>> a result we frequently get it wrong or take shortcuts
>>>> (eg forgetting the error-handling entirely, calling
>>>> error_propagate just once for a whole sequence of
>>>> calls, taking the lazy approach and using err_abort
>>>> or err_fatal when we ought really to be propagating
>>>> an error, etc). I haven't looked at 'auto propagation'
>>>> yet, hopefully it will help?
>>>
>>> Yes, after it the code above will look like this:
>>>
>>> ... some_func(..., errp)
>>> {
>>>    ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error *err" definition
>>>
>>> ...
>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>                                   "memory", errp);
>>>          if (*errp) {
>>>              return;
>>>          }
>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>>>          if (*errp) {
>>>              return;
>>>          }
>>>          object_property_set_bool(cpuobj, true, "realized", errp);
>>>          if (*errp) {
>>>              return;
>>>          }
>>> ...
>>> }
>>>
>>> - propagation is automatic, errp is used directly and may be safely dereferenced.
>>
>> Not much better. Could it be something like:
>
> Actually, much better, as it solves some real problems around error propagation.

The auto propagation patches' stated aim is to fix &error_fatal not to
eat hints, and to provide more useful stack backtraces with
&error_abort.  The slight shrinking of boilerplate is a welcome bonus.

For a bigger improvement, have the functions return a useful value, as
discussed elsewhere in this thread.

>>
>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>                                        "memory", errp));
>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
>>      ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));
>>
>
> and turn all
>
> ret = func(...);
> if (ret < 0) {
>     return ret;
> }
>
> into
>
> FAIL_RET(func(...))
>
> ?
>
> Not a problem to make such macro.. But I think it's a bad idea to turn all the code
> into sequence of macro invocations. It's hard to debug and follow.

Yes.  Hiding control flow in macros is almost always too much magic.
There are exceptions, but this doesn't look like one.

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

02.04.2020 11:55, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
>> <vsementsov@virtuozzo.com> wrote:
>>> Somehow, in general, especially with long function names and long parameter lists I prefer
>>>
>>> ret = func(..);
>>> if (ret < 0) {
>>>       return ret;
>>> }
>>
>> Personally I prefer the other approach -- this one has an extra line
>> in the source and
>> needs an extra local variable.
> 
> Me too, except when func(...) is so long that
> 
>      if (func(...) < 0) {
> 
> becomes illegible like
> 
>      if (func(... yadda, yadda,
>               yadda, ...) < 0) {
> 
> Then an extra variable can improve things.
> 
>>> Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
>>>
>>> Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.
>>
>> When would we want to return an errno? I thought the whole point of the
>> Error* was that that was where information about the error was returned.
>> If all your callsites are just going to do "if (ret < 0) { ... } then having
>> the functions pick an errno value to return is just extra work.
> 
> 0/-1 vs. true/false is a matter of convention.  Lacking convention, it's
> a matter of taste. >
> 0/-1 vs. 0/-errno depends on the function and its callers.  When -errno
> enables callers to distinguish failures in a sane and simple way, use
> it.  When -errno feels "natural", I'd say feel free to use it even when
> all existing callers only check < 0.
> 
> When you return non-null/null or true/false on success/error, neglecting
> to document that in a function contract can perhaps be tolerated; you're
> using the return type the common, obvious way.  But when you return 0/-1
> or 0/-errno, please spell it out.  I've seen too many "Operation not
> permitted" that were actually -1 mistaken for -EPERM.  Also too many
> functions that return -1 for some failures and -errno for others.
> 

I just want to add one note:

OK, you like the pattern

   if (func()) {
       <handle error>
   }

, I can live with it.

I believe, we have a lot of such patterns already in code.

Now, we are going to add a lot of functions, returning true on success and false on failure, so add a lot of patterns

   if (!func()) {
       <handle error>
   }

---

After it, looking at something like

   if (!func()) {} / if (func()) {}

I'll have to always jump to function definition, to check is it int or bool function, to understand what exactly is meant and is there a mistake in the code..
So, I'm afraid that such conversion will not help reviewing/understanding the code. I'd prefer to avoid using two opposite conventions in on project.

I can also imagine combining different function types (int/bool) in if conditions o_O, what will save us from it?

And don't forget about bool functions, which just check something, and false is not an error, but just negative answer on some question.

-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 02.04.2020 11:55, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com> wrote:
>>>> Somehow, in general, especially with long function names and long parameter lists I prefer
>>>>
>>>> ret = func(..);
>>>> if (ret < 0) {
>>>>       return ret;
>>>> }
>>>
>>> Personally I prefer the other approach -- this one has an extra line
>>> in the source and
>>> needs an extra local variable.
>>
>> Me too, except when func(...) is so long that
>>
>>      if (func(...) < 0) {
>>
>> becomes illegible like
>>
>>      if (func(... yadda, yadda,
>>               yadda, ...) < 0) {
>>
>> Then an extra variable can improve things.
>>
>>>> Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
>>>>
>>>> Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.
>>>
>>> When would we want to return an errno? I thought the whole point of the
>>> Error* was that that was where information about the error was returned.
>>> If all your callsites are just going to do "if (ret < 0) { ... } then having
>>> the functions pick an errno value to return is just extra work.
>>
>> 0/-1 vs. true/false is a matter of convention.  Lacking convention, it's
>> a matter of taste. >
>> 0/-1 vs. 0/-errno depends on the function and its callers.  When -errno
>> enables callers to distinguish failures in a sane and simple way, use
>> it.  When -errno feels "natural", I'd say feel free to use it even when
>> all existing callers only check < 0.
>>
>> When you return non-null/null or true/false on success/error, neglecting
>> to document that in a function contract can perhaps be tolerated; you're
>> using the return type the common, obvious way.  But when you return 0/-1
>> or 0/-errno, please spell it out.  I've seen too many "Operation not
>> permitted" that were actually -1 mistaken for -EPERM.  Also too many
>> functions that return -1 for some failures and -errno for others.
>>
>
> I just want to add one note:
>
> OK, you like the pattern
>
>   if (func()) {
>       <handle error>
>   }
>
> , I can live with it.
>
> I believe, we have a lot of such patterns already in code.
>
> Now, we are going to add a lot of functions, returning true on success and false on failure, so add a lot of patterns
>
>   if (!func()) {
>       <handle error>
>   }

We already have this pattern all over the place with functions returning
non-null pointers on success, null pointer on failure.

> ---
>
> After it, looking at something like
>
>   if (!func()) {} / if (func()) {}
>
> I'll have to always jump to function definition, to check is it int or bool function, to understand what exactly is meant and is there a mistake in the code..
> So, I'm afraid that such conversion will not help reviewing/understanding the code. I'd prefer to avoid using two opposite conventions in on project.

C sucks :)

Conventions help mitigate.  Here's one: when fun() returns
non-negative/negative on success/error, always use

    fun(...) < 0

or

    fun(...) >= 0

to check.  I dislike the latter.

When returns 0/negative, always use

    fun(...) < 0

Avoid

    fun(...) >= 0

because that suggests it could return a positive value, which is wrong.

Avoid

    fun(...)

because that requires the reader to know the return type.

When its returns non-null/null or true/false on success/failure, always
use

    !fun(...)

Avoid

    fun(...)

because that requires the reader to know the return type.

Note that we have a usable check for failure in all cases.  Goes well
with the habit to handle errors like this whenever practical

    if (failed) {
        handle failure
        bail out
    }
    handle success

> I can also imagine combining different function types (int/bool) in if conditions o_O, what will save us from it?

Can you give an example?

> And don't forget about bool functions, which just check something, and false is not an error, but just negative answer on some question.

For what it's worth, GLib specifically advises against such function
contracts:

    By convention, if you return a boolean value indicating success then
    TRUE means success and FALSE means failure.  Avoid creating
    functions which have a boolean return value and a GError parameter,
    but where the boolean does something other than signal whether the
    GError is set.  Among other problems, it requires C callers to
    allocate a temporary error.  Instead, provide a "gboolean *" out
    parameter.  There are functions in GLib itself such as
    g_key_file_has_key() that are deprecated because of this.

Re: Questionable aspects of QEMU Error's design

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

On Thu, 2 Apr 2020, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 02.04.2020 12:36, BALATON Zoltan wrote:
>>> On Thu, 2 Apr 2020, Vladimir Sementsov-Ogievskiy wrote:
>>>> 01.04.2020 23:15, Peter Maydell wrote:
>>>>> On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>
>>>>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>>>>
>>>>>  From my POV the major problem with Error as we have it today
>>>>> is that it makes the simple process of writing code like
>>>>> device realize functions horrifically boilerplate heavy;
>>>>> for instance this is from hw/arm/armsse.c:
>>>>>
>>>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>>>                                   "memory", &err);
>>>>>          if (err) {
>>>>>              error_propagate(errp, err);
>>>>>              return;
>>>>>          }
>>>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
>>>>>          if (err) {
>>>>>              error_propagate(errp, err);
>>>>>              return;
>>>>>          }
>>>>>          object_property_set_bool(cpuobj, true, "realized", &err);
>>>>>          if (err) {
>>>>>              error_propagate(errp, err);
>>>>>              return;
>>>>>          }
>>>>>
>>>>> 16 lines of code just to set 2 properties on an object
>>>>> and realize it. It's a lot of boilerplate and as
>>>>> a result we frequently get it wrong or take shortcuts
>>>>> (eg forgetting the error-handling entirely, calling
>>>>> error_propagate just once for a whole sequence of
>>>>> calls, taking the lazy approach and using err_abort
>>>>> or err_fatal when we ought really to be propagating
>>>>> an error, etc). I haven't looked at 'auto propagation'
>>>>> yet, hopefully it will help?
>>>>
>>>> Yes, after it the code above will look like this:
>>>>
>>>> ... some_func(..., errp)
>>>> {
>>>>    ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error *err" definition
>>>>
>>>> ...
>>>>          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>>                                   "memory", errp);
>>>>          if (*errp) {
>>>>              return;
>>>>          }
>>>>          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
>>>>          if (*errp) {
>>>>              return;
>>>>          }
>>>>          object_property_set_bool(cpuobj, true, "realized", errp);
>>>>          if (*errp) {
>>>>              return;
>>>>          }
>>>> ...
>>>> }
>>>>
>>>> - propagation is automatic, errp is used directly and may be safely dereferenced.
>>>
>>> Not much better. Could it be something like:
>>
>> Actually, much better, as it solves some real problems around error propagation.
>
> The auto propagation patches' stated aim is to fix &error_fatal not to
> eat hints, and to provide more useful stack backtraces with
> &error_abort.  The slight shrinking of boilerplate is a welcome bonus.
>
> For a bigger improvement, have the functions return a useful value, as
> discussed elsewhere in this thread.
>
>>>
>>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>                                        "memory", errp));
>>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
>>>      ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));
>>>
>>
>> and turn all
>>
>> ret = func(...);
>> if (ret < 0) {
>>     return ret;
>> }
>>
>> into
>>
>> FAIL_RET(func(...))
>>
>> ?
>>
>> Not a problem to make such macro.. But I think it's a bad idea to turn all the code
>> into sequence of macro invocations. It's hard to debug and follow.
>
> Yes.  Hiding control flow in macros is almost always too much magic.
> There are exceptions, but this doesn't look like one.

I did't like this idea of mine too much either so I agree but I see no 
other easy way to simplify this. If you propose changing function return 
values maybe these should return errp instead of passing it as a func 
parameter? Could that result in simpler code and less macro magic needed?

Regards,
BALATON Zoltan

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

02.04.2020 18:06, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 02.04.2020 11:55, Markus Armbruster wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
>>>> <vsementsov@virtuozzo.com> wrote:
>>>>> Somehow, in general, especially with long function names and long parameter lists I prefer
>>>>>
>>>>> ret = func(..);
>>>>> if (ret < 0) {
>>>>>        return ret;
>>>>> }
>>>>
>>>> Personally I prefer the other approach -- this one has an extra line
>>>> in the source and
>>>> needs an extra local variable.
>>>
>>> Me too, except when func(...) is so long that
>>>
>>>       if (func(...) < 0) {
>>>
>>> becomes illegible like
>>>
>>>       if (func(... yadda, yadda,
>>>                yadda, ...) < 0) {
>>>
>>> Then an extra variable can improve things.
>>>
>>>>> Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
>>>>>
>>>>> Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.
>>>>
>>>> When would we want to return an errno? I thought the whole point of the
>>>> Error* was that that was where information about the error was returned.
>>>> If all your callsites are just going to do "if (ret < 0) { ... } then having
>>>> the functions pick an errno value to return is just extra work.
>>>
>>> 0/-1 vs. true/false is a matter of convention.  Lacking convention, it's
>>> a matter of taste. >
>>> 0/-1 vs. 0/-errno depends on the function and its callers.  When -errno
>>> enables callers to distinguish failures in a sane and simple way, use
>>> it.  When -errno feels "natural", I'd say feel free to use it even when
>>> all existing callers only check < 0.
>>>
>>> When you return non-null/null or true/false on success/error, neglecting
>>> to document that in a function contract can perhaps be tolerated; you're
>>> using the return type the common, obvious way.  But when you return 0/-1
>>> or 0/-errno, please spell it out.  I've seen too many "Operation not
>>> permitted" that were actually -1 mistaken for -EPERM.  Also too many
>>> functions that return -1 for some failures and -errno for others.
>>>
>>
>> I just want to add one note:
>>
>> OK, you like the pattern
>>
>>    if (func()) {
>>        <handle error>
>>    }
>>
>> , I can live with it.
>>
>> I believe, we have a lot of such patterns already in code.
>>
>> Now, we are going to add a lot of functions, returning true on success and false on failure, so add a lot of patterns
>>
>>    if (!func()) {
>>        <handle error>
>>    }
> 
> We already have this pattern all over the place with functions returning
> non-null pointers on success, null pointer on failure.

Functions returning pointer are simpler to distinguish by name.

Hmm, strange. But this pattern lose the pointer.. You mean

ptr = func();
if (!ptr) {
   <handle error>
}

this is much more understandable. Usually ptr variable name and function name - all will help to understand that it's all about pointer.


> 
>> ---
>>
>> After it, looking at something like
>>
>>    if (!func()) {} / if (func()) {}
>>
>> I'll have to always jump to function definition, to check is it int or bool function, to understand what exactly is meant and is there a mistake in the code..
>> So, I'm afraid that such conversion will not help reviewing/understanding the code. I'd prefer to avoid using two opposite conventions in on project.
> 
> C sucks :)
> 
> Conventions help mitigate.  Here's one: when fun() returns
> non-negative/negative on success/error, always use
> 
>      fun(...) < 0

This reduces chances that it fit in one line..

But yes, if all use this convention, it makes it obvious what happening.

> 
> or
> 
>      fun(...) >= 0
> 
> to check.  I dislike the latter.
> 
> When returns 0/negative, always use
> 
>      fun(...) < 0
> 
> Avoid
> 
>      fun(...) >= 0
> 
> because that suggests it could return a positive value, which is wrong.
> 
> Avoid
> 
>      fun(...)
> 
> because that requires the reader to know the return type.

Exactly the problem I mention. To follow your suggestion, we'll have to update
the whole code base.. However, why not.

> 
> When its returns non-null/null or true/false on success/failure, always
> use
> 
>      !fun(...)
> 
> Avoid
> 
>      fun(...)
> 
> because that requires the reader to know the return type.
> 
> Note that we have a usable check for failure in all cases.  Goes well
> with the habit to handle errors like this whenever practical
> 
>      if (failed) {
>          handle failure
>          bail out
>      }
>      handle success
> 

OK. If convert everything to your suggestion it looks good.

The only possible problem is boolean functions, which just check something, not returning the error..

With a function like is_x_in_set(x, set), it's native to write

if (is_x_in_set(x, set)) {

    ...

}

which is a bit in conflict with your suggestion. Still, such functions should be simply distinguished by name.

>> I can also imagine combining different function types (int/bool) in if conditions o_O, what will save us from it?
> 
> Can you give an example?

I just meant something like if (f1() || !f2()) { < error > }, nothing special.

> 
>> And don't forget about bool functions, which just check something, and false is not an error, but just negative answer on some question.
> 
> For what it's worth, GLib specifically advises against such function
> contracts:
> 
>      By convention, if you return a boolean value indicating success then
>      TRUE means success and FALSE means failure.  Avoid creating
>      functions which have a boolean return value and a GError parameter,
>      but where the boolean does something other than signal whether the
>      GError is set.  Among other problems, it requires C callers to
>      allocate a temporary error.  Instead, provide a "gboolean *" out
>      parameter.  There are functions in GLib itself such as
>      g_key_file_has_key() that are deprecated because of this.
> 

Sounds good. But we are speaking about all functions, not only with errp parameter.. Or not?

-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Paolo Bonzini]

On 02/04/20 10:55, Markus Armbruster wrote:
> 
> When you return non-null/null or true/false on success/error, neglecting
> to document that in a function contract can perhaps be tolerated; you're
> using the return type the common, obvious way.  But when you return 0/-1
> or 0/-errno, please spell it out.  I've seen too many "Operation not
> permitted" that were actually -1 mistaken for -EPERM.

Hopefully that would be avoided by the usage of Error itself.

Paolo

> Also too many
> functions that return -1 for some failures and -errno for others.

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> On Thu, 2 Apr 2020, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>> 02.04.2020 12:36, BALATON Zoltan wrote:
[...]
>>>> Not much better. Could it be something like:
[...]
>>>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>>>>                                        "memory", errp));
>>>>      ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
>>>>      ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));
>>>>
>>>
>>> and turn all
>>>
>>> ret = func(...);
>>> if (ret < 0) {
>>>     return ret;
>>> }
>>>
>>> into
>>>
>>> FAIL_RET(func(...))
>>>
>>> ?
>>>
>>> Not a problem to make such macro.. But I think it's a bad idea to turn all the code
>>> into sequence of macro invocations. It's hard to debug and follow.
>>
>> Yes.  Hiding control flow in macros is almost always too much magic.
>> There are exceptions, but this doesn't look like one.
>
> I did't like this idea of mine too much either so I agree but I see no
> other easy way to simplify this. If you propose changing function
> return values maybe these should return errp instead of passing it as
> a func parameter? Could that result in simpler code and less macro
> magic needed?

I don't think so.  Let's compare a few error handling patterns from
error.h.

 * Call a function and receive an error from it:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *     }

Making foo() return the error object instead of void looks like an
obvious win:

       if (foo(arg)) {
           handle the error...
       }

Except it does not work, because surely a use of @err is hiding in
"handle the error" (or else we'd leak it).  So you actually need

       err = foo(arg)
       if (err) {
           handle the error...
       }

All this saves you is initializing @err.

You can save a bit more

       if ((err = foo(arg))) {
           handle the error...
       }

We generally frown upon assignemnts within if controlling expressions.

Next:

 * Receive an error and pass it on to the caller:
 [...]
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.

The obvious conversion

       *errp = foo(arg);

is wrong for !errp, errp == &error_fatal, and errp == &error_abort.
You'd need

       err = foo(arg);
       error_propagate(errp, err);

or, if you're so inclined

       error_propagate(errp, foo(arg));

Less legible, and it creates and destroys error objects where the
current method doesn't.

Returning the error object is even less attractive for functions that
now return values.

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 02.04.2020 18:06, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 02.04.2020 11:55, Markus Armbruster wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> On Thu, 2 Apr 2020 at 07:11, Vladimir Sementsov-Ogievskiy
>>>>> <vsementsov@virtuozzo.com> wrote:
>>>>>> Somehow, in general, especially with long function names and long parameter lists I prefer
>>>>>>
>>>>>> ret = func(..);
>>>>>> if (ret < 0) {
>>>>>>        return ret;
>>>>>> }
>>>>>
>>>>> Personally I prefer the other approach -- this one has an extra line
>>>>> in the source and
>>>>> needs an extra local variable.
>>>>
>>>> Me too, except when func(...) is so long that
>>>>
>>>>       if (func(...) < 0) {
>>>>
>>>> becomes illegible like
>>>>
>>>>       if (func(... yadda, yadda,
>>>>                yadda, ...) < 0) {
>>>>
>>>> Then an extra variable can improve things.
>>>>
>>>>>> Are you sure that adding a lot of boolean functions is a good idea? I somehow feel better with more usual int functions with -errno on failure.
>>>>>>
>>>>>> Bool is a good return value for functions which are boolean by nature: checks, is something correspond to some criteria. But for reporting an error I'd prefer -errno.
>>>>>
>>>>> When would we want to return an errno? I thought the whole point of the
>>>>> Error* was that that was where information about the error was returned.
>>>>> If all your callsites are just going to do "if (ret < 0) { ... } then having
>>>>> the functions pick an errno value to return is just extra work.
>>>>
>>>> 0/-1 vs. true/false is a matter of convention.  Lacking convention, it's
>>>> a matter of taste. >
>>>> 0/-1 vs. 0/-errno depends on the function and its callers.  When -errno
>>>> enables callers to distinguish failures in a sane and simple way, use
>>>> it.  When -errno feels "natural", I'd say feel free to use it even when
>>>> all existing callers only check < 0.
>>>>
>>>> When you return non-null/null or true/false on success/error, neglecting
>>>> to document that in a function contract can perhaps be tolerated; you're
>>>> using the return type the common, obvious way.  But when you return 0/-1
>>>> or 0/-errno, please spell it out.  I've seen too many "Operation not
>>>> permitted" that were actually -1 mistaken for -EPERM.  Also too many
>>>> functions that return -1 for some failures and -errno for others.
>>>>
>>>
>>> I just want to add one note:
>>>
>>> OK, you like the pattern
>>>
>>>    if (func()) {
>>>        <handle error>
>>>    }
>>>
>>> , I can live with it.
>>>
>>> I believe, we have a lot of such patterns already in code.
>>>
>>> Now, we are going to add a lot of functions, returning true on success and false on failure, so add a lot of patterns
>>>
>>>    if (!func()) {
>>>        <handle error>
>>>    }
>>
>> We already have this pattern all over the place with functions returning
>> non-null pointers on success, null pointer on failure.
>
> Functions returning pointer are simpler to distinguish by name.
>
> Hmm, strange. But this pattern lose the pointer.. You mean
>
> ptr = func();
> if (!ptr) {
>   <handle error>
> }

Yes, when you actually need the pointer.  Common, but not universal.

> this is much more understandable. Usually ptr variable name and function name - all will help to understand that it's all about pointer.
>
>
>>
>>> ---
>>>
>>> After it, looking at something like
>>>
>>>    if (!func()) {} / if (func()) {}
>>>
>>> I'll have to always jump to function definition, to check is it int or bool function, to understand what exactly is meant and is there a mistake in the code..
>>> So, I'm afraid that such conversion will not help reviewing/understanding the code. I'd prefer to avoid using two opposite conventions in on project.
>>
>> C sucks :)
>>
>> Conventions help mitigate.  Here's one: when fun() returns
>> non-negative/negative on success/error, always use
>>
>>      fun(...) < 0
>
> This reduces chances that it fit in one line..

Yes, that's a drawback.

> But yes, if all use this convention, it makes it obvious what happening.
>
>>
>> or
>>
>>      fun(...) >= 0
>>
>> to check.  I dislike the latter.
>>
>> When returns 0/negative, always use
>>
>>      fun(...) < 0
>>
>> Avoid
>>
>>      fun(...) >= 0
>>
>> because that suggests it could return a positive value, which is wrong.
>>
>> Avoid
>>
>>      fun(...)
>>
>> because that requires the reader to know the return type.
>
> Exactly the problem I mention. To follow your suggestion, we'll have to update
> the whole code base.. However, why not.

Only if we value the consistency more than the update labor and churn.

>>
>> When its returns non-null/null or true/false on success/failure, always
>> use
>>
>>      !fun(...)
>>
>> Avoid
>>
>>      fun(...)
>>
>> because that requires the reader to know the return type.
>>
>> Note that we have a usable check for failure in all cases.  Goes well
>> with the habit to handle errors like this whenever practical
>>
>>      if (failed) {
>>          handle failure
>>          bail out
>>      }
>>      handle success
>>
>
> OK. If convert everything to your suggestion it looks good.
>
> The only possible problem is boolean functions, which just check something, not returning the error..
>
> With a function like is_x_in_set(x, set), it's native to write
>
> if (is_x_in_set(x, set)) {
>
>    ...
>
> }
>
> which is a bit in conflict with your suggestion. Still, such functions should be simply distinguished by name.

The conventions I described only apply to error checking.  I certainly
don't mean to discourage things like

    while (*p && qemu_isspace(*p)) {
        p++;
    }

Also, they're *conventions*, not law.  The purpose of conventions is to
help us write clearer code.  Don't make code less clear just to conform
to a convention.  Use your judgement.

>>> I can also imagine combining different function types (int/bool) in if conditions o_O, what will save us from it?
>>
>> Can you give an example?
>
> I just meant something like if (f1() || !f2()) { < error > }, nothing special.

Perfectly consistent error checking style, idiomatic C, pick one.

>>> And don't forget about bool functions, which just check something, and false is not an error, but just negative answer on some question.
>>
>> For what it's worth, GLib specifically advises against such function
>> contracts:
>>
>>      By convention, if you return a boolean value indicating success then
>>      TRUE means success and FALSE means failure.  Avoid creating
>>      functions which have a boolean return value and a GError parameter,
>>      but where the boolean does something other than signal whether the
>>      GError is set.  Among other problems, it requires C callers to
>>      allocate a temporary error.  Instead, provide a "gboolean *" out
>>      parameter.  There are functions in GLib itself such as
>>      g_key_file_has_key() that are deprecated because of this.
>>
>
> Sounds good. But we are speaking about all functions, not only with errp parameter.. Or not?

The conventions I described apply to error checking, regardless of Error
object use.  For instance, they'd apply to error-checking
remove("some-file").

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> QEMU's Error was patterned after GLib's GError.  Differences include:
[...]
> * Return value conventions
>
>   Common: non-void functions return a distinct error value on failure
>   when such a value can be defined.  Patterns:
>
>   - Functions returning non-null pointers on success return null pointer
>     on failure.
>
>   - Functions returning non-negative integers on success return a
>     negative error code on failure.
>
>   Different: GLib discourages void functions, because these lead to
>   awkward error checking code.  We have tons of them, and tons of
>   awkward error checking code:
>
>     Error *err = NULL;
>     frobnicate(arg, &err);
>     if (err) {
>         ... recover ...
>         error_propagate(errp, err);
>     }
>
>   instead of
>
>     if (!frobnicate(arg, errp))
>         ... recover ...
>     }
>
>   Can also lead to pointless creation of Error objects.
>
>   I consider this a design mistake.  Can we still fix it?  We have more
>   than 2000 void functions taking an Error ** parameter...
>
>   Transforming code that receives and checks for errors with Coccinelle
>   shouldn't be hard.  Transforming code that returns errors seems more
>   difficult.  We need to transform explicit and implicit return to
>   either return true or return false, depending on what we did to the
>   @errp parameter on the way to the return.  Hmm.
[...]

To figure out what functions with an Error ** parameter return, I used
Coccinelle to find such function definitions and print the return types.
Summary of results:

   2155 void
    873 signed integer
    494 pointer
    153 bool
     33 unsigned integer
      6 enum
   ---------------------
   3714 total

I then used Coccinelle to find checked calls of void functions (passing
&error_fatal or &error_abort is not considered "checking" here).  These
calls become simpler if we make the functions return a useful value.  I
found a bit under 600 direct calls, and some 50 indirect calls.

Most frequent direct calls:

    127 object_property_set_bool
     27 qemu_opts_absorb_qdict
     16 visit_type_str
     14 visit_type_int
     10 visit_type_uint32

Let's have a closer look at object_property_set() & friends.  Out of
almost 1000 calls, some 150 are checked.  While I'm sure many of the
unchecked calls can't actually fail, I am concerned some unchecked calls
can.

If we adopt the convention to return a value that indicates success /
failure, we should consider converting object.h to it sooner rather than
later.

Please understand these are rough numbers from quick & dirty scripts.

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> QEMU's Error was patterned after GLib's GError.  Differences include:
> [...]
>> * Return value conventions
>>
>>   Common: non-void functions return a distinct error value on failure
>>   when such a value can be defined.  Patterns:
>>
>>   - Functions returning non-null pointers on success return null pointer
>>     on failure.
>>
>>   - Functions returning non-negative integers on success return a
>>     negative error code on failure.
>>
>>   Different: GLib discourages void functions, because these lead to
>>   awkward error checking code.  We have tons of them, and tons of
>>   awkward error checking code:
>>
>>     Error *err = NULL;
>>     frobnicate(arg, &err);
>>     if (err) {
>>         ... recover ...
>>         error_propagate(errp, err);
>>     }
>>
>>   instead of
>>
>>     if (!frobnicate(arg, errp))
>>         ... recover ...
>>     }
>>
>>   Can also lead to pointless creation of Error objects.
>>
>>   I consider this a design mistake.  Can we still fix it?  We have more
>>   than 2000 void functions taking an Error ** parameter...
>>
>>   Transforming code that receives and checks for errors with Coccinelle
>>   shouldn't be hard.  Transforming code that returns errors seems more
>>   difficult.  We need to transform explicit and implicit return to
>>   either return true or return false, depending on what we did to the
>>   @errp parameter on the way to the return.  Hmm.
> [...]
>
> To figure out what functions with an Error ** parameter return, I used
> Coccinelle to find such function definitions and print the return types.
> Summary of results:
>
>    2155 void
>     873 signed integer
>     494 pointer
>     153 bool
>      33 unsigned integer
>       6 enum
>    ---------------------
>    3714 total
>
> I then used Coccinelle to find checked calls of void functions (passing
> &error_fatal or &error_abort is not considered "checking" here).  These
> calls become simpler if we make the functions return a useful value.  I
> found a bit under 600 direct calls, and some 50 indirect calls.
>
> Most frequent direct calls:
>
>     127 object_property_set_bool
>      27 qemu_opts_absorb_qdict
>      16 visit_type_str
>      14 visit_type_int
>      10 visit_type_uint32
>
> Let's have a closer look at object_property_set() & friends.  Out of
> almost 1000 calls, some 150 are checked.  While I'm sure many of the
> unchecked calls can't actually fail, I am concerned some unchecked calls
> can.
>
> If we adopt the convention to return a value that indicates success /
> failure, we should consider converting object.h to it sooner rather than
> later.
>
> Please understand these are rough numbers from quick & dirty scripts.

Paolo, Daniel, Eduardo,

Please pick one for QOM:

* Do nothing.  Looks like

     object_property_set_bool(..., &err);
     if (err) {
         error_propagate(errp, err);
         return;
     }

* Return true on success, false on error.  Looks like

     if (!object_property_set_bool(..., errp)) {
         return;
     }

* Return 0 on success, -1 on error.  Looks like

     if (object_property_set_bool(..., errp) < 0) {
         return;
     }

  This is slightly more likely to require line wrapping than the
  previous one.

Re: Questionable aspects of QEMU Error's design

[Eduardo Habkost]

On Sat, Apr 04, 2020 at 12:59:27PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> QEMU's Error was patterned after GLib's GError.  Differences include:
> > [...]
> >> * Return value conventions
> >>
> >>   Common: non-void functions return a distinct error value on failure
> >>   when such a value can be defined.  Patterns:
> >>
> >>   - Functions returning non-null pointers on success return null pointer
> >>     on failure.
> >>
> >>   - Functions returning non-negative integers on success return a
> >>     negative error code on failure.
> >>
> >>   Different: GLib discourages void functions, because these lead to
> >>   awkward error checking code.  We have tons of them, and tons of
> >>   awkward error checking code:
> >>
> >>     Error *err = NULL;
> >>     frobnicate(arg, &err);
> >>     if (err) {
> >>         ... recover ...
> >>         error_propagate(errp, err);
> >>     }
> >>
> >>   instead of
> >>
> >>     if (!frobnicate(arg, errp))
> >>         ... recover ...
> >>     }
> >>
> >>   Can also lead to pointless creation of Error objects.
> >>
> >>   I consider this a design mistake.  Can we still fix it?  We have more
> >>   than 2000 void functions taking an Error ** parameter...
> >>
> >>   Transforming code that receives and checks for errors with Coccinelle
> >>   shouldn't be hard.  Transforming code that returns errors seems more
> >>   difficult.  We need to transform explicit and implicit return to
> >>   either return true or return false, depending on what we did to the
> >>   @errp parameter on the way to the return.  Hmm.
> > [...]
> >
> > To figure out what functions with an Error ** parameter return, I used
> > Coccinelle to find such function definitions and print the return types.
> > Summary of results:
> >
> >    2155 void
> >     873 signed integer
> >     494 pointer
> >     153 bool
> >      33 unsigned integer
> >       6 enum
> >    ---------------------
> >    3714 total
> >
> > I then used Coccinelle to find checked calls of void functions (passing
> > &error_fatal or &error_abort is not considered "checking" here).  These
> > calls become simpler if we make the functions return a useful value.  I
> > found a bit under 600 direct calls, and some 50 indirect calls.
> >
> > Most frequent direct calls:
> >
> >     127 object_property_set_bool
> >      27 qemu_opts_absorb_qdict
> >      16 visit_type_str
> >      14 visit_type_int
> >      10 visit_type_uint32
> >
> > Let's have a closer look at object_property_set() & friends.  Out of
> > almost 1000 calls, some 150 are checked.  While I'm sure many of the
> > unchecked calls can't actually fail, I am concerned some unchecked calls
> > can.
> >
> > If we adopt the convention to return a value that indicates success /
> > failure, we should consider converting object.h to it sooner rather than
> > later.
> >
> > Please understand these are rough numbers from quick & dirty scripts.
> 
> Paolo, Daniel, Eduardo,
> 
> Please pick one for QOM:

Replying this without reading the whole discussion and context:

> 
> * Do nothing.  Looks like
> 
>      object_property_set_bool(..., &err);
>      if (err) {
>          error_propagate(errp, err);
>          return;
>      }
> 
> * Return true on success, false on error.  Looks like
> 

I prefer this one.

>      if (!object_property_set_bool(..., errp)) {
>          return;
>      }
> 
> * Return 0 on success, -1 on error.  Looks like
> 
>      if (object_property_set_bool(..., errp) < 0) {
>          return;
>      }
> 
>   This is slightly more likely to require line wrapping than the
>   previous one.

-- 
Eduardo

Re: Questionable aspects of QEMU Error's design

[Daniel P. Berrangé]

On Sat, Apr 04, 2020 at 12:59:27PM +0200, Markus Armbruster wrote:
> Paolo, Daniel, Eduardo,
> 
> Please pick one for QOM:

> * Return true on success, false on error.  Looks like
> 
>      if (!object_property_set_bool(..., errp)) {
>          return;
>      }

My preference is this one, since it aligns with GLib recommendations
for error reporting & is simple & concise.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Questionable aspects of QEMU Error's design

[Eduardo Habkost]

On Mon, Apr 06, 2020 at 11:05:36AM -0300, Eduardo Habkost wrote:
> On Sat, Apr 04, 2020 at 12:59:27PM +0200, Markus Armbruster wrote:
[...]
> > Paolo, Daniel, Eduardo,
> > 
> > Please pick one for QOM:
> 
> Replying this without reading the whole discussion and context:
> 
> > 
> > * Do nothing.  Looks like
> > 
> >      object_property_set_bool(..., &err);
> >      if (err) {
> >          error_propagate(errp, err);
> >          return;
> >      }
> > 
> > * Return true on success, false on error.  Looks like
> > 
> 
> I prefer this one.

My weird quoting choice was probably confusing, so for the sake
of clarity: I prefer the "Return true on success, false on error"
option.

> 
> >      if (!object_property_set_bool(..., errp)) {
> >          return;
> >      }
> > 
> > * Return 0 on success, -1 on error.  Looks like
> > 
> >      if (object_property_set_bool(..., errp) < 0) {
> >          return;
> >      }
> > 
> >   This is slightly more likely to require line wrapping than the
> >   previous one.
> 
> -- 
> Eduardo

-- 
Eduardo

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> QEMU's Error was patterned after GLib's GError.  Differences include:
> [...]
>> * Return value conventions
>>
>>   Common: non-void functions return a distinct error value on failure
>>   when such a value can be defined.  Patterns:
>>
>>   - Functions returning non-null pointers on success return null pointer
>>     on failure.
>>
>>   - Functions returning non-negative integers on success return a
>>     negative error code on failure.
>>
>>   Different: GLib discourages void functions, because these lead to
>>   awkward error checking code.  We have tons of them, and tons of
>>   awkward error checking code:
>>
>>     Error *err = NULL;
>>     frobnicate(arg, &err);
>>     if (err) {
>>         ... recover ...
>>         error_propagate(errp, err);
>>     }
>>
>>   instead of
>>
>>     if (!frobnicate(arg, errp))
>>         ... recover ...
>>     }
>>
>>   Can also lead to pointless creation of Error objects.
>>
>>   I consider this a design mistake.  Can we still fix it?  We have more
>>   than 2000 void functions taking an Error ** parameter...
>>
>>   Transforming code that receives and checks for errors with Coccinelle
>>   shouldn't be hard.  Transforming code that returns errors seems more
>>   difficult.  We need to transform explicit and implicit return to
>>   either return true or return false, depending on what we did to the
>>   @errp parameter on the way to the return.  Hmm.
> [...]
>
> To figure out what functions with an Error ** parameter return, I used
> Coccinelle to find such function definitions and print the return types.
> Summary of results:
>
>    2155 void
>     873 signed integer
>     494 pointer
>     153 bool
>      33 unsigned integer
>       6 enum
>    ---------------------
>    3714 total
>
> I then used Coccinelle to find checked calls of void functions (passing
> &error_fatal or &error_abort is not considered "checking" here).  These
> calls become simpler if we make the functions return a useful value.  I
> found a bit under 600 direct calls, and some 50 indirect calls.
>
> Most frequent direct calls:
>
>     127 object_property_set_bool
>      27 qemu_opts_absorb_qdict
>      16 visit_type_str
>      14 visit_type_int
>      10 visit_type_uint32
>
> Let's have a closer look at object_property_set() & friends.  Out of
> almost 1000 calls, some 150 are checked.  While I'm sure many of the
> unchecked calls can't actually fail, I am concerned some unchecked calls
> can.
>
> If we adopt the convention to return a value that indicates success /
> failure, we should consider converting object.h to it sooner rather than
> later.
>
> Please understand these are rough numbers from quick & dirty scripts.

FYI, I'm working on converting QemuOpts, QAPI visitors and QOM.  I keep
running into bugs.  So far:

    [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
    [PATCH for-5.1 0/5] qobject: Minor spring cleaning
    [PATCH v2 00/14] Miscellaneous error handling fixes
    [PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes
    [PATCH 0/3] fuzz: Probably there is a better way to do this
    [PATCH v2 00/15] qapi: Spring cleaning
    [PATCH 00/11] More miscellaneous error handling fixes

I got another one coming for QOM and qdev before I can post the
conversion.

Vladimir, since the conversion will mess with error_propagate(), I'd
like to get it in before your auto-propagation work.

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

27.04.2020 18:36, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>> [...]
>>> * Return value conventions
>>>
>>>    Common: non-void functions return a distinct error value on failure
>>>    when such a value can be defined.  Patterns:
>>>
>>>    - Functions returning non-null pointers on success return null pointer
>>>      on failure.
>>>
>>>    - Functions returning non-negative integers on success return a
>>>      negative error code on failure.
>>>
>>>    Different: GLib discourages void functions, because these lead to
>>>    awkward error checking code.  We have tons of them, and tons of
>>>    awkward error checking code:
>>>
>>>      Error *err = NULL;
>>>      frobnicate(arg, &err);
>>>      if (err) {
>>>          ... recover ...
>>>          error_propagate(errp, err);
>>>      }
>>>
>>>    instead of
>>>
>>>      if (!frobnicate(arg, errp))
>>>          ... recover ...
>>>      }
>>>
>>>    Can also lead to pointless creation of Error objects.
>>>
>>>    I consider this a design mistake.  Can we still fix it?  We have more
>>>    than 2000 void functions taking an Error ** parameter...
>>>
>>>    Transforming code that receives and checks for errors with Coccinelle
>>>    shouldn't be hard.  Transforming code that returns errors seems more
>>>    difficult.  We need to transform explicit and implicit return to
>>>    either return true or return false, depending on what we did to the
>>>    @errp parameter on the way to the return.  Hmm.
>> [...]
>>
>> To figure out what functions with an Error ** parameter return, I used
>> Coccinelle to find such function definitions and print the return types.
>> Summary of results:
>>
>>     2155 void
>>      873 signed integer
>>      494 pointer
>>      153 bool
>>       33 unsigned integer
>>        6 enum
>>     ---------------------
>>     3714 total
>>
>> I then used Coccinelle to find checked calls of void functions (passing
>> &error_fatal or &error_abort is not considered "checking" here).  These
>> calls become simpler if we make the functions return a useful value.  I
>> found a bit under 600 direct calls, and some 50 indirect calls.
>>
>> Most frequent direct calls:
>>
>>      127 object_property_set_bool
>>       27 qemu_opts_absorb_qdict
>>       16 visit_type_str
>>       14 visit_type_int
>>       10 visit_type_uint32
>>
>> Let's have a closer look at object_property_set() & friends.  Out of
>> almost 1000 calls, some 150 are checked.  While I'm sure many of the
>> unchecked calls can't actually fail, I am concerned some unchecked calls
>> can.
>>
>> If we adopt the convention to return a value that indicates success /
>> failure, we should consider converting object.h to it sooner rather than
>> later.
>>
>> Please understand these are rough numbers from quick & dirty scripts.
> 
> FYI, I'm working on converting QemuOpts, QAPI visitors and QOM.  I keep
> running into bugs.  So far:
> 
>      [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
>      [PATCH for-5.1 0/5] qobject: Minor spring cleaning
>      [PATCH v2 00/14] Miscellaneous error handling fixes
>      [PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes
>      [PATCH 0/3] fuzz: Probably there is a better way to do this
>      [PATCH v2 00/15] qapi: Spring cleaning
>      [PATCH 00/11] More miscellaneous error handling fixes
> 
> I got another one coming for QOM and qdev before I can post the
> conversion.
> 
> Vladimir, since the conversion will mess with error_propagate(), I'd
> like to get it in before your auto-propagation work.
> 

OK, just let me know when to regenerate the series, it's not hard.

-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

28.04.2020 08:20, Vladimir Sementsov-Ogievskiy wrote:
> 27.04.2020 18:36, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Markus Armbruster <armbru@redhat.com> writes:
>>>
>>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>> [...]
>>>> * Return value conventions
>>>>
>>>>    Common: non-void functions return a distinct error value on failure
>>>>    when such a value can be defined.  Patterns:
>>>>
>>>>    - Functions returning non-null pointers on success return null pointer
>>>>      on failure.
>>>>
>>>>    - Functions returning non-negative integers on success return a
>>>>      negative error code on failure.
>>>>
>>>>    Different: GLib discourages void functions, because these lead to
>>>>    awkward error checking code.  We have tons of them, and tons of
>>>>    awkward error checking code:
>>>>
>>>>      Error *err = NULL;
>>>>      frobnicate(arg, &err);
>>>>      if (err) {
>>>>          ... recover ...
>>>>          error_propagate(errp, err);
>>>>      }
>>>>
>>>>    instead of
>>>>
>>>>      if (!frobnicate(arg, errp))
>>>>          ... recover ...
>>>>      }
>>>>
>>>>    Can also lead to pointless creation of Error objects.
>>>>
>>>>    I consider this a design mistake.  Can we still fix it?  We have more
>>>>    than 2000 void functions taking an Error ** parameter...
>>>>
>>>>    Transforming code that receives and checks for errors with Coccinelle
>>>>    shouldn't be hard.  Transforming code that returns errors seems more
>>>>    difficult.  We need to transform explicit and implicit return to
>>>>    either return true or return false, depending on what we did to the
>>>>    @errp parameter on the way to the return.  Hmm.
>>> [...]
>>>
>>> To figure out what functions with an Error ** parameter return, I used
>>> Coccinelle to find such function definitions and print the return types.
>>> Summary of results:
>>>
>>>     2155 void
>>>      873 signed integer
>>>      494 pointer
>>>      153 bool
>>>       33 unsigned integer
>>>        6 enum
>>>     ---------------------
>>>     3714 total
>>>
>>> I then used Coccinelle to find checked calls of void functions (passing
>>> &error_fatal or &error_abort is not considered "checking" here).  These
>>> calls become simpler if we make the functions return a useful value.  I
>>> found a bit under 600 direct calls, and some 50 indirect calls.
>>>
>>> Most frequent direct calls:
>>>
>>>      127 object_property_set_bool
>>>       27 qemu_opts_absorb_qdict
>>>       16 visit_type_str
>>>       14 visit_type_int
>>>       10 visit_type_uint32
>>>
>>> Let's have a closer look at object_property_set() & friends.  Out of
>>> almost 1000 calls, some 150 are checked.  While I'm sure many of the
>>> unchecked calls can't actually fail, I am concerned some unchecked calls
>>> can.
>>>
>>> If we adopt the convention to return a value that indicates success /
>>> failure, we should consider converting object.h to it sooner rather than
>>> later.
>>>
>>> Please understand these are rough numbers from quick & dirty scripts.
>>
>> FYI, I'm working on converting QemuOpts, QAPI visitors and QOM.  I keep
>> running into bugs.  So far:
>>
>>      [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
>>      [PATCH for-5.1 0/5] qobject: Minor spring cleaning
>>      [PATCH v2 00/14] Miscellaneous error handling fixes
>>      [PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes
>>      [PATCH 0/3] fuzz: Probably there is a better way to do this
>>      [PATCH v2 00/15] qapi: Spring cleaning
>>      [PATCH 00/11] More miscellaneous error handling fixes
>>
>> I got another one coming for QOM and qdev before I can post the
>> conversion.
>>
>> Vladimir, since the conversion will mess with error_propagate(), I'd
>> like to get it in before your auto-propagation work.
>>
> 
> OK, just let me know when to regenerate the series, it's not hard.
> 

Hi! Is all that merged? Should I resend now?

-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 28.04.2020 08:20, Vladimir Sementsov-Ogievskiy wrote:
>> 27.04.2020 18:36, Markus Armbruster wrote:
>>> Markus Armbruster <armbru@redhat.com> writes:
>>>
>>>> Markus Armbruster <armbru@redhat.com> writes:
>>>>
>>>>> QEMU's Error was patterned after GLib's GError.  Differences include:
>>>> [...]
>>>>> * Return value conventions
>>>>>
>>>>>    Common: non-void functions return a distinct error value on failure
>>>>>    when such a value can be defined.  Patterns:
>>>>>
>>>>>    - Functions returning non-null pointers on success return null pointer
>>>>>      on failure.
>>>>>
>>>>>    - Functions returning non-negative integers on success return a
>>>>>      negative error code on failure.
>>>>>
>>>>>    Different: GLib discourages void functions, because these lead to
>>>>>    awkward error checking code.  We have tons of them, and tons of
>>>>>    awkward error checking code:
>>>>>
>>>>>      Error *err = NULL;
>>>>>      frobnicate(arg, &err);
>>>>>      if (err) {
>>>>>          ... recover ...
>>>>>          error_propagate(errp, err);
>>>>>      }
>>>>>
>>>>>    instead of
>>>>>
>>>>>      if (!frobnicate(arg, errp))
>>>>>          ... recover ...
>>>>>      }
>>>>>
>>>>>    Can also lead to pointless creation of Error objects.
>>>>>
>>>>>    I consider this a design mistake.  Can we still fix it?  We have more
>>>>>    than 2000 void functions taking an Error ** parameter...
>>>>>
>>>>>    Transforming code that receives and checks for errors with Coccinelle
>>>>>    shouldn't be hard.  Transforming code that returns errors seems more
>>>>>    difficult.  We need to transform explicit and implicit return to
>>>>>    either return true or return false, depending on what we did to the
>>>>>    @errp parameter on the way to the return.  Hmm.
>>>> [...]
>>>>
>>>> To figure out what functions with an Error ** parameter return, I used
>>>> Coccinelle to find such function definitions and print the return types.
>>>> Summary of results:
>>>>
>>>>     2155 void
>>>>      873 signed integer
>>>>      494 pointer
>>>>      153 bool
>>>>       33 unsigned integer
>>>>        6 enum
>>>>     ---------------------
>>>>     3714 total
>>>>
>>>> I then used Coccinelle to find checked calls of void functions (passing
>>>> &error_fatal or &error_abort is not considered "checking" here).  These
>>>> calls become simpler if we make the functions return a useful value.  I
>>>> found a bit under 600 direct calls, and some 50 indirect calls.
>>>>
>>>> Most frequent direct calls:
>>>>
>>>>      127 object_property_set_bool
>>>>       27 qemu_opts_absorb_qdict
>>>>       16 visit_type_str
>>>>       14 visit_type_int
>>>>       10 visit_type_uint32
>>>>
>>>> Let's have a closer look at object_property_set() & friends.  Out of
>>>> almost 1000 calls, some 150 are checked.  While I'm sure many of the
>>>> unchecked calls can't actually fail, I am concerned some unchecked calls
>>>> can.
>>>>
>>>> If we adopt the convention to return a value that indicates success /
>>>> failure, we should consider converting object.h to it sooner rather than
>>>> later.
>>>>
>>>> Please understand these are rough numbers from quick & dirty scripts.
>>>
>>> FYI, I'm working on converting QemuOpts, QAPI visitors and QOM.  I keep
>>> running into bugs.  So far:
>>>
>>>      [PATCH v2 for-5.1 0/9] qemu-option: Fix corner cases and clean up
>>>      [PATCH for-5.1 0/5] qobject: Minor spring cleaning
>>>      [PATCH v2 00/14] Miscellaneous error handling fixes
>>>      [PATCH 0/4] Subject: [PATCH 0/4] smbus: SPD fixes
>>>      [PATCH 0/3] fuzz: Probably there is a better way to do this
>>>      [PATCH v2 00/15] qapi: Spring cleaning
>>>      [PATCH 00/11] More miscellaneous error handling fixes
>>>
>>> I got another one coming for QOM and qdev before I can post the
>>> conversion.
>>>
>>> Vladimir, since the conversion will mess with error_propagate(), I'd
>>> like to get it in before your auto-propagation work.
>>>
>>
>> OK, just let me know when to regenerate the series, it's not hard.
>>
>
> Hi! Is all that merged? Should I resend now?

I ran into many bugs and fell into a few rabbit holes.  I'm busy
finishing and flushing the patches.

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 28.04.2020 08:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.04.2020 18:36, Markus Armbruster wrote:
>>>> FYI, I'm working on converting QemuOpts, QAPI visitors and QOM.  I keep
>>>> running into bugs.  So far:
[...]
>>>> I got another one coming for QOM and qdev before I can post the
>>>> conversion.
>>>>
>>>> Vladimir, since the conversion will mess with error_propagate(), I'd
>>>> like to get it in before your auto-propagation work.
>>>>
>>>
>>> OK, just let me know when to regenerate the series, it's not hard.
>>>
>>
>> Hi! Is all that merged? Should I resend now?
>
> I ran into many bugs and fell into a few rabbit holes.  I'm busy
> finishing and flushing the patches.

All merged except for the final series "[PATCH v2 00/44] Less clumsy
error checking".  v2 has a lot of change within the series, but in
aggregate it's really close to v1.  This makes be optimistic it can
serve as a base for your auto-propagation work.  To get it into 5.1, we
need a respin, a re-review, and a pull request.  Time is awfully short.
Sorry for taking so long!  If you want to try, I can give it priority on
my side.

Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

03.07.2020 10:38, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 28.04.2020 08:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> 27.04.2020 18:36, Markus Armbruster wrote:
>>>>> FYI, I'm working on converting QemuOpts, QAPI visitors and QOM.  I keep
>>>>> running into bugs.  So far:
> [...]
>>>>> I got another one coming for QOM and qdev before I can post the
>>>>> conversion.
>>>>>
>>>>> Vladimir, since the conversion will mess with error_propagate(), I'd
>>>>> like to get it in before your auto-propagation work.
>>>>>
>>>>
>>>> OK, just let me know when to regenerate the series, it's not hard.
>>>>
>>>
>>> Hi! Is all that merged? Should I resend now?
>>
>> I ran into many bugs and fell into a few rabbit holes.  I'm busy
>> finishing and flushing the patches.
> 
> All merged except for the final series "[PATCH v2 00/44] Less clumsy
> error checking".  v2 has a lot of change within the series, but in
> aggregate it's really close to v1.  This makes be optimistic it can
> serve as a base for your auto-propagation work.  To get it into 5.1, we
> need a respin, a re-review, and a pull request.  Time is awfully short.
> Sorry for taking so long!  If you want to try, I can give it priority on
> my side.
> 

Of course let's try)


-- 
Best regards,
Vladimir

Re: Questionable aspects of QEMU Error's design

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> QEMU's Error was patterned after GLib's GError.  Differences include:
> [...]
>> * Return value conventions
>>
>>   Common: non-void functions return a distinct error value on failure
>>   when such a value can be defined.  Patterns:
>>
>>   - Functions returning non-null pointers on success return null pointer
>>     on failure.
>>
>>   - Functions returning non-negative integers on success return a
>>     negative error code on failure.
>>
>>   Different: GLib discourages void functions, because these lead to
>>   awkward error checking code.  We have tons of them, and tons of
>>   awkward error checking code:
>>
>>     Error *err = NULL;
>>     frobnicate(arg, &err);
>>     if (err) {
>>         ... recover ...
>>         error_propagate(errp, err);
>>     }
>>
>>   instead of
>>
>>     if (!frobnicate(arg, errp))
>>         ... recover ...
>>     }
>>
>>   Can also lead to pointless creation of Error objects.
>>
>>   I consider this a design mistake.  Can we still fix it?  We have more
>>   than 2000 void functions taking an Error ** parameter...
>>
>>   Transforming code that receives and checks for errors with Coccinelle
>>   shouldn't be hard.  Transforming code that returns errors seems more
>>   difficult.  We need to transform explicit and implicit return to
>>   either return true or return false, depending on what we did to the
>>   @errp parameter on the way to the return.  Hmm.
> [...]
>
> To figure out what functions with an Error ** parameter return, I used
> Coccinelle to find such function definitions and print the return types.
> Summary of results:
>
>    2155 void
>     873 signed integer
>     494 pointer
>     153 bool
>      33 unsigned integer
>       6 enum
>    ---------------------
>    3714 total

With my "[PATCH v2 00/44] Less clumsy error checking" applied, I now count

     1946 void
      879 signed integer
      484 pointer
      301 bool
       33 unsigned integer
        3 GuestFsfreezeStatus
        1 gnutls_x509_crt_t
        1 QCryptoCipherAlgorithm
        1 COLOMessage
        1 BlockdevDetectZeroesOptions
     ---------------------
     3650 total

About 7% complete for function definitions.

> I then used Coccinelle to find checked calls of void functions (passing
> &error_fatal or &error_abort is not considered "checking" here).  These
> calls become simpler if we make the functions return a useful value.  I
> found a bit under 600 direct calls, and some 50 indirect calls.

Different method this time: I count any direct function call that takes
&err other than &error_abort, &error_fatal, and whose value, if any, is
not used.

Current master: 1050

With my "[PATCH v2 00/44] Less clumsy error checking" applied: 649

About 38% complete for function calls.

> Most frequent direct calls:
>
>     127 object_property_set_bool
>      27 qemu_opts_absorb_qdict
>      16 visit_type_str
>      14 visit_type_int
>      10 visit_type_uint32

Top scorers master:

    151 sysbus_realize()
     34 qemu_opts_absorb_qdict()
     29 visit_type_int()
     24 visit_type_str()
     23 cpu_exec_realizefn()
     19 visit_type_size()
     16 qdev_realize()
     14 visit_type_bool()
     12 visit_type_uint32()
     11 visit_type_int32()
     11 object_property_set_bool()
     10 object_property_set_uint()
     10 object_property_set_int()
    +420 functions with fewer than 10 calls

Top scorers with my patches applied:

     23 cpu_exec_realizefn()
     15 visit_type_int()
     10 visit_type_size()
    +387 functions with fewer than 10 calls

Looks like this is going to be a long slog.

With functions into buckets by same name prefix up to the first '_':

     63 visit
     57 qmp
     33 bdrv
     29 cpu
     26 xen
     25 memory
   +113 buckets with fewer than 25 calls

[...]
>
> Please understand these are rough numbers from quick & dirty scripts.

Still are.