On 18.09.19 18:01, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2019 16:56, 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 <mreitz@redhat.com>
>> ---
>>   qapi/block-core.json |  29 +++++++++++-
>>   block/blkdebug.c     | 106 ++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 133 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e6edd641f1..336043e02c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3347,6 +3347,21 @@
>>               '*state': 'int',
>>               'new_state': 'int' } }
>>   
>> +##
>> +# @BlockdevPermission:
>> +#
>> +# Permissions that an edge in the block graph can take or share.
>> +#
>> +# Since: 4.2
>> +##
>> +{ 'enum': 'BlockdevPermission',
>> +  'data': [
>> +      'consistent-read',
>> +      'write',
>> +      'write-unchanged',
>> +      'resize',
>> +      'graph-mod' ] }
> 
> :)
> 
> BlockPermission is already here since 4.0 and has exactly same content. (And better documented)

Oops.  Very good, then, thanks. :-)

[...]

>> @@ -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;
>> +        BlockdevPermission 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(&BlockdevPermission_lookup, perm_name,
>> +                                   BLOCKDEV_PERMISSION__MAX, errp);
>> +        if (perm_bit == BLOCKDEV_PERMISSION__MAX) {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +
>> +        *dest |= UINT64_C(1) << perm_bit;
>> +    }
>> +
>> +out:
>> +    qobject_unref(subqdict);
>> +    qobject_unref(crumpled_subqdict);
>> +    return ret;
>> +}
>> +
>> +static int blkdebug_parse_perms(BDRVBlkdebugState *s, QDict *options,
>> +                                Error **errp)
>> +{
>> +    int ret;
>> +
>> +    ret = blkdebug_parse_perm_list(&s->take_child_perms, options,
>> +                                   "take-child-perms.", errp);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    ret = blkdebug_parse_perm_list(&s->unshare_child_perms, options,
>> +                                   "unshare-child-perms.", errp);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> 
> It's a pity that being described in json, these new parameters still not parsed automatically..

That would require changing the whole bdrv_open() infrastructure to use
QAPI types.

>> +
>>   static QemuOptsList runtime_opts = {
> 
> and that we have to keep these runtime_opts everywhere, which duplicates json definitions..

Well, it duplicates some json definitions.  I don’t add the new
parameters added in this patch to the list, because there is no point in
supporting them outside of blockdev-add.

Max