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

[Peter Maydell]

On Thu, 20 Feb 2020 at 16:11, Max Reitz <address@hidden> 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 <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>

Hi; Coverity thinks there's a memory leak in the error
codepaths in this function (CID 1419884): is it right?

> +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.)

> +
> +    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?

> +
> +    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