Re: [Qemu-devel] [PATCH 06/17] qcow2-dirty-bitmap: add qcow2_dirty_bitmap_load()

[Eric Blake <eblake@redhat.com>]

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

On 10/06/2015 05:01 PM, John Snow wrote:

>> +    ret = load_bitmap(bs_file, dirty_bitmap_table, bmh->dirty_bitmap_table_size, bitmap);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not read bitmap from image");
>> +        goto finish;
>> +    }
>> +
>> +finish:
>> +    if (*errp != NULL) {
>> +        bdrv_release_dirty_bitmap(bs_for, bitmap);
>> +        bitmap = NULL;
>> +    }
>> +    g_free(dirty_bitmap_table);
> 
> 
> I think we're not supposed to be reaching into errp to check its
> implementation detail like this ... the usual paradigm I see is just
> "goto fail" or similar statements instead of checking for
> error-or-success in a shared return block.
> 


If you have to make a decision based on whether an error was detected,
then you MUST pass a local error (as your caller may have passed NULL
because they don't care if you fail, even though you care if your helper
fails).  As in:

Error *err = NULL;
...
if (ret < 0) {
    error_setg_errno(&err, -ret, "Could not read...");
}
finish:
if (err) {
    bdrv_release_dirty_bitmap(...);
    error_propagate(errp, err);
    bitmap = NULL;
}

In short, any code that does *errp is a potential NULL dereference.  The
comments in error.h help explain the paradigms.

-- 
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: 604 bytes --]