[Qemu-devel] [PATCH v7 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Qemu-devel] [PATCH v7 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Changes from previous version 6 [1]:

- bdrv_delete_file() now uses BlockDriverState as a parameter rather
than a filename string.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00139.html

Daniel Henrique Barboza (4):
  block: introducing 'bdrv_co_delete_file' interface
  block.c: adding bdrv_delete_file
  crypto.c: cleanup created file when block_crypto_co_create_opts_luks
    fails
  qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

 block.c                    | 73 ++++++++++++++++++++++++++++++++++++++
 block/crypto.c             | 22 ++++++++++++
 block/file-posix.c         | 28 +++++++++++++++
 include/block/block.h      |  1 +
 include/block/block_int.h  |  6 ++++
 tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/259.out | 11 ++++++
 tests/qemu-iotests/group   |  1 +
 8 files changed, 209 insertions(+)
 create mode 100755 tests/qemu-iotests/259
 create mode 100644 tests/qemu-iotests/259.out

-- 
2.21.0

[Qemu-devel] [PATCH v7 1/4] block: introducing 'bdrv_co_delete_file' interface

[Daniel Henrique Barboza]

Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.

This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the 'file' driver in file-posix.c.The
implementation is given by 'raw_co_delete_file'.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/file-posix.c        | 28 ++++++++++++++++++++++++++++
 include/block/block_int.h |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index fbeb0068db..52756de522 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2390,6 +2390,33 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
     return raw_co_create(&options, errp);
 }
 
+/**
+ * Co-routine function that erases a regular file.
+ */
+static int coroutine_fn raw_co_delete_file(BlockDriverState *bs,
+                                           Error **errp)
+{
+    struct stat st;
+    int ret;
+
+    if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) {
+        ret = -ENOENT;
+        error_setg_errno(errp, -ret, "%s is not a regular file",
+                         bs->filename);
+        goto done;
+    }
+
+    ret = unlink(bs->filename);
+    if (ret < 0) {
+        ret = -errno;
+        error_setg_errno(errp, -ret, "Error when deleting file %s",
+                         bs->filename);
+    }
+
+done:
+    return ret;
+}
+
 /*
  * Find allocation range in @bs around offset @start.
  * May change underlying file descriptor's file offset.
@@ -2942,6 +2969,7 @@ BlockDriver bdrv_file = {
     .bdrv_co_block_status = raw_co_block_status,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
     .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
+    .bdrv_co_delete_file = raw_co_delete_file,
 
     .bdrv_co_preadv         = raw_co_preadv,
     .bdrv_co_pwritev        = raw_co_pwritev,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0422acdf1c..a959ec2d1e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -314,6 +314,12 @@ struct BlockDriver {
      */
     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
 
