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 =
[PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions
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)) {
+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;
+}
+
+for (element = perm_list; element; element = element->next) {
+*dest |= UINT64_C(1) << element->value;
+}
+
+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 = blkdebug_parse_perm_list(>take_child_perms, options,
+ "take-child-perms.", errp);
+if (ret < 0) {
+return ret;
+}
+
+ret = blkdebug_parse_perm_list(>unshare_child_perms, options,
+ "unshare-child-perms.", errp);
+if (ret < 0) {
+return ret;
+}
+
+return 0;
+}
+
static QemuOptsList runtime_opts = {
.name = "blkdebug",
.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -419,6 +487,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict
*options, int flags,
/* Set initial state */
s->state = 1;
+/* Parse permissions modifiers before opening the image file */
+ret = blkdebug_parse_perms(s, options, errp);
+if (ret < 0) {
+
