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

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

[Daniel Henrique Barboza]

Changes from previous version 4 [1], all suggested by Kevin
Wolf:

- changed bdrv_co_delete_file interface to receive a BlockDriverState
instead of a file name;
- delete created files even if pre-existent, since they'll be
truncated/corrupted if the process fails anyway

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg06435.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                    | 77 ++++++++++++++++++++++++++++++++++++++
 block/crypto.c             | 20 ++++++++++
 block/file-posix.c         | 28 ++++++++++++++
 include/block/block.h      |  1 +
 include/block/block_int.h  |  6 +++
 tests/qemu-iotests/257     | 67 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/257.out | 11 ++++++
 tests/qemu-iotests/group   |  1 +
 8 files changed, 211 insertions(+)
 create mode 100755 tests/qemu-iotests/257
 create mode 100644 tests/qemu-iotests/257.out

-- 
2.21.0

[Qemu-devel] [PATCH v5 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 4479cc7ab4..278952d5a2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2376,6 +2376,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.
@@ -2927,6 +2954,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 3aa1e832a8..4cb3232dc4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -309,6 +309,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 v5 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               | 77 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 78 insertions(+)

diff --git a/block.c b/block.c
index cbd8da5f3b..1e20250627 100644
--- a/block.c
+++ b/block.c
@@ -547,6 +547,83 @@ 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;
+
+    assert(dco->bs);
+
+    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
+    error_propagate(&dco->err, local_err);
+}
+
+int bdrv_delete_file(const char *filename, Error **errp)
+{
+    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
+    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
+                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
+    DeleteCo dco = {
+        .drv = drv,
+        .bs = bs,
+        .ret = NOT_DONE,
+        .err = NULL,
+    };
+    Coroutine *co;
+    int ret;
+
+    if (!drv) {
+        error_setg(errp, "File '%s' has unknown format", filename);
+        ret = -ENOENT;
+        goto out;
+    }
+
+    if (!drv->bdrv_co_delete_file) {
+        error_setg(errp, "Driver '%s' does not support image delete",
+                   drv->format_name);
+        ret = -ENOTSUP;
+        goto out;
+    }
+
+    if (!bs) {
+        error_setg(errp, "Could not open image '%s' for erasing",
+                   filename);
+        ret = 1;
+        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:
+    bdrv_unref(bs);
+    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 50a07c1c33..5e83532364 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -369,6 +369,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(const char *filename, Error **errp);
 
 
 typedef struct BdrvCheckResult {
-- 
2.21.0

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

[John Snow]

On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
> 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               | 77 +++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  1 +
>  2 files changed, 78 insertions(+)
> 
> diff --git a/block.c b/block.c
> index cbd8da5f3b..1e20250627 100644
> --- a/block.c
> +++ b/block.c
> @@ -547,6 +547,83 @@ 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;
> +
> +    assert(dco->bs);
> +
> +    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
> +    error_propagate(&dco->err, local_err);
> +}
> +
> +int bdrv_delete_file(const char *filename, Error **errp)
> +{
> +    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
> +    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
> +                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
> +    DeleteCo dco = {
> +        .drv = drv,
> +        .bs = bs,
> +        .ret = NOT_DONE,
> +        .err = NULL,
> +    };
> +    Coroutine *co;
> +    int ret;
> +
> +    if (!drv) {
> +        error_setg(errp, "File '%s' has unknown format", filename);
> +        ret = -ENOENT;
> +        goto out;
> +    }
> +

I was going to say that ENOENT is a weird error here, but I see it used
for !drv a few other places in block.c too, alongside EINVAL and
ENOMEDIUM. ENOMEDIUM loks like the most popular.

> +    if (!drv->bdrv_co_delete_file) {
> +        error_setg(errp, "Driver '%s' does not support image delete",
> +                   drv->format_name);
> +        ret = -ENOTSUP;
> +        goto out;
> +    }
> +
> +    if (!bs) {
> +        error_setg(errp, "Could not open image '%s' for erasing",
> +                   filename);
> +        ret = 1;

Please keep all errors negative (or at least consistent within a function).


I'm also wondering if we want a version of delete that doesn't try to
open a file directly -- i.e. a version that exists like this:

bdrv_co_delete_file(BlockDriverState *bs, Error **errp);

That simply dispatches based on bs->drv to the correct routine.

Then, you are free to have bdrv_delete_file handle the open (and let the
opening figure out what driver it needs), and just hand off the bds to
bdrv_co_delete_file.

I'm not the authority for block.c, though, so maaaybe I'm giving you bad
advice here. Kevin's away on PTO for a bit and gave you advice most
recently, so I might try to gently ask him for more feedback next week.

> +        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:
> +    bdrv_unref(bs);
> +    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 50a07c1c33..5e83532364 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -369,6 +369,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(const char *filename, Error **errp);
>  
>  
>  typedef struct BdrvCheckResult {
> 

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

[Daniel Henrique Barboza]

On 8/28/19 11:07 PM, John Snow wrote:
>
> On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
>> 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               | 77 +++++++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h |  1 +
>>   2 files changed, 78 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index cbd8da5f3b..1e20250627 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -547,6 +547,83 @@ 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;
>> +
>> +    assert(dco->bs);
>> +
>> +    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
>> +    error_propagate(&dco->err, local_err);
>> +}
>> +
>> +int bdrv_delete_file(const char *filename, Error **errp)
>> +{
>> +    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
>> +    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
>> +                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
>> +    DeleteCo dco = {
>> +        .drv = drv,
>> +        .bs = bs,
>> +        .ret = NOT_DONE,
>> +        .err = NULL,
>> +    };
>> +    Coroutine *co;
>> +    int ret;
>> +
>> +    if (!drv) {
>> +        error_setg(errp, "File '%s' has unknown format", filename);
>> +        ret = -ENOENT;
>> +        goto out;
>> +    }
>> +
> I was going to say that ENOENT is a weird error here, but I see it used
> for !drv a few other places in block.c too, alongside EINVAL and
> ENOMEDIUM. ENOMEDIUM loks like the most popular.

Didn't spend too much time thinking about it. I copied the same behavior 
from
bdrv_create_file:

---------

int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
{
     (...)

     drv = bdrv_find_protocol(filename, true, errp);
     if (drv == NULL) {
         return -ENOENT;
     }
-----

I can change to ENOMEDIUM if it's indeed more informative than ENOENT.


>
>> +    if (!drv->bdrv_co_delete_file) {
>> +        error_setg(errp, "Driver '%s' does not support image delete",
>> +                   drv->format_name);
>> +        ret = -ENOTSUP;
>> +        goto out;
>> +    }
>> +
>> +    if (!bs) {
>> +        error_setg(errp, "Could not open image '%s' for erasing",
>> +                   filename);
>> +        ret = 1;
> Please keep all errors negative (or at least consistent within a function).

Got it. I'll fix it in the re-spin.


>
>
> I'm also wondering if we want a version of delete that doesn't try to
> open a file directly -- i.e. a version that exists like this:
>
> bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
>
> That simply dispatches based on bs->drv to the correct routine.
>
> Then, you are free to have bdrv_delete_file handle the open (and let the
> opening figure out what driver it needs), and just hand off the bds to
> bdrv_co_delete_file.
>
> I'm not the authority for block.c, though, so maaaybe I'm giving you bad
> advice here. Kevin's away on PTO for a bit and gave you advice most
> recently, so I might try to gently ask him for more feedback next week.

I appreciate. I'm not acquainted with the block code at all - I'm playing
by ear since the first version. Any tip is appreciated :)


Thanks,


DHB

>
>> +        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:
>> +    bdrv_unref(bs);
>> +    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 50a07c1c33..5e83532364 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -369,6 +369,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(const char *filename, Error **errp);
>>   
>>   
>>   typedef struct BdrvCheckResult {
>>

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

[Kevin Wolf]

Am 29.08.2019 um 04:07 hat John Snow geschrieben:
> 
> 
> On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
> > 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               | 77 +++++++++++++++++++++++++++++++++++++++++++
> >  include/block/block.h |  1 +
> >  2 files changed, 78 insertions(+)
> > 
> > diff --git a/block.c b/block.c
> > index cbd8da5f3b..1e20250627 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -547,6 +547,83 @@ 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;
> > +
> > +    assert(dco->bs);
> > +
> > +    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
> > +    error_propagate(&dco->err, local_err);
> > +}
> > +
> > +int bdrv_delete_file(const char *filename, Error **errp)
> > +{
> > +    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
> > +    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
> > +                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
> > +    DeleteCo dco = {
> > +        .drv = drv,
> > +        .bs = bs,
> > +        .ret = NOT_DONE,
> > +        .err = NULL,
> > +    };
> > +    Coroutine *co;
> > +    int ret;
> > +
> > +    if (!drv) {
> > +        error_setg(errp, "File '%s' has unknown format", filename);
> > +        ret = -ENOENT;
> > +        goto out;
> > +    }
> > +
> 
> I was going to say that ENOENT is a weird error here, but I see it used
> for !drv a few other places in block.c too, alongside EINVAL and
> ENOMEDIUM. ENOMEDIUM loks like the most popular.
> 
> > +    if (!drv->bdrv_co_delete_file) {
> > +        error_setg(errp, "Driver '%s' does not support image delete",
> > +                   drv->format_name);
> > +        ret = -ENOTSUP;
> > +        goto out;
> > +    }
> > +
> > +    if (!bs) {
> > +        error_setg(errp, "Could not open image '%s' for erasing",
> > +                   filename);
> > +        ret = 1;
> 
> Please keep all errors negative (or at least consistent within a function).
> 
> 
> I'm also wondering if we want a version of delete that doesn't try to
> open a file directly -- i.e. a version that exists like this:
> 
> bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
> 
> That simply dispatches based on bs->drv to the correct routine.
> 
> Then, you are free to have bdrv_delete_file handle the open (and let the
> opening figure out what driver it needs), and just hand off the bds to
> bdrv_co_delete_file.
> 
> I'm not the authority for block.c, though, so maaaybe I'm giving you bad
> advice here. Kevin's away on PTO for a bit and gave you advice most
> recently, so I might try to gently ask him for more feedback next week.

Yes, this was definitely the idea I had in mind when I suggested that
bdrv_co_delete_file() should take a BDS.

And I think the callers that want to call this function (for failures
during image creation) all already have a BDS pointer, so nobody will
actually need the additional open.

const char *filename only works for the local filesystem (and even then
I think not for all filenames) and some URLs, so this is not what we
want to have in a public interface to identify an image file.

Kevin

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

[Daniel Henrique Barboza]

On 9/3/19 6:22 AM, Kevin Wolf wrote:
> Am 29.08.2019 um 04:07 hat John Snow geschrieben:
>>
>> On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
>>> 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               | 77 +++++++++++++++++++++++++++++++++++++++++++
>>>   include/block/block.h |  1 +
>>>   2 files changed, 78 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index cbd8da5f3b..1e20250627 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -547,6 +547,83 @@ 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;
>>> +
>>> +    assert(dco->bs);
>>> +
>>> +    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
>>> +    error_propagate(&dco->err, local_err);
>>> +}
>>> +
>>> +int bdrv_delete_file(const char *filename, Error **errp)
>>> +{
>>> +    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
>>> +    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
>>> +                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
>>> +    DeleteCo dco = {
>>> +        .drv = drv,
>>> +        .bs = bs,
>>> +        .ret = NOT_DONE,
>>> +        .err = NULL,
>>> +    };
>>> +    Coroutine *co;
>>> +    int ret;
>>> +
>>> +    if (!drv) {
>>> +        error_setg(errp, "File '%s' has unknown format", filename);
>>> +        ret = -ENOENT;
>>> +        goto out;
>>> +    }
>>> +
>> I was going to say that ENOENT is a weird error here, but I see it used
>> for !drv a few other places in block.c too, alongside EINVAL and
>> ENOMEDIUM. ENOMEDIUM loks like the most popular.
>>
>>> +    if (!drv->bdrv_co_delete_file) {
>>> +        error_setg(errp, "Driver '%s' does not support image delete",
>>> +                   drv->format_name);
>>> +        ret = -ENOTSUP;
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (!bs) {
>>> +        error_setg(errp, "Could not open image '%s' for erasing",
>>> +                   filename);
>>> +        ret = 1;
>> Please keep all errors negative (or at least consistent within a function).
>>
>>
>> I'm also wondering if we want a version of delete that doesn't try to
>> open a file directly -- i.e. a version that exists like this:
>>
>> bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
>>
>> That simply dispatches based on bs->drv to the correct routine.
>>
>> Then, you are free to have bdrv_delete_file handle the open (and let the
>> opening figure out what driver it needs), and just hand off the bds to
>> bdrv_co_delete_file.
>>
>> I'm not the authority for block.c, though, so maaaybe I'm giving you bad
>> advice here. Kevin's away on PTO for a bit and gave you advice most
>> recently, so I might try to gently ask him for more feedback next week.
> Yes, this was definitely the idea I had in mind when I suggested that
> bdrv_co_delete_file() should take a BDS.
>
> And I think the callers that want to call this function (for failures
> during image creation) all already have a BDS pointer, so nobody will
> actually need the additional open.
>
> const char *filename only works for the local filesystem (and even then
> I think not for all filenames) and some URLs, so this is not what we
> want to have in a public interface to identify an image file.

Hmpf, I understood your idead wrong in the v4 review and ended up
changing the co_routine (bdrv_co_delete_file) to use the BlockDriverState
instead of the public interface bdrv_delete_file that will be called 
inside crypto.c.

I'll respin it again with this change. Thanks for clarifying!



>
> Kevin

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

[Kevin Wolf]

Am 03.09.2019 um 11:55 hat Daniel Henrique Barboza geschrieben:
> 
> 
> On 9/3/19 6:22 AM, Kevin Wolf wrote:
> > Am 29.08.2019 um 04:07 hat John Snow geschrieben:
> > > 
> > > On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
> > > > 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               | 77 +++++++++++++++++++++++++++++++++++++++++++
> > > >   include/block/block.h |  1 +
> > > >   2 files changed, 78 insertions(+)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index cbd8da5f3b..1e20250627 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -547,6 +547,83 @@ 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;
> > > > +
> > > > +    assert(dco->bs);
> > > > +
> > > > +    dco->ret = dco->drv->bdrv_co_delete_file(dco->bs, &local_err);
> > > > +    error_propagate(&dco->err, local_err);
> > > > +}
> > > > +
> > > > +int bdrv_delete_file(const char *filename, Error **errp)
> > > > +{
> > > > +    BlockDriver *drv = bdrv_find_protocol(filename, true, NULL);
> > > > +    BlockDriverState *bs = bdrv_open(filename, NULL, NULL,
> > > > +                                     BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL);
> > > > +    DeleteCo dco = {
> > > > +        .drv = drv,
> > > > +        .bs = bs,
> > > > +        .ret = NOT_DONE,
> > > > +        .err = NULL,
> > > > +    };
> > > > +    Coroutine *co;
> > > > +    int ret;
> > > > +
> > > > +    if (!drv) {
> > > > +        error_setg(errp, "File '%s' has unknown format", filename);
> > > > +        ret = -ENOENT;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > I was going to say that ENOENT is a weird error here, but I see it used
> > > for !drv a few other places in block.c too, alongside EINVAL and
> > > ENOMEDIUM. ENOMEDIUM loks like the most popular.
> > > 
> > > > +    if (!drv->bdrv_co_delete_file) {
> > > > +        error_setg(errp, "Driver '%s' does not support image delete",
> > > > +                   drv->format_name);
> > > > +        ret = -ENOTSUP;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    if (!bs) {
> > > > +        error_setg(errp, "Could not open image '%s' for erasing",
> > > > +                   filename);
> > > > +        ret = 1;
> > > Please keep all errors negative (or at least consistent within a function).
> > > 
> > > 
> > > I'm also wondering if we want a version of delete that doesn't try to
> > > open a file directly -- i.e. a version that exists like this:
> > > 
> > > bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
> > > 
> > > That simply dispatches based on bs->drv to the correct routine.
> > > 
> > > Then, you are free to have bdrv_delete_file handle the open (and let the
> > > opening figure out what driver it needs), and just hand off the bds to
> > > bdrv_co_delete_file.
> > > 
> > > I'm not the authority for block.c, though, so maaaybe I'm giving you bad
> > > advice here. Kevin's away on PTO for a bit and gave you advice most
> > > recently, so I might try to gently ask him for more feedback next week.
> > Yes, this was definitely the idea I had in mind when I suggested that
> > bdrv_co_delete_file() should take a BDS.
> > 
> > And I think the callers that want to call this function (for failures
> > during image creation) all already have a BDS pointer, so nobody will
> > actually need the additional open.
> > 
> > const char *filename only works for the local filesystem (and even then
> > I think not for all filenames) and some URLs, so this is not what we
> > want to have in a public interface to identify an image file.
> 
> Hmpf, I understood your idead wrong in the v4 review and ended up
> changing the co_routine (bdrv_co_delete_file) to use the BlockDriverState
> instead of the public interface bdrv_delete_file that will be called inside
> crypto.c.
> 
> I'll respin it again with this change. Thanks for clarifying!

Yes, I see that I only really commented on the BlockDriver callback, so
it wasn't very clear what I actually meant. Sorry for that.

Kevin

[Qemu-devel] [PATCH v5 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 8237424ae6..8ffca81df6 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;
@@ -575,6 +576,25 @@ fail:
     bdrv_unref(bs);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
     qobject_unref(cryptoopts);
+
+    /*
+     * If an error occurred, delete the file. 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(filename, &local_err);
+        /*
+         * ENOTSUP will happen if the block driver doesn't support
+         * 'bdrv_co_delete_file'. ENOENT will happen if the file
+         * doesn't exist. Both are predictable and shouldn't be
+         * reported back to the user.
+         */
+        if ((r_del < 0) && (r_del != -ENOTSUP) && (r_del != -ENOENT)) {
+            error_reportf_err(local_err, "%s: ", bs->filename);
+        }
+    }
+
     return ret;
 }
 
-- 
2.21.0

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

[John Snow]

On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
> 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 | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 8237424ae6..8ffca81df6 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;
> @@ -575,6 +576,25 @@ fail:
>      bdrv_unref(bs);
>      qapi_free_QCryptoBlockCreateOptions(create_opts);
>      qobject_unref(cryptoopts);
> +
> +    /*
> +     * If an error occurred, delete the file. 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(filename, &local_err);
> +        /*
> +         * ENOTSUP will happen if the block driver doesn't support
> +         * 'bdrv_co_delete_file'. ENOENT will happen if the file
> +         * doesn't exist. Both are predictable and shouldn't be
> +         * reported back to the user.
> +         */

Hm, actually, didn't you use ENOENT to mean that we couldn't figure out
which driver to use?

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

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

[Daniel Henrique Barboza]

On 8/28/19 11:10 PM, John Snow wrote:
>
> On 8/7/19 10:21 AM, Daniel Henrique Barboza wrote:
>> 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 | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/block/crypto.c b/block/crypto.c
>> index 8237424ae6..8ffca81df6 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;
>> @@ -575,6 +576,25 @@ fail:
>>       bdrv_unref(bs);
>>       qapi_free_QCryptoBlockCreateOptions(create_opts);
>>       qobject_unref(cryptoopts);
>> +
>> +    /*
>> +     * If an error occurred, delete the file. 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(filename, &local_err);
>> +        /*
>> +         * ENOTSUP will happen if the block driver doesn't support
>> +         * 'bdrv_co_delete_file'. ENOENT will happen if the file
>> +         * doesn't exist. Both are predictable and shouldn't be
>> +         * reported back to the user.
>> +         */
> Hm, actually, didn't you use ENOENT to mean that we couldn't figure out
> which driver to use?


True. In this context though I am referring to the co_routine function
that deletes the file:

-------
(file-posix.c)
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;
     }
(...)
-----


I'll make it clearer in the comment where ENOENT is coming from.

(In fact, this is a good reason to change the !drv error in patch 2 from
ENOENT to ENOMEDIUM ....)







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

[Qemu-devel] [PATCH v5 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/257     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/257.out | 11 +++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 79 insertions(+)
 create mode 100755 tests/qemu-iotests/257
 create mode 100644 tests/qemu-iotests/257.out

diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
new file mode 100755
index 0000000000..cb362598b4
--- /dev/null
+++ b/tests/qemu-iotests/257
@@ -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/257.out b/tests/qemu-iotests/257.out
new file mode 100644
index 0000000000..3803933571
--- /dev/null
+++ b/tests/qemu-iotests/257.out
@@ -0,0 +1,11 @@
+QA output created by 257
+== 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 f13e5f2e23..ae7bb89642 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -271,3 +271,4 @@
 254 rw backing quick
 255 rw quick
 256 rw quick
+257 rw auto quick
-- 
2.21.0