[for-2.5,v2,2/6] support nbd driver in blockdev-add

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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 qapi/block-core.json | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

On 08/11/2015 01:51 AM, 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>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  qapi/block-core.json | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7b2efb8..3ed8114 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1383,7 +1383,7 @@
>              'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
>              'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'parallels',
>              'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
> -            'vmdk', 'vpc', 'vvfat' ] }
> +            'vmdk', 'vpc', 'vvfat', 'nbd' ] }

Please keep the list alphabetical. Also, this is missing documentation
(see how BlockDeviceInfo has listed which releases have added which
formats; so it should add a listing of 'nbd' in 2.5).

>  
>  ##
>  # @BlockdevOptionsBase
> @@ -1789,6 +1789,19 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @BlockdevOptionsNBD
> +#
> +# Driver specific block device options for NBD
> +#
> +# @export: #options the NBD export name
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsNBD',
> +  'base': 'InetSocketAddress',
> +  'data': { '*export': 'str' } }

So does NBD really support a range of ports?  Or is it an error to
specify 'to'?  Perhaps what you should really be doing is making a
simpler base class for representing a single-port IP address, then
making InetSocketAddress a child class of the simpler one to add in
ranging, and make BlockdevOptionsNBD a child class of the simpler one to
add in an export name.

Also, InetSocketAddress appears to be limited to IPv4 and IPv6
addresses; but what about nbd+unix transport?  It feels like you need a
flat union with a discriminator that describes what address family (IP
vs. unix socket) and then further details based on that discriminator.

> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1815,7 +1828,7 @@
>        'http':       'BlockdevOptionsFile',
>        'https':      'BlockdevOptionsFile',
>  # TODO iscsi: Wait for structured options
> -# TODO nbd: Should take InetSocketAddress for 'host'?
> +      'nbd':        'BlockdevOptionsNBD',

So while you are indeed literally taking the TODO suggestion, I'm not
sure that the literal interpretation is the best.

Remember, the current NBD source code accepts:

    if (is_unix) {
        /* nbd+unix:///export?socket=path */
        if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
            ret = -EINVAL;
            goto out;
        }
        qdict_put(options, "path", qstring_from_str(qp->p[0].value));
    } else {
        QString *host;
        /* nbd[+tcp]://host[:port]/export */

and we want to be able to represent all of those possibilities in QMP.

On 09/01/2015 01:19 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  qapi/block-core.json | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 7b2efb8..3ed8114 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1383,7 +1383,7 @@
>>              'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
>>              'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'parallels',
>>              'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
>> -            'vmdk', 'vpc', 'vvfat' ] }
>> +            'vmdk', 'vpc', 'vvfat', 'nbd' ] }
> 
> Please keep the list alphabetical. Also, this is missing documentation
> (see how BlockDeviceInfo has listed which releases have added which
> formats; so it should add a listing of 'nbd' in 2.5).
> 
>>  
>>  ##
>>  # @BlockdevOptionsBase
>> @@ -1789,6 +1789,19 @@
>>              '*read-pattern': 'QuorumReadPattern' } }
>>  
>>  ##
>> +# @BlockdevOptionsNBD
>> +#
>> +# Driver specific block device options for NBD
>> +#
>> +# @export: #options the NBD export name
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'struct': 'BlockdevOptionsNBD',
>> +  'base': 'InetSocketAddress',
>> +  'data': { '*export': 'str' } }
> 
> So does NBD really support a range of ports?  Or is it an error to
> specify 'to'?  Perhaps what you should really be doing is making a
> simpler base class for representing a single-port IP address, then
> making InetSocketAddress a child class of the simpler one to add in
> ranging, and make BlockdevOptionsNBD a child class of the simpler one to
> add in an export name.

NBD doesn;t support a range of ports. I will update it in the next version.

> 
> Also, InetSocketAddress appears to be limited to IPv4 and IPv6
> addresses; but what about nbd+unix transport?  It feels like you need a
> flat union with a discriminator that describes what address family (IP
> vs. unix socket) and then further details based on that discriminator.

Hmm, It is SocketAddress, but it contains fd, which nbd doesn't support.
Another problem is that: NBD doesn't support type. It check the key path
and host:

    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
        if (qdict_haskey(options, "path")) {
            error_setg(errp, "path and host may not be used at the same time.");
        } else {
            error_setg(errp, "one of path and host must be specified.");
        }
        return;
    }

    s->client.is_unix = qdict_haskey(options, "path");

Update NBD to support it?

Thanks
Wen Congyang

> 
>> +
>> +##
>>  # @BlockdevOptions
>>  #
>>  # Options for creating a block device.
>> @@ -1815,7 +1828,7 @@
>>        'http':       'BlockdevOptionsFile',
>>        'https':      'BlockdevOptionsFile',
>>  # TODO iscsi: Wait for structured options
>> -# TODO nbd: Should take InetSocketAddress for 'host'?
>> +      'nbd':        'BlockdevOptionsNBD',
> 
> So while you are indeed literally taking the TODO suggestion, I'm not
> sure that the literal interpretation is the best.
> 
> Remember, the current NBD source code accepts:
> 
>     if (is_unix) {
>         /* nbd+unix:///export?socket=path */
>         if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
>             ret = -EINVAL;
>             goto out;
>         }
>         qdict_put(options, "path", qstring_from_str(qp->p[0].value));
>     } else {
>         QString *host;
>         /* nbd[+tcp]://host[:port]/export */
> 
> and we want to be able to represent all of those possibilities in QMP.
>