Fam Zheng <f...@redhat.com> writes:
> On Mon, 02/10 15:54, Luiz Capitulino wrote:
>> > @@ -875,13 +875,13 @@ DriveInfo *drive_init(QemuOpts *all_opts,
>> > BlockInterfaceType block_default_type)
>> > /* Actual block device init: Functionality shared with blockdev-add */
>> > dinfo = blockdev_init(filename, bs_opts, type, &local_err);
>> > if (dinfo == NULL) {
>> > - if (error_is_set(&local_err)) {
>> > + if (local_err) {
>> > qerror_report_err(local_err);
>> > error_free(local_err);
>> > }
>> > goto fail;
>> > } else {
>> > - assert(!error_is_set(&local_err));
>> > + assert(!local_err);
>> > }
>>
>> Not related to this patch, but this else clause is checking if
>> dinfo != NULL and local_err != NULL, right? Shouldn't it be moved
>> into blockdev_init() instead?
>
> This is just an caller checking that the output of blockdev_init is sane, so I
> think it's OK to put an assertion here.
Yes, this is a common problem with functions returning a non-null
pointer on success, null pointer and an error object on failure. Which
of the two should the caller test to figure out success vs. failure?
If it only tests the return value, and a buggy callee returns an error
object along with a null return value, the error object is leaked.
If it only tests for an error object, and a buggy callee returns a null
pointer along with an error object, we'll almost certainly crash some
time later dereferencing the null return value.
If it tests both, it'll have to deal with inconsistencies. This is what
this caller does.
