On 3/20/19 6:52 AM, Daniel P. Berrangé wrote:
On Tue, Mar 19, 2019 at 07:03:30PM -0300, 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 fixes it by adding a flag to indicate whether bdrv_create_file() created the file, and then unlinking it in the 'fail' label if necessary. * 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 <bssrika...@in.ibm.com> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com> --- block/crypto.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/block/crypto.c b/block/crypto.c index fd8c7cfac6..5b43424845 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -533,6 +533,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, BlockDriverState *bs = NULL; QDict *cryptoopts; int64_t size; + bool filecreated = false; int ret;/* Parse options */@@ -554,6 +555,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, if (ret < 0) { goto fail; } + filecreated = true;bs = bdrv_open(filename, NULL, NULL,BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp); @@ -569,7 +571,14 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, }ret = 0;+ goto cleanup; + fail: + if (filecreated) { + unlink(filename);You can't do this I'm afraid. "filename" cannot be assumed to be a local file - it can be an arbitrary block device backend string. Contrary to what you might expect, given the call to bdrv_create_file, we can't actually assume this method created the file either. The above bdrv_create_file method may have merely formatted a pre-existing volume. To fix this requires a "bdrv_delete" callback in the BlockDriver code, and the actual deletion would need to be done in the "qemu-img create" code.
Thanks for the pointers. I'll see if I can make this happen in v2. DHB
Regards, Daniel
