qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>,
	Markus Armbruster <armbru@redhat.com>,
	afaerber@suse.de, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
Date: Thu, 05 Dec 2013 16:59:32 +0100	[thread overview]
Message-ID: <52A0A2E4.7060903@redhat.com> (raw)
In-Reply-To: <20131205163228.4700f3cc@nial.usersys.redhat.com>

Il 05/12/2013 16:32, Igor Mammedov ha scritto:
>>> > > * make Error* mandatory for all void functions
> one more advantage:
> + not need to pepper every property setter/getter with local_error + error_propagate(),
>   i.e. reduced code duplication.

Note that for something like tail calls there is no need for
error_propagate.  See get_enum/set_enum (and a lot more examples) in
hw/core/qdev-properties.c.

With your proposal, this:

    visit_type_bool(v, &value, name, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }
    bit_prop_set(dev, prop, value);

could indeed become

    visit_type_bool(v, &value, name, errp);
    bit_prop_set(dev, prop, value);

because both caller and callee return void.  This is because both the
caller (prop_set_bit) and callee (visit_type_bool) are void.  But here
come the next point:

>>> > >   + consistent
>>> > >   + almost predictable (because in C you can ignore return values)
> there is no return values from void functions.

You can ignore return values from int-returning functions.  So if I see

   func(..., NULL);
   func(..., errp);

it's not clear if it aborts on error, or just eats the error.

> >   - requires manual effort to abide to the policy
> with assert inside API there is no manual effort. But as Marcus
> noted these errors will be only runtime detectable :(

I referred to manual effort of adding assertions in leaf functions.
Just one missing assertion can be very problematic if non-leafs start
relying on this behavior.

I think Error is hard to misuse if you don't mind being verbose.  This
proposal would cut the verbosity (not sure how much, but it definitely
could) but it would be easier to misuse.

In terms of Rusty's API design manifesto
(http://sweng.the-davies.net/Home/rustys-api-design-manifesto) your
proposal would move the API from 4 to 3 or 2:

    4. Follow common convention and you'll get it right.
    3. Read the documentation and you'll get it right.
    2. Read the implementation and you'll get it right.

Paolo

      reply	other threads:[~2013-12-05 15:59 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
2013-12-03  5:49 ` [Qemu-devel] [RFC PATCH v1 1/5] error: Add error_abort Peter Crosthwaite
2013-12-03  5:50 ` [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages Peter Crosthwaite
2013-12-03  9:35   ` Markus Armbruster
2013-12-03 10:04     ` Peter Crosthwaite
2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 3/5] target-i386: Remove assert_no_error usage Peter Crosthwaite
2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail Peter Crosthwaite
2013-12-03  9:42   ` Markus Armbruster
2013-12-03 10:17     ` Peter Crosthwaite
2013-12-03 10:44       ` Markus Armbruster
2013-12-04  6:45     ` Peter Crosthwaite
2013-12-03  5:52 ` [Qemu-devel] [RFC PATCH v1 5/5] qerror: Remove assert_no_error() Peter Crosthwaite
2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
2013-12-03 11:49   ` Igor Mammedov
2013-12-03 11:57   ` Paolo Bonzini
2013-12-03 12:03     ` Peter Crosthwaite
2013-12-03 12:58   ` Eric Blake
2013-12-03 13:53     ` Markus Armbruster
2013-12-03 20:33       ` Igor Mammedov
2013-12-03 20:43         ` Eric Blake
2013-12-04  9:11           ` Markus Armbruster
2013-12-04 14:46             ` Eric Blake
2013-12-05 10:37         ` Paolo Bonzini
2013-12-05 15:32           ` Igor Mammedov
2013-12-05 15:59             ` Paolo Bonzini [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52A0A2E4.7060903@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=afaerber@suse.de \
    --cc=armbru@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=peter.crosthwaite@xilinx.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).