Re: [PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions
28.10.2019 13:46, Max Reitz wrote:
> 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
>>> ---
>>>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, , 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, _list, _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
Re: [PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions
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
>> ---
>> 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, , 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, _list, _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 +
Re: [PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions
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
> ---
> 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, , 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.
> +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, _list, _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.
> +}
> +
> +out:
> +qapi_free_BlockPermissionList(perm_list);
> +visit_free(v);
> +qobject_unref(subqdict);
> +qobject_unref(crumpled_subqdict);
> +return ret;
> +}
> +
> +static int blkdebug_parse_perms(BDRVBlkdebugState *s, QDict *options,
> +Error **errp)
> +{
> +int ret;
> +
> +ret =