+    /*
+     * Delete a local created file.
+     */
+    int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,
+                                            Error **errp);
+
     /*
      * Flushes all data that was already written to the OS all the way down to
      * the disk (for example file-posix.c calls fsync()).
-- 
2.21.0

[Qemu-devel] [PATCH v7 2/4] block.c: adding bdrv_delete_file

[Daniel Henrique Barboza]

Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
can be used in a way similar of the existing bdrv_create_file to
to clean up a created file.

The logic is also similar to what is already done in bdrv_create_file:
a qemu_coroutine is created if needed, a specialized function
bdrv_delete_co_entry is used to call the bdrv_co_delete_file
co-routine of the driver, if the driver implements it.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block.c               | 73 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 74 insertions(+)

diff --git a/block.c b/block.c
index 874a29a983..250c69ca7a 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,79 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
+typedef struct DeleteCo {
+    BlockDriver *drv;
+    BlockDriverState *bs;
+    int ret;
+    Error *err;
+} DeleteCo;
+
+static void coroutine_fn bdrv_delete_co_entry(void *opaque)
+{
+    Error *local_err = NULL;
+    DeleteCo *dco = opaque;
+    BlockDriver *drv = dco->bs->drv;
+
+    assert(dco->bs);
+
+    dco->ret = drv->bdrv_co_delete_file(dco->bs, &local_err);
+    error_propagate(&dco->err, local_err);
+}
+
+int bdrv_delete_file(BlockDriverState *bs, Error **errp)
+{
+    DeleteCo dco = {
+        .bs = bs,
+        .ret = NOT_DONE,
+        .err = NULL,
+    };
+    Coroutine *co;
+    int ret;
+
+    if (!bs) {
+        error_setg(errp, "Could not open image '%s' for erasing",
+                   bs->filename);
+        ret = -1;
+        goto out;
+    }
+
+    if (!bs->drv) {
+        error_setg(errp, "File '%s' has unknown format", bs->filename);
+        ret = -ENOMEDIUM;
+        goto out;
+    }
+
+    if (!bs->drv->bdrv_co_delete_file) {
+        error_setg(errp, "Driver '%s' does not support image delete",
+                   bs->drv->format_name);
+        ret = -ENOTSUP;
+        goto out;
+    }
+
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_delete_co_entry(&dco);
+    } else {
+        co = qemu_coroutine_create(bdrv_delete_co_entry, &dco);
+        qemu_coroutine_enter(co);
+        while (dco.ret == NOT_DONE) {
+            aio_poll(qemu_get_aio_context(), true);
+        }
+    }
+
+    ret = dco.ret;
+    if (ret < 0) {
+        if (dco.err) {
+            error_propagate(errp, dco.err);
+        } else {
+            error_setg_errno(errp, -ret, "Could not delete image");
+        }
+    }
+
+out:
+    return ret;
+}
+
 /**
  * Try to get @bs's logical and physical block size.
  * On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index 124ad40809..00fe8d6534 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -374,6 +374,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
 int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+int bdrv_delete_file(BlockDriverState *bs, Error **errp);
 
 
 typedef struct BdrvCheckResult {
-- 
2.21.0

[Qemu-devel] [PATCH v7 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:

$ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K

Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8

However, the created file '/var/tmp/pool_target/vol_1' is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.

The volume file is created inside block_crypto_co_create_opts_luks(), in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.

This patch changes block_crypto_co_create_opts_luks() to delete
'filename' in case of failure. A failure in this point means that
the volume is now truncated/corrupted, so even if 'filename' was an
existing volume before calling qemu-img, it is now unusable. Deleting
the file it is not much worse than leaving it in the filesystem in
this scenario, and we don't have to deal with checking the file
pre-existence in the code.

* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.

Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/crypto.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 7eb698774e..29496d247e 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -30,6 +30,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 #include "crypto.h"
 
 typedef struct BlockCrypto BlockCrypto;
@@ -596,9 +597,30 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
 
     ret = 0;
 fail:
+    /*
+     * If an error occurred, delete 'filename'. Even if the file existed
+     * beforehand, it has been truncated and corrupted in the process.
+     */
+    if (ret) {
+        Error *local_err;
+        int r_del = bdrv_delete_file(bs, &local_err);
+        /*
+         * ENOTSUP will happen if the block driver doesn't support
+         * 'bdrv_co_delete_file'. ENOENT will be fired by
+         * 'raw_co_delete_file' if the file doesn't exist. Both are
+         * predictable (we're not verifying if the driver supports
+         * file deletion or if the file was created), thus we
+         * shouldn't report this back to the user.
+         */
+        if ((r_del < 0) && (r_del != -ENOTSUP) && (r_del != -ENOENT)) {
+            error_reportf_err(local_err, "%s: ", bs->filename);
+        }
+    }
+
     bdrv_unref(bs);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
     qobject_unref(cryptoopts);
+
     return ret;
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH v7 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

[Daniel Henrique Barboza]

This patch adds a new test file, 257, to exercise the case where
qemu-img fails to complete for the LUKS format when a non-UTF8
secret is used.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/259.out | 11 +++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 79 insertions(+)
 create mode 100755 tests/qemu-iotests/259
 create mode 100644 tests/qemu-iotests/259.out

