Re: [PULL 11/18] block: Generic file creation fallback

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3114 bytes --]

On 25.02.20 11:05, Peter Maydell wrote:
> On Thu, 20 Feb 2020 at 16:11, Max Reitz <mreitz@redhat.com> wrote:
>>
>> If a protocol driver does not support image creation, we can see whether
>> maybe the file exists already.  If so, just truncating it will be
>> sufficient.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Message-Id: <20200122164532.178040-3-mreitz@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Hi; Coverity thinks there's a memory leak in the error
> codepaths in this function (CID 1419884): is it right?

Yes, it is, I’ll write a patch.

>> +static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
>> +                                     QemuOpts *opts, Error **errp)
>> +{
>> +    BlockBackend *blk;
>> +    QDict *options = qdict_new();
> 
> We create the QDict here...
> 
>> +    int64_t size = 0;
>> +    char *buf = NULL;
>> +    PreallocMode prealloc;
>>      Error *local_err = NULL;
>>      int ret;
>>
>> +    size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
>> +    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
>> +    prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
>> +                               PREALLOC_MODE_OFF, &local_err);
>> +    g_free(buf);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return -EINVAL;
> 
> ...but here and in other error return paths we don't
> free it (I think that might need a qobject_unref() but
> am not sure).
> 
>> +    }
>> +
>> +    if (prealloc != PREALLOC_MODE_OFF) {
>> +        error_setg(errp, "Unsupported preallocation mode '%s'",
>> +                   PreallocMode_str(prealloc));
>> +        return -ENOTSUP;
>> +    }
> 
> (You could probably postpone qdict_new() to here to avoid
> having to change the error handling paths above this point, but
> you still need to deal with the error path for blk_new_open failing.)

Indeed.  Or maybe put the unref() under the out label and set @options
to NULL after it’s captured by @blk.  I’ll see.

>> +
>> +    qdict_put_str(options, "driver", drv->format_name);
>> +
>> +    blk = blk_new_open(filename, NULL, options,
>> +                       BDRV_O_RDWR | BDRV_O_RESIZE, errp);
>> +    if (!blk) {
>> +        error_prepend(errp, "Protocol driver '%s' does not support image "
>> +                      "creation, and opening the image failed: ",
>> +                      drv->format_name);
>> +        return -EINVAL;
>> +    }
> 
> I guess for error-paths after blk_new_open() succeeds
> that the blk object owns the options dictionary and
> will deal with unreffing it for us?

Yes.

Max

>> +
>> +    size = create_file_fallback_truncate(blk, size, errp);
>> +    if (size < 0) {
>> +        ret = size;
>> +        goto out;
>> +    }
>> +
>> +    ret = create_file_fallback_zero_first_sector(blk, size, errp);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>> +
>> +    ret = 0;
>> +out:
>> +    blk_unref(blk);
>> +    return ret;
>> +}
> 
> thanks
> -- PMM
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]