[Qemu-devel] [PATCH v3 0/5] qapi: child add/delete support

[Qemu-devel] [PATCH v3 0/5] qapi: child add/delete support

[Wen Congyang]

If quorum's child is broken, we can use mirror job to replace it.
But sometimes, the user only need to remove the broken child, and
add it later when the problem is fixed.

ChangLog:
v3:
1. Don't open BDS in bdrv_add_child(). Use the existing BDS which is
   created by the QMP command blockdev-add.
2. The driver NBD can support filename, path, host:port now.
v2:
1. Use bdrv_get_device_or_node_name() instead of new function
   bdrv_get_id_or_node_name()
2. Update the error message
3. Update the documents in block-core.json

Wen Congyang (5):
  support nbd driver in blockdev-add
  Add new block driver interface to add/delete a BDS's child
  quorum: implement bdrv_add_child() and bdrv_del_child()
  qmp: add monitor command to add/remove a child
  hmp: add monitor command to add/remove a child

 block.c                   | 58 ++++++++++++++++++++++++++++++++++--
 block/quorum.c            | 72 ++++++++++++++++++++++++++++++++++++++++++--
 blockdev.c                | 47 +++++++++++++++++++++++++++++
 hmp-commands.hx           | 28 +++++++++++++++++
 hmp.c                     | 20 +++++++++++++
 hmp.h                     |  2 ++
 include/block/block.h     |  8 +++++
 include/block/block_int.h |  5 ++++
 qapi/block-core.json      | 76 +++++++++++++++++++++++++++++++++++++++++++++--
 qmp-commands.hx           | 53 +++++++++++++++++++++++++++++++++
 10 files changed, 362 insertions(+), 7 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

The NBD driver needs: filename, path or (host, port, exportname).
It checks which key exists and decides use unix or inet socket.
It doesn't recognize the key type, so we can't use union, and
can't reuse InetSocketAddress.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 11134a8..e68a59f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1376,12 +1376,14 @@
 #
 # @host_device, @host_cdrom: Since 2.1
 #
+# @nbd: Since 2.5
+#
 # Since: 2.0
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
             'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
-            'http', 'https', 'null-aio', 'null-co', 'parallels',
+            'http', 'https', 'nbd', 'null-aio', 'null-co', 'parallels',
             'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
             'vmdk', 'vpc', 'vvfat' ] }
 
@@ -1797,6 +1799,42 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @BlockdevOptionsNBD
+#
+# Driver specific block device options for NBD
+#
+# @filename: #optional unix or inet path. The format is:
+#            unix: nbd+unix:///export?socket=path or
+#                  nbd:unix:path:exportname=export
+#            inet: nbd[+tcp]://host[:port]/export or
+#                  nbd:host[:port]:exportname=export
+#
+# @path: #optional filesystem path to use
+#
+# @host: #optional host part of the address
+#
+# @port: #optional port part of the address
+#
+# @ipv4: #optional whether to accept IPv4 addresses, default try both IPv4
+#        and IPv6
+#
+# @ipv6: #optional whether to accept IPv6 addresses, default try both IPv4
+#        and IPv6
+#
+# @export: #optional the NBD export name
+#
+# Since: 2.5
+##
+{ 'struct': 'BlockdevOptionsNBD',
+  'data': { '*filename': 'str',
+            '*path':     'str',
+            '*host':     'str',
+            '*port':     'str',
+            '*ipv4':     'bool',
+            '*ipv6':     'bool',
+            '*export':   'str' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.
@@ -1822,7 +1860,7 @@
       'http':       'BlockdevOptionsFile',
       'https':      'BlockdevOptionsFile',
 # TODO iscsi: Wait for structured options
-# TODO nbd: Should take InetSocketAddress for 'host'?
+      'nbd':        'BlockdevOptionsNBD',
 # TODO nfs: Wait for structured options
       'null-aio':   'BlockdevOptionsNull',
       'null-co':    'BlockdevOptionsNull',
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> The NBD driver needs: filename, path or (host, port, exportname).
> It checks which key exists and decides use unix or inet socket.
> It doesn't recognize the key type, so we can't use union, and
> can't reuse InetSocketAddress.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 11134a8..e68a59f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1376,12 +1376,14 @@
>  #
>  # @host_device, @host_cdrom: Since 2.1
>  #
> +# @nbd: Since 2.5
> +#
>  # Since: 2.0
>  ##
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>              'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -            'http', 'https', 'null-aio', 'null-co', 'parallels',
> +            'http', 'https', 'nbd', 'null-aio', 'null-co', 'parallels',
>              'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
>              'vmdk', 'vpc', 'vvfat' ] }
>  
> @@ -1797,6 +1799,42 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @BlockdevOptionsNBD
> +#
> +# Driver specific block device options for NBD
> +#
> +# @filename: #optional unix or inet path. The format is:
> +#            unix: nbd+unix:///export?socket=path or
> +#                  nbd:unix:path:exportname=export
> +#            inet: nbd[+tcp]://host[:port]/export or
> +#                  nbd:host[:port]:exportname=export
> +#
> +# @path: #optional filesystem path to use
> +#
> +# @host: #optional host part of the address
> +#
> +# @port: #optional port part of the address
> +#
> +# @ipv4: #optional whether to accept IPv4 addresses, default try both IPv4
> +#        and IPv6
> +#
> +# @ipv6: #optional whether to accept IPv6 addresses, default try both IPv4
> +#        and IPv6
> +#
> +# @export: #optional the NBD export name
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsNBD',
> +  'data': { '*filename': 'str',
> +            '*path':     'str',
> +            '*host':     'str',
> +            '*port':     'str',
> +            '*ipv4':     'bool',
> +            '*ipv6':     'bool',
> +            '*export':   'str' } }
> +
> +##

I'm afraid this doesn't address Eric's review of your v2.

In v2, you had

    { 'struct': 'BlockdevOptionsNBD',
      'base': 'InetSocketAddress',
      'data': { '*export': 'str' } }

Eric observed that InetSocketAddress can represent a port range.  Since
NBD doesn't support a range, he suggested to have two types rather than
InetSocketAddress: one to represent a single port, and another one to
represent a port range.  He further suggested to make the single port
type a base type of the port range type.

Eric further observed that support for the nbd+unix transport was
missing, and suggested to have a union type combining the various
transports.

Instead of that, you simply replaced InetSocketAddress by a bunch of
optional arguments with (unspoken!) conditions, such as "either
filename, or path+host".  That's not how we want things done in the
schema.

If we decide we want types separate for single ports and port range, you
need those two types.  For either one, you need a union type combining
transports.  It already exists for port ranges: SocketAddress.  You need
to create the other.

We can then discuss names.  For me, InetSocketAddress and SocketAddress
strongly suggest single port.  I'd prefer naming the port range types
*AddressRange or something.

If we decide separate types for single port and port ranges aren't
worthwhile, you can simply use SocketAddress where your v2 used
InetSocketAddress.

Eric, how strongly do you feel about separating the two?

>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1822,7 +1860,7 @@
>        'http':       'BlockdevOptionsFile',
>        'https':      'BlockdevOptionsFile',
>  # TODO iscsi: Wait for structured options
> -# TODO nbd: Should take InetSocketAddress for 'host'?
> +      'nbd':        'BlockdevOptionsNBD',
>  # TODO nfs: Wait for structured options
>        'null-aio':   'BlockdevOptionsNull',
>        'null-co':    'BlockdevOptionsNull',

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3311 bytes --]

On 09/14/2015 08:27 AM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> The NBD driver needs: filename, path or (host, port, exportname).
>> It checks which key exists and decides use unix or inet socket.
>> It doesn't recognize the key type, so we can't use union, and
>> can't reuse InetSocketAddress.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>

>>  ##
>> +# @BlockdevOptionsNBD
>> +#
>> +# Driver specific block device options for NBD
>> +#
>> +# @filename: #optional unix or inet path. The format is:
>> +#            unix: nbd+unix:///export?socket=path or
>> +#                  nbd:unix:path:exportname=export
>> +#            inet: nbd[+tcp]://host[:port]/export or
>> +#                  nbd:host[:port]:exportname=export

Yuck. You are passing structured data through a single 'str', when you
SHOULD be passing it through structured JSON.  Just because we have a
filename shorthand for convenience does NOT mean that we want to expose
that convenience in QMP.  Instead, we really want the breakdown of the
pieces (here, using abbreviated syntax of an inline base, since there
are pending qapi patches that will allow it):

{ 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
{ 'union': 'BlockdevOptionsNBD',
  'base': { 'transport': 'NBDTransport', 'export': 'str' },
  'discriminator': 'transport',
  'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
{ 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
{ 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
     '*ipv4': 'bool', '*ipv6': 'bool' } }


> I'm afraid this doesn't address Eric's review of your v2.

Agreed; we still don't have the right interface.


> Eric further observed that support for the nbd+unix transport was
> missing, and suggested to have a union type combining the various
> transports.

And I just spelled out above what that should look like.

> 
> If we decide separate types for single port and port ranges aren't
> worthwhile, you can simply use SocketAddress where your v2 used
> InetSocketAddress.

I'm not sure if my 'NBDInet' above makes any more sense than reusing
'SocketAddress' (and if we do reuse 'SocketAddress', we have the further
question of whether to split out socket ranges as a separate type so
that SocketAddress becomes a single-port identity).

> 
> Eric, how strongly do you feel about separating the two?

I'm more worried about properly using a union type to represent unix vs.
tcp, and less worried about SocketAddress vs. range types vs creating a
new type (although in the long run, fixing ranges to be in a properly
named type rather than re-inventing the struct across multiple
transports is a good goal).  But you are quite correct that I do not
like the v3 proposal, because it encodes far too much information into a
single '*filename':'str', which is not the qapi way.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/14/2015 11:47 PM, Eric Blake wrote:
> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>
>>> The NBD driver needs: filename, path or (host, port, exportname).
>>> It checks which key exists and decides use unix or inet socket.
>>> It doesn't recognize the key type, so we can't use union, and
>>> can't reuse InetSocketAddress.
>>>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>
> 
>>>  ##
>>> +# @BlockdevOptionsNBD
>>> +#
>>> +# Driver specific block device options for NBD
>>> +#
>>> +# @filename: #optional unix or inet path. The format is:
>>> +#            unix: nbd+unix:///export?socket=path or
>>> +#                  nbd:unix:path:exportname=export
>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>> +#                  nbd:host[:port]:exportname=export
> 
> Yuck. You are passing structured data through a single 'str', when you
> SHOULD be passing it through structured JSON.  Just because we have a
> filename shorthand for convenience does NOT mean that we want to expose
> that convenience in QMP.  Instead, we really want the breakdown of the
> pieces (here, using abbreviated syntax of an inline base, since there
> are pending qapi patches that will allow it):

Do you mean that: there is no need to support filename?

> 
> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }

NBD only uses tcp, it doesn't support udp.

> { 'union': 'BlockdevOptionsNBD',
>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>   'discriminator': 'transport',
>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }

unix socket needs a path, and I think we can use UnixSocketAddress here.

> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>      '*ipv4': 'bool', '*ipv6': 'bool' } }

Thanks for the above, and I will try it.

