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. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|