qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] Use error_is_set() only when necessary


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH] Use error_is_set() only when necessary
Date: Thu, 30 Jan 2014 08:45:35 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

On 01/30/2014 07:07 AM, Markus Armbruster wrote:
> error_is_set(&var) is the same as var != NULL, but it takes
> whole-program analysis to figure that out.  Unnecessarily hard for
> optimizers, static checkers, and human readers.  Dumb it down to
> obvious.
> 
> Gets rid of several dozen Coverity false positives.
> 
> Note that the obvious form is already used in many places.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---

>  37 files changed, 156 insertions(+), 156 deletions(-)

Good diffstat - shows it should be fairly mechanical.

> @@ -1399,7 +1399,7 @@ fail:
>      QDECREF(bs->options);
>      QDECREF(options);
>      bs->options = NULL;
> -    if (error_is_set(&local_err)) {
> +    if (local_err) {
>          error_propagate(errp, local_err);
>      }
>      return ret;

Is it worth a further cleanup on instances like this?  That is,
error_propagate(errp, NULL) is a safe no-op, so we can avoid the 'if
(local_err)' conditional.  But that should not be in this patch (keep
the mechanical changes easy).

> +++ b/block/snapshot.c
> @@ -345,7 +345,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState 
> *bs,
>          ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, &local_err);
>      }
>  
> -    if (error_is_set(&local_err)) {
> +    if (local_err) {
>          error_propagate(errp, local_err);
>      }

Another example that can be simplified.

> +++ b/tests/test-qmp-input-strict.c
> @@ -92,7 +92,7 @@ static void test_validate_struct(TestInputVisitorData *data,
>      v = validate_test_init(data, "{ 'integer': -42, 'boolean': true, 
> 'string': 'foo' }");
>  
>      visit_type_TestStruct(v, &p, NULL, &errp);
> -    g_assert(!error_is_set(&errp));
> +    g_assert(!errp);

This (and other places in test files) chould use
visit_type_TestStruct(v, &p, NULL, &error_abort) and ditch local errp.
But that's a separate patch as well.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]