Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
On 11/09/2015 10:42 PM, Alberto Garcia wrote:
> Sorry again for the late review, here are my comments:
>
> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
>> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
>> + bool has_child, const char *child,
>> + bool has_new_node, const char *new_node,
>> + Error **errp)
>
> You are using different names for the parameters here: 'op', 'parent',
> 'child', 'new_node'; in the JSON file the first and last one are named
> 'operation' and 'node'.
OK, I will fix it in the next version
>
>> +parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +if (!parent_bs) {
>> +error_propagate(errp, local_err);
>> +return;
>> +}
>
> You don't need to change it if you don't want but you can use errp
> directly here and spare the error_propagate() call.
Too many codes in qemu use local_err and error_propagate(). I think
errp can be NOT NULL here(in which case?).
>
>> +
>> +switch(op) {
>> +case CHANGE_OPERATION_ADD:
>> +if (has_child) {
>> +error_setg(errp, "The operation %s doesn't support the
>> parameter child",
>> + ChangeOperation_lookup[op]);
>> +return;
>> +}
>
> This line goes over 80 columns, please use scripts/checkpatch.pl to
> check the style of the code.
I forgot to do it...
>
>> +if (!has_new_node) {
>> +error_setg(errp, "The operation %s needs the parameter
>> new_node",
>> + ChangeOperation_lookup[op]);
>> +return;
>> +}
>> +break;
>> +case CHANGE_OPERATION_DELETE:
>> +if (has_new_node) {
>> +error_setg(errp, "The operation %s doesn't support the
>> parameter node",
>> + ChangeOperation_lookup[op]);
>> +return;
>> +}
>> +if (!has_child) {
>> +error_setg(errp, "The operation %s needs the parameter child",
>> + ChangeOperation_lookup[op]);
>> +return;
>> +}
>
> I still think that having two optional parameters here makes things
> unnecessarily complex.
>
> If in the future we want to add a new operation that needs a new
> parameter then we can add it then, but I think that both 'add' and
> 'delete' can work perfectly fine with a single 'node' parameter.
>
> Does anyone else have an opinion about this?
>
>> +default:
>> +break;
>> +}
>
> This is unreachable so you can add a g_assert_not_reached() here.
OK
>
>> +##
>> +# @ChangeOperation:
>> +#
>> +# An enumeration of block device change operation.
>
> How about something like "An enumeration of possible change operations
> on a block device" ?
>
>> +# @add: Add a new block driver state to a existed block driver state.
>
> s/a existed/an existing/
>
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
>
> "Dynamically reconfigure"
>
>> +# add, remove, insert, replace a block driver state. Currently only
>
> "insert or replace"
>
>> +# the Quorum driver implements this feature to add and remove its child.
>> +# This is useful to fix a broken quorum child.
>
> "add and remove its child" -> "add or remove a child"
>
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
>
> s/chanage/change/
>
>> +#
>> +# @parent: the id or node name of which node will be changed.
>
> How about "the id or name of the node that will be changed" ?
>
>> +#
>> +# @child: the child node-name which will be deleted.
>> +#
>> +# @node: the new node-name which will be added.
>
> "The name of the node that will be deleted"
> "The name of the node that will be added"
>
> Or, if you merge both parameters, "...that will be added or deleted".
>
>> +#
>> +# Note: this command is experimental, and not a stable API.
>
> "and not a stable API" -> "and does not have a stable API", or "and its
> API is not stable".
Thanks for your review, all will be fixed in the next version
Wen Congyang
>
> Berto
> .
>
Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
On 11/10/2015 12:04 AM, Kevin Wolf wrote:
> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
>> +##
>> +# @ChangeOperation:
>> +#
>> +# An enumeration of block device change operation.
>> +#
>> +# @add: Add a new block driver state to a existed block driver state.
>> +#
>> +# @delete: Delete a block driver state's child.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'enum': 'ChangeOperation',
>> + 'data': [ 'add', 'delete' ] }
>
> What's the advantage of this enum compared to separate QMP commands? The
> way it is specified here, ChangeOperation is already implicit by whether
> or not child and node are given.
>
>> +##
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
>> +# add, remove, insert, replace a block driver state. Currently only
>> +# the Quorum driver implements this feature to add and remove its child.
>> +# This is useful to fix a broken quorum child.
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
>> +#
>> +# @parent: the id or node name of which node will be changed.
>> +#
>> +# @child: the child node-name which will be deleted.
>
> #optional
>
> Must be present for operation = delete, must not be present otherwise.
>
>> +# @node: the new node-name which will be added.
>
> #optional
>
> Must be present for operation = add, must not be present otherwise.
>
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-blockdev-change',
>> + 'data' : { 'operation': 'ChangeOperation',
>> + 'parent': 'str',
>> + '*child': 'str',
>> + '*node': 'str' } }
>
> Let me suggest this alternative:
>
> { 'command': 'x-blockdev-change',
> 'data' : { 'parent': 'str',
> 'child': 'str',
> '*node': 'str' } }
>
> child doesn't describe a node name then, but a child name (adds a
> dependency on my patches which add a name to BdrvChild, though).
Where is the patch? I don't find it.
> Depending on whether node is given and whether the child already exists,
> this may add, remove or replace a child.
If the user wants to insert a filter driver between parent and child, we
also needs three parameters: parent, child, node. So it is why I add the
parameter operation.
Thanks
Wen Congyang
>
> Kevin
> .
>
Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
> +##
> +# @ChangeOperation:
> +#
> +# An enumeration of block device change operation.
> +#
> +# @add: Add a new block driver state to a existed block driver state.
> +#
> +# @delete: Delete a block driver state's child.
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'ChangeOperation',
> + 'data': [ 'add', 'delete' ] }
What's the advantage of this enum compared to separate QMP commands? The
way it is specified here, ChangeOperation is already implicit by whether
or not child and node are given.
> +##
> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to
> +# add, remove, insert, replace a block driver state. Currently only
> +# the Quorum driver implements this feature to add and remove its child.
> +# This is useful to fix a broken quorum child.
> +#
> +# @operation: the chanage operation. It can be add, delete.
> +#
> +# @parent: the id or node name of which node will be changed.
> +#
> +# @child: the child node-name which will be deleted.
#optional
Must be present for operation = delete, must not be present otherwise.
> +# @node: the new node-name which will be added.
#optional
Must be present for operation = add, must not be present otherwise.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-blockdev-change',
> + 'data' : { 'operation': 'ChangeOperation',
> + 'parent': 'str',
> + '*child': 'str',
> + '*node': 'str' } }
Let me suggest this alternative:
{ 'command': 'x-blockdev-change',
'data' : { 'parent': 'str',
'child': 'str',
'*node': 'str' } }
child doesn't describe a node name then, but a child name (adds a
dependency on my patches which add a name to BdrvChild, though).
Depending on whether node is given and whether the child already exists,
this may add, remove or replace a child.
Kevin
Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
Sorry again for the late review, here are my comments:
On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
> + bool has_child, const char *child,
> + bool has_new_node, const char *new_node,
> + Error **errp)
You are using different names for the parameters here: 'op', 'parent',
'child', 'new_node'; in the JSON file the first and last one are named
'operation' and 'node'.
> +parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +if (!parent_bs) {
> +error_propagate(errp, local_err);
> +return;
> +}
You don't need to change it if you don't want but you can use errp
directly here and spare the error_propagate() call.
> +
> +switch(op) {
> +case CHANGE_OPERATION_ADD:
> +if (has_child) {
> +error_setg(errp, "The operation %s doesn't support the parameter
> child",
> + ChangeOperation_lookup[op]);
> +return;
> +}
This line goes over 80 columns, please use scripts/checkpatch.pl to
check the style of the code.
> +if (!has_new_node) {
> +error_setg(errp, "The operation %s needs the parameter new_node",
> + ChangeOperation_lookup[op]);
> +return;
> +}
> +break;
> +case CHANGE_OPERATION_DELETE:
> +if (has_new_node) {
> +error_setg(errp, "The operation %s doesn't support the parameter
> node",
> + ChangeOperation_lookup[op]);
> +return;
> +}
> +if (!has_child) {
> +error_setg(errp, "The operation %s needs the parameter child",
> + ChangeOperation_lookup[op]);
> +return;
> +}
I still think that having two optional parameters here makes things
unnecessarily complex.
If in the future we want to add a new operation that needs a new
parameter then we can add it then, but I think that both 'add' and
'delete' can work perfectly fine with a single 'node' parameter.
Does anyone else have an opinion about this?
> +default:
> +break;
> +}
This is unreachable so you can add a g_assert_not_reached() here.
> +##
> +# @ChangeOperation:
> +#
> +# An enumeration of block device change operation.
How about something like "An enumeration of possible change operations
on a block device" ?
> +# @add: Add a new block driver state to a existed block driver state.
s/a existed/an existing/
> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to
"Dynamically reconfigure"
> +# add, remove, insert, replace a block driver state. Currently only
"insert or replace"
> +# the Quorum driver implements this feature to add and remove its child.
> +# This is useful to fix a broken quorum child.
"add and remove its child" -> "add or remove a child"
> +#
> +# @operation: the chanage operation. It can be add, delete.
s/chanage/change/
> +#
> +# @parent: the id or node name of which node will be changed.
How about "the id or name of the node that will be changed" ?
> +#
> +# @child: the child node-name which will be deleted.
> +#
> +# @node: the new node-name which will be added.
"The name of the node that will be deleted"
"The name of the node that will be added"
Or, if you merge both parameters, "...that will be added or deleted".
> +#
> +# Note: this command is experimental, and not a stable API.
"and not a stable API" -> "and does not have a stable API", or "and its
API is not stable".
Berto
Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
On 11/05/2015 09:49 PM, Alberto Garcia wrote:
> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang
> wrote:
>
>> The new QMP command name is x-blockdev-change. It justs for
>> adding/removing quorum's child now, and don't support all kinds of
>> children, all kinds of operations, nor all block drivers. So it is
>> experimental now.
>
> I might have missed some discussion, why were the -add and -delete
This monitor command can be used to implement: add, delete, insert, remove,
replace... Currently, I only implement add and delete operation.
>
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
>> +# add, remove, insert, replace a block driver state. Currently only
>> +# the Quorum driver implements this feature to add and remove its child.
>> +# This is useful to fix a broken quorum child.
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
>> +#
>> +# @parent: the id or node name of which node will be changed.
>> +#
>> +# @child: the child node-name which will be deleted.
>> +#
>> +# @node: the new node-name which will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-blockdev-change',
>> + 'data' : { 'operation': 'ChangeOperation',
>> + 'parent': 'str',
>> + '*child': 'str',
>> + '*node': 'str' } }
>
> Do you really need two separate 'child' and 'node' parameters? If the
> operation is 'add' you can only use 'node', if it is 'delete, you can
> only use 'child'. It seems to me that you can simply have one 'node'
> parameter and use it for both ...
parent and child already exist in the BDS graph, and node is a new node.
In the furture, we may need to implement insert opetioan, and this operation
needs such three BDSes.
Thanks
Wen Congyang
>
> Berto
> .
>
Re: [Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
> The new QMP command name is x-blockdev-change. It justs for
> adding/removing quorum's child now, and don't support all kinds of
> children, all kinds of operations, nor all block drivers. So it is
> experimental now.
I might have missed some discussion, why were the -add and -delete
> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to
> +# add, remove, insert, replace a block driver state. Currently only
> +# the Quorum driver implements this feature to add and remove its child.
> +# This is useful to fix a broken quorum child.
> +#
> +# @operation: the chanage operation. It can be add, delete.
> +#
> +# @parent: the id or node name of which node will be changed.
> +#
> +# @child: the child node-name which will be deleted.
> +#
> +# @node: the new node-name which will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-blockdev-change',
> + 'data' : { 'operation': 'ChangeOperation',
> + 'parent': 'str',
> + '*child': 'str',
> + '*node': 'str' } }
Do you really need two separate 'child' and 'node' parameters? If the
operation is 'add' you can only use 'node', if it is 'delete, you can
only use 'child'. It seems to me that you can simply have one 'node'
parameter and use it for both ...
Berto
[Qemu-block] [PATCH v6 3/4] qmp: add monitor command to add/remove a child
The new QMP command name is x-blockdev-change. It justs for adding/removing
quorum's child now, and don't support all kinds of children, all kinds of
operations, nor all block drivers. So it is experimental now.
Signed-off-by: Wen Congyang
Signed-off-by: zhanghailiang
Signed-off-by: Gonglei
---
blockdev.c | 76
qapi/block-core.json | 40 +++
qmp-commands.hx | 50 ++
3 files changed, 166 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index 6c8cce4..72efe5d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3086,6 +3086,82 @@ fail:
qmp_output_visitor_cleanup(ov);
}
+void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
+ bool has_child, const char *child,
+ bool has_new_node, const char *new_node,
+ Error **errp)
+{
+BlockDriverState *parent_bs, *child_bs, *new_bs;
+Error *local_err = NULL;
+
+parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
+if (!parent_bs) {
+error_propagate(errp, local_err);
+return;
+}
+
+switch(op) {
+case CHANGE_OPERATION_ADD:
+if (has_child) {
+error_setg(errp, "The operation %s doesn't support the parameter
child",
+ ChangeOperation_lookup[op]);
+return;
+}
+if (!has_new_node) {
+error_setg(errp, "The operation %s needs the parameter new_node",
+ ChangeOperation_lookup[op]);
+return;
+}
+break;
+case CHANGE_OPERATION_DELETE:
+if (has_new_node) {
+error_setg(errp, "The operation %s doesn't support the parameter
node",
+ ChangeOperation_lookup[op]);
+return;
+}
+if (!has_child) {
+error_setg(errp, "The operation %s needs the parameter child",
+ ChangeOperation_lookup[op]);
+return;
+}
+default:
+break;
+}
+
+if (has_child) {
+child_bs = bdrv_find_node(child);
+if (!child_bs) {
+error_setg(errp, "Node '%s' not found", child);
+return;
+}
+}
+
+if (has_new_node) {
+new_bs = bdrv_find_node(new_node);
+if (!new_bs) {
+error_setg(errp, "Node '%s' not found", new_node);
+return;
+}
+}
+
+switch(op) {
+case CHANGE_OPERATION_ADD:
+bdrv_add_child(parent_bs, new_bs, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+}
+break;
+case CHANGE_OPERATION_DELETE:
+bdrv_del_child(parent_bs, child_bs, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+}
+break;
+default:
+break;
+}
+}
+
BlockJobInfoList *qmp_query_block_jobs(Error **errp)
{
BlockJobInfoList *head = NULL, **p_next = &head;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bb2189e..361588f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2114,3 +2114,43 @@
##
{ 'command': 'block-set-write-threshold',
'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+##
+# @ChangeOperation:
+#
+# An enumeration of block device change operation.
+#
+# @add: Add a new block driver state to a existed block driver state.
+#
+# @delete: Delete a block driver state's child.
+#
+# Since: 2.5
+##
+{ 'enum': 'ChangeOperation',
+ 'data': [ 'add', 'delete' ] }
+
+##
+# @x-blockdev-change
+#
+# Dynamic reconfigure the block driver state graph. It can be used to
+# add, remove, insert, replace a block driver state. Currently only
+# the Quorum driver implements this feature to add and remove its child.
+# This is useful to fix a broken quorum child.
+#
+# @operation: the chanage operation. It can be add, delete.
+#
+# @parent: the id or node name of which node will be changed.
+#
+# @child: the child node-name which will be deleted.
+#
+# @node: the new node-name which will be added.
+#
+# Note: this command is experimental, and not a stable API.
+#
+# Since: 2.5
+##
+{ 'command': 'x-blockdev-change',
+ 'data' : { 'operation': 'ChangeOperation',
+ 'parent': 'str',
+ '*child': 'str',
+ '*node': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d2ba800..ede7b71 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3921,6 +3921,56 @@ Example (2):
EQMP
{
+.name = "x-blockdev-change",
+.args_type = "operation:s,parent:B,child:B?,node:B?",
+.mhandler.cmd_new = qmp_marshal_x_blockdev_change,
+},
+
+SQMP
+x-blockdev-change
+
+
+Dynamic reconfigure the block driver state graph. It can be used to
+add, remove, insert, replace a block driver state. Currently only
+the Quorum driver implements this feature to add and remo
