| Message ID | 1441878905-5272-2-git-send-email-wency@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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',
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.
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. >
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. >
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. >> > > > . >
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).
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 >
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. [...]
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. > > [...] > . >
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. >> >> [...] >> . >>
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.
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.
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
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. >>> >>> [...] >>> . >>> > . >
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. >
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?
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 > . >
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.
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).
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.
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. > . >
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). >
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. [...]
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',