block: implement the bdrv_reopen_prepare helper for LUKS driver
PATCH 1/1 block: implement the bdrv_reopen_prepare helper for LUKS driver
If the bdrv_reopen_prepare helper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.
--- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+)
block/crypto.c +7 -0
@@ -382,6 +382,12 @@ static void block_crypto_close(BlockDriverState *bs)qcrypto_block_free (crypto->block); } +static int block_crypto_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +/* nothing needs checking */ +return 0 ; +}/* * 1 MB bounce buffer gives good performance / memory tradeoff
@@ -620,6 +626,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, + .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev,
Signature · 1 line
2.14.3
Quoted 9 lines · Daniel P. Berrange
If the
bdrv_reopen_preparehelper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+)
I'm hoping another block-layer expert chimes in, as I'm not quite sure what the full reopen rules are; but the idea makes sense to me.
block/crypto.c static void block_crypto_close(BlockDriverState *bs) · line 388
Quoted 13 lines
block/crypto.c +4
-0@@ -382,6 +382,12 @@ static void block_crypto_close(BlockDriverState *bs)qcrypto_block_free (crypto->block); } +static int block_crypto_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +/* nothing needs checking */
Are we sure that even changes such as moving from read-only to read-write need no checking?
Quoted 14 lines
block/crypto.c +3
-0+return 0 ; +}/* * 1 MB bounce buffer gives good performance / memory tradeoff @@ -620,6 +626,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, + .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 6 lines · Eric Blake
Quoted 5 lines · Daniel P. Berrange
If the
bdrv_reopen_preparehelper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+)I'm hoping another block-layer expert chimes in, as I'm not quite sure what the full reopen rules are; but the idea makes sense to me.
Yeah, likewise - it is hard to understand what is required, but I see lots of other block drivers just put a no-op impl here. I'm relying on Kevin/Max to tell me if i'm wrong here
Quoted 8 lines
Quoted 8 lines
block/crypto.c +0
-0@@ -382,6 +382,12 @@ static void block_crypto_close(BlockDriverState *bs)qcrypto_block_free (crypto->block); }
Quoted 7 lines
block/crypto.c static void block_crypto_close(BlockDriverState *bs) · line 388
Quoted 4 lines
block/crypto.c +4
-0+static int block_crypto_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +/* nothing needs checking */ Are we sure that even changes such as moving from read-only to read-write need no checking?
LUKS doesn't do anything differently with ro vs rw images, so I'm assuming any such checks are handled by the layer below.
Quoted 10 lines
Quoted 10 lines
block/crypto.c +2
-0+return 0 ; +}/* * 1 MB bounce buffer gives good performance / memory tradeoff @@ -620,6 +626,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks,
Quoted 4 lines
block/crypto.c BlockDriver bdrv_crypto_luks = { · line 632
Quoted 4 lines
block/crypto.c +1
-0+ .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev,
Regards,
Daniel
Signature · 3 lines
|: 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 :|
Quoted 15 lines · Daniel P. Berrange
If the
bdrv_reopen_preparehelper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+)block/crypto.c +0
-0
block/crypto.c static void block_crypto_close(BlockDriverState *bs) · line 390
Quoted 9 lines
block/crypto.c +6
-0@@ -382,6 +382,12 @@ static void block_crypto_close(BlockDriverState *bs)qcrypto_block_free (crypto->block); } +static int block_crypto_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +/* nothing needs checking */ +return 0 ; +}
Unfortunately I have to admit I'm not quite an expert on reopen myself... But generally it is used to change options, so in theory one could provide a new key-secret here. It's up to you whether you want to support that or not (with this implementation the user will get an error when they want to change the key-secret).
Apart from that, I think a no-op should be OK.
Max
Quoted 12 lines
block/crypto.c +1
-0/* * 1 MB bounce buffer gives good performance / memory tradeoff @@ -620,6 +626,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, + .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev,
Ping, can this be queued in the block tree, since it appears the no-op impl is ok ?
Quoted 38 lines · Daniel P. Berrange
If the
bdrv_reopen_preparehelper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+)block/crypto.c +7
-0@@ -382,6 +382,12 @@ static void block_crypto_close(BlockDriverState *bs)qcrypto_block_free (crypto->block); } +static int block_crypto_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +/* nothing needs checking */ +return 0 ; +}/* * 1 MB bounce buffer gives good performance / memory tradeoff @@ -620,6 +626,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks, + .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev,Signature · 1 line
2.14.3
Regards,
Daniel
Signature · 3 lines
|: 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 :|
Ping
Quoted 35 lines · Daniel P. Berrangé
Ping, can this be queued in the block tree, since it appears the no-op impl is ok ?
Quoted 31 lines · Daniel P. Berrange
If the
bdrv_reopen_preparehelper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.Signed-off-by Daniel P. Berrange <[email protected]>--- block/crypto.c | 7 +++++++ 1 file changed, 7 insertions(+)block/crypto.c +6
-0@@ -382,6 +382,12 @@ static void block_crypto_close(BlockDriverState *bs)qcrypto_block_free (crypto->block); } +static int block_crypto_reopen_prepare (BDRVReopenState *state, +BlockReopenQueue *queue,Error **errp) +{ +/* nothing needs checking */ +return 0 ; +}/* * 1 MB bounce buffer gives good performance / memory tradeoff @@ -620,6 +626,7 @@ BlockDriver bdrv_crypto_luks = { .bdrv_truncate = block_crypto_truncate, .create_opts = &block_crypto_create_opts_luks,
Quoted 15 lines
Quoted 7 lines
block/crypto.c +1
-0+ .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, .bdrv_co_preadv = block_crypto_co_preadv, .bdrv_co_pwritev = block_crypto_co_pwritev,Signature · 1 line
2.14.3Regards,
DanielSignature · 3 lines
|: 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 :|
Regards,
Daniel
Signature · 3 lines
|: 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 :|
Kevin Wolf
AppliedAm 18.01.2018 um 11:31 hat Daniel P. Berrange geschrieben:
Quoted 6 lines
If the
bdrv_reopen_preparehelper isn't provided, the qemu-img commit command fails to re-open the base layer after committing changes into it. Provide a no-op implementation for the LUKS driver, since there is not any custom work that needs doing to re-open it.Signed-off-by Daniel P. Berrange <[email protected]>
Thanks, applied to the block branch.
Kevin