[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Patch for-2.5 v2 2/6] support nbd driver in blockdev-a
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Patch for-2.5 v2 2/6] support nbd driver in blockdev-add |
Date: |
Mon, 31 Aug 2015 11:19:58 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
On 08/11/2015 01:51 AM, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> ---
> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature