[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions
From: |
Max Reitz |
Subject: |
Re: [PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions |
Date: |
Mon, 28 Oct 2019 11:46:39 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 |
On 16.10.19 13:13, Vladimir Sementsov-Ogievskiy wrote:
> 14.10.2019 18:39, Max Reitz wrote:
>> Sometimes it is useful to be able to add a node to the block graph that
>> takes or unshare a certain set of permissions for debugging purposes.
>> This patch adds this capability to blkdebug.
>>
>> (Note that you cannot make blkdebug release or share permissions that it
>> needs to take or cannot share, because this might result in assertion
>> failures in the block layer. But if the blkdebug node has no parents,
>> it will not take any permissions and share everything by default, so you
>> can then freely choose what permissions to take and share.)
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>> qapi/block-core.json | 14 ++++++-
>> block/blkdebug.c | 91 +++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 103 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index f66553aac7..054ce651de 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3453,6 +3453,16 @@
>> #
>> # @set-state: array of state-change descriptions
>> #
>> +# @take-child-perms: Permissions to take on @image in addition to what
>> +# is necessary anyway (which depends on how the
>> +# blkdebug node is used). Defaults to none.
>> +# (since 4.2)
>> +#
>> +# @unshare-child-perms: Permissions not to share on @image in addition
>> +# to what cannot be shared anyway (which depends
>> +# on how the blkdebug node is used). Defaults
>> +# to none. (since 4.2)
>> +#
>> # Since: 2.9
>> ##
>> { 'struct': 'BlockdevOptionsBlkdebug',
>> @@ -3462,7 +3472,9 @@
>> '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
>> '*opt-discard': 'int32', '*max-discard': 'int32',
>> '*inject-error': ['BlkdebugInjectErrorOptions'],
>> - '*set-state': ['BlkdebugSetStateOptions'] } }
>> + '*set-state': ['BlkdebugSetStateOptions'],
>> + '*take-child-perms': ['BlockPermission'],
>> + '*unshare-child-perms': ['BlockPermission'] } }
>>
>> ##
>> # @BlockdevOptionsBlklogwrites:
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 5ae96c52b0..6807c03065 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -28,10 +28,14 @@
>> #include "qemu/cutils.h"
>> #include "qemu/config-file.h"
>> #include "block/block_int.h"
>> +#include "block/qdict.h"
>> #include "qemu/module.h"
>> #include "qemu/option.h"
>> +#include "qapi/qapi-visit-block-core.h"
>> #include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qlist.h"
>> #include "qapi/qmp/qstring.h"
>> +#include "qapi/qobject-input-visitor.h"
>> #include "sysemu/qtest.h"
>>
>> typedef struct BDRVBlkdebugState {
>> @@ -44,6 +48,9 @@ typedef struct BDRVBlkdebugState {
>> uint64_t opt_discard;
>> uint64_t max_discard;
>>
>> + uint64_t take_child_perms;
>> + uint64_t unshare_child_perms;
>> +
>> /* For blkdebug_refresh_filename() */
>> char *config_file;
>>
>> @@ -344,6 +351,67 @@ static void blkdebug_parse_filename(const char
>> *filename, QDict *options,
>> qdict_put_str(options, "x-image", filename);
>> }
>>
>> +static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
>> + const char *prefix, Error **errp)
>> +{
>> + int ret = 0;
>> + QDict *subqdict = NULL;
>> + QObject *crumpled_subqdict = NULL;
>> + Visitor *v = NULL;
>> + BlockPermissionList *perm_list = NULL, *element;
>> + Error *local_err = NULL;
>> +
>> + qdict_extract_subqdict(options, &subqdict, prefix);
>> + if (!qdict_size(subqdict)) {
>
>
> Hmm, you consider it as a success, so you mean default. Then, it's safer to
> set *dest = 0 here.
“Safer” depends on the purpose of this function, and right now it’s
simply to set all fields that are given in the options; not to reset any
that aren’t.
I suppose there’s no harm in changing the purpose of the function, though.
>> + goto out;
>> + }
>> +
>> + crumpled_subqdict = qdict_crumple(subqdict, errp);
>> + if (!crumpled_subqdict) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + v = qobject_input_visitor_new(crumpled_subqdict);
>> + visit_type_BlockPermissionList(v, NULL, &perm_list, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>
> I'd prefer explicitly set *dest = 0 here too.
>
>> + for (element = perm_list; element; element = element->next) {
>> + *dest |= UINT64_C(1) << element->value;
>
> Hmm, so, you rely on correct correspondence between generated BlockPermission
> enum
> and unnamed enum with BLK_PERM_* constants...
>
> I'm afraid it's unsafe, so, in xdbg_graph_add_edge() special mapping variable
> is
> used + QEMU_BUILD_BUG_ON on BLK_PERM_ALL.
>
> I think something like this should be done here.
I don’t really like it because I think it’s cool to have a 1-to-1
relationship between the BLK_PERM_* constants and what’s defined in
QAPI. I don’t quite like assuming there isn’t, because there’s no
reason why they wouldn’t correspond.
In fact, I’d rather define the BLK_PERM_* constants based on the QAPI
enum, but I don’t know whether we want to include the QAPI header into
block.h. ...Well, judging from a quick test it looks like the header is
included into it recursively anyway. So maybe I’ll do that instead.
Max
signature.asc
Description: OpenPGP digital signature