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

Commit Message

Max Reitz Feb. 20, 2020, 4:07 p.m. UTC

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 <[e-mail]>
Message-Id: <[e-mail]>
Signed-off-by: Max Reitz <[e-mail]>
---
 block.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 147 insertions(+), 12 deletions(-)

Comments

Peter Maydell Feb. 25, 2020, 10:05 a.m. UTC | #1

On Thu, 20 Feb 2020 at 16:11, Max Reitz <[e-mail]> 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 <[e-mail]>
> Message-Id: <[e-mail]>
> Signed-off-by: Max Reitz <[e-mail]>

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

Max Reitz Feb. 25, 2020, 10:19 a.m. UTC | #2

On 25.02.20 11:05, Peter Maydell wrote:
> On Thu, 20 Feb 2020 at 16:11, Max Reitz <[e-mail]> 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 <[e-mail]>
>> Message-Id: <[e-mail]>
>> Signed-off-by: Max Reitz <[e-mail]>
> 
> 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
>

Patch

diff --git a/block.c b/block.c
index 08372ced26..308a91c96b 100644
--- a/block.c
+++ b/block.c
@@ -532,20 +532,139 @@  out:
     return ret;
 }
 
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+/**
+ * Helper function for bdrv_create_file_fallback(): Resize @blk to at
+ * least the given @minimum_size.
+ *
+ * On success, return @blk's actual length.
+ * Otherwise, return -errno.
+ */
+static int64_t create_file_fallback_truncate(BlockBackend *blk,
+                                             int64_t minimum_size, Error **errp)
 {
-    BlockDriver *drv;
+    Error *local_err = NULL;
+    int64_t size;
+    int ret;
+
+    ret = blk_truncate(blk, minimum_size, false, PREALLOC_MODE_OFF, &local_err);
+    if (ret < 0 && ret != -ENOTSUP) {
+        error_propagate(errp, local_err);
+        return ret;
+    }
+
+    size = blk_getlength(blk);
+    if (size < 0) {
+        error_free(local_err);
+        error_setg_errno(errp, -size,
+                         "Failed to inquire the new image file's length");
+        return size;
+    }
+
+    if (size < minimum_size) {
+        /* Need to grow the image, but we failed to do that */
+        error_propagate(errp, local_err);
+        return -ENOTSUP;
+    }
+
+    error_free(local_err);
+    local_err = NULL;
+
+    return size;
+}
+
+/**
+ * Helper function for bdrv_create_file_fallback(): Zero the first
+ * sector to remove any potentially pre-existing image header.
+ */
+static int create_file_fallback_zero_first_sector(BlockBackend *blk,
+                                                  int64_t current_size,
+                                                  Error **errp)
+{
+    int64_t bytes_to_clear;
+    int ret;
+
+    bytes_to_clear = MIN(current_size, BDRV_SECTOR_SIZE);
+    if (bytes_to_clear) {
+        ret = blk_pwrite_zeroes(blk, 0, bytes_to_clear, BDRV_REQ_MAY_UNMAP);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Failed to clear the new image's first sector");
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
+static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
+                                     QemuOpts *opts, Error **errp)
+{
+    BlockBackend *blk;
+    QDict *options = qdict_new();
+    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;
+    }
+
+    if (prealloc != PREALLOC_MODE_OFF) {
+        error_setg(errp, "Unsupported preallocation mode '%s'",
+                   PreallocMode_str(prealloc));
+        return -ENOTSUP;
+    }
+
+    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;
+    }
+
+    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;
+}
+
+int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+{
+    BlockDriver *drv;
+
     drv = bdrv_find_protocol(filename, true, errp);
     if (drv == NULL) {
         return -ENOENT;
     }
 
-    ret = bdrv_create(drv, filename, opts, &local_err);
-    error_propagate(errp, local_err);
-    return ret;
+    if (drv->bdrv_co_create_opts) {
+        return bdrv_create(drv, filename, opts, errp);
+    } else {
+        return bdrv_create_file_fallback(filename, drv, opts, errp);
+    }
 }
 
 /**
@@ -1444,6 +1563,24 @@  QemuOptsList bdrv_runtime_opts = {
     },
 };
 
+static QemuOptsList fallback_create_opts = {
+    .name = "fallback-create-opts",
+    .head = QTAILQ_HEAD_INITIALIZER(fallback_create_opts.head),
+    .desc = {
+        {
+            .name = BLOCK_OPT_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Virtual disk size"
+        },
+        {
+            .name = BLOCK_OPT_PREALLOC,
+            .type = QEMU_OPT_STRING,
+            .help = "Preallocation mode (allowed values: off)"
+        },
+        { /* end of list */ }
+    }
+};
+
 /*
  * Common part for opening disk images and files
  *
@@ -5772,15 +5909,13 @@  void bdrv_img_create(const char *filename, const char *fmt,
         return;
     }
 
-    if (!proto_drv->create_opts) {
-        error_setg(errp, "Protocol driver '%s' does not support image creation",
-                   proto_drv->format_name);
-        return;
-    }
-
     /* Create parameter list */
     create_opts = qemu_opts_append(create_opts, drv->create_opts);
-    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+    if (proto_drv->create_opts) {
+        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+    } else {
+        create_opts = qemu_opts_append(create_opts, &fallback_create_opts);
+    }
 
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);