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

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

[Daniel Henrique Barboza]

Changes from previous version [1]:
- added an extra patch including a new qemu-iotest to exercise the fix


[1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07159.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                    | 82 ++++++++++++++++++++++++++++++++++++++
 block/crypto.c             | 31 ++++++++++++++
 block/file-posix.c         | 28 +++++++++++++
 include/block/block.h      |  3 ++
 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, 229 insertions(+)
 create mode 100755 tests/qemu-iotests/257
 create mode 100644 tests/qemu-iotests/257.out

-- 
2.20.1

[Qemu-devel] [PATCH v4 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'. The helper
'bdrv_path_is_regular_file' is being used only in raw_co_delete_file
at this moment, but it will be used inside LUKS in a later patch.
Foreseeing this future use, let's put it in block.c and make it
public.

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

diff --git a/block.c b/block.c
index c139540f2b..6e2b0f528d 100644
--- a/block.c
+++ b/block.c
@@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
 #endif
 }
 
+/**
+ * Helper that checks if a given string represents a regular
+ * local file.
+ */
+bool bdrv_path_is_regular_file(const char *path)
+{
+    struct stat st;
+
+    return (stat(path, &st) == 0) && S_ISREG(st.st_mode);
+}
+
 /*
  * Detect host devices. By convention, /dev/cdrom[N] is always
  * recognized as a host CDROM.
diff --git a/block/file-posix.c b/block/file-posix.c
index ab05b51a66..c8a0b109c2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2374,6 +2374,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(const char *filename,
+                                           Error **errp)
+{
+    int ret;
+
+    /* Skip file: protocol prefix */
+    strstart(filename, "file:", &filename);
+
+    if (!bdrv_path_is_regular_file(filename)) {
+        ret = -ENOENT;
+        error_setg_errno(errp, -ret, "%s is not a regular file", filename);
+        goto done;
+    }
+
+    ret = unlink(filename);
+    if (ret < 0) {
+        ret = -errno;
+        error_setg_errno(errp, -ret, "Error when deleting file %s", filename);
+    }
+
+done:
+    return ret;
+}
+
 /*
  * Find allocation range in @bs around offset @start.
  * May change underlying file descriptor's file offset.
@@ -2925,6 +2952,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.h b/include/block/block.h
index f9415ed740..d287eaa9a6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -370,6 +370,7 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
 
+bool bdrv_path_is_regular_file(const char *path);
 
 typedef struct BdrvCheckResult {
     int corruptions;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d6415b53c1..6d4135ff54 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)(const char *filename,
+                                            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.20.1

[Qemu-devel] [PATCH v4 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               | 71 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  2 ++
 2 files changed, 73 insertions(+)

diff --git a/block.c b/block.c
index 6e2b0f528d..11675cfcee 100644
--- a/block.c
+++ b/block.c
@@ -547,6 +547,77 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
+static void coroutine_fn bdrv_delete_co_entry(void *opaque)
+{
+    Error *local_err = NULL;
+    int ret;
+
+    CreateCo *cco = opaque;
+    assert(cco->drv);
+
+    ret = cco->drv->bdrv_co_delete_file(cco->filename, &local_err);
+    error_propagate(&cco->err, local_err);
+    cco->ret = ret;
+}
+
+int bdrv_delete_file(const char *filename, Error **errp)
+{
+
+    BlockDriver *drv = bdrv_find_protocol(filename, true, errp);
+    CreateCo cco = {
+        .drv = drv,
+        .filename = g_strdup(filename),
+        .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 (!drv->bdrv_co_delete_file) {
+        error_setg(errp, "Driver '%s' does not support image delete",
+                   drv->format_name);
+        ret = -ENOTSUP;
+        goto out;
+    }
+
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_delete_co_entry(&cco);
+    } else {
+        co = qemu_coroutine_create(bdrv_delete_co_entry, &cco);
+        qemu_coroutine_enter(co);
+        while (cco.ret == NOT_DONE) {
+            aio_poll(qemu_get_aio_context(), true);
+        }
+    }
+
+    ret = cco.ret;
+    if (ret < 0) {
+        if (cco.err) {
+            error_propagate(errp, cco.err);
+        } else {
+            error_setg_errno(errp, -ret, "Could not delete image");
+        }
+    }
+
+out:
+    g_free(cco.filename);
+    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 d287eaa9a6..5747f2a060 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -371,6 +371,8 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
 
 bool bdrv_path_is_regular_file(const char *path);
+int bdrv_delete_file(const char *filename, Error **errp);
+
 
 typedef struct BdrvCheckResult {
     int corruptions;
-- 
2.20.1

[Qemu-devel] [PATCH v4 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 check if @filename
is an existing file before bdrv_create_file is called. In case of failure,
if @filename didn't exist before, check again for its existence and,
if affirmative, erase it by calling bdrv_delete_file.

* 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 | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 8237424ae6..146f3eb721 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;
@@ -535,6 +536,8 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
     BlockDriverState *bs = NULL;
     QDict *cryptoopts;
     int64_t size;
+    const char *path;
+    bool file_already_existed = false;
     int ret;
 
     /* Parse options */
