Re: [PATCH v2 2/4] blkdebug: Allow taking/unsharing permissions

[Max Reitz]

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 <address@hidden>
>> ---
>>   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