Message ID | 1439279489-13338-3-git-send-email-wency@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
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. >
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' ] } ## # @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' } } + +## # @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', # TODO nfs: Wait for structured options 'null-aio': 'BlockdevOptionsNull', 'null-co': 'BlockdevOptionsNull',