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 <mreitz@redhat.com>
>> ---
>>   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