> 
> 
>> I'm afraid this doesn't address Eric's review of your v2.
> 
> Agreed; we still don't have the right interface.
> 
> 
>> Eric further observed that support for the nbd+unix transport was
>> missing, and suggested to have a union type combining the various
>> transports.
> 
> And I just spelled out above what that should look like.
> 
>>
>> If we decide separate types for single port and port ranges aren't
>> worthwhile, you can simply use SocketAddress where your v2 used
>> InetSocketAddress.
> 
> I'm not sure if my 'NBDInet' above makes any more sense than reusing
> 'SocketAddress' (and if we do reuse 'SocketAddress', we have the further
> question of whether to split out socket ranges as a separate type so
> that SocketAddress becomes a single-port identity).
> 
>>
>> Eric, how strongly do you feel about separating the two?
> 
> I'm more worried about properly using a union type to represent unix vs.
> tcp, and less worried about SocketAddress vs. range types vs creating a
> new type (although in the long run, fixing ranges to be in a properly
> named type rather than re-inventing the struct across multiple
> transports is a good goal).  But you are quite correct that I do not
> like the v3 proposal, because it encodes far too much information into a
> single '*filename':'str', which is not the qapi way.
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/14/2015 11:47 PM, Eric Blake wrote:
>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>> It checks which key exists and decides use unix or inet socket.
>>>> It doesn't recognize the key type, so we can't use union, and
>>>> can't reuse InetSocketAddress.
>>>>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>> ---
>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>> 
>>>>  ##
>>>> +# @BlockdevOptionsNBD
>>>> +#
>>>> +# Driver specific block device options for NBD
>>>> +#
>>>> +# @filename: #optional unix or inet path. The format is:
>>>> +#            unix: nbd+unix:///export?socket=path or
>>>> +#                  nbd:unix:path:exportname=export
>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>> +#                  nbd:host[:port]:exportname=export
>> 
>> Yuck. You are passing structured data through a single 'str', when you
>> SHOULD be passing it through structured JSON.  Just because we have a
>> filename shorthand for convenience does NOT mean that we want to expose
>> that convenience in QMP.  Instead, we really want the breakdown of the
>> pieces (here, using abbreviated syntax of an inline base, since there
>> are pending qapi patches that will allow it):
>
> Do you mean that: there is no need to support filename?

Rule of thumb: if the QMP command handler needs to parse a string
argument, that argument should be a complex QAPI type instead.

Example: @filename needs to be parsed into its components, either

    * protocol unix, socket path, export name, or
    * protocol tcp, host, port, export name

Since there's an either/or, the complex QAPI type should be a union.

