On 25.09.19 12:12, Vladimir Sementsov-Ogievskiy wrote:
> 23.09.2019 14:57, 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 <mre...@redhat.com>
>> ---
>> qapi/block-core.json | 14 +++++-
>> block/blkdebug.c | 106 ++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 118 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b5cd00c361..572c5756f1 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3394,6 +3394,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',
>> @@ -3403,7 +3413,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..f3c1e4ad7b 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -28,9 +28,11 @@
>> #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/qmp/qdict.h"
>> +#include "qapi/qmp/qlist.h"
>> #include "qapi/qmp/qstring.h"
>> #include "sysemu/qtest.h"
>>
>> @@ -44,6 +46,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 +349,84 @@ 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;
>> + QList *perm_list;
>> + const QListEntry *perm;
>> +
>> + qdict_extract_subqdict(options, &subqdict, prefix);
>> + if (!qdict_size(subqdict)) {
>> + goto out;
>> + }
>> +
>> + crumpled_subqdict = qdict_crumple(subqdict, errp);
>> + if (!crumpled_subqdict) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + perm_list = qobject_to(QList, crumpled_subqdict);
>> + if (!perm_list) {
>> + /* Omit the trailing . from the prefix */
>> + error_setg(errp, "%.*s expects a list",
>> + (int)(strlen(prefix) - 1), prefix);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + for (perm = qlist_first(perm_list); perm; perm = qlist_next(perm)) {
>> + const char *perm_name;
>> + BlockPermission perm_bit;
>> +
>> + perm_name = qstring_get_try_str(qobject_to(QString, perm->value));
>> + if (!perm_name) {
>> + /* Omit the trailing . from the prefix */
>> + error_setg(errp, "%.*s expects a list of enum strings",
>> + (int)(strlen(prefix) - 1), prefix);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + perm_bit = qapi_enum_parse(&BlockPermission_lookup, perm_name,
>> + BLOCK_PERMISSION__MAX, errp);
>> + if (perm_bit == BLOCK_PERMISSION__MAX) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + *dest |= UINT64_C(1) << perm_bit;
>> + }
>
>
> I still think that parsing it all by hand is a bad idea. We already have
> functions which does it, why to opencode? The following works for me:Ah, yes. I don’t know why I didn’t think of just using a visitor for the crumpled sub-QDict. Will do, thanks. Max
signature.asc
Description: OpenPGP digital signature
