From: Eric Blake <eblake@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>, qemu-devel@nongnu.org
Cc: pbonzini@redhat.com, peter.crosthwaite@xilinx.com,
afaerber@suse.de, anthony@codemonkey.ws, mst@redhat.com
Subject: Re: [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL
Date: Wed, 27 Nov 2013 21:58:18 -0700 [thread overview]
Message-ID: <5296CD6A.1060008@redhat.com> (raw)
In-Reply-To: <1385601858-8065-1-git-send-email-imammedo@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]
On 11/27/2013 06:24 PM, Igor Mammedov wrote:
> in case if caller setting property doesn't care about error and
> passes in NULL as errp argument but error occurs in property setter,
> it is silently discarded leaving object in undefined state.
>
> As result it leads to hard to find bugs, so if caller doesn't
> care about error it must be sure that property exists and
> accepts provided value, otherwise it's better to abort early
> since error case couldn't be handled gracefully and find
> invalid usecase early.
>
> In addition multitude of property setters will be always
> guarantied to have error object present and won't be required
s/guarantied/guaranteed/
> to handle this condition individually.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> +out:
> + if (local_error) {
This says local_error was set...
> + if (!errp) {
> + assert_no_error(local_error);
so this assert_no_error() is dead code in its current position. To be
useful, you probably want:
if (!errp) {
assert_no_error(local_error);
} else if (local_error) {
error_propagate(errp, local_error);
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
next prev parent reply other threads:[~2013-11-28 4:58 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-28 1:24 [Qemu-devel] [PATCH] qom: abort on error in property setter if caller passed errp == NULL Igor Mammedov
2013-11-28 4:58 ` Eric Blake [this message]
2013-11-28 13:01 ` Igor Mammedov
2013-11-28 5:10 ` Peter Crosthwaite
2013-11-28 7:53 ` Markus Armbruster
2013-11-28 13:23 ` Igor Mammedov
2013-11-28 13:41 ` Paolo Bonzini
2013-11-28 15:03 ` Markus Armbruster
2013-11-29 0:21 ` Peter Crosthwaite
2013-11-29 7:56 ` Markus Armbruster
2013-12-03 5:51 ` Peter Crosthwaite
2013-11-28 15:00 ` Markus Armbruster
2013-11-28 13:42 ` Andreas Färber
2013-11-28 13:48 ` Igor Mammedov
2013-11-28 14:00 ` Andreas Färber
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=5296CD6A.1060008@redhat.com \
--to=eblake@redhat.com \
--cc=afaerber@suse.de \
--cc=anthony@codemonkey.ws \
--cc=imammedo@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@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).