diff --git a/tests/qemu-iotests/259 b/tests/qemu-iotests/259
new file mode 100755
index 0000000000..cb362598b4
--- /dev/null
+++ b/tests/qemu-iotests/259
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret
+#
+# Copyright (C) 2019, IBM Corporation.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+TEST_IMAGE_FILE='vol.img'
+
+_cleanup()
+{
+  _cleanup_test_img
+  rm non_utf8_secret
+  rm -f $TEST_IMAGE_FILE
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt luks
+_supported_proto generic
+_unsupported_proto vxhs
+
+echo "== Create non-UTF8 secret =="
+echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret
+SECRET="secret,id=sec0,file=non_utf8_secret"
+
+echo "== Throws an error because of invalid UTF-8 secret =="
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M
+
+echo "== Image file should not exist after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+    exit 1
+fi
+
+echo "== Create a stub image file and run qemu-img again =="
+touch $TEST_IMAGE_FILE
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M
+
+echo "== Pre-existing image file should also be deleted after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+    exit 1
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/259.out b/tests/qemu-iotests/259.out
new file mode 100644
index 0000000000..6b0188111c
--- /dev/null
+++ b/tests/qemu-iotests/259.out
@@ -0,0 +1,11 @@
+QA output created by 259
+== Create non-UTF8 secret ==
+== Throws an error because of invalid UTF-8 secret ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Image file should not exist after the error ==
+== Create a stub image file and run qemu-img again ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Pre-existing image file should also be deleted after the error ==
+ *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..8f50bd91e5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -273,4 +273,5 @@
 256 rw quick
 257 rw
 258 rw quick
+259 rw img quick
 262 rw quick migration
-- 
2.21.0

Re: [PATCH v7 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

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

Ping

On 9/3/19 10:57 AM, Daniel Henrique Barboza wrote:
> Changes from previous version 6 [1]:
>
> - bdrv_delete_file() now uses BlockDriverState as a parameter rather
> than a filename string.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00139.html
>
> Daniel Henrique Barboza (4):
>    block: introducing 'bdrv_co_delete_file' interface
>    block.c: adding bdrv_delete_file
>    crypto.c: cleanup created file when block_crypto_co_create_opts_luks
>      fails
>    qemu-iotests: adding LUKS cleanup for non-UTF8 secret error
>
>   block.c                    | 73 ++++++++++++++++++++++++++++++++++++++
>   block/crypto.c             | 22 ++++++++++++
>   block/file-posix.c         | 28 +++++++++++++++
>   include/block/block.h      |  1 +
>   include/block/block_int.h  |  6 ++++
>   tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/259.out | 11 ++++++
>   tests/qemu-iotests/group   |  1 +
>   8 files changed, 209 insertions(+)
>   create mode 100755 tests/qemu-iotests/259
>   create mode 100644 tests/qemu-iotests/259.out
>


[-- Attachment #2: Type: text/html, Size: 1529 bytes --]

Re: [PATCH v7 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Ping

On 9/3/19 10:57 AM, Daniel Henrique Barboza wrote:
> Changes from previous version 6 [1]:
> 
> - bdrv_delete_file() now uses BlockDriverState as a parameter rather
> than a filename string.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg00139.html
> 
> Daniel Henrique Barboza (4):
>    block: introducing 'bdrv_co_delete_file' interface
>    block.c: adding bdrv_delete_file
>    crypto.c: cleanup created file when block_crypto_co_create_opts_luks
>      fails
>    qemu-iotests: adding LUKS cleanup for non-UTF8 secret error
> 
>   block.c                    | 73 ++++++++++++++++++++++++++++++++++++++
>   block/crypto.c             | 22 ++++++++++++
>   block/file-posix.c         | 28 +++++++++++++++
>   include/block/block.h      |  1 +
>   include/block/block_int.h  |  6 ++++
>   tests/qemu-iotests/259     | 67 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/259.out | 11 ++++++
>   tests/qemu-iotests/group   |  1 +
>   8 files changed, 209 insertions(+)
>   create mode 100755 tests/qemu-iotests/259
>   create mode 100644 tests/qemu-iotests/259.out
> 

Re: [PATCH v7 1/4] block: introducing 'bdrv_co_delete_file' interface

[Kevin Wolf]

Am 03.09.2019 um 15:57 hat Daniel Henrique Barboza geschrieben:
> Adding to Block Drivers the capability of being able to clean up
> its created files can be useful in certain situations. For the
> LUKS driver, for instance, a failure in one of its authentication
> steps can leave files in the host that weren't there before.
> 
> This patch adds the 'bdrv_co_delete_file' interface to block
> drivers and add it to the 'file' driver in file-posix.c.The

Missing space between "file-posix.c.The"

> implementation is given by 'raw_co_delete_file'.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  block/file-posix.c        | 28 ++++++++++++++++++++++++++++
>  include/block/block_int.h |  6 ++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index fbeb0068db..52756de522 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2390,6 +2390,33 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
>      return raw_co_create(&options, errp);
>  }
>  
> +/**
> + * Co-routine function that erases a regular file.
> + */

We don't have function comments for other BdrvDriver callbacks. It may
be appropriate to have a comment when there is something special going
on here that differs from a normal implementation. But this specific
comment is even redundant with the function name, so I think we don't
need it.

> +static int coroutine_fn raw_co_delete_file(BlockDriverState *bs,
> +                                           Error **errp)
> +{
> +    struct stat st;
> +    int ret;
> +
> +    if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) {
> +        ret = -ENOENT;
> +        error_setg_errno(errp, -ret, "%s is not a regular file",
> +                         bs->filename);
> +        goto done;

There is no cleanup code, so a direct return -ENOENT would be simpler.

> +    }
> +
> +    ret = unlink(bs->filename);
> +    if (ret < 0) {
> +        ret = -errno;
> +        error_setg_errno(errp, -ret, "Error when deleting file %s",
> +                         bs->filename);
> +    }
> +
> +done:
> +    return ret;
> +}
> +
>  /*
>   * Find allocation range in @bs around offset @start.
>   * May change underlying file descriptor's file offset.
> @@ -2942,6 +2969,7 @@ BlockDriver bdrv_file = {
>      .bdrv_co_block_status = raw_co_block_status,
>      .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>      .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
> +    .bdrv_co_delete_file = raw_co_delete_file,
>  
>      .bdrv_co_preadv         = raw_co_preadv,
>      .bdrv_co_pwritev        = raw_co_pwritev,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 0422acdf1c..a959ec2d1e 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -314,6 +314,12 @@ struct BlockDriver {
>       */
>      int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
>  
> +    /*
> +     * Delete a local created file.
> +     */

No reason to restrict this to "local", even if for now file-posix is the
only driver that implements it. This can change later.

The comment fits in a single line.

> +    int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,
> +                                            Error **errp);
> +

Kevin

Re: [PATCH v7 2/4] block.c: adding bdrv_delete_file

[Kevin Wolf]

Am 03.09.2019 um 15:57 hat Daniel Henrique Barboza geschrieben:
> Using the new 'bdrv_co_delete_file' interface, bdrv_delete_file
> can be used in a way similar of the existing bdrv_create_file to
> to clean up a created file.
> 
> The logic is also similar to what is already done in bdrv_create_file:
> a qemu_coroutine is created if needed, a specialized function
> bdrv_delete_co_entry is used to call the bdrv_co_delete_file
> co-routine of the driver, if the driver implements it.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

The only caller of bdrv_delete_file() is in coroutine context, so I
think this patch can be made much simpler by turning it into a pure
coroutine function bdrv_co_delete_file().

> diff --git a/block.c b/block.c
> index 874a29a983..250c69ca7a 100644
> --- a/block.c
> +++ b/block.c
> @@ -548,6 +548,79 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>      return ret;
>  }
>  
> +typedef struct DeleteCo {
> +    BlockDriver *drv;
> +    BlockDriverState *bs;
> +    int ret;
> +    Error *err;
> +} DeleteCo;
> +
> +static void coroutine_fn bdrv_delete_co_entry(void *opaque)
> +{
> +    Error *local_err = NULL;
> +    DeleteCo *dco = opaque;
> +    BlockDriver *drv = dco->bs->drv;
> +
> +    assert(dco->bs);
> +
> +    dco->ret = drv->bdrv_co_delete_file(dco->bs, &local_err);
> +    error_propagate(&dco->err, local_err);
> +}
> +
> +int bdrv_delete_file(BlockDriverState *bs, Error **errp)
> +{
> +    DeleteCo dco = {
> +        .bs = bs,
> +        .ret = NOT_DONE,
> +        .err = NULL,
> +    };
> +    Coroutine *co;
> +    int ret;
> +
> +    if (!bs) {
> +        error_setg(errp, "Could not open image '%s' for erasing",
> +                   bs->filename);
> +        ret = -1;

For a function returning 0/-errno, -1 is not a good return code because
it could be any error (or an undefined one). On Linux, this happens to
be -EPERM.

> +        goto out;
> +    }

We're not trying to open it here, so the error message is odd.

I think the caller should make sure that bs != NULL.

> +    if (!bs->drv) {
> +        error_setg(errp, "File '%s' has unknown format", bs->filename);
> +        ret = -ENOMEDIUM;
> +        goto out;
> +    }

bs->drv means that file isn't open. It has nothing to do with an unknown
format. Maybe you can combine both cases into one with an error message
"block node is not opened".

> +    if (!bs->drv->bdrv_co_delete_file) {
> +        error_setg(errp, "Driver '%s' does not support image delete",

s/delete/deletion/

> +                   bs->drv->format_name);
> +        ret = -ENOTSUP;
> +        goto out;
> +    }
> +
> +    if (qemu_in_coroutine()) {
> +        /* Fast-path if already in coroutine context */
> +        bdrv_delete_co_entry(&dco);
> +    } else {
> +        co = qemu_coroutine_create(bdrv_delete_co_entry, &dco);
> +        qemu_coroutine_enter(co);
> +        while (dco.ret == NOT_DONE) {
> +            aio_poll(qemu_get_aio_context(), true);
> +        }
> +    }

We don't really want to have this kind of different behaviour for
coroutine and non-coroutine contexts. It only exists for compatibility
reasons in other places (when we already had callers that didn't know
whether they were run inside a coroutine or not).

With a bdrv_co_delete_file(), it will go away.

> +
> +    ret = dco.ret;
> +    if (ret < 0) {
> +        if (dco.err) {
> +            error_propagate(errp, dco.err);
> +        } else {
> +            error_setg_errno(errp, -ret, "Could not delete image");
> +        }
> +    }
> +
> +out:
> +    return ret;

No cleanup code, so all "ret = ...; goto out;" instances above could be
replaced with a direct return.

Kevin

Re: [PATCH v7 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails

[Kevin Wolf]

Am 03.09.2019 um 15:57 hat Daniel Henrique Barboza geschrieben:
> When using a non-UTF8 secret to create a volume using qemu-img, the
> following error happens:
> 
> $ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K
> 
> Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0
> qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8
> 
> However, the created file '/var/tmp/pool_target/vol_1' is left behind in the
> file system after the failure. This behavior can be observed when creating
> the volume using Libvirt, via 'virsh vol-create', and then getting "volume
> target path already exist" errors when trying to re-create the volume.
> 
> The volume file is created inside block_crypto_co_create_opts_luks(), in
> block/crypto.c. If the bdrv_create_file() call is successful but any
> succeeding step fails*, the existing 'fail' label does not take into
> account the created file, leaving it behind.
> 
> This patch changes block_crypto_co_create_opts_luks() to delete
> 'filename' in case of failure. A failure in this point means that
> the volume is now truncated/corrupted, so even if 'filename' was an
> existing volume before calling qemu-img, it is now unusable. Deleting
> the file it is not much worse than leaving it in the filesystem in
> this scenario, and we don't have to deal with checking the file
> pre-existence in the code.
> 
> * in our case, block_crypto_co_create_generic calls qcrypto_block_create,
> which calls qcrypto_block_luks_create, and this function fails when
> calling qcrypto_secret_lookup_as_utf8.
> 
> Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com>
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  block/crypto.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 7eb698774e..29496d247e 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -30,6 +30,7 @@
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> +#include "qemu/cutils.h"
>  #include "crypto.h"
>  
>  typedef struct BlockCrypto BlockCrypto;
> @@ -596,9 +597,30 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
>  
>      ret = 0;
>  fail:
> +    /*
> +     * If an error occurred, delete 'filename'. Even if the file existed
> +     * beforehand, it has been truncated and corrupted in the process.
> +     */
> +    if (ret) {

if (ret && bs)

There are error paths before creating and opening the image, and trying
to delete bs can't succeed then.

> +        Error *local_err;

This shadows the function-scope local_err. Worse, it isn't even
initialised to NULL here.

> +        int r_del = bdrv_delete_file(bs, &local_err);
> +        /*
> +         * ENOTSUP will happen if the block driver doesn't support
> +         * 'bdrv_co_delete_file'. ENOENT will be fired by
> +         * 'raw_co_delete_file' if the file doesn't exist. Both are
> +         * predictable (we're not verifying if the driver supports
> +         * file deletion or if the file was created), thus we
> +         * shouldn't report this back to the user.
> +         */

When you check bs above, you don't have to worry about -ENOENT any more
here.

> +        if ((r_del < 0) && (r_del != -ENOTSUP) && (r_del != -ENOENT)) {
> +            error_reportf_err(local_err, "%s: ", bs->filename);

Hm... This will end up on stderr instead of being reported as an error.
Okay because we already have an error about creating the image, which is
more important to report.

However, don't prepend bs->filename here. You already included the
filename in the file-posix driver, so you'd print the filename twice in
the same message.

> +        }
> +    }
> +
>      bdrv_unref(bs);
>      qapi_free_QCryptoBlockCreateOptions(create_opts);
>      qobject_unref(cryptoopts);
> +
>      return ret;
>  }

This additional empty line looks unrelated.

Kevin