On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
These two callbacks will be invoked by job callbacks to execute
driver-specific code while still being in BQL.
In this example, we want the amend JobDriver to execute the
permission check (bdrv_child_refresh_perms) currently only
done in block/crypto.c block_crypto_amend_options_generic_luks()
to all its bdrv.
This is achieved by introducing callbacks in the JobDriver, but
we also need to make sure that crypto->updating_keys is true
before refreshing the permissions the first time, so that
WRITE perm is temporarly given to qcrypto_block_amend_options(),
and set it to false when permissions are restored.
Therefore bdrv_amend_pre_run() and bdrv_amend_clean() will take care of
just temporarly setting the crypto-specific updating_keys flag.
Note that at this stage, they are not yet invoked.
Signed-off-by: Emanuele Giuseppe Esposito <eespo...@redhat.com>
---
block/crypto.c | 16 ++++++++++++++++
include/block/block_int-common.h | 13 +++++++++++++
2 files changed, 29 insertions(+)
diff --git a/block/crypto.c b/block/crypto.c
index c8ba4681e2..f5e0c7b7c0 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -777,6 +777,20 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs,
Error **errp)
return spec_info;
}
+static void
+block_crypto_amend_pre_run(BlockDriverState *bs)
+{
+ BlockCrypto *crypto = bs->opaque;
+ crypto->updating_keys = true;
+}
So I see you decided to leave actually updating the permissions to the
caller, i.e. blockdev_amend_pre_run(). Why?
I’m asking because:
(1) If the .bdrv_amend_pre_run() isn’t implemented, I don’t think
refreshing the permissions in blockdev_amend_pre_run() is necessary. So
if it is implemented, the implementation might as well do it itself.
(2) The way you implemented it, it’s actually kind of hard to see why
this is even necessary. Both of these functions
(block_crypto_amend_{pre_run,cleanup}()) don’t require the BQL, so the
explanation for .bdrv_amend_pre_run() (“useful to perform
driver-specific initialization code under BQL”) doesn’t really apply.
If you want to explain (and we should) why this is necessary, then the
.bdrv_amend_pre_run() documentation needs to state that we will refresh
the permissions after this function has run and before .bdrv_co_amend()
will run, and so it’s also useful to put code here that will change the
permissions on permission update, but that just really gets complicated
to explain. Naively, I find it simpler to just say “Put BQL code here,
this will run before .bdrv_co_amend()” and have every implementation
that needs it refresh the permissions itself.
(3) In patch 32, you add
block_crypto_amend_options_{prepare,cleanup}(). If the functions added
here (block_crypto_amend_{pre_run,cleanup}()) would refresh the
permissions by themselves, they’d be exactly the same functions, so you
could reuse the ones from here in patch 32.
+
+static void
+block_crypto_amend_cleanup(BlockDriverState *bs)
+{
+ BlockCrypto *crypto = bs->opaque;
+ crypto->updating_keys = false;
+}
+
static int
block_crypto_amend_options_generic_luks(BlockDriverState *bs,
QCryptoBlockAmendOptions
*amend_options,
@@ -931,6 +945,8 @@ static BlockDriver bdrv_crypto_luks = {
.bdrv_get_specific_info = block_crypto_get_specific_info_luks,
.bdrv_amend_options = block_crypto_amend_options_luks,
.bdrv_co_amend = block_crypto_co_amend_luks,
+ .bdrv_amend_pre_run = block_crypto_amend_pre_run,
+ .bdrv_amend_clean = block_crypto_amend_cleanup,
.is_format = true,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index cc8c8835ba..9d28396978 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -189,6 +189,19 @@ struct BlockDriver {
* the GS API.
*/
+ /*
+ * Called inside job->pre_run() callback, it is useful
+ * to perform driver-specific initialization code under
+ * BQL, like setting up specific permission flags.
I wouldn’t necessarily state that this function is called by
`job->pre_run()`, but rather paint the picture of how it’s used together
with `.bdrv_co_amend()`, e.g. something like:
“This function is invoked under the BQL before .bdrv_co_amend() (which
in contrast does not necessarily run under the BQL) to allow
driver-specific initialization code that requires the BQL, like setting
up specific permission flags.”
(Though this comment should be much more specific if we keep updating
the permissions in the caller, because then the comment also needs to
explain that we do that.)
+ */
+ void (*bdrv_amend_pre_run)(BlockDriverState *bs);
+ /*
+ * Called inside job->clean() callback, it undoes
+ * the driver-specific initialization code done in amend_pre_run.
+ * Also this function is under BQL.
Here too I’d omit the job reference and just say that (e.g.)
“This function is invoked under the BQL after .bdrv_co_amend() to allow
cleaning up what was done in .bdrv_amend_pre_run().”
Hanna
+ */
+ void (*bdrv_amend_clean)(BlockDriverState *bs);
+
/*
* Return true if @to_replace can be replaced by a BDS with the
* same data as @bs without it affecting @bs's behavior (that is,