>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>
> NBD only uses tcp, it doesn't support udp.
>
>> { 'union': 'BlockdevOptionsNBD',
>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>   'discriminator': 'transport',
>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>
> unix socket needs a path, and I think we can use UnixSocketAddress here.

Yes, we should try to reuse common types like SocketAddress,
InetSocketAddress, UnixSocketAddress.

Perhaps it could be as simple as

    { 'struct': 'BlockdevOptionsNBD',
      'data': { 'addr: 'SocketAddress', 'export': 'str' } }

Eric, what do you think?

>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>
> Thanks for the above, and I will try it.

[...]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/15/2015 03:37 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>
>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>> It checks which key exists and decides use unix or inet socket.
>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>> can't reuse InetSocketAddress.
>>>>>
>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>> ---
>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>
>>>
>>>>>  ##
>>>>> +# @BlockdevOptionsNBD
>>>>> +#
>>>>> +# Driver specific block device options for NBD
>>>>> +#
>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>> +#                  nbd:unix:path:exportname=export
>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>> +#                  nbd:host[:port]:exportname=export
>>>
>>> Yuck. You are passing structured data through a single 'str', when you
>>> SHOULD be passing it through structured JSON.  Just because we have a
>>> filename shorthand for convenience does NOT mean that we want to expose
>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>> pieces (here, using abbreviated syntax of an inline base, since there
>>> are pending qapi patches that will allow it):
>>
>> Do you mean that: there is no need to support filename?
> 
> Rule of thumb: if the QMP command handler needs to parse a string
> argument, that argument should be a complex QAPI type instead.
> 
> Example: @filename needs to be parsed into its components, either
> 
>     * protocol unix, socket path, export name, or
>     * protocol tcp, host, port, export name
> 
> Since there's an either/or, the complex QAPI type should be a union.
> 
>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>
>> NBD only uses tcp, it doesn't support udp.
>>
>>> { 'union': 'BlockdevOptionsNBD',
>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>   'discriminator': 'transport',
>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>
>> unix socket needs a path, and I think we can use UnixSocketAddress here.
> 
> Yes, we should try to reuse common types like SocketAddress,
> InetSocketAddress, UnixSocketAddress.
> 
> Perhaps it could be as simple as
> 
>     { 'struct': 'BlockdevOptionsNBD',
>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }

The problem is that: NBD doesn't use the fd.

Another question is: what key will we see in nbd_open()? "addr.host"
or "host"?

Thanks
Wen Congyang

> 
> Eric, what do you think?
> 
>>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>>
>> Thanks for the above, and I will try it.
> 
> [...]
> .
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>
>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>> can't reuse InetSocketAddress.
>>>>>>
>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>> ---
>>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>
>>>>
>>>>>>  ##
>>>>>> +# @BlockdevOptionsNBD
>>>>>> +#
>>>>>> +# Driver specific block device options for NBD
>>>>>> +#
>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>
>>>> Yuck. You are passing structured data through a single 'str', when you
>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>> filename shorthand for convenience does NOT mean that we want to expose
>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>> are pending qapi patches that will allow it):
>>>
>>> Do you mean that: there is no need to support filename?
>> 
>> Rule of thumb: if the QMP command handler needs to parse a string
>> argument, that argument should be a complex QAPI type instead.
>> 
>> Example: @filename needs to be parsed into its components, either
>> 
>>     * protocol unix, socket path, export name, or
>>     * protocol tcp, host, port, export name
>> 
>> Since there's an either/or, the complex QAPI type should be a union.
>> 
>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>
>>> NBD only uses tcp, it doesn't support udp.
>>>
>>>> { 'union': 'BlockdevOptionsNBD',
>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>   'discriminator': 'transport',
>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>
>>> unix socket needs a path, and I think we can use UnixSocketAddress here.
>> 
>> Yes, we should try to reuse common types like SocketAddress,
>> InetSocketAddress, UnixSocketAddress.
>> 
>> Perhaps it could be as simple as
>> 
>>     { 'struct': 'BlockdevOptionsNBD',
>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>
> The problem is that: NBD doesn't use the fd.

Is that fundamental, or just a matter of implementation?

> Another question is: what key will we see in nbd_open()? "addr.host"
> or "host"?

As long as nbd_config() looks for "host" in the options QDict, we need
to put "host".

> Thanks
> Wen Congyang
>
>> 
>> Eric, what do you think?
>> 
>>>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>>>
>>> Thanks for the above, and I will try it.
>> 
>> [...]
>> .
>> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/15/2015 07:12 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>
>>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>>> can't reuse InetSocketAddress.
>>>>>>>
>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>> ---
>>>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>
>>>>>>>  ##
>>>>>>> +# @BlockdevOptionsNBD
>>>>>>> +#
>>>>>>> +# Driver specific block device options for NBD
>>>>>>> +#
>>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>>
>>>>> Yuck. You are passing structured data through a single 'str', when you
>>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>>> filename shorthand for convenience does NOT mean that we want to expose
>>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>>> are pending qapi patches that will allow it):
>>>>
>>>> Do you mean that: there is no need to support filename?
>>>
>>> Rule of thumb: if the QMP command handler needs to parse a string
>>> argument, that argument should be a complex QAPI type instead.
>>>
>>> Example: @filename needs to be parsed into its components, either
>>>
>>>     * protocol unix, socket path, export name, or
>>>     * protocol tcp, host, port, export name
>>>
>>> Since there's an either/or, the complex QAPI type should be a union.
>>>
>>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>>
>>>> NBD only uses tcp, it doesn't support udp.
>>>>
>>>>> { 'union': 'BlockdevOptionsNBD',
>>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>>   'discriminator': 'transport',
>>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>>
>>>> unix socket needs a path, and I think we can use UnixSocketAddress here.
>>>
>>> Yes, we should try to reuse common types like SocketAddress,
>>> InetSocketAddress, UnixSocketAddress.
>>>
>>> Perhaps it could be as simple as
>>>
>>>     { 'struct': 'BlockdevOptionsNBD',
>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>
>> The problem is that: NBD doesn't use the fd.
> 
> Is that fundamental, or just a matter of implementation?
> 
>> Another question is: what key will we see in nbd_open()? "addr.host"
>> or "host"?
> 
> As long as nbd_config() looks for "host" in the options QDict, we need
> to put "host".

Yes, we need "host" in nbd_config(), but we pass "addr.data.host" to it.

How to avoid this problem?

Thanks
Wen Congyang

> 
>> Thanks
>> Wen Congyang
>>
>>>
>>> Eric, what do you think?
>>>
>>>>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>>>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>>>>
>>>> Thanks for the above, and I will try it.
>>>
>>> [...]
>>> .
>>>
> .
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>
>>>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>
>>>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>>>> can't reuse InetSocketAddress.
>>>>>>>>
>>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>>> ---
>>>>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>
>>>>>>>>  ##
>>>>>>>> +# @BlockdevOptionsNBD
>>>>>>>> +#
>>>>>>>> +# Driver specific block device options for NBD
>>>>>>>> +#
>>>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>>>
>>>>>> Yuck. You are passing structured data through a single 'str', when you
>>>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>>>> filename shorthand for convenience does NOT mean that we want to expose
>>>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>>>> are pending qapi patches that will allow it):
>>>>>
>>>>> Do you mean that: there is no need to support filename?
>>>>
>>>> Rule of thumb: if the QMP command handler needs to parse a string
>>>> argument, that argument should be a complex QAPI type instead.
>>>>
>>>> Example: @filename needs to be parsed into its components, either
>>>>
>>>>     * protocol unix, socket path, export name, or
>>>>     * protocol tcp, host, port, export name
>>>>
>>>> Since there's an either/or, the complex QAPI type should be a union.
>>>>
>>>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>>>
>>>>> NBD only uses tcp, it doesn't support udp.
>>>>>
>>>>>> { 'union': 'BlockdevOptionsNBD',
>>>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>>>   'discriminator': 'transport',
>>>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>>>
>>>>> unix socket needs a path, and I think we can use UnixSocketAddress here.
>>>>
>>>> Yes, we should try to reuse common types like SocketAddress,
>>>> InetSocketAddress, UnixSocketAddress.
>>>>
>>>> Perhaps it could be as simple as
>>>>
>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>
>>> The problem is that: NBD doesn't use the fd.
>> 
>> Is that fundamental, or just a matter of implementation?

Question still open.

>>> Another question is: what key will we see in nbd_open()? "addr.host"
>>> or "host"?
>> 
>> As long as nbd_config() looks for "host" in the options QDict, we need
>> to put "host".
>
> Yes, we need "host" in nbd_config(), but we pass "addr.data.host" to it.
>
> How to avoid this problem?

Where is the code constructing the QDict?

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/16/2015 04:21 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>
>>>>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>>
>>>>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>>>>> can't reuse InetSocketAddress.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>>>> ---
>>>>>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>
>>>>>>>>>  ##
>>>>>>>>> +# @BlockdevOptionsNBD
>>>>>>>>> +#
>>>>>>>>> +# Driver specific block device options for NBD
>>>>>>>>> +#
>>>>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>>>>
>>>>>>> Yuck. You are passing structured data through a single 'str', when you
>>>>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>>>>> filename shorthand for convenience does NOT mean that we want to expose
>>>>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>>>>> are pending qapi patches that will allow it):
>>>>>>
>>>>>> Do you mean that: there is no need to support filename?
>>>>>
>>>>> Rule of thumb: if the QMP command handler needs to parse a string
>>>>> argument, that argument should be a complex QAPI type instead.
>>>>>
>>>>> Example: @filename needs to be parsed into its components, either
>>>>>
>>>>>     * protocol unix, socket path, export name, or
>>>>>     * protocol tcp, host, port, export name
>>>>>
>>>>> Since there's an either/or, the complex QAPI type should be a union.
>>>>>
>>>>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>>>>
>>>>>> NBD only uses tcp, it doesn't support udp.
>>>>>>
>>>>>>> { 'union': 'BlockdevOptionsNBD',
>>>>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>>>>   'discriminator': 'transport',
>>>>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>>>>
>>>>>> unix socket needs a path, and I think we can use UnixSocketAddress here.
>>>>>
>>>>> Yes, we should try to reuse common types like SocketAddress,
>>>>> InetSocketAddress, UnixSocketAddress.
>>>>>
>>>>> Perhaps it could be as simple as
>>>>>
>>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>>
>>>> The problem is that: NBD doesn't use the fd.
>>>
>>> Is that fundamental, or just a matter of implementation?
> 
> Question still open.
> 
>>>> Another question is: what key will we see in nbd_open()? "addr.host"
>>>> or "host"?
>>>
>>> As long as nbd_config() looks for "host" in the options QDict, we need
>>> to put "host".
>>
>> Yes, we need "host" in nbd_config(), but we pass "addr.data.host" to it.
>>
>> How to avoid this problem?
> 
> Where is the code constructing the QDict?

The QDict is constructed in qmp_blockdev_add():
    visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
                               &options, NULL, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        goto fail;
    }

    obj = qmp_output_get_qobject(ov);
    qdict = qobject_to_qdict(obj);

    qdict_flatten(qdict);

Thanks
Wen Congyang
> .
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/16/2015 04:21 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>
>>>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>
>>>>>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>>>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>>>
>>>>>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>>>>>> can't reuse InetSocketAddress.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>
>>>>>>>>>>  ##
>>>>>>>>>> +# @BlockdevOptionsNBD
>>>>>>>>>> +#
>>>>>>>>>> +# Driver specific block device options for NBD
>>>>>>>>>> +#
>>>>>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>>>>>
>>>>>>>> Yuck. You are passing structured data through a single 'str', when you
>>>>>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>>>>>> filename shorthand for convenience does NOT mean that we want to expose
>>>>>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>>>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>>>>>> are pending qapi patches that will allow it):
>>>>>>>
>>>>>>> Do you mean that: there is no need to support filename?
>>>>>>
>>>>>> Rule of thumb: if the QMP command handler needs to parse a string
>>>>>> argument, that argument should be a complex QAPI type instead.
>>>>>>
>>>>>> Example: @filename needs to be parsed into its components, either
>>>>>>
>>>>>>     * protocol unix, socket path, export name, or
>>>>>>     * protocol tcp, host, port, export name
>>>>>>
>>>>>> Since there's an either/or, the complex QAPI type should be a union.
>>>>>>
>>>>>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>>>>>
>>>>>>> NBD only uses tcp, it doesn't support udp.
>>>>>>>
>>>>>>>> { 'union': 'BlockdevOptionsNBD',
>>>>>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>>>>>   'discriminator': 'transport',
>>>>>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>>>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>>>>>
>>>>>>> unix socket needs a path, and I think we can use UnixSocketAddress here.
>>>>>>
>>>>>> Yes, we should try to reuse common types like SocketAddress,
>>>>>> InetSocketAddress, UnixSocketAddress.
>>>>>>
>>>>>> Perhaps it could be as simple as
>>>>>>
>>>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>>>
>>>>> The problem is that: NBD doesn't use the fd.
>>>>
>>>> Is that fundamental, or just a matter of implementation?
>> 
>> Question still open.

Question still open.

>>>>> Another question is: what key will we see in nbd_open()? "addr.host"
>>>>> or "host"?
>>>>
>>>> As long as nbd_config() looks for "host" in the options QDict, we need
>>>> to put "host".
>>>
>>> Yes, we need "host" in nbd_config(), but we pass "addr.data.host" to it.
>>>
>>> How to avoid this problem?
>> 
>> Where is the code constructing the QDict?
>
> The QDict is constructed in qmp_blockdev_add():
>     visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
>                                &options, NULL, &local_err);
>     if (local_err) {
>         error_propagate(errp, local_err);
>         goto fail;
>     }
>
>     obj = qmp_output_get_qobject(ov);
>     qdict = qobject_to_qdict(obj);
>
>     qdict_flatten(qdict);

Okay.

Long term, I'd like to see us get rid of the conversions between
QAPI-generated types and QDict / QemuOpts.

Short term, you need to co-evolve the QDict-based code such as
nbd_config() with the QAPI interfaces.

Keeping the QAPI interface clean is more important than minimizing the
implementation work, because we're free to mess with the implementation,
but releasing a QAPI interface makes it ABI, so we better get it right.

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/5] support nbd driver in blockdev-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On 09/16/2015 05:18 AM, Markus Armbruster wrote:

>>>>>>> Perhaps it could be as simple as
>>>>>>>
>>>>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>>>>
>>>>>> The problem is that: NBD doesn't use the fd.
>>>>>
>>>>> Is that fundamental, or just a matter of implementation?
>>>
>>> Question still open.
> 
> Question still open.

Dan's patches didn't address it...

> Long term, I'd like to see us get rid of the conversions between
> QAPI-generated types and QDict / QemuOpts.
> 
> Short term, you need to co-evolve the QDict-based code such as
> nbd_config() with the QAPI interfaces.

...but DO affect the short-term, by starting the conversion over to
using the QAPI type more fully:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04383.html

> 
> Keeping the QAPI interface clean is more important than minimizing the
> implementation work, because we're free to mess with the implementation,
> but releasing a QAPI interface makes it ABI, so we better get it right.

Especially once the QAPI interface is actually used by a QMP command
(there are places where we are using qapi internally for ease in command
line handling, but not exposing the structures through QMP yet).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/16/2015 10:53 PM, Eric Blake wrote:
> On 09/16/2015 05:18 AM, Markus Armbruster wrote:
> 
>>>>>>>> Perhaps it could be as simple as
>>>>>>>>
>>>>>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>>>>>
>>>>>>> The problem is that: NBD doesn't use the fd.
>>>>>>
>>>>>> Is that fundamental, or just a matter of implementation?
>>>>
>>>> Question still open.
>>
>> Question still open.
> 
> Dan's patches didn't address it...
> 
>> Long term, I'd like to see us get rid of the conversions between
>> QAPI-generated types and QDict / QemuOpts.
>>
>> Short term, you need to co-evolve the QDict-based code such as
>> nbd_config() with the QAPI interfaces.
> 
> ...but DO affect the short-term, by starting the conversion over to
> using the QAPI type more fully:
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04383.html

The problem still exists with this patch, because we pass "addr.data.host"
to nbd_open().

Thanks
Wen Congyang

> 
>>
>> Keeping the QAPI interface clean is more important than minimizing the
>> implementation work, because we're free to mess with the implementation,
>> but releasing a QAPI interface makes it ABI, so we better get it right.
> 
> Especially once the QAPI interface is actually used by a QMP command
> (there are places where we are using qapi internally for ease in command
> line handling, but not exposing the structures through QMP yet).
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/16/2015 07:18 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/16/2015 04:21 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>
>>>>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>
>>>>>>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>>>>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>>>>
>>>>>>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>>>>>>> can't reuse InetSocketAddress.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>  ##
>>>>>>>>>>> +# @BlockdevOptionsNBD
>>>>>>>>>>> +#
>>>>>>>>>>> +# Driver specific block device options for NBD
>>>>>>>>>>> +#
>>>>>>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>>>>>>
>>>>>>>>> Yuck. You are passing structured data through a single 'str', when you
>>>>>>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>>>>>>> filename shorthand for convenience does NOT mean that we want to expose
>>>>>>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>>>>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>>>>>>> are pending qapi patches that will allow it):
>>>>>>>>
>>>>>>>> Do you mean that: there is no need to support filename?
>>>>>>>
>>>>>>> Rule of thumb: if the QMP command handler needs to parse a string
>>>>>>> argument, that argument should be a complex QAPI type instead.
>>>>>>>
>>>>>>> Example: @filename needs to be parsed into its components, either
>>>>>>>
>>>>>>>     * protocol unix, socket path, export name, or
>>>>>>>     * protocol tcp, host, port, export name
>>>>>>>
>>>>>>> Since there's an either/or, the complex QAPI type should be a union.
>>>>>>>
>>>>>>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>>>>>>
>>>>>>>> NBD only uses tcp, it doesn't support udp.
>>>>>>>>
>>>>>>>>> { 'union': 'BlockdevOptionsNBD',
>>>>>>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>>>>>>   'discriminator': 'transport',
>>>>>>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>>>>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>>>>>>
>>>>>>>> unix socket needs a path, and I think we can use UnixSocketAddress here.
>>>>>>>
>>>>>>> Yes, we should try to reuse common types like SocketAddress,
>>>>>>> InetSocketAddress, UnixSocketAddress.
>>>>>>>
>>>>>>> Perhaps it could be as simple as
>>>>>>>
>>>>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>>>>
>>>>>> The problem is that: NBD doesn't use the fd.
>>>>>
>>>>> Is that fundamental, or just a matter of implementation?
>>>
>>> Question still open.
> 
> Question still open.

It is just a matter of implementation. For other drivers, if the user specifies
an unknown option, we will report the error before calling qmp_blockdev_add().

If we allow the user specify fd, we may report the error in bdrv_open().

> 
>>>>>> Another question is: what key will we see in nbd_open()? "addr.host"
>>>>>> or "host"?
>>>>>
>>>>> As long as nbd_config() looks for "host" in the options QDict, we need
>>>>> to put "host".
>>>>
>>>> Yes, we need "host" in nbd_config(), but we pass "addr.data.host" to it.
>>>>
>>>> How to avoid this problem?
>>>
>>> Where is the code constructing the QDict?
>>
>> The QDict is constructed in qmp_blockdev_add():
>>     visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
>>                                &options, NULL, &local_err);
>>     if (local_err) {
>>         error_propagate(errp, local_err);
>>         goto fail;
>>     }
>>
>>     obj = qmp_output_get_qobject(ov);
>>     qdict = qobject_to_qdict(obj);
>>
>>     qdict_flatten(qdict);
> 
> Okay.
> 
> Long term, I'd like to see us get rid of the conversions between
> QAPI-generated types and QDict / QemuOpts.
> 
> Short term, you need to co-evolve the QDict-based code such as
> nbd_config() with the QAPI interfaces.
> 
> Keeping the QAPI interface clean is more important than minimizing the
> implementation work, because we're free to mess with the implementation,
> but releasing a QAPI interface makes it ABI, so we better get it right.
> .
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/16/2015 07:18 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> On 09/16/2015 04:21 PM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>
>>>>> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>
>>>>>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>>
>>>>>>>>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>>>>>>>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>>>>>>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>>>>>>>>
>>>>>>>>>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>>>>>>>>>> It checks which key exists and decides use unix or inet socket.
>>>>>>>>>>>> It doesn't recognize the key type, so we can't use union, and
>>>>>>>>>>>> can't reuse InetSocketAddress.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  qapi/block-core.json | 42
>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>  ##
>>>>>>>>>>>> +# @BlockdevOptionsNBD
>>>>>>>>>>>> +#
>>>>>>>>>>>> +# Driver specific block device options for NBD
>>>>>>>>>>>> +#
>>>>>>>>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>>>>>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>>>>>>>>> +#                  nbd:unix:path:exportname=export
>>>>>>>>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>>>>>>>>> +#                  nbd:host[:port]:exportname=export
>>>>>>>>>>
>>>>>>>>>> Yuck. You are passing structured data through a single 'str', when you
>>>>>>>>>> SHOULD be passing it through structured JSON.  Just because we have a
>>>>>>>>>> filename shorthand for convenience does NOT mean that we
>>>>>>>>>> want to expose
>>>>>>>>>> that convenience in QMP.  Instead, we really want the breakdown of the
>>>>>>>>>> pieces (here, using abbreviated syntax of an inline base, since there
>>>>>>>>>> are pending qapi patches that will allow it):
>>>>>>>>>
>>>>>>>>> Do you mean that: there is no need to support filename?
>>>>>>>>
>>>>>>>> Rule of thumb: if the QMP command handler needs to parse a string
>>>>>>>> argument, that argument should be a complex QAPI type instead.
>>>>>>>>
>>>>>>>> Example: @filename needs to be parsed into its components, either
>>>>>>>>
>>>>>>>>     * protocol unix, socket path, export name, or
>>>>>>>>     * protocol tcp, host, port, export name
>>>>>>>>
>>>>>>>> Since there's an either/or, the complex QAPI type should be a union.
>>>>>>>>
>>>>>>>>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>>>>>>>>
>>>>>>>>> NBD only uses tcp, it doesn't support udp.
>>>>>>>>>
>>>>>>>>>> { 'union': 'BlockdevOptionsNBD',
>>>>>>>>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>>>>>>>>   'discriminator': 'transport',
>>>>>>>>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>>>>>>>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>>>>>>>>
>>>>>>>>> unix socket needs a path, and I think we can use
>>>>>>>>> UnixSocketAddress here.
>>>>>>>>
>>>>>>>> Yes, we should try to reuse common types like SocketAddress,
>>>>>>>> InetSocketAddress, UnixSocketAddress.
>>>>>>>>
>>>>>>>> Perhaps it could be as simple as
>>>>>>>>
>>>>>>>>     { 'struct': 'BlockdevOptionsNBD',
>>>>>>>>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
>>>>>>>
>>>>>>> The problem is that: NBD doesn't use the fd.
>>>>>>
>>>>>> Is that fundamental, or just a matter of implementation?
>>>>
>>>> Question still open.
>> 
>> Question still open.
>
> It is just a matter of implementation. For other drivers, if the user specifies
> an unknown option, we will report the error before calling qmp_blockdev_add().
>
> If we allow the user specify fd, we may report the error in bdrv_open().

In general, we want fd support, because it lets us confine QEMU more
tightly, and open / connect / bind stuff in libvirt.  Not this patch's
problem, of course.

In this particular case, since fd support is possible, we want its
introduction be visible in introspection.  The obvious way for that is
to use a union of exactly the *SocketAddress types that are actually
supported.  Now: new one containing InetSocketAddress and
UnixSocketAddress.  Once fd is supported as well, SocketAddress.

[...]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On 09/15/2015 01:37 AM, Markus Armbruster wrote:

>>>>> +# @filename: #optional unix or inet path. The format is:
>>>>> +#            unix: nbd+unix:///export?socket=path or
>>>>> +#                  nbd:unix:path:exportname=export
>>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>>> +#                  nbd:host[:port]:exportname=export
>>>

> Since there's an either/or, the complex QAPI type should be a union.
> 
>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>
>> NBD only uses tcp, it doesn't support udp.

Fair enough.

>>
>>> { 'union': 'BlockdevOptionsNBD',
>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>   'discriminator': 'transport',
>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>
>> unix socket needs a path, and I think we can use UnixSocketAddress here.

Sure, if that works, you could do 'unix':'UnixSocketAddress' instead of
inventing 'NBDUnix'.

> 
> Yes, we should try to reuse common types like SocketAddress,
> InetSocketAddress, UnixSocketAddress.

Well, we've already questioned whether 'InetSocketAddress' needs to be
fixed to be separate from a socket range, but it can be a separate fix.

> 
> Perhaps it could be as simple as
> 
>     { 'struct': 'BlockdevOptionsNBD',
>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
> 
> Eric, what do you think?

It has more nesting on the wire, but should work.  That is, to express
"nbd+unix:///export?socket=path", the QMP would be:

{ "export":"export", "addr":{ "type":"unix", "data":{ "path": "str"}}}

instead of a nicer:

{ "export":"export", "type":"unix", "path":"str" }

but the advantage of working now rather than waiting on qapi fixes in
the pipeline has its benefit.

There's also the question of how to handle 'fd', if NBD can't reuse an
existing fd but must be given a unix socket filename or tcp host/port
destination.  Documenting that we reject a combination may be okay,
except that it is harder to introspect later if the combination is no
longer rejected because we later add support.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

Am 15.09.2015 um 15:03 hat Eric Blake geschrieben:
> On 09/15/2015 01:37 AM, Markus Armbruster wrote:
> 
> >>>>> +# @filename: #optional unix or inet path. The format is:
> >>>>> +#            unix: nbd+unix:///export?socket=path or
> >>>>> +#                  nbd:unix:path:exportname=export
> >>>>> +#            inet: nbd[+tcp]://host[:port]/export or
> >>>>> +#                  nbd:host[:port]:exportname=export
> >>>
> 
> > Since there's an either/or, the complex QAPI type should be a union.
> > 
> >>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
> >>
> >> NBD only uses tcp, it doesn't support udp.
> 
> Fair enough.
> 
> >>
> >>> { 'union': 'BlockdevOptionsNBD',
> >>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
> >>>   'discriminator': 'transport',
> >>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
> >>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
> >>
> >> unix socket needs a path, and I think we can use UnixSocketAddress here.
> 
> Sure, if that works, you could do 'unix':'UnixSocketAddress' instead of
> inventing 'NBDUnix'.
> 
> > 
> > Yes, we should try to reuse common types like SocketAddress,
> > InetSocketAddress, UnixSocketAddress.
> 
> Well, we've already questioned whether 'InetSocketAddress' needs to be
> fixed to be separate from a socket range, but it can be a separate fix.
> 
> > 
> > Perhaps it could be as simple as
> > 
> >     { 'struct': 'BlockdevOptionsNBD',
> >       'data': { 'addr: 'SocketAddress', 'export': 'str' } }
> > 
> > Eric, what do you think?
> 
> It has more nesting on the wire, but should work.  That is, to express
> "nbd+unix:///export?socket=path", the QMP would be:
> 
> { "export":"export", "addr":{ "type":"unix", "data":{ "path": "str"}}}
> 
> instead of a nicer:
> 
> { "export":"export", "type":"unix", "path":"str" }
> 
> but the advantage of working now rather than waiting on qapi fixes in
> the pipeline has its benefit.

Both patches are for 2.5 anyway. Is the benefit of getting this in
before the QAPI series really that big that we should go with an ugly
syntax?

> There's also the question of how to handle 'fd', if NBD can't reuse an
> existing fd but must be given a unix socket filename or tcp host/port
> destination.  Documenting that we reject a combination may be okay,
> except that it is harder to introspect later if the combination is no
> longer rejected because we later add support.

How about implementing fd suppor instead?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/14/2015 11:47 PM, Eric Blake wrote:
> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>
>>> The NBD driver needs: filename, path or (host, port, exportname).
>>> It checks which key exists and decides use unix or inet socket.
>>> It doesn't recognize the key type, so we can't use union, and
>>> can't reuse InetSocketAddress.
>>>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>
> 
>>>  ##
>>> +# @BlockdevOptionsNBD
>>> +#
>>> +# Driver specific block device options for NBD
>>> +#
>>> +# @filename: #optional unix or inet path. The format is:
>>> +#            unix: nbd+unix:///export?socket=path or
>>> +#                  nbd:unix:path:exportname=export
>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>> +#                  nbd:host[:port]:exportname=export
> 
> Yuck. You are passing structured data through a single 'str', when you
> SHOULD be passing it through structured JSON.  Just because we have a
> filename shorthand for convenience does NOT mean that we want to expose
> that convenience in QMP.  Instead, we really want the breakdown of the
> pieces (here, using abbreviated syntax of an inline base, since there
> are pending qapi patches that will allow it):
> 
> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
> { 'union': 'BlockdevOptionsNBD',
>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>   'discriminator': 'transport',
>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }

Building fails:
  GEN   qmp-commands.h
In file included from /work/src/qemu/qapi-schema.json:9:
In file included from /work/src/qemu/qapi/block.json:6:
/work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must have a string base field
Makefile:286: recipe for target 'qmp-commands.h' failed
make: *** [qmp-commands.h] Error 1

What about this:
{ 'struct': 'BlockdevOptionsNBDBase',
  'data': {  'transport': 'NBDTransport', 'export': 'str' } }
{ 'union': 'BlockdevOptionsNBD',
  'base': 'BlockdevOptionsNBDBase',
  'discriminator': 'transport',
  'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }

Thanks
Wen Congyang

> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>      '*ipv4': 'bool', '*ipv6': 'bool' } }
> 
> 
>> I'm afraid this doesn't address Eric's review of your v2.
> 
> Agreed; we still don't have the right interface.
> 
> 
>> Eric further observed that support for the nbd+unix transport was
>> missing, and suggested to have a union type combining the various
>> transports.
> 
> And I just spelled out above what that should look like.
> 
>>
>> If we decide separate types for single port and port ranges aren't
>> worthwhile, you can simply use SocketAddress where your v2 used
>> InetSocketAddress.
> 
> I'm not sure if my 'NBDInet' above makes any more sense than reusing
> 'SocketAddress' (and if we do reuse 'SocketAddress', we have the further
> question of whether to split out socket ranges as a separate type so
> that SocketAddress becomes a single-port identity).
> 
>>
>> Eric, how strongly do you feel about separating the two?
> 
> I'm more worried about properly using a union type to represent unix vs.
> tcp, and less worried about SocketAddress vs. range types vs creating a
> new type (although in the long run, fixing ranges to be in a properly
> named type rather than re-inventing the struct across multiple
> transports is a good goal).  But you are quite correct that I do not
> like the v3 proposal, because it encodes far too much information into a
> single '*filename':'str', which is not the qapi way.
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/15/2015 10:20 AM, Wen Congyang wrote:
> On 09/14/2015 11:47 PM, Eric Blake wrote:
>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>> It checks which key exists and decides use unix or inet socket.
>>>> It doesn't recognize the key type, so we can't use union, and
>>>> can't reuse InetSocketAddress.
>>>>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>> ---
>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>
>>>>  ##
>>>> +# @BlockdevOptionsNBD
>>>> +#
>>>> +# Driver specific block device options for NBD
>>>> +#
>>>> +# @filename: #optional unix or inet path. The format is:
>>>> +#            unix: nbd+unix:///export?socket=path or
>>>> +#                  nbd:unix:path:exportname=export
>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>> +#                  nbd:host[:port]:exportname=export
>>
>> Yuck. You are passing structured data through a single 'str', when you
>> SHOULD be passing it through structured JSON.  Just because we have a
>> filename shorthand for convenience does NOT mean that we want to expose
>> that convenience in QMP.  Instead, we really want the breakdown of the
>> pieces (here, using abbreviated syntax of an inline base, since there
>> are pending qapi patches that will allow it):
>>
>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>> { 'union': 'BlockdevOptionsNBD',
>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>   'discriminator': 'transport',
>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
> 
> Building fails:
>   GEN   qmp-commands.h
> In file included from /work/src/qemu/qapi-schema.json:9:
> In file included from /work/src/qemu/qapi/block.json:6:
> /work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must have a string base field
> Makefile:286: recipe for target 'qmp-commands.h' failed
> make: *** [qmp-commands.h] Error 1
> 
> What about this:
> { 'struct': 'BlockdevOptionsNBDBase',
>   'data': {  'transport': 'NBDTransport', 'export': 'str' } }
> { 'union': 'BlockdevOptionsNBD',
>   'base': 'BlockdevOptionsNBDBase',
>   'discriminator': 'transport',
>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }

Another problem:
In file included from /work/src/qemu/qapi-schema.json:9:
In file included from /work/src/qemu/qapi/block.json:6:
/work/src/qemu/qapi/block-core.json:1866: Member 'nbd' of union 'BlockdevOptions' cannot use union type 'BlockdevOptionsNBD'
Makefile:286: recipe for target 'qmp-commands.h' failed

Thanks
Wen Congyang

> 
> Thanks
> Wen Congyang
> 
>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>>
>>
>>> I'm afraid this doesn't address Eric's review of your v2.
>>
>> Agreed; we still don't have the right interface.
>>
>>
>>> Eric further observed that support for the nbd+unix transport was
>>> missing, and suggested to have a union type combining the various
>>> transports.
>>
>> And I just spelled out above what that should look like.
>>
>>>
>>> If we decide separate types for single port and port ranges aren't
>>> worthwhile, you can simply use SocketAddress where your v2 used
>>> InetSocketAddress.
>>
>> I'm not sure if my 'NBDInet' above makes any more sense than reusing
>> 'SocketAddress' (and if we do reuse 'SocketAddress', we have the further
>> question of whether to split out socket ranges as a separate type so
>> that SocketAddress becomes a single-port identity).
>>
>>>
>>> Eric, how strongly do you feel about separating the two?
>>
>> I'm more worried about properly using a union type to represent unix vs.
>> tcp, and less worried about SocketAddress vs. range types vs creating a
>> new type (although in the long run, fixing ranges to be in a properly
>> named type rather than re-inventing the struct across multiple
>> transports is a good goal).  But you are quite correct that I do not
>> like the v3 proposal, because it encodes far too much information into a
>> single '*filename':'str', which is not the qapi way.
>>
> 
> 
> .
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On 09/14/2015 08:27 PM, Wen Congyang wrote:
>> Building fails:
>>   GEN   qmp-commands.h
>> In file included from /work/src/qemu/qapi-schema.json:9:
>> In file included from /work/src/qemu/qapi/block.json:6:
>> /work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must have a string base field
>> Makefile:286: recipe for target 'qmp-commands.h' failed
>> make: *** [qmp-commands.h] Error 1

Yep, doesn't work until pending qapi patches land.

>>
>> What about this:
>> { 'struct': 'BlockdevOptionsNBDBase',
>>   'data': {  'transport': 'NBDTransport', 'export': 'str' } }
>> { 'union': 'BlockdevOptionsNBD',
>>   'base': 'BlockdevOptionsNBDBase',
>>   'discriminator': 'transport',
>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
> 
> Another problem:
> In file included from /work/src/qemu/qapi-schema.json:9:
> In file included from /work/src/qemu/qapi/block.json:6:
> /work/src/qemu/qapi/block-core.json:1866: Member 'nbd' of union 'BlockdevOptions' cannot use union type 'BlockdevOptionsNBD'
> Makefile:286: recipe for target 'qmp-commands.h' failed

Yep. Artificial restriction; we hope to lift it soon, but don't have
enough qapi patches in place for that one yet (I have not posted my work
in progress to get us that far).

Possible workaround in the meantime - instead of trying to go with a
nice flat union (where all QMP keys are in the same {} level), we can
use nesting (structs that add another {} to include the unions).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/15/2015 11:46 AM, Eric Blake wrote:
> On 09/14/2015 08:27 PM, Wen Congyang wrote:
>>> Building fails:
>>>   GEN   qmp-commands.h
>>> In file included from /work/src/qemu/qapi-schema.json:9:
>>> In file included from /work/src/qemu/qapi/block.json:6:
>>> /work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must have a string base field
>>> Makefile:286: recipe for target 'qmp-commands.h' failed
>>> make: *** [qmp-commands.h] Error 1
> 
> Yep, doesn't work until pending qapi patches land.

This patchset: qapi: QMP introspection?

> 
>>>
>>> What about this:
>>> { 'struct': 'BlockdevOptionsNBDBase',
>>>   'data': {  'transport': 'NBDTransport', 'export': 'str' } }
>>> { 'union': 'BlockdevOptionsNBD',
>>>   'base': 'BlockdevOptionsNBDBase',
>>>   'discriminator': 'transport',
>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>
>> Another problem:
>> In file included from /work/src/qemu/qapi-schema.json:9:
>> In file included from /work/src/qemu/qapi/block.json:6:
>> /work/src/qemu/qapi/block-core.json:1866: Member 'nbd' of union 'BlockdevOptions' cannot use union type 'BlockdevOptionsNBD'
>> Makefile:286: recipe for target 'qmp-commands.h' failed
> 
> Yep. Artificial restriction; we hope to lift it soon, but don't have
> enough qapi patches in place for that one yet (I have not posted my work
> in progress to get us that far).
> 
> Possible workaround in the meantime - instead of trying to go with a
> nice flat union (where all QMP keys are in the same {} level), we can
> use nesting (structs that add another {} to include the unions).

How to include the unions to a structs? Use 'base'?

Thanks
Wen Congyang

> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On 09/14/2015 09:58 PM, Wen Congyang wrote:
> On 09/15/2015 11:46 AM, Eric Blake wrote:
>> On 09/14/2015 08:27 PM, Wen Congyang wrote:
>>>> Building fails:
>>>>   GEN   qmp-commands.h
>>>> In file included from /work/src/qemu/qapi-schema.json:9:
>>>> In file included from /work/src/qemu/qapi/block.json:6:
>>>> /work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must have a string base field
>>>> Makefile:286: recipe for target 'qmp-commands.h' failed
>>>> make: *** [qmp-commands.h] Error 1
>>
>> Yep, doesn't work until pending qapi patches land.
> 
> This patchset: qapi: QMP introspection?

That, and "qapi-ify netdev_add, and other post-introspection cleanups"
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html

and "qapi: support anonymous inline base"
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02346.html
[still needs rebasing to latest versions of the other series]


>>
>> Possible workaround in the meantime - instead of trying to go with a
>> nice flat union (where all QMP keys are in the same {} level), we can
>> use nesting (structs that add another {} to include the unions).
> 
> How to include the unions to a structs? Use 'base'?

Conceptually, by adding a layer of nesting.  On the wire, instead of:

{ "switch1":"value", "switch2":"value", "body2":"blah" }

you would instead have:

{ "switch1":"value", "data": { "switch2":"value", "body2":"blah" } }

Anywhere in qapi that you try to have:
{ 'union': ..., 'data':{'switch1':'Union'}}

you instead create a wrapper type:
{ 'struct':'Wrapper', 'data':{'data':'Union'}}
{ 'union': ..., 'data':{'switch1':'Wrapper'}}


What I don't know is whether the extra QMP nesting makes it easier or
harder to support the existing NBD command line options, and it would
ultimately be nice to have unified support so that anything we can do on
the command line can be expressed in QMP; and anything we can do in QMP
can be expressed on the command line without undue nesting.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/15/2015 09:11 PM, Eric Blake wrote:
> On 09/14/2015 09:58 PM, Wen Congyang wrote:
>> On 09/15/2015 11:46 AM, Eric Blake wrote:
>>> On 09/14/2015 08:27 PM, Wen Congyang wrote:
>>>>> Building fails:
>>>>>   GEN   qmp-commands.h
>>>>> In file included from /work/src/qemu/qapi-schema.json:9:
>>>>> In file included from /work/src/qemu/qapi/block.json:6:
>>>>> /work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must have a string base field
>>>>> Makefile:286: recipe for target 'qmp-commands.h' failed
>>>>> make: *** [qmp-commands.h] Error 1
>>>
>>> Yep, doesn't work until pending qapi patches land.
>>
>> This patchset: qapi: QMP introspection?
> 
> That, and "qapi-ify netdev_add, and other post-introspection cleanups"
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html
> 
> and "qapi: support anonymous inline base"
> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02346.html
> [still needs rebasing to latest versions of the other series]
> 
> 
>>>
>>> Possible workaround in the meantime - instead of trying to go with a
>>> nice flat union (where all QMP keys are in the same {} level), we can
>>> use nesting (structs that add another {} to include the unions).
>>
>> How to include the unions to a structs? Use 'base'?
> 
> Conceptually, by adding a layer of nesting.  On the wire, instead of:
> 
> { "switch1":"value", "switch2":"value", "body2":"blah" }
> 
> you would instead have:
> 
> { "switch1":"value", "data": { "switch2":"value", "body2":"blah" } }
> 
> Anywhere in qapi that you try to have:
> { 'union': ..., 'data':{'switch1':'Union'}}
> 
> you instead create a wrapper type:
> { 'struct':'Wrapper', 'data':{'data':'Union'}}
> { 'union': ..., 'data':{'switch1':'Wrapper'}}

If so, the option is "data.switch1" not "switch1"

> 
> 
> What I don't know is whether the extra QMP nesting makes it easier or
> harder to support the existing NBD command line options, and it would

Yes, it is harder to support it.

Thanks
Wen Congyang

> ultimately be nice to have unified support so that anything we can do on
> the command line can be expressed in QMP; and anything we can do in QMP
> can be expressed on the command line without undue nesting.
> 

Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On 09/16/2015 01:11 AM, Wen Congyang wrote:

>>>> Possible workaround in the meantime - instead of trying to go with a
>>>> nice flat union (where all QMP keys are in the same {} level), we can
>>>> use nesting (structs that add another {} to include the unions).
>>>
>>> How to include the unions to a structs? Use 'base'?
>>
>> Conceptually, by adding a layer of nesting.  On the wire, instead of:
>>
>> { "switch1":"value", "switch2":"value", "body2":"blah" }
>>
>> you would instead have:
>>
>> { "switch1":"value", "data": { "switch2":"value", "body2":"blah" } }
>>
>> Anywhere in qapi that you try to have:
>> { 'union': ..., 'data':{'switch1':'Union'}}
>>
>> you instead create a wrapper type:
>> { 'struct':'Wrapper', 'data':{'data':'Union'}}
>> { 'union': ..., 'data':{'switch1':'Wrapper'}}
> 
> If so, the option is "data.switch1" not "switch1"
> 
>>
>>
>> What I don't know is whether the extra QMP nesting makes it easier or
>> harder to support the existing NBD command line options, and it would
> 
> Yes, it is harder to support it.

All the more reason to push the qapi improvements through, so that qapi
can expose a flat union and not make the command line have to go through
ugly nesting.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v3 2/5] Add new block driver interface to add/delete a BDS's child

[Wen Congyang]

In some cases, we want to take a quorum child offline, and take
another child online.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 block.c                   | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  5 +++++
 include/block/block_int.h |  5 +++++
 3 files changed, 62 insertions(+)

diff --git a/block.c b/block.c
index d004bd0..682d2ec 100644
--- a/block.c
+++ b/block.c
@@ -4115,3 +4115,55 @@ void bdrv_refresh_filename(BlockDriverState *bs)
         QDECREF(json);
     }
 }
+
+/*
+ * Hot add/remove a BDS's child. So the user can take a child offline when
+ * it is broken and take a new child online
+ */
+void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) {
+        error_setg(errp, "The BDS %s doesn't support adding a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    if (child_bs->blk) {
+        error_setg(errp, "The BDS %s is used by the block device %s",
+                   child_bs->node_name, blk_name(child_bs->blk));
+        return;
+    }
+
+    /* TODO: check if the child bs has parent */
+
+    parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp);
+}
+
+void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+    BdrvChild *child;
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) {
+        error_setg(errp, "The BDS %s doesn't support removing a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    QLIST_FOREACH(child, &parent_bs->children, next) {
+        if (child->bs == child_bs) {
+            break;
+        }
+    }
+
+    if (!child) {
+        error_setg(errp, "The BDS %s is not the BDS %s's child",
+                   bdrv_get_device_or_node_name(child_bs),
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index a4e31af..8480af7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -603,4 +603,9 @@ void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
 void bdrv_flush_io_queue(BlockDriverState *bs);
 
+void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
+                    Error **errp);
+void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child,
+                    Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ca1eefa..b4d5250 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -288,6 +288,11 @@ struct BlockDriver {
      */
     int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
+    void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+    void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH v3 3/5] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 block.c               |  6 ++---
 block/quorum.c        | 72 +++++++++++++++++++++++++++++++++++++++++++++++++--
 include/block/block.h |  3 +++
 3 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 682d2ec..6ffd27b 100644
--- a/block.c
+++ b/block.c
@@ -1100,9 +1100,9 @@ static int bdrv_fill_options(QDict **options, const char **pfilename,
     return 0;
 }
 
-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-                                    BlockDriverState *child_bs,
-                                    const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const BdrvChildRole *child_role)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index 72183d6..13e33ad 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -66,6 +66,9 @@ typedef struct QuorumVotes {
 typedef struct BDRVQuorumState {
     BlockDriverState **bs; /* children BlockDriverStates */
     int num_children;      /* children count */
+    int max_children;      /* The maximum children count, we need to reallocate
+                            * bs if num_children grows larger than maximum.
+                            */
     int threshold;         /* if less than threshold children reads gave the
                             * same result a quorum error occurs.
                             */
@@ -874,9 +877,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto exit;
     }
-    if (s->num_children < 2) {
+    if (s->num_children < 1) {
         error_setg(&local_err,
-                   "Number of provided children must be greater than 1");
+                   "Number of provided children must be 1 or more");
         ret = -EINVAL;
         goto exit;
     }
@@ -925,6 +928,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     /* allocate the children BlockDriverState array */
     s->bs = g_new0(BlockDriverState *, s->num_children);
     opened = g_new0(bool, s->num_children);
+    s->max_children = s->num_children;
 
     for (i = 0; i < s->num_children; i++) {
         char indexstr[32];
@@ -995,6 +999,67 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+
+    bdrv_drain(bs);
+
+    if (s->num_children == s->max_children) {
+        if (s->max_children >= INT_MAX) {
+            error_setg(errp, "Too many children");
+            return;
+        }
+
+        s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
+        s->bs[s->num_children] = NULL;
+        s->max_children++;
+    }
+
+    bdrv_ref(child_bs);
+    bdrv_attach_child(bs, child_bs, &child_format);
+    s->bs[s->num_children++] = child_bs;
+}
+
+static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    int i;
+
+    for (i = 0; i < s->num_children; i++) {
+        if (s->bs[i] == child_bs) {
+            break;
+        }
+    }
+
+    QLIST_FOREACH(child, &bs->children, next) {
+        if (child->bs == child_bs) {
+            break;
+        }
+    }
+
+    /* we have checked it in bdrv_del_child() */
+    assert(i < s->num_children && child);
+
+    if (s->num_children <= s->threshold) {
+        error_setg(errp,
+            "The number of children cannot be lower than the vote threshold %d",
+            s->threshold);
+        return;
+    }
+
+    bdrv_drain(bs);
+    /* We can safely remove this child now */
+    memmove(&s->bs[i], &s->bs[i + 1],
+            (s->num_children - i - 1) * sizeof(void *));
+    s->num_children--;
+    s->bs[s->num_children] = NULL;
+    bdrv_unref_child(bs, child);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1063,6 +1128,9 @@ static BlockDriver bdrv_quorum = {
     .bdrv_detach_aio_context            = quorum_detach_aio_context,
     .bdrv_attach_aio_context            = quorum_attach_aio_context,
 
+    .bdrv_add_child                     = quorum_add_child,
+    .bdrv_del_child                     = quorum_del_child,
+
     .is_filter                          = true,
     .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
 
diff --git a/include/block/block.h b/include/block/block.h
index 8480af7..97188b1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -503,6 +503,9 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const BdrvChildRole *child_role);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
-- 
2.4.3

[Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index bd47756..0a40607 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3413,6 +3413,53 @@ fail:
     qmp_output_visitor_cleanup(ov);
 }
 
+void qmp_x_child_add(const char *parent, const char *child,
+                     Error **errp)
+{
+    BlockDriverState *parent_bs, *child_bs;
+    Error *local_err = NULL;
+
+    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
+    if (!parent_bs) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    child_bs = bdrv_find_node(child);
+    if (!child_bs) {
+        error_setg(errp, "Node '%s' not found", child);
+        return;
+    }
+
+    bdrv_add_child(parent_bs, child_bs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
+void qmp_child_del(const char *parent, const char *child, Error **errp)
+{
+    BlockDriverState *parent_bs, *child_bs;
+    Error *local_err = NULL;
+
+    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
+    if (!parent_bs) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    child_bs = bdrv_find_node(child);
+    if (!child_bs) {
+        error_setg(errp, "Node '%s' not found", child);
+        return;
+    }
+
+    bdrv_del_child(parent_bs, child_bs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
 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 e68a59f..b959577 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2272,3 +2272,37 @@
 ##
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+##
+# @x-child-add
+#
+# Add a new child to the parent BDS. Currently only the Quorum driver
+# implements this feature. This is useful to fix a broken quorum child.
+#
+# @parent: graph node name or id which the child will be added to.
+#
+# @child: graph node name that will be added.
+#
+# Note: this command is experimental, and not a stable API.
+#
+# Since: 2.5
+##
+{ 'command': 'x-child-add',
+  'data' : { 'parent': 'str', 'child': 'str' } }
+
+##
+# @child-del
+#
+# Remove a child from the parent BDS. Currently only the Quorum driver
+# implements this feature. This is useful to fix a broken quorum child.
+# Note, you can't remove a child if it would bring the quorum below its
+# threshold.
+#
+# @parent: graph node name or id from which the child will removed.
+#
+# @child: graph node name that will be removed.
+#
+# Since: 2.5
+##
+{ 'command': 'child-del',
+  'data' : { 'parent': 'str', 'child': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 495670b..139a23b 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4053,6 +4053,59 @@ Example:
 EQMP
 
     {
+        .name       = "x-child-add",
+        .args_type  = "parent:B,child:B",
+        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
+    },
+
+SQMP
+x-child-add
+------------
+
+Add a child to a quorum node.
+
+Arguments:
+
+- "parent": the quorum's id or node name
+- "child": the child node-name which will be added
+
+Note: this command is experimental, and not a stable API.
+
+Example:
+
+-> { "execute": "x-child-add",
+    "arguments": { "parent": "disk1", "child": "new_node" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name        = "child-del",
+        .args_type   = "parent:B,child:B",
+        .mhandler.cmd_new = qmp_marshal_input_child_del,
+    },
+
+SQMP
+child-del
+------------
+
+Delete a child from a quorum node. It can be used to remove a broken
+quorum child.
+
+Arguments:
+
+- "parent": the quorum's id or node name
+- "child": the child node-name which will be removed
+
+Example:
+
+-> { "execute": "child-del",
+    "arguments": { "parent": "disk1", "child": "new_node" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Daniel P. Berrange]

On Thu, Sep 10, 2015 at 05:55:04PM +0800, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e68a59f..b959577 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2272,3 +2272,37 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +#
> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }
> +
> +##
> +# @child-del
> +#
> +# Remove a child from the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +# Note, you can't remove a child if it would bring the quorum below its
> +# threshold.
> +#
> +# @parent: graph node name or id from which the child will removed.
> +#
> +# @child: graph node name that will be removed.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'child-del',
> +  'data' : { 'parent': 'str', 'child': 'str' } }

These command names are faaaar too generic. If this only applies to
block devices, then I'd expect something like 'block' as a prefix for
the command names. Likewise with your next hmp patch

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Wen Congyang]

On 09/10/2015 06:04 PM, Daniel P. Berrange wrote:
> On Thu, Sep 10, 2015 at 05:55:04PM +0800, Wen Congyang wrote:
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 134 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e68a59f..b959577 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2272,3 +2272,37 @@
>>  ##
>>  { 'command': 'block-set-write-threshold',
>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @x-child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +#
>> +# @parent: graph node name or id which the child will be added to.
>> +#
>> +# @child: graph node name that will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>> +
>> +##
>> +# @child-del
>> +#
>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +# Note, you can't remove a child if it would bring the quorum below its
>> +# threshold.
>> +#
>> +# @parent: graph node name or id from which the child will removed.
>> +#
>> +# @child: graph node name that will be removed.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'child-del',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
> 
> These command names are faaaar too generic. If this only applies to
> block devices, then I'd expect something like 'block' as a prefix for
> the command names. Likewise with your next hmp patch

OK, I will fix it in the next version. I guess blockdev may be better.

Thanks
Wen Congyang

> 
> Regards,
> Daniel
> 

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index bd47756..0a40607 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3413,6 +3413,53 @@ fail:
>      qmp_output_visitor_cleanup(ov);
>  }
>  
> +void qmp_x_child_add(const char *parent, const char *child,
> +                     Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs;
> +    Error *local_err = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    child_bs = bdrv_find_node(child);
> +    if (!child_bs) {
> +        error_setg(errp, "Node '%s' not found", child);
> +        return;
> +    }
> +
> +    bdrv_add_child(parent_bs, child_bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +void qmp_child_del(const char *parent, const char *child, Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs;
> +    Error *local_err = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    child_bs = bdrv_find_node(child);
> +    if (!child_bs) {
> +        error_setg(errp, "Node '%s' not found", child);
> +        return;
> +    }
> +
> +    bdrv_del_child(parent_bs, child_bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
>  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 e68a59f..b959577 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2272,3 +2272,37 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +#
> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }
> +
> +##
> +# @child-del
> +#
> +# Remove a child from the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +# Note, you can't remove a child if it would bring the quorum below its
> +# threshold.
> +#
> +# @parent: graph node name or id from which the child will removed.
> +#
> +# @child: graph node name that will be removed.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'child-del',
> +  'data' : { 'parent': 'str', 'child': 'str' } }

Why is x-child-add experimental, but child-del isn't?  Please explain
both in the schema and in the commit message.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 495670b..139a23b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4053,6 +4053,59 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "x-child-add",
> +        .args_type  = "parent:B,child:B",
> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
> +    },
> +
> +SQMP
> +x-child-add
> +------------
> +
> +Add a child to a quorum node.
> +
> +Arguments:
> +
> +- "parent": the quorum's id or node name
> +- "child": the child node-name which will be added

Node name parameters are usually named node-name or, if there's more
than one, FOO-node-name.  Unless we want to abandon that convention,
this should therefore be node-name and child-node-name, or parent-node
name and child-node-name.

> +
> +Note: this command is experimental, and not a stable API.
> +
> +Example:
> +
> +-> { "execute": "x-child-add",
> +    "arguments": { "parent": "disk1", "child": "new_node" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name        = "child-del",

Documentation and schema have x-child-add, actual command is child-add.
Oops.

> +        .args_type   = "parent:B,child:B",
> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
> +    },
> +
> +SQMP
> +child-del
> +------------
> +
> +Delete a child from a quorum node. It can be used to remove a broken
> +quorum child.
> +
> +Arguments:
> +
> +- "parent": the quorum's id or node name
> +- "child": the child node-name which will be removed

Same comment as on x-child-add's parameter names.

> +
> +Example:
> +
> +-> { "execute": "child-del",
> +    "arguments": { "parent": "disk1", "child": "new_node" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "query-named-block-nodes",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Kevin Wolf]

Am 14.09.2015 um 16:36 hat Markus Armbruster geschrieben:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 495670b..139a23b 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -4053,6 +4053,59 @@ Example:
> >  EQMP
> >  
> >      {
> > +        .name       = "x-child-add",
> > +        .args_type  = "parent:B,child:B",
> > +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
> > +    },
> > +
> > +SQMP
> > +x-child-add
> > +------------
> > +
> > +Add a child to a quorum node.
> > +
> > +Arguments:
> > +
> > +- "parent": the quorum's id or node name
> > +- "child": the child node-name which will be added
> 
> Node name parameters are usually named node-name or, if there's more
> than one, FOO-node-name.  Unless we want to abandon that convention,
> this should therefore be node-name and child-node-name, or parent-node
> name and child-node-name.

Didn't we come to the conclusion that foo-node-name is redundant and
should just be foo, leaving things like node-name only for cases where
there is no foo?

Kevin

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2015 um 16:36 hat Markus Armbruster geschrieben:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> > diff --git a/qmp-commands.hx b/qmp-commands.hx
>> > index 495670b..139a23b 100644
>> > --- a/qmp-commands.hx
>> > +++ b/qmp-commands.hx
>> > @@ -4053,6 +4053,59 @@ Example:
>> >  EQMP
>> >  
>> >      {
>> > +        .name       = "x-child-add",
>> > +        .args_type  = "parent:B,child:B",
>> > +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>> > +    },
>> > +
>> > +SQMP
>> > +x-child-add
>> > +------------
>> > +
>> > +Add a child to a quorum node.
>> > +
>> > +Arguments:
>> > +
>> > +- "parent": the quorum's id or node name
>> > +- "child": the child node-name which will be added
>> 
>> Node name parameters are usually named node-name or, if there's more
>> than one, FOO-node-name.  Unless we want to abandon that convention,
>> this should therefore be node-name and child-node-name, or parent-node
>> name and child-node-name.
>
> Didn't we come to the conclusion that foo-node-name is redundant and
> should just be foo, leaving things like node-name only for cases where
> there is no foo?

I don't remember :)

I'm fine with simpler names.

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Wen Congyang]

On 09/14/2015 10:36 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 134 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index bd47756..0a40607 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3413,6 +3413,53 @@ fail:
>>      qmp_output_visitor_cleanup(ov);
>>  }
>>  
>> +void qmp_x_child_add(const char *parent, const char *child,
>> +                     Error **errp)
>> +{
>> +    BlockDriverState *parent_bs, *child_bs;
>> +    Error *local_err = NULL;
>> +
>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +    if (!parent_bs) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    child_bs = bdrv_find_node(child);
>> +    if (!child_bs) {
>> +        error_setg(errp, "Node '%s' not found", child);
>> +        return;
>> +    }
>> +
>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +}
>> +
>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>> +{
>> +    BlockDriverState *parent_bs, *child_bs;
>> +    Error *local_err = NULL;
>> +
>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +    if (!parent_bs) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    child_bs = bdrv_find_node(child);
>> +    if (!child_bs) {
>> +        error_setg(errp, "Node '%s' not found", child);
>> +        return;
>> +    }
>> +
>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +}
>> +
>>  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 e68a59f..b959577 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2272,3 +2272,37 @@
>>  ##
>>  { 'command': 'block-set-write-threshold',
>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @x-child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +#
>> +# @parent: graph node name or id which the child will be added to.
>> +#
>> +# @child: graph node name that will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>> +
>> +##
>> +# @child-del
>> +#
>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +# Note, you can't remove a child if it would bring the quorum below its
>> +# threshold.
>> +#
>> +# @parent: graph node name or id from which the child will removed.
>> +#
>> +# @child: graph node name that will be removed.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'child-del',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
> 
> Why is x-child-add experimental, but child-del isn't?  Please explain
> both in the schema and in the commit message.

No special reason. Should I put child-del in experimental namespace?

> 
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 495670b..139a23b 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4053,6 +4053,59 @@ Example:
>>  EQMP
>>  
>>      {
>> +        .name       = "x-child-add",
>> +        .args_type  = "parent:B,child:B",
>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>> +    },
>> +
>> +SQMP
>> +x-child-add
>> +------------
>> +
>> +Add a child to a quorum node.
>> +
>> +Arguments:
>> +
>> +- "parent": the quorum's id or node name
>> +- "child": the child node-name which will be added
> 
> Node name parameters are usually named node-name or, if there's more
> than one, FOO-node-name.  Unless we want to abandon that convention,
> this should therefore be node-name and child-node-name, or parent-node
> name and child-node-name.

parent can be top BDS, so it can be id. node-name is a very common name,
and I think child or child-node-name is better.

> 
>> +
>> +Note: this command is experimental, and not a stable API.
>> +
>> +Example:
>> +
>> +-> { "execute": "x-child-add",
>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>> +        .name        = "child-del",
> 
> Documentation and schema have x-child-add, actual command is child-add.
> Oops.

Here is child-del....

> 
>> +        .args_type   = "parent:B,child:B",
>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>> +    },
>> +
>> +SQMP
>> +child-del
>> +------------
>> +
>> +Delete a child from a quorum node. It can be used to remove a broken
>> +quorum child.
>> +
>> +Arguments:
>> +
>> +- "parent": the quorum's id or node name
>> +- "child": the child node-name which will be removed
> 
> Same comment as on x-child-add's parameter names.
> 
>> +
>> +Example:
>> +
>> +-> { "execute": "child-del",
>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>>          .name       = "query-named-block-nodes",
>>          .args_type  = "",
>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
> .
> 

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 134 insertions(+)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index bd47756..0a40607 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3413,6 +3413,53 @@ fail:
>>>      qmp_output_visitor_cleanup(ov);
>>>  }
>>>  
>>> +void qmp_x_child_add(const char *parent, const char *child,
>>> +                     Error **errp)
>>> +{
>>> +    BlockDriverState *parent_bs, *child_bs;
>>> +    Error *local_err = NULL;
>>> +
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    child_bs = bdrv_find_node(child);
>>> +    if (!child_bs) {
>>> +        error_setg(errp, "Node '%s' not found", child);
>>> +        return;
>>> +    }
>>> +
>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +    }
>>> +}
>>> +
>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>> +{
>>> +    BlockDriverState *parent_bs, *child_bs;
>>> +    Error *local_err = NULL;
>>> +
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    child_bs = bdrv_find_node(child);
>>> +    if (!child_bs) {
>>> +        error_setg(errp, "Node '%s' not found", child);
>>> +        return;
>>> +    }
>>> +
>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +    }
>>> +}
>>> +
>>>  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 e68a59f..b959577 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -2272,3 +2272,37 @@
>>>  ##
>>>  { 'command': 'block-set-write-threshold',
>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>> +
>>> +##
>>> +# @x-child-add
>>> +#
>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>> +# implements this feature. This is useful to fix a broken quorum child.
>>> +#
>>> +# @parent: graph node name or id which the child will be added to.
>>> +#
>>> +# @child: graph node name that will be added.
>>> +#
>>> +# Note: this command is experimental, and not a stable API.
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'command': 'x-child-add',
>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>> +
>>> +##
>>> +# @child-del
>>> +#
>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>> +# implements this feature. This is useful to fix a broken quorum child.
>>> +# Note, you can't remove a child if it would bring the quorum below its
>>> +# threshold.
>>> +#
>>> +# @parent: graph node name or id from which the child will removed.
>>> +#
>>> +# @child: graph node name that will be removed.
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'command': 'child-del',
>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>> 
>> Why is x-child-add experimental, but child-del isn't?  Please explain
>> both in the schema and in the commit message.
>
> No special reason. Should I put child-del in experimental namespace?

I found the reason for x-child-add in your v2:

    child-add
    ------------

    Add a child to a quorum node.

    This command is still a work in progress. It doesn't support all
    block drivers. Stay away from it unless you want it to help with
    its development.

Eric suggested to rename it to x-child-add, and you did.  Good.  You
also shortened the "work in progress" note to just "Note: this command
is experimental, and not a stable API."  I'd like to have a more verbose
note explaining *why* the command is experimental, both here and in
qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
Are the any others?

Is child-del similarly unfinished?  If yes, make it x-child-del to save
us from later grief.

If no: is child-del is only useful together with x-child-add?  Then make
it x-child-del regardless.

>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 495670b..139a23b 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -4053,6 +4053,59 @@ Example:
>>>  EQMP
>>>  
>>>      {
>>> +        .name       = "x-child-add",
>>> +        .args_type  = "parent:B,child:B",
>>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>>> +    },
>>> +
>>> +SQMP
>>> +x-child-add
>>> +------------
>>> +
>>> +Add a child to a quorum node.
>>> +
>>> +Arguments:
>>> +
>>> +- "parent": the quorum's id or node name
>>> +- "child": the child node-name which will be added
>> 
>> Node name parameters are usually named node-name or, if there's more
>> than one, FOO-node-name.  Unless we want to abandon that convention,
>> this should therefore be node-name and child-node-name, or parent-node
>> name and child-node-name.
>
> parent can be top BDS, so it can be id. node-name is a very common name,
> and I think child or child-node-name is better.

Kevin pointed out we want to move to names without a -node-name suffix.

>>> +
>>> +Note: this command is experimental, and not a stable API.
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "x-child-add",
>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>> +<- { "return": {} }
>>> +
>>> +EQMP
>>> +
>>> +    {
>>> +        .name        = "child-del",
>> 
>> Documentation and schema have x-child-add, actual command is child-add.
>> Oops.
>
> Here is child-del....

You're right.  I got confused...

>>> +        .args_type   = "parent:B,child:B",
>>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>>> +    },
>>> +
>>> +SQMP
>>> +child-del
>>> +------------
>>> +
>>> +Delete a child from a quorum node. It can be used to remove a broken
>>> +quorum child.
>>> +
>>> +Arguments:
>>> +
>>> +- "parent": the quorum's id or node name
>>> +- "child": the child node-name which will be removed
>> 
>> Same comment as on x-child-add's parameter names.
>> 
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "child-del",
>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>> +<- { "return": {} }
>>> +
>>> +EQMP
>>> +
>>> +    {
>>>          .name       = "query-named-block-nodes",
>>>          .args_type  = "",
>>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
>> .
>> 

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Wen Congyang]

On 09/15/2015 03:49 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>> ---
>>>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 134 insertions(+)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index bd47756..0a40607 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>      qmp_output_visitor_cleanup(ov);
>>>>  }
>>>>  
>>>> +void qmp_x_child_add(const char *parent, const char *child,
>>>> +                     Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>>  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 e68a59f..b959577 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -2272,3 +2272,37 @@
>>>>  ##
>>>>  { 'command': 'block-set-write-threshold',
>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>> +
>>>> +##
>>>> +# @x-child-add
>>>> +#
>>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +#
>>>> +# @parent: graph node name or id which the child will be added to.
>>>> +#
>>>> +# @child: graph node name that will be added.
>>>> +#
>>>> +# Note: this command is experimental, and not a stable API.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'x-child-add',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>> +
>>>> +##
>>>> +# @child-del
>>>> +#
>>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>> +# threshold.
>>>> +#
>>>> +# @parent: graph node name or id from which the child will removed.
>>>> +#
>>>> +# @child: graph node name that will be removed.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'child-del',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>
>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>> both in the schema and in the commit message.
>>
>> No special reason. Should I put child-del in experimental namespace?
> 
> I found the reason for x-child-add in your v2:
> 
>     child-add
>     ------------
> 
>     Add a child to a quorum node.
> 
>     This command is still a work in progress. It doesn't support all
>     block drivers. Stay away from it unless you want it to help with
>     its development.
> 
> Eric suggested to rename it to x-child-add, and you did.  Good.  You
> also shortened the "work in progress" note to just "Note: this command
> is experimental, and not a stable API."  I'd like to have a more verbose
> note explaining *why* the command is experimental, both here and in
> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
> Are the any others?

Currently, it only for quorum. But in the future, we can use this command
to do more thing. For example: bs->file, bs->backing_hd, ...

> 
> Is child-del similarly unfinished?  If yes, make it x-child-del to save
> us from later grief.
> 
> If no: is child-del is only useful together with x-child-add?  Then make
> it x-child-del regardless.

child-del is only useful together with x-child-add. I will rename it to
x-child-del in the next version.

Thanks
Wen Congyang

> 
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index 495670b..139a23b 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -4053,6 +4053,59 @@ Example:
>>>>  EQMP
>>>>  
>>>>      {
>>>> +        .name       = "x-child-add",
>>>> +        .args_type  = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +x-child-add
>>>> +------------
>>>> +
>>>> +Add a child to a quorum node.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be added
>>>
>>> Node name parameters are usually named node-name or, if there's more
>>> than one, FOO-node-name.  Unless we want to abandon that convention,
>>> this should therefore be node-name and child-node-name, or parent-node
>>> name and child-node-name.
>>
>> parent can be top BDS, so it can be id. node-name is a very common name,
>> and I think child or child-node-name is better.
> 
> Kevin pointed out we want to move to names without a -node-name suffix.
> 
>>>> +
>>>> +Note: this command is experimental, and not a stable API.
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "x-child-add",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>> +        .name        = "child-del",
>>>
>>> Documentation and schema have x-child-add, actual command is child-add.
>>> Oops.
>>
>> Here is child-del....
> 
> You're right.  I got confused...
> 
>>>> +        .args_type   = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +child-del
>>>> +------------
>>>> +
>>>> +Delete a child from a quorum node. It can be used to remove a broken
>>>> +quorum child.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be removed
>>>
>>> Same comment as on x-child-add's parameter names.
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "child-del",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>>          .name       = "query-named-block-nodes",
>>>>          .args_type  = "",
>>>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
>>> .
>>>
> .
> 

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Wen Congyang]

On 09/15/2015 03:49 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>> ---
>>>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 134 insertions(+)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index bd47756..0a40607 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>      qmp_output_visitor_cleanup(ov);
>>>>  }
>>>>  
>>>> +void qmp_x_child_add(const char *parent, const char *child,
>>>> +                     Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>>  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 e68a59f..b959577 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -2272,3 +2272,37 @@
>>>>  ##
>>>>  { 'command': 'block-set-write-threshold',
>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>> +
>>>> +##
>>>> +# @x-child-add
>>>> +#
>>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +#
>>>> +# @parent: graph node name or id which the child will be added to.
>>>> +#
>>>> +# @child: graph node name that will be added.
>>>> +#
>>>> +# Note: this command is experimental, and not a stable API.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'x-child-add',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>> +
>>>> +##
>>>> +# @child-del
>>>> +#
>>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>> +# threshold.
>>>> +#
>>>> +# @parent: graph node name or id from which the child will removed.
>>>> +#
>>>> +# @child: graph node name that will be removed.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'child-del',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>
>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>> both in the schema and in the commit message.
>>
>> No special reason. Should I put child-del in experimental namespace?
> 
> I found the reason for x-child-add in your v2:
> 
>     child-add
>     ------------
> 
>     Add a child to a quorum node.
> 
>     This command is still a work in progress. It doesn't support all
>     block drivers. Stay away from it unless you want it to help with
>     its development.
> 
> Eric suggested to rename it to x-child-add, and you did.  Good.  You
> also shortened the "work in progress" note to just "Note: this command
> is experimental, and not a stable API."  I'd like to have a more verbose
> note explaining *why* the command is experimental, both here and in
> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
> Are the any others?
> 
> Is child-del similarly unfinished?  If yes, make it x-child-del to save
> us from later grief.
> 
> If no: is child-del is only useful together with x-child-add?  Then make
> it x-child-del regardless.

I have another question: if the command is experimental, we have the prefix "x-".
Which prefix is used for hmp command?

Thanks
Wen Congyang

> 
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index 495670b..139a23b 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -4053,6 +4053,59 @@ Example:
>>>>  EQMP
>>>>  
>>>>      {
>>>> +        .name       = "x-child-add",
>>>> +        .args_type  = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +x-child-add
>>>> +------------
>>>> +
>>>> +Add a child to a quorum node.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be added
>>>
>>> Node name parameters are usually named node-name or, if there's more
>>> than one, FOO-node-name.  Unless we want to abandon that convention,
>>> this should therefore be node-name and child-node-name, or parent-node
>>> name and child-node-name.
>>
>> parent can be top BDS, so it can be id. node-name is a very common name,
>> and I think child or child-node-name is better.
> 
> Kevin pointed out we want to move to names without a -node-name suffix.
> 
>>>> +
>>>> +Note: this command is experimental, and not a stable API.
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "x-child-add",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>> +        .name        = "child-del",
>>>
>>> Documentation and schema have x-child-add, actual command is child-add.
>>> Oops.
>>
>> Here is child-del....
> 
> You're right.  I got confused...
> 
>>>> +        .args_type   = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +child-del
>>>> +------------
>>>> +
>>>> +Delete a child from a quorum node. It can be used to remove a broken
>>>> +quorum child.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be removed
>>>
>>> Same comment as on x-child-add's parameter names.
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "child-del",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>>          .name       = "query-named-block-nodes",
>>>>          .args_type  = "",
>>>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
>>> .
>>>
> .
> 

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 09/15/2015 03:49 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>
>>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>> ---
>>>>>  blockdev.c           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 134 insertions(+)
>>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index bd47756..0a40607 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>>      qmp_output_visitor_cleanup(ov);
>>>>>  }
>>>>>  
>>>>> +void qmp_x_child_add(const char *parent, const char *child,
>>>>> +                     Error **errp)
>>>>> +{
>>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>>> +    if (!parent_bs) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    child_bs = bdrv_find_node(child);
>>>>> +    if (!child_bs) {
>>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>>> +    if (local_err) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>>>> +{
>>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>>> +    if (!parent_bs) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    child_bs = bdrv_find_node(child);
>>>>> +    if (!child_bs) {
>>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>>> +    if (local_err) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  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 e68a59f..b959577 100644
>>>>> --- a/qapi/block-core.json
>>>>> +++ b/qapi/block-core.json
>>>>> @@ -2272,3 +2272,37 @@
>>>>>  ##
>>>>>  { 'command': 'block-set-write-threshold',
>>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>>> +
>>>>> +##
>>>>> +# @x-child-add
>>>>> +#
>>>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>>> +#
>>>>> +# @parent: graph node name or id which the child will be added to.
>>>>> +#
>>>>> +# @child: graph node name that will be added.
>>>>> +#
>>>>> +# Note: this command is experimental, and not a stable API.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'x-child-add',
>>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>>> +
>>>>> +##
>>>>> +# @child-del
>>>>> +#
>>>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>>> +# threshold.
>>>>> +#
>>>>> +# @parent: graph node name or id from which the child will removed.
>>>>> +#
>>>>> +# @child: graph node name that will be removed.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'child-del',
>>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>>
>>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>>> both in the schema and in the commit message.
>>>
>>> No special reason. Should I put child-del in experimental namespace?
>> 
>> I found the reason for x-child-add in your v2:
>> 
>>     child-add
>>     ------------
>> 
>>     Add a child to a quorum node.
>> 
>>     This command is still a work in progress. It doesn't support all
>>     block drivers. Stay away from it unless you want it to help with
>>     its development.
>> 
>> Eric suggested to rename it to x-child-add, and you did.  Good.  You
>> also shortened the "work in progress" note to just "Note: this command
>> is experimental, and not a stable API."  I'd like to have a more verbose
>> note explaining *why* the command is experimental, both here and in
>> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
>> Are the any others?
>> 
>> Is child-del similarly unfinished?  If yes, make it x-child-del to save
>> us from later grief.
>> 
>> If no: is child-del is only useful together with x-child-add?  Then make
>> it x-child-del regardless.
>
> I have another question: if the command is experimental, we have the
> prefix "x-".
> Which prefix is used for hmp command?

HMP is not a stable interface, so generally don't bother marking
experimental interfaces.

That said, I'd probably keep HMP and QMP command name the same to
minimize confusion.

[...]

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Kevin Wolf]

Am 10.09.2015 um 11:55 hat Wen Congyang geschrieben:
> +##
> +# @x-child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +#
> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }

This is probably not future-proof and only made for the special case of
quorum. Specifically, one thing I'm missing is some way to specfiy what
kind of child the new node is when a node can take different types of
children (e.g. bs->file and bs->backing_hd).

Kevin

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Wen Congyang]

On 09/14/2015 11:37 PM, Kevin Wolf wrote:
> Am 10.09.2015 um 11:55 hat Wen Congyang geschrieben:
>> +##
>> +# @x-child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +#
>> +# @parent: graph node name or id which the child will be added to.
>> +#
>> +# @child: graph node name that will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
> 
> This is probably not future-proof and only made for the special case of
> quorum. Specifically, one thing I'm missing is some way to specfiy what
> kind of child the new node is when a node can take different types of
> children (e.g. bs->file and bs->backing_hd).

Currently, we only add/remove quorum's child. We can add a new parameter
to specify the type in the furture to support more things.

Thanks
Wen Congyang

> 
> Kevin
> .
> 

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Alberto Garcia]

On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:

>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>
> This is probably not future-proof and only made for the special case
> of quorum. Specifically, one thing I'm missing is some way to specfiy
> what kind of child the new node is when a node can take different
> types of children (e.g. bs->file and bs->backing_hd).

Children here are specified by child roles, aren't they?

We could have a ChildRole enum with 'file', 'format' and 'backing' that
would be passed to this command and use the same enumeration in
bdrv_open_image() rather than a pointer to BdrvChildRole.

Berto

Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Kevin Wolf]

Am 15.09.2015 um 10:56 hat Alberto Garcia geschrieben:
> On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> >> +{ 'command': 'x-child-add',
> >> +  'data' : { 'parent': 'str', 'child': 'str' } }
> >
> > This is probably not future-proof and only made for the special case
> > of quorum. Specifically, one thing I'm missing is some way to specfiy
> > what kind of child the new node is when a node can take different
> > types of children (e.g. bs->file and bs->backing_hd).
> 
> Children here are specified by child roles, aren't they?

Possibly, but actually, currently I'm inclined to think that they
aren't. Child roles are still relatively new, though, so it's hard to
say if this is just because we didn't use them in that way so far, or
because they are really the wrong tool.

What I can say is that traditionally children are identified by option
names. Block drivers assign a ChildRole when processing the option, and
both are equivalent only if the same ChildRole is never used for two
different children. I believe that that's currently true, but I'm
doubtful whether it will remain this way (even looking at blkverify,
which has one &child_file and one &child_format, things start to look a
bit confusing).

Possibly the right and consistent way to change the set of children
would be through a QMP command exposing bdrv_reopen(), which would also
be used for changing other options at runtime.

My current pull request adds the qemu-io (and therefore HMP) way of
doing this, but finding a good QMP interface will still be a challenge.

> We could have a ChildRole enum with 'file', 'format' and 'backing' that
> would be passed to this command and use the same enumeration in
> bdrv_open_image() rather than a pointer to BdrvChildRole.

Yes, that would be an option if ChildRole turned out to be enough.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v3 4/5] qmp: add monitor command to add/remove a child

[Kevin Wolf]

Am 15.09.2015 um 11:20 hat Kevin Wolf geschrieben:
> Am 15.09.2015 um 10:56 hat Alberto Garcia geschrieben:
> > On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > >> +{ 'command': 'x-child-add',
> > >> +  'data' : { 'parent': 'str', 'child': 'str' } }
> > >
> > > This is probably not future-proof and only made for the special case
> > > of quorum. Specifically, one thing I'm missing is some way to specfiy
> > > what kind of child the new node is when a node can take different
> > > types of children (e.g. bs->file and bs->backing_hd).
> > 
> > Children here are specified by child roles, aren't they?
> 
> Possibly, but actually, currently I'm inclined to think that they
> aren't. Child roles are still relatively new, though, so it's hard to
> say if this is just because we didn't use them in that way so far, or
> because they are really the wrong tool.
> 
> What I can say is that traditionally children are identified by option
> names. Block drivers assign a ChildRole when processing the option, and
> both are equivalent only if the same ChildRole is never used for two
> different children. I believe that that's currently true, but I'm
> doubtful whether it will remain this way (even looking at blkverify,
> which has one &child_file and one &child_format, things start to look a
> bit confusing).

What I wanted to mention, but forgot to do, is that obviously we could
use more specific ChildRoles than &child_file and &child_format,
essentially creating one ChildRole per option and making them equivalent
this way.

I'm not sure if that would be a good or a bad idea.

> Possibly the right and consistent way to change the set of children
> would be through a QMP command exposing bdrv_reopen(), which would also
> be used for changing other options at runtime.
> 
> My current pull request adds the qemu-io (and therefore HMP) way of
> doing this, but finding a good QMP interface will still be a challenge.
> 
> > We could have a ChildRole enum with 'file', 'format' and 'backing' that
> > would be passed to this command and use the same enumeration in
> > bdrv_open_image() rather than a pointer to BdrvChildRole.
> 
> Yes, that would be an option if ChildRole turned out to be enough.
> 
> Kevin
> 

[Qemu-devel] [PATCH v3 5/5] hmp: add monitor command to add/remove a child

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx | 28 ++++++++++++++++++++++++++++
 hmp.c           | 20 ++++++++++++++++++++
 hmp.h           |  2 ++
 3 files changed, 50 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index fece64f..bd365bb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -193,6 +193,34 @@ actions (drive options rerror, werror).
 ETEXI
 
     {
+        .name       = "child_add",
+        .args_type  = "id:B,child:B",
+        .params     = "parent child",
+        .help       = "add a child to a BDS",
+        .mhandler.cmd = hmp_child_add,
+    },
+
+STEXI
+@item child_add @var{parent device} @var{child device}
+@findex child_add
+Add a child to the block device.
+ETEXI
+
+    {
+        .name       = "child_del",
+        .args_type  = "id:B,child:B",
+        .params     = "parent child",
+        .help       = "remove a child from a BDS",
+        .mhandler.cmd = hmp_child_del,
+    },
+
+STEXI
+@item child_del @var{parent device} @var{child device}
+@findex child_del
+Remove a child from the parent device.
+ETEXI
+
+    {
         .name       = "change",
         .args_type  = "device:B,target:F,arg:s?,read-only-mode:s?",
         .params     = "device filename [format [read-only-mode]]",
diff --git a/hmp.c b/hmp.c
index 6e2bbc8..0d6f6c9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2348,3 +2348,23 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_child_add(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    const char *child_id = qdict_get_str(qdict, "child");
+    Error *local_err = NULL;
+
+    qmp_x_child_add(id, child_id, &local_err);
+    hmp_handle_error(mon, &local_err);
+}
+
+void hmp_child_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    const char *child_id = qdict_get_str(qdict, "child");
+    Error *local_err = NULL;
+
+    qmp_child_del(id, child_id, &local_err);
+    hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index 81656c3..5bd780c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -130,5 +130,7 @@ void hmp_rocker(Monitor *mon, const QDict *qdict);
 void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
+void hmp_child_add(Monitor *mon, const QDict *qdict);
+void hmp_child_del(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
2.4.3