@@ -551,6 +554,15 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
         goto fail;
     }
 
+    /*
+     * Check if 'filename' represents a local file that already
+     * exists in the file system prior to bdrv_create_file. Strip
+     * the leading 'file:' from the filename if it exists.
+     */
+    path = filename;
+    strstart(path, "file:", &path);
+    file_already_existed = bdrv_path_is_regular_file(path);
+
     /* Create protocol layer */
     ret = bdrv_create_file(filename, opts, errp);
     if (ret < 0) {
@@ -575,6 +587,25 @@ fail:
     bdrv_unref(bs);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
     qobject_unref(cryptoopts);
+
+    /*
+     * If an error occurred and we ended up creating a bogus
+     * 'filename' file, delete it
+     */
+    if (ret && !file_already_existed && bdrv_path_is_regular_file(path)) {
+        Error *local_err;
+        int r_del = bdrv_delete_file(path, &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: ", path);
+        }
+    }
+
     return ret;
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v4 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. If using an existing image file, do not erase it.
If the file was created by the failed qemu-img call, clean it up.

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..2402217619
--- /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 can not 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..5efeb1ed29
--- /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 can not be deleted after the error ==
+ *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b34c8e3c0c..00fbfefc8e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -269,3 +269,4 @@
 254 rw auto backing quick
 255 rw auto quick
 256 rw auto quick
+257 rw auto quick
-- 
2.20.1

Re: [Qemu-devel] [PATCH v4 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Ping

On 6/28/19 4:45 PM, Daniel Henrique Barboza wrote:
> Changes from previous version [1]:
> - added an extra patch including a new qemu-iotest to exercise the fix
>
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07159.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                    | 82 ++++++++++++++++++++++++++++++++++++++
>   block/crypto.c             | 31 ++++++++++++++
>   block/file-posix.c         | 28 +++++++++++++
>   include/block/block.h      |  3 ++
>   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, 229 insertions(+)
>   create mode 100755 tests/qemu-iotests/257
>   create mode 100644 tests/qemu-iotests/257.out
>

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

[Kevin Wolf]

Am 28.06.2019 um 21:45 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
> implementation is given by 'raw_co_delete_file'. The helper
> 'bdrv_path_is_regular_file' is being used only in raw_co_delete_file
> at this moment, but it will be used inside LUKS in a later patch.
> Foreseeing this future use, let's put it in block.c and make it
> public.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  block.c                   | 11 +++++++++++
>  block/file-posix.c        | 28 ++++++++++++++++++++++++++++
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  6 ++++++
>  4 files changed, 46 insertions(+)
> 
> --- 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)(const char *filename,
> +                                            Error **errp);

I wonder if it wouldn't make more sense to pass a BlockDriverState
instead of a filename. In the create options we usually have a BDS
around, so it should be possible to use.

The only case where it wouldn't work is if creating the image file
worked, but bdrv_open() fails. I think this is unlikely, and it's even
more unlikely that unlinking such a file would then work, so I wouldn't
see it as a problem.

The reason why I'm suggesting this is that we've spent a lot of time in
the past years to change open and create to an interface that doesn't
use only filenames, because filenames cover only part of the possibe
block devices.

So I'm kind of hesitant to add a new interface that can only use
filenames, while we know that filenames just don't quite cut it in the
general case - especially if using a BDS, which already has all the
information needed, is possible as an alternative.

>      /*
>       * 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.20.1
> 
> diff --git a/block.c b/block.c
> index c139540f2b..6e2b0f528d 100644
> --- a/block.c
> +++ b/block.c
> @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
>  #endif
>  }
>  
> +/**
> + * Helper that checks if a given string represents a regular
> + * local file.
> + */
> +bool bdrv_path_is_regular_file(const char *path)
> +{
> +    struct stat st;
> +
> +    return (stat(path, &st) == 0) && S_ISREG(st.st_mode);
> +}
> +
>  /*
>   * Detect host devices. By convention, /dev/cdrom[N] is always
>   * recognized as a host CDROM.

This hunk isn't generic, it belong in file-posix.c

Kevin

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

[Daniel Henrique Barboza]

Hey,

On 8/2/19 1:07 PM, Kevin Wolf wrote:
> Am 28.06.2019 um 21:45 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
>> implementation is given by 'raw_co_delete_file'. The helper
>> 'bdrv_path_is_regular_file' is being used only in raw_co_delete_file
>> at this moment, but it will be used inside LUKS in a later patch.
>> Foreseeing this future use, let's put it in block.c and make it
>> public.
>>
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   block.c                   | 11 +++++++++++
>>   block/file-posix.c        | 28 ++++++++++++++++++++++++++++
>>   include/block/block.h     |  1 +
>>   include/block/block_int.h |  6 ++++++
>>   4 files changed, 46 insertions(+)
>>
>> --- 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)(const char *filename,
>> +                                            Error **errp);
> I wonder if it wouldn't make more sense to pass a BlockDriverState
> instead of a filename. In the create options we usually have a BDS
> around, so it should be possible to use.
>
> The only case where it wouldn't work is if creating the image file
> worked, but bdrv_open() fails. I think this is unlikely, and it's even
> more unlikely that unlinking such a file would then work, so I wouldn't
> see it as a problem.
>
> The reason why I'm suggesting this is that we've spent a lot of time in
> the past years to change open and create to an interface that doesn't
> use only filenames, because filenames cover only part of the possibe
> block devices.
>
> So I'm kind of hesitant to add a new interface that can only use
> filenames, while we know that filenames just don't quite cut it in the
> general case - especially if using a BDS, which already has all the
> information needed, is possible as an alternative.

I'll change the parameter to use a BDS instead of a file name in
this new interface.


>
>>       /*
>>        * 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.20.1
>>
>> diff --git a/block.c b/block.c
>> index c139540f2b..6e2b0f528d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
>>   #endif
>>   }
>>   
>> +/**
>> + * Helper that checks if a given string represents a regular
>> + * local file.
>> + */
>> +bool bdrv_path_is_regular_file(const char *path)
>> +{
>> +    struct stat st;
>> +
>> +    return (stat(path, &st) == 0) && S_ISREG(st.st_mode);
>> +}
>> +
>>   /*
>>    * Detect host devices. By convention, /dev/cdrom[N] is always
>>    * recognized as a host CDROM.
> This hunk isn't generic, it belong in file-posix.c

Patch 3 uses this function as part of the core logic of this fix (do not
delete the file if the file already existed) inside block/cryptoc. This
is the reason it is exposed here. I assumed that we do not want a
public function inside file-posix.c (since there is none - everything
is done using the BD interfaces).

I think it would be sensible to simply this code as a static inside
crypto.c, since it's used twice there, and do the regular file check in
raw_co_delete_file using S_ISREG() directly - like it is already done
inside file-posix.c in other circunstances. Another alternative would
be a new bdrv_path_is_regular_file() interface but I don't think the
use I'm making here justifies this new interface as well.


Thanks,


DHB



>
> Kevin

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

[Kevin Wolf]

Am 06.08.2019 um 15:27 hat Daniel Henrique Barboza geschrieben:
> > > diff --git a/block.c b/block.c
> > > index c139540f2b..6e2b0f528d 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
> > >   #endif
> > >   }
> > > +/**
> > > + * Helper that checks if a given string represents a regular
> > > + * local file.
> > > + */
> > > +bool bdrv_path_is_regular_file(const char *path)
> > > +{
> > > +    struct stat st;
> > > +
> > > +    return (stat(path, &st) == 0) && S_ISREG(st.st_mode);
> > > +}
> > > +
> > >   /*
> > >    * Detect host devices. By convention, /dev/cdrom[N] is always
> > >    * recognized as a host CDROM.
> > This hunk isn't generic, it belong in file-posix.c
> 
> Patch 3 uses this function as part of the core logic of this fix (do not
> delete the file if the file already existed) inside block/cryptoc. This
> is the reason it is exposed here. I assumed that we do not want a
> public function inside file-posix.c (since there is none - everything
> is done using the BD interfaces).

Hm... This doesn't feel right. crypto can't assume that it's working on
a local file. It's working on some lower level BlockDriverState,
whatever that may be. Remember that you could pass all kind of URLs e.g.
for network protocols like NBD or gluster. You don't want to check
whether a local filename exists then.

In fact, I'm not sure if having a special case for an already existing
file is even worth it: By the time we fail, we'll already have truncated
the file, so the old data is lost anyway. Deleting that empty or
half-initialised file doesn't seem much worse than leaving a broken file
behind.

Kevin

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

[Daniel Henrique Barboza]

On 8/6/19 12:21 PM, Kevin Wolf wrote:
> Am 06.08.2019 um 15:27 hat Daniel Henrique Barboza geschrieben:
>>>> diff --git a/block.c b/block.c
>>>> index c139540f2b..6e2b0f528d 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -621,6 +621,17 @@ int get_tmp_filename(char *filename, int size)
>>>>    #endif
>>>>    }
>>>> +/**
>>>> + * Helper that checks if a given string represents a regular
>>>> + * local file.
>>>> + */
>>>> +bool bdrv_path_is_regular_file(const char *path)
>>>> +{
>>>> +    struct stat st;
>>>> +
>>>> +    return (stat(path, &st) == 0) && S_ISREG(st.st_mode);
>>>> +}
>>>> +
>>>>    /*
>>>>     * Detect host devices. By convention, /dev/cdrom[N] is always
>>>>     * recognized as a host CDROM.
>>> This hunk isn't generic, it belong in file-posix.c
>> Patch 3 uses this function as part of the core logic of this fix (do not
>> delete the file if the file already existed) inside block/cryptoc. This
>> is the reason it is exposed here. I assumed that we do not want a
>> public function inside file-posix.c (since there is none - everything
>> is done using the BD interfaces).
> Hm... This doesn't feel right. crypto can't assume that it's working on
> a local file. It's working on some lower level BlockDriverState,
> whatever that may be. Remember that you could pass all kind of URLs e.g.
> for network protocols like NBD or gluster. You don't want to check
> whether a local filename exists then.
>
> In fact, I'm not sure if having a special case for an already existing
> file is even worth it: By the time we fail, we'll already have truncated
> the file, so the old data is lost anyway. Deleting that empty or
> half-initialised file doesn't seem much worse than leaving a broken file
> behind.

Makes sense. I'll use your argument to justify not handling this 'file
already exists' scenario and simply delete the file in the fail scenario
inside block_crypto_co_create_opts_luks. Then we can move this
path_is_regular_file function inside file-posix.c as you suggested.


>
> Kevin