| Message ID | 1468594858-26889-5-git-send-email-prasanna.kalever@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > this patch adds 'GlusterServer' related schema in qapi/block-core.json > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > --- > block/gluster.c | 111 +++++++++++++++++++++++++++++---------------------- > qapi/block-core.json | 94 ++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 153 insertions(+), 52 deletions(-) [Skipping ahead to QAPI schema...] > diff --git a/qapi/block-core.json b/qapi/block-core.json > index a7fdb28..d7b5c76 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1658,13 +1658,14 @@ > # @host_device, @host_cdrom: Since 2.1 > # > # Since: 2.0 > +# @gluster: Since 2.7 > ## > { 'enum': 'BlockdevDriver', > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', > - 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels', > - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', > - 'vmdk', 'vpc', 'vvfat' ] } > + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > + 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', > + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > ## > # @BlockdevOptionsFile > @@ -2057,6 +2058,89 @@ > '*read-pattern': 'QuorumReadPattern' } } > > ## > +# @GlusterTransport > +# > +# An enumeration of Gluster transport type > +# > +# @tcp: TCP - Transmission Control Protocol > +# > +# @unix: UNIX - Unix domain socket > +# > +# Since: 2.7 > +## > +{ 'enum': 'GlusterTransport', > + 'data': [ 'unix', 'tcp' ] } > + > +## > +# @GlusterUnixSocketAddress > +# > +# Captures a socket address in the local ("Unix socket") namespace. > +# > +# @socket: absolute path to socket file > +# > +# Since 2.7 > +## > +{ 'struct': 'GlusterUnixSocketAddress', > + 'data': { 'socket': 'str' } } Compare: ## # @UnixSocketAddress # # Captures a socket address in the local ("Unix socket") namespace. # # @path: filesystem path to use # # Since 1.3 ## { 'struct': 'UnixSocketAddress', 'data': { 'path': 'str' } } > + > +## > +# @GlusterInetSocketAddress > +# > +# Captures a socket address or address range in the Internet namespace. > +# > +# @host: host part of the address > +# > +# @port: #optional port part of the address, or lowest port if @to is present There is no @to. What's the default port? > +# > +# Since 2.7 > +## > +{ 'struct': 'GlusterInetSocketAddress', > + 'data': { 'host': 'str', > + '*port': 'uint16' } } Compare: ## # @InetSocketAddress # # Captures a socket address or address range in the Internet namespace. # # @host: host part of the address # # @port: port part of the address, or lowest port if @to is present # # @to: highest port to try # # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6 # #optional # # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6 # #optional # # Since 1.3 ## { 'struct': 'InetSocketAddress', 'data': { 'host': 'str', 'port': 'str', '*to': 'uint16', '*ipv4': 'bool', '*ipv6': 'bool' } } > + > +## > +# @GlusterServer > +# > +# Captures the address of a socket > +# > +# Details for connecting to a gluster server > +# > +# @type: Transport type used for gluster connection > +# > +# @unix: socket file > +# > +# @tcp: host address and port number > +# > +# Since: 2.7 > +## > +{ 'union': 'GlusterServer', > + 'base': { 'type': 'GlusterTransport' }, > + 'discriminator': 'type', > + 'data': { 'unix': 'GlusterUnixSocketAddress', > + 'tcp': 'GlusterInetSocketAddress' } } > + Compare: ## # @SocketAddress # # Captures the address of a socket, which could also be a named file descriptor # # Since 1.3 ## { 'union': 'SocketAddress', 'data': { 'inet': 'InetSocketAddress', 'unix': 'UnixSocketAddress', 'fd': 'String' } } You cleaned up the confusing abuse of @host as Unix domain socket file name. Good. You're still defining your own QAPI types instead of using the existing ones. To see whether that's a good idea, let's compare your definitions to the existing ones: * GlusterUnixSocketAddress vs. UnixSocketAddress Identical but for the member name. Why can't we use UnixSocketAddress? * GlusterInetSocketAddress vs. InetSocketAddress Changes in GlusterInetSocketAddress over InetSocketAddress: - @port is optional Convenience feature. We can discuss making it optional in InetSocketAddress, too. - @port has type 'uint16' instead of 'str' No support for service names. Why is that a good idea? - Lacks @to No support for trying a range of ports. I guess that simply makes no sense for a Gluster client. I guess makes no sense in some other uses of InetSocketAddress, too. Eric has proposed to split off InetSocketAddressRange off InetSocketAddress. - Lacks @ipv4, @ipv6 No control over IPv4 vs. IPv6 use. Immediate show-stopper. Can we use InetSocketAddress? * GlusterServer vs. SocketAddress GlusterServer lacks case 'fd'. Do file descriptors make no sense here, or is it merely an implementation restriction? GlusterServer is a flat union, SocketAddress is a simple union. The flat unions is nicer on the wire: { "type": "tcp", "host": "gluster.example.com", ... } vs. { "type": "tcp", "data": { "host": "gluster.example.com", ... } Perhaps we should use a flat union for new interfaces. > +## > +# @BlockdevOptionsGluster > +# > +# Driver specific block device options for Gluster > +# > +# @volume: name of gluster volume where VM image resides > +# > +# @path: absolute path to image file in gluster volume > +# > +# @server: gluster server description > +# > +# @debug_level: #optional libgfapi log level (default '4' which is Error) > +# > +# Since: 2.7 > +## > +{ 'struct': 'BlockdevOptionsGluster', > + 'data': { 'volume': 'str', > + 'path': 'str', > + 'server': 'GlusterServer', > + '*debug_level': 'int' } } > + > +## > # @BlockdevOptions > # > # Options for creating a block device. Many options are available for all > @@ -2119,7 +2203,7 @@ > 'file': 'BlockdevOptionsFile', > 'ftp': 'BlockdevOptionsFile', > 'ftps': 'BlockdevOptionsFile', > -# TODO gluster: Wait for structured options > + 'gluster': 'BlockdevOptionsGluster', > 'host_cdrom': 'BlockdevOptionsFile', > 'host_device':'BlockdevOptionsFile', > 'http': 'BlockdevOptionsFile',
On Mon, Jul 18, 2016 at 2:59 PM, Markus Armbruster <armbru@redhat.com> wrote: > Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > >> this patch adds 'GlusterServer' related schema in qapi/block-core.json >> >> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >> --- >> block/gluster.c | 111 +++++++++++++++++++++++++++++---------------------- >> qapi/block-core.json | 94 ++++++++++++++++++++++++++++++++++++++++--- >> 2 files changed, 153 insertions(+), 52 deletions(-) > [Skipping ahead to QAPI schema...] >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index a7fdb28..d7b5c76 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1658,13 +1658,14 @@ >> # @host_device, @host_cdrom: Since 2.1 >> # >> # Since: 2.0 >> +# @gluster: Since 2.7 >> ## >> { 'enum': 'BlockdevDriver', >> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >> - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', >> - 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels', >> - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', >> - 'vmdk', 'vpc', 'vvfat' ] } >> + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >> + 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >> + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', >> + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> >> ## >> # @BlockdevOptionsFile >> @@ -2057,6 +2058,89 @@ >> '*read-pattern': 'QuorumReadPattern' } } >> >> ## >> +# @GlusterTransport >> +# >> +# An enumeration of Gluster transport type >> +# >> +# @tcp: TCP - Transmission Control Protocol >> +# >> +# @unix: UNIX - Unix domain socket >> +# >> +# Since: 2.7 >> +## >> +{ 'enum': 'GlusterTransport', >> + 'data': [ 'unix', 'tcp' ] } >> + >> +## >> +# @GlusterUnixSocketAddress >> +# >> +# Captures a socket address in the local ("Unix socket") namespace. >> +# >> +# @socket: absolute path to socket file >> +# >> +# Since 2.7 >> +## >> +{ 'struct': 'GlusterUnixSocketAddress', >> + 'data': { 'socket': 'str' } } > > Compare: > > ## > # @UnixSocketAddress > # > # Captures a socket address in the local ("Unix socket") namespace. > # > # @path: filesystem path to use > # > # Since 1.3 > ## > { 'struct': 'UnixSocketAddress', > 'data': { > 'path': 'str' } } > >> + >> +## >> +# @GlusterInetSocketAddress >> +# >> +# Captures a socket address or address range in the Internet namespace. >> +# >> +# @host: host part of the address >> +# >> +# @port: #optional port part of the address, or lowest port if @to is present > > There is no @to. > > What's the default port? #define GLUSTER_DEFAULT_PORT 24007 > >> +# >> +# Since 2.7 >> +## >> +{ 'struct': 'GlusterInetSocketAddress', >> + 'data': { 'host': 'str', >> + '*port': 'uint16' } } > > Compare: > > ## > # @InetSocketAddress > # > # Captures a socket address or address range in the Internet namespace. > # > # @host: host part of the address > # > # @port: port part of the address, or lowest port if @to is present > # > # @to: highest port to try > # > # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6 > # #optional > # > # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6 > # #optional > # > # Since 1.3 > ## > { 'struct': 'InetSocketAddress', > 'data': { > 'host': 'str', > 'port': 'str', > '*to': 'uint16', > '*ipv4': 'bool', > '*ipv6': 'bool' } } > >> + >> +## >> +# @GlusterServer >> +# >> +# Captures the address of a socket >> +# >> +# Details for connecting to a gluster server >> +# >> +# @type: Transport type used for gluster connection >> +# >> +# @unix: socket file >> +# >> +# @tcp: host address and port number >> +# >> +# Since: 2.7 >> +## >> +{ 'union': 'GlusterServer', >> + 'base': { 'type': 'GlusterTransport' }, >> + 'discriminator': 'type', >> + 'data': { 'unix': 'GlusterUnixSocketAddress', >> + 'tcp': 'GlusterInetSocketAddress' } } >> + > > Compare: > > ## > # @SocketAddress > # > # Captures the address of a socket, which could also be a named file descriptor > # > # Since 1.3 > ## > { 'union': 'SocketAddress', > 'data': { > 'inet': 'InetSocketAddress', > 'unix': 'UnixSocketAddress', > 'fd': 'String' } } > > You cleaned up the confusing abuse of @host as Unix domain socket file > name. Good. > > You're still defining your own QAPI types instead of using the existing > ones. To see whether that's a good idea, let's compare your definitions > to the existing ones: > > * GlusterUnixSocketAddress vs. UnixSocketAddress > > Identical but for the member name. Why can't we use > UnixSocketAddress? May be you are right, it may not worth just for a member name. > > * GlusterInetSocketAddress vs. InetSocketAddress > > Changes in GlusterInetSocketAddress over InetSocketAddress: > > - @port is optional > > Convenience feature. We can discuss making it optional in > InetSocketAddress, too. sure. > > - @port has type 'uint16' instead of 'str' > > No support for service names. Why is that a good idea? I honestly do not understand tie service names to port. As far all I understand at least from gluster use-case wise its just an integer ranging from 0 - 65535 (mostly 24007) I am happy to learn this > > - Lacks @to > > No support for trying a range of ports. I guess that simply makes > no sense for a Gluster client. I guess makes no sense in some other > uses of InetSocketAddress, too. Eric has proposed to split off > InetSocketAddressRange off InetSocketAddress. I still don't understand the essence, why we need to loop through the ports ? > > - Lacks @ipv4, @ipv6 Gluster don't support ipv6 addresses (there is some work needed in it rpc code) Do you think its worth to have a control over ipv4/ipv6 just to restrict from ipv6 addresses? > > No control over IPv4 vs. IPv6 use. Immediate show-stopper. > > Can we use InetSocketAddress? sorry, I don't have an answer, since I was unclear in my comments above. > > * GlusterServer vs. SocketAddress > > GlusterServer lacks case 'fd'. Do file descriptors make no sense > here, or is it merely an implementation restriction? Again, Gluster doesn't support. > > GlusterServer is a flat union, SocketAddress is a simple union. The flat > unions is nicer on the wire: > { "type": "tcp", "host": "gluster.example.com", ... } > vs. > { "type": "tcp", "data": { "host": "gluster.example.com", ... } > > Perhaps we should use a flat union for new interfaces. > >> +## >> +# @BlockdevOptionsGluster >> +# >> +# Driver specific block device options for Gluster >> +# >> +# @volume: name of gluster volume where VM image resides >> +# >> +# @path: absolute path to image file in gluster volume >> +# >> +# @server: gluster server description >> +# >> +# @debug_level: #optional libgfapi log level (default '4' which is Error) >> +# >> +# Since: 2.7 >> +## >> +{ 'struct': 'BlockdevOptionsGluster', >> + 'data': { 'volume': 'str', >> + 'path': 'str', >> + 'server': 'GlusterServer', >> + '*debug_level': 'int' } } >> + >> +## >> # @BlockdevOptions >> # >> # Options for creating a block device. Many options are available for all >> @@ -2119,7 +2203,7 @@ >> 'file': 'BlockdevOptionsFile', >> 'ftp': 'BlockdevOptionsFile', >> 'ftps': 'BlockdevOptionsFile', >> -# TODO gluster: Wait for structured options >> + 'gluster': 'BlockdevOptionsGluster', >> 'host_cdrom': 'BlockdevOptionsFile', >> 'host_device':'BlockdevOptionsFile', >> 'http': 'BlockdevOptionsFile', Thanks Markus. -- Prasanna
Prasanna Kalever <pkalever@redhat.com> writes: > On Mon, Jul 18, 2016 at 2:59 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: >> >>> this patch adds 'GlusterServer' related schema in qapi/block-core.json >>> >>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >>> --- >>> block/gluster.c | 111 +++++++++++++++++++++++++++++---------------------- >>> qapi/block-core.json | 94 ++++++++++++++++++++++++++++++++++++++++--- >>> 2 files changed, 153 insertions(+), 52 deletions(-) >> [Skipping ahead to QAPI schema...] >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index a7fdb28..d7b5c76 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -1658,13 +1658,14 @@ >>> # @host_device, @host_cdrom: Since 2.1 >>> # >>> # Since: 2.0 >>> +# @gluster: Since 2.7 >>> ## >>> { 'enum': 'BlockdevDriver', >>> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >>> - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', >>> - 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels', >>> - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', >>> - 'vmdk', 'vpc', 'vvfat' ] } >>> + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >>> + 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >>> + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', >>> + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >>> >>> ## >>> # @BlockdevOptionsFile >>> @@ -2057,6 +2058,89 @@ >>> '*read-pattern': 'QuorumReadPattern' } } >>> >>> ## >>> +# @GlusterTransport >>> +# >>> +# An enumeration of Gluster transport type >>> +# >>> +# @tcp: TCP - Transmission Control Protocol >>> +# >>> +# @unix: UNIX - Unix domain socket >>> +# >>> +# Since: 2.7 >>> +## >>> +{ 'enum': 'GlusterTransport', >>> + 'data': [ 'unix', 'tcp' ] } >>> + >>> +## >>> +# @GlusterUnixSocketAddress >>> +# >>> +# Captures a socket address in the local ("Unix socket") namespace. >>> +# >>> +# @socket: absolute path to socket file >>> +# >>> +# Since 2.7 >>> +## >>> +{ 'struct': 'GlusterUnixSocketAddress', >>> + 'data': { 'socket': 'str' } } >> >> Compare: >> >> ## >> # @UnixSocketAddress >> # >> # Captures a socket address in the local ("Unix socket") namespace. >> # >> # @path: filesystem path to use >> # >> # Since 1.3 >> ## >> { 'struct': 'UnixSocketAddress', >> 'data': { >> 'path': 'str' } } >> >>> + >>> +## >>> +# @GlusterInetSocketAddress >>> +# >>> +# Captures a socket address or address range in the Internet namespace. >>> +# >>> +# @host: host part of the address >>> +# >>> +# @port: #optional port part of the address, or lowest port if @to is present >> >> There is no @to. >> >> What's the default port? > > #define GLUSTER_DEFAULT_PORT 24007 I know, but the poor reader of the docs may not, so the docs better spell it out :) >>> +# >>> +# Since 2.7 >>> +## >>> +{ 'struct': 'GlusterInetSocketAddress', >>> + 'data': { 'host': 'str', >>> + '*port': 'uint16' } } >> >> Compare: >> >> ## >> # @InetSocketAddress >> # >> # Captures a socket address or address range in the Internet namespace. >> # >> # @host: host part of the address >> # >> # @port: port part of the address, or lowest port if @to is present >> # >> # @to: highest port to try >> # >> # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6 >> # #optional >> # >> # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6 >> # #optional >> # >> # Since 1.3 >> ## >> { 'struct': 'InetSocketAddress', >> 'data': { >> 'host': 'str', >> 'port': 'str', >> '*to': 'uint16', >> '*ipv4': 'bool', >> '*ipv6': 'bool' } } >> >>> + >>> +## >>> +# @GlusterServer >>> +# >>> +# Captures the address of a socket >>> +# >>> +# Details for connecting to a gluster server >>> +# >>> +# @type: Transport type used for gluster connection >>> +# >>> +# @unix: socket file >>> +# >>> +# @tcp: host address and port number >>> +# >>> +# Since: 2.7 >>> +## >>> +{ 'union': 'GlusterServer', >>> + 'base': { 'type': 'GlusterTransport' }, >>> + 'discriminator': 'type', >>> + 'data': { 'unix': 'GlusterUnixSocketAddress', >>> + 'tcp': 'GlusterInetSocketAddress' } } >>> + >> >> Compare: >> >> ## >> # @SocketAddress >> # >> # Captures the address of a socket, which could also be a named file descriptor >> # >> # Since 1.3 >> ## >> { 'union': 'SocketAddress', >> 'data': { >> 'inet': 'InetSocketAddress', >> 'unix': 'UnixSocketAddress', >> 'fd': 'String' } } >> >> You cleaned up the confusing abuse of @host as Unix domain socket file >> name. Good. >> >> You're still defining your own QAPI types instead of using the existing >> ones. To see whether that's a good idea, let's compare your definitions >> to the existing ones: I've since been gently referred to this note in the cover letter: patch 4/5 (i.e. 3/4 in v18) now uses union discriminator, I have made a choice to use gluster with custom schema since @UnixSocketAddress uses 'path' as key, which may be confusing with gluster, Can you briefly explain why 'path' may be confusing? and in @InetSocketAddress port was str again I have made a choice to keep it uint16 which really make sense. A port can be given in symbolic form (service name) and in numeric form (port number), just like a host. For instance, TCP service name "smtp" means port number 25. See also services(5). Naturally, a symbolic name only exists for sufficiently well-known ports. Interfaces should accept both service name and port. InetSocketAddress does, in the same way as getaddrinfo(): it takes a string, which may either be a service name or a port number. Perhaps it should take an alternate of int16 and string instead, but that's a separate question. Hmmm.. As Markus suggested in v18 qemu_gluster_parseuri() is *parse_uri() same with *parse_json() (in 5/5) Not sure I got that. Do you mean "I renamed qemu_gluster_parseuri() to qemu_gluster_parse_uri() for consistency with qemu_gluster_parse_json()"? >> >> * GlusterUnixSocketAddress vs. UnixSocketAddress >> >> Identical but for the member name. Why can't we use >> UnixSocketAddress? > > May be you are right, it may not worth just for a member name. Can't say, yet; that's why I ask you to explain why it may be confusing. >> * GlusterInetSocketAddress vs. InetSocketAddress >> >> Changes in GlusterInetSocketAddress over InetSocketAddress: >> >> - @port is optional >> >> Convenience feature. We can discuss making it optional in >> InetSocketAddress, too. > > sure. > >> >> - @port has type 'uint16' instead of 'str' >> >> No support for service names. Why is that a good idea? > > I honestly do not understand tie service names to port. > As far all I understand at least from gluster use-case wise its just > an integer ranging from 0 - 65535 (mostly 24007) > I am happy to learn this Hope I was able to explain this above. >> - Lacks @to >> >> No support for trying a range of ports. I guess that simply makes >> no sense for a Gluster client. I guess makes no sense in some other >> uses of InetSocketAddress, too. Eric has proposed to split off >> InetSocketAddressRange off InetSocketAddress. > I still don't understand the essence, why we need to loop through the ports ? Best explained by looking at a use of this feature. With -display vnc, we start a VNC server. By default, it listens on port 5900. If this port isn't available, it fails like this: qemu-system-x86_64:-display vnc=:0: Failed to start VNC server: Failed to bind socket: Address already in use If you don't care about the port, you can use something like "-display vnc=:0,to=9" to bind the first free port in the range of 5900 to 5909. >> - Lacks @ipv4, @ipv6 > > Gluster don't support ipv6 addresses (there is some work needed in it rpc code) > Do you think its worth to have a control over ipv4/ipv6 just to > restrict from ipv6 addresses? In that case, it's not a show-stopper. When Gluster acquires IPv6 support, we'll need them. Until then, we can omit them. If we don't omit them, the gluster driver should reject "ipv4": false. >> No control over IPv4 vs. IPv6 use. Immediate show-stopper. >> >> Can we use InetSocketAddress? > > sorry, I don't have an answer, since I was unclear in my comments above. > >> >> * GlusterServer vs. SocketAddress >> >> GlusterServer lacks case 'fd'. Do file descriptors make no sense >> here, or is it merely an implementation restriction? > > Again, Gluster doesn't support. Yes, the library we use to talk to Gluster doesn't let you pass in a file descriptor today. My question is whether this *could* be supported. The answer is probably "yes". Fd support is usually desirable for privilege separation. There, we want to run QEMU with the least possible privileges. Ideally no way to open TCP connections. Instead, the management application does the opening, and passes the open fd to QEMU. Makes sense because it limits what a malicious guest can gain by attacking QEMU. >> GlusterServer is a flat union, SocketAddress is a simple union. The flat >> unions is nicer on the wire: >> { "type": "tcp", "host": "gluster.example.com", ... } >> vs. >> { "type": "tcp", "data": { "host": "gluster.example.com", ... } >> >> Perhaps we should use a flat union for new interfaces. >> >>> +## >>> +# @BlockdevOptionsGluster >>> +# >>> +# Driver specific block device options for Gluster >>> +# >>> +# @volume: name of gluster volume where VM image resides >>> +# >>> +# @path: absolute path to image file in gluster volume >>> +# >>> +# @server: gluster server description >>> +# >>> +# @debug_level: #optional libgfapi log level (default '4' which is Error) >>> +# >>> +# Since: 2.7 >>> +## >>> +{ 'struct': 'BlockdevOptionsGluster', >>> + 'data': { 'volume': 'str', >>> + 'path': 'str', >>> + 'server': 'GlusterServer', >>> + '*debug_level': 'int' } } >>> + >>> +## >>> # @BlockdevOptions >>> # >>> # Options for creating a block device. Many options are available for all >>> @@ -2119,7 +2203,7 @@ >>> 'file': 'BlockdevOptionsFile', >>> 'ftp': 'BlockdevOptionsFile', >>> 'ftps': 'BlockdevOptionsFile', >>> -# TODO gluster: Wait for structured options >>> + 'gluster': 'BlockdevOptionsGluster', >>> 'host_cdrom': 'BlockdevOptionsFile', >>> 'host_device':'BlockdevOptionsFile', >>> 'http': 'BlockdevOptionsFile', > > Thanks Markus. > > -- > Prasanna
On Mon, Jul 18, 2016 at 6:41 PM, Markus Armbruster <armbru@redhat.com> wrote: > Prasanna Kalever <pkalever@redhat.com> writes: > >> On Mon, Jul 18, 2016 at 2:59 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: >>> >>>> this patch adds 'GlusterServer' related schema in qapi/block-core.json >>>> >>>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >>>> --- >>>> block/gluster.c | 111 +++++++++++++++++++++++++++++---------------------- >>>> qapi/block-core.json | 94 ++++++++++++++++++++++++++++++++++++++++--- >>>> 2 files changed, 153 insertions(+), 52 deletions(-) >>> [Skipping ahead to QAPI schema...] >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index a7fdb28..d7b5c76 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -1658,13 +1658,14 @@ >>>> # @host_device, @host_cdrom: Since 2.1 >>>> # >>>> # Since: 2.0 >>>> +# @gluster: Since 2.7 >>>> ## >>>> { 'enum': 'BlockdevDriver', >>>> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >>>> - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', >>>> - 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels', >>>> - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', >>>> - 'vmdk', 'vpc', 'vvfat' ] } >>>> + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >>>> + 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >>>> + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', >>>> + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >>>> >>>> ## >>>> # @BlockdevOptionsFile >>>> @@ -2057,6 +2058,89 @@ >>>> '*read-pattern': 'QuorumReadPattern' } } >>>> >>>> ## >>>> +# @GlusterTransport >>>> +# >>>> +# An enumeration of Gluster transport type >>>> +# >>>> +# @tcp: TCP - Transmission Control Protocol >>>> +# >>>> +# @unix: UNIX - Unix domain socket >>>> +# >>>> +# Since: 2.7 >>>> +## >>>> +{ 'enum': 'GlusterTransport', >>>> + 'data': [ 'unix', 'tcp' ] } >>>> + >>>> +## >>>> +# @GlusterUnixSocketAddress >>>> +# >>>> +# Captures a socket address in the local ("Unix socket") namespace. >>>> +# >>>> +# @socket: absolute path to socket file >>>> +# >>>> +# Since 2.7 >>>> +## >>>> +{ 'struct': 'GlusterUnixSocketAddress', >>>> + 'data': { 'socket': 'str' } } >>> >>> Compare: >>> >>> ## >>> # @UnixSocketAddress >>> # >>> # Captures a socket address in the local ("Unix socket") namespace. >>> # >>> # @path: filesystem path to use >>> # >>> # Since 1.3 >>> ## >>> { 'struct': 'UnixSocketAddress', >>> 'data': { >>> 'path': 'str' } } >>> >>>> + >>>> +## >>>> +# @GlusterInetSocketAddress >>>> +# >>>> +# Captures a socket address or address range in the Internet namespace. >>>> +# >>>> +# @host: host part of the address >>>> +# >>>> +# @port: #optional port part of the address, or lowest port if @to is present >>> >>> There is no @to. >>> >>> What's the default port? >> >> #define GLUSTER_DEFAULT_PORT 24007 > > I know, but the poor reader of the docs may not, so the docs better > spell it out :) :) Done > >>>> +# >>>> +# Since 2.7 >>>> +## >>>> +{ 'struct': 'GlusterInetSocketAddress', >>>> + 'data': { 'host': 'str', >>>> + '*port': 'uint16' } } >>> >>> Compare: >>> >>> ## >>> # @InetSocketAddress >>> # >>> # Captures a socket address or address range in the Internet namespace. >>> # >>> # @host: host part of the address >>> # >>> # @port: port part of the address, or lowest port if @to is present >>> # >>> # @to: highest port to try >>> # >>> # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6 >>> # #optional >>> # >>> # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6 >>> # #optional >>> # >>> # Since 1.3 >>> ## >>> { 'struct': 'InetSocketAddress', >>> 'data': { >>> 'host': 'str', >>> 'port': 'str', >>> '*to': 'uint16', >>> '*ipv4': 'bool', >>> '*ipv6': 'bool' } } >>> >>>> + >>>> +## >>>> +# @GlusterServer >>>> +# >>>> +# Captures the address of a socket >>>> +# >>>> +# Details for connecting to a gluster server >>>> +# >>>> +# @type: Transport type used for gluster connection >>>> +# >>>> +# @unix: socket file >>>> +# >>>> +# @tcp: host address and port number >>>> +# >>>> +# Since: 2.7 >>>> +## >>>> +{ 'union': 'GlusterServer', >>>> + 'base': { 'type': 'GlusterTransport' }, >>>> + 'discriminator': 'type', >>>> + 'data': { 'unix': 'GlusterUnixSocketAddress', >>>> + 'tcp': 'GlusterInetSocketAddress' } } >>>> + >>> >>> Compare: >>> >>> ## >>> # @SocketAddress >>> # >>> # Captures the address of a socket, which could also be a named file descriptor >>> # >>> # Since 1.3 >>> ## >>> { 'union': 'SocketAddress', >>> 'data': { >>> 'inet': 'InetSocketAddress', >>> 'unix': 'UnixSocketAddress', >>> 'fd': 'String' } } >>> >>> You cleaned up the confusing abuse of @host as Unix domain socket file >>> name. Good. >>> >>> You're still defining your own QAPI types instead of using the existing >>> ones. To see whether that's a good idea, let's compare your definitions >>> to the existing ones: > > I've since been gently referred to this note in the cover letter: > > patch 4/5 (i.e. 3/4 in v18) now uses union discriminator, I have > made a choice to use gluster with custom schema since > @UnixSocketAddress uses 'path' as key, which may be confusing with > gluster, > > Can you briefly explain why 'path' may be confusing? > As you would have noticed below in my previous reply about my ACK > and in @InetSocketAddress port was str again I have made a choice to > keep it uint16 which really make sense. > > A port can be given in symbolic form (service name) and in numeric form > (port number), just like a host. For instance, TCP service name "smtp" > means port number 25. See also services(5). Naturally, a symbolic name > only exists for sufficiently well-known ports. > > Interfaces should accept both service name and port. InetSocketAddress > does, in the same way as getaddrinfo(): it takes a string, which may > either be a service name or a port number. Perhaps it should take an > alternate of int16 and string instead, but that's a separate question. This really improved my understanding, thanks Markus Having agreed that, I need to say about feeder api glfs_setvolfile_server() accept only int; look at the scaffolding here int pub_glfs_set_volfile_server (struct glfs *fs, const char *transport, const char *host, int port) So, I hope you stand with me, in making port as int; > > Hmmm.. As Markus suggested in v18 qemu_gluster_parseuri() is > *parse_uri() same with *parse_json() (in 5/5) > > Not sure I got that. Do you mean "I renamed qemu_gluster_parseuri() to > qemu_gluster_parse_uri() for consistency with > qemu_gluster_parse_json()"? In a comment you did mentioned "parsejson isn't a word. "parse_json" would be two :)" So to maintain consistency for parseuri with parsejson, I made the changes. If I have over taken them, please excuse me for this :) > >>> >>> * GlusterUnixSocketAddress vs. UnixSocketAddress >>> >>> Identical but for the member name. Why can't we use >>> UnixSocketAddress? >> >> May be you are right, it may not worth just for a member name. > > Can't say, yet; that's why I ask you to explain why it may be confusing. > >>> * GlusterInetSocketAddress vs. InetSocketAddress >>> >>> Changes in GlusterInetSocketAddress over InetSocketAddress: >>> >>> - @port is optional >>> >>> Convenience feature. We can discuss making it optional in >>> InetSocketAddress, too. >> >> sure. >> >>> >>> - @port has type 'uint16' instead of 'str' >>> >>> No support for service names. Why is that a good idea? >> >> I honestly do not understand tie service names to port. >> As far all I understand at least from gluster use-case wise its just >> an integer ranging from 0 - 65535 (mostly 24007) >> I am happy to learn this > > Hope I was able to explain this above. yes! > >>> - Lacks @to >>> >>> No support for trying a range of ports. I guess that simply makes >>> no sense for a Gluster client. I guess makes no sense in some other >>> uses of InetSocketAddress, too. Eric has proposed to split off >>> InetSocketAddressRange off InetSocketAddress. >> I still don't understand the essence, why we need to loop through the ports ? > > Best explained by looking at a use of this feature. With -display vnc, > we start a VNC server. By default, it listens on port 5900. If this > port isn't available, it fails like this: > > qemu-system-x86_64:-display vnc=:0: Failed to start VNC server: Failed to bind socket: Address already in use > > If you don't care about the port, you can use something like "-display > vnc=:0,to=9" to bind the first free port in the range of 5900 to 5909. > In gluster case arriving at the port that is in use, is non trivial. Its mostly 24007 or something user choice (say 5007) which is something unpredictable or looping for that may not worth; So, IMO this is not the case in gluster port pickup; >>> - Lacks @ipv4, @ipv6 >> >> Gluster don't support ipv6 addresses (there is some work needed in it rpc code) >> Do you think its worth to have a control over ipv4/ipv6 just to >> restrict from ipv6 addresses? > > In that case, it's not a show-stopper. When Gluster acquires IPv6 > support, we'll need them. Until then, we can omit them. If we don't > omit them, the gluster driver should reject "ipv4": false. > So I am dropping this for now; will send a patch soon after gluster fully supports ipv6 addressing >>> No control over IPv4 vs. IPv6 use. Immediate show-stopper. >>> >>> Can we use InetSocketAddress? >> >> sorry, I don't have an answer, since I was unclear in my comments above. >> >>> >>> * GlusterServer vs. SocketAddress >>> >>> GlusterServer lacks case 'fd'. Do file descriptors make no sense >>> here, or is it merely an implementation restriction? >> >> Again, Gluster doesn't support. > > Yes, the library we use to talk to Gluster doesn't let you pass in a > file descriptor today. > > My question is whether this *could* be supported. The answer is > probably "yes". > > Fd support is usually desirable for privilege separation. There, we > want to run QEMU with the least possible privileges. Ideally no way to > open TCP connections. Instead, the management application does the > opening, and passes the open fd to QEMU. Makes sense because it limits > what a malicious guest can gain by attacking QEMU. > I got your point here; since we are clear that gluster doesn't support this at least for now; Somehow I came to a opinion from all the points described above, we don't need 'SocketAddress' for the same reasons that gluster needs a tweaked 'InetSocketAddress' , so lets also keep-off the fd for now. { 'struct': 'InetSocketAddress', 'data': { 'host': 'str', 'port': 'str', '*to': 'uint16', '*ipv4': 'bool', '*ipv6': 'bool' } } tweaked version of InetSocketAddress i.e GlusterInetSocketAddress * not require 'ipv6' * doesn't need 'to' and * 'port' should be optional and integer, as glfs_set_volfile_server() demands so, { 'struct': 'GlusterInetSocketAddress', 'data': { 'host': 'str', '*port': 'str' }} Hence, { 'union': 'SocketAddress', 'data': { 'inet': 'InetSocketAddress', 'unix': 'UnixSocketAddress', 'fd': 'String' } } after removing 'fd' part which is not supported now, this look like { 'union': 'GlusterSocketAddress', 'data': { 'inet': 'GlusterInetSocketAddress', 'unix': 'UnixSocketAddress' } } What do you think ? Thanks, -- Prasanna >>> GlusterServer is a flat union, SocketAddress is a simple union. The flat >>> unions is nicer on the wire: >>> { "type": "tcp", "host": "gluster.example.com", ... } >>> vs. >>> { "type": "tcp", "data": { "host": "gluster.example.com", ... } >>> >>> Perhaps we should use a flat union for new interfaces. >>> >>>> +## >>>> +# @BlockdevOptionsGluster >>>> +# >>>> +# Driver specific block device options for Gluster >>>> +# >>>> +# @volume: name of gluster volume where VM image resides >>>> +# >>>> +# @path: absolute path to image file in gluster volume >>>> +# >>>> +# @server: gluster server description >>>> +# >>>> +# @debug_level: #optional libgfapi log level (default '4' which is Error) >>>> +# >>>> +# Since: 2.7 >>>> +## >>>> +{ 'struct': 'BlockdevOptionsGluster', >>>> + 'data': { 'volume': 'str', >>>> + 'path': 'str', >>>> + 'server': 'GlusterServer', >>>> + '*debug_level': 'int' } } >>>> + >>>> +## >>>> # @BlockdevOptions >>>> # >>>> # Options for creating a block device. Many options are available for all >>>> @@ -2119,7 +2203,7 @@ >>>> 'file': 'BlockdevOptionsFile', >>>> 'ftp': 'BlockdevOptionsFile', >>>> 'ftps': 'BlockdevOptionsFile', >>>> -# TODO gluster: Wait for structured options >>>> + 'gluster': 'BlockdevOptionsGluster', >>>> 'host_cdrom': 'BlockdevOptionsFile', >>>> 'host_device':'BlockdevOptionsFile', >>>> 'http': 'BlockdevOptionsFile', >> >> Thanks Markus. >> >> -- >> Prasanna
Cc'ing Eric, because I'd like his advice on a couple of points. Prasanna Kalever <pkalever@redhat.com> writes: > On Mon, Jul 18, 2016 at 6:41 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Prasanna Kalever <pkalever@redhat.com> writes: >> >>> On Mon, Jul 18, 2016 at 2:59 PM, Markus Armbruster <armbru@redhat.com> wrote: >>>> Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: >>>> >>>>> this patch adds 'GlusterServer' related schema in qapi/block-core.json >>>>> >>>>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> >>>>> --- >>>>> block/gluster.c | 111 +++++++++++++++++++++++++++++---------------------- >>>>> qapi/block-core.json | 94 ++++++++++++++++++++++++++++++++++++++++--- >>>>> 2 files changed, 153 insertions(+), 52 deletions(-) >>>> [Skipping ahead to QAPI schema...] >>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>>> index a7fdb28..d7b5c76 100644 >>>>> --- a/qapi/block-core.json >>>>> +++ b/qapi/block-core.json >>>>> @@ -1658,13 +1658,14 @@ >>>>> # @host_device, @host_cdrom: Since 2.1 >>>>> # >>>>> # Since: 2.0 >>>>> +# @gluster: Since 2.7 >>>>> ## >>>>> { 'enum': 'BlockdevDriver', >>>>> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >>>>> - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', >>>>> - 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels', >>>>> - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', >>>>> - 'vmdk', 'vpc', 'vvfat' ] } >>>>> + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >>>>> + 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >>>>> + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', >>>>> + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >>>>> >>>>> ## >>>>> # @BlockdevOptionsFile >>>>> @@ -2057,6 +2058,89 @@ >>>>> '*read-pattern': 'QuorumReadPattern' } } >>>>> >>>>> ## >>>>> +# @GlusterTransport >>>>> +# >>>>> +# An enumeration of Gluster transport type >>>>> +# >>>>> +# @tcp: TCP - Transmission Control Protocol >>>>> +# >>>>> +# @unix: UNIX - Unix domain socket >>>>> +# >>>>> +# Since: 2.7 >>>>> +## >>>>> +{ 'enum': 'GlusterTransport', >>>>> + 'data': [ 'unix', 'tcp' ] } >>>>> + >>>>> +## >>>>> +# @GlusterUnixSocketAddress >>>>> +# >>>>> +# Captures a socket address in the local ("Unix socket") namespace. >>>>> +# >>>>> +# @socket: absolute path to socket file >>>>> +# >>>>> +# Since 2.7 >>>>> +## >>>>> +{ 'struct': 'GlusterUnixSocketAddress', >>>>> + 'data': { 'socket': 'str' } } >>>> >>>> Compare: >>>> >>>> ## >>>> # @UnixSocketAddress >>>> # >>>> # Captures a socket address in the local ("Unix socket") namespace. >>>> # >>>> # @path: filesystem path to use >>>> # >>>> # Since 1.3 >>>> ## >>>> { 'struct': 'UnixSocketAddress', >>>> 'data': { >>>> 'path': 'str' } } >>>> >>>>> + >>>>> +## >>>>> +# @GlusterInetSocketAddress >>>>> +# >>>>> +# Captures a socket address or address range in the Internet namespace. >>>>> +# >>>>> +# @host: host part of the address >>>>> +# >>>>> +# @port: #optional port part of the address, or lowest port if @to is present >>>> >>>> There is no @to. >>>> >>>> What's the default port? >>> >>> #define GLUSTER_DEFAULT_PORT 24007 >> >> I know, but the poor reader of the docs may not, so the docs better >> spell it out :) > > :) Done > >> >>>>> +# >>>>> +# Since 2.7 >>>>> +## >>>>> +{ 'struct': 'GlusterInetSocketAddress', >>>>> + 'data': { 'host': 'str', >>>>> + '*port': 'uint16' } } >>>> >>>> Compare: >>>> >>>> ## >>>> # @InetSocketAddress >>>> # >>>> # Captures a socket address or address range in the Internet namespace. >>>> # >>>> # @host: host part of the address >>>> # >>>> # @port: port part of the address, or lowest port if @to is present >>>> # >>>> # @to: highest port to try >>>> # >>>> # @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6 >>>> # #optional >>>> # >>>> # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6 >>>> # #optional >>>> # >>>> # Since 1.3 >>>> ## >>>> { 'struct': 'InetSocketAddress', >>>> 'data': { >>>> 'host': 'str', >>>> 'port': 'str', >>>> '*to': 'uint16', >>>> '*ipv4': 'bool', >>>> '*ipv6': 'bool' } } >>>> >>>>> + >>>>> +## >>>>> +# @GlusterServer >>>>> +# >>>>> +# Captures the address of a socket >>>>> +# >>>>> +# Details for connecting to a gluster server >>>>> +# >>>>> +# @type: Transport type used for gluster connection >>>>> +# >>>>> +# @unix: socket file >>>>> +# >>>>> +# @tcp: host address and port number >>>>> +# >>>>> +# Since: 2.7 >>>>> +## >>>>> +{ 'union': 'GlusterServer', >>>>> + 'base': { 'type': 'GlusterTransport' }, >>>>> + 'discriminator': 'type', >>>>> + 'data': { 'unix': 'GlusterUnixSocketAddress', >>>>> + 'tcp': 'GlusterInetSocketAddress' } } >>>>> + >>>> >>>> Compare: >>>> >>>> ## >>>> # @SocketAddress >>>> # >>>> # Captures the address of a socket, which could also be a named file descriptor >>>> # >>>> # Since 1.3 >>>> ## >>>> { 'union': 'SocketAddress', >>>> 'data': { >>>> 'inet': 'InetSocketAddress', >>>> 'unix': 'UnixSocketAddress', >>>> 'fd': 'String' } } >>>> >>>> You cleaned up the confusing abuse of @host as Unix domain socket file >>>> name. Good. >>>> >>>> You're still defining your own QAPI types instead of using the existing >>>> ones. To see whether that's a good idea, let's compare your definitions >>>> to the existing ones: >> >> I've since been gently referred to this note in the cover letter: >> >> patch 4/5 (i.e. 3/4 in v18) now uses union discriminator, I have >> made a choice to use gluster with custom schema since >> @UnixSocketAddress uses 'path' as key, which may be confusing with >> gluster, >> >> Can you briefly explain why 'path' may be confusing? >> > > As you would have noticed below in my previous reply about my ACK > >> and in @InetSocketAddress port was str again I have made a choice to >> keep it uint16 which really make sense. >> >> A port can be given in symbolic form (service name) and in numeric form >> (port number), just like a host. For instance, TCP service name "smtp" >> means port number 25. See also services(5). Naturally, a symbolic name >> only exists for sufficiently well-known ports. >> >> Interfaces should accept both service name and port. InetSocketAddress >> does, in the same way as getaddrinfo(): it takes a string, which may >> either be a service name or a port number. Perhaps it should take an >> alternate of int16 and string instead, but that's a separate question. > > This really improved my understanding, thanks Markus > Having agreed that, I need to say about feeder api glfs_setvolfile_server() > accept only int; > > look at the scaffolding here > > int > pub_glfs_set_volfile_server (struct glfs *fs, const char *transport, > const char *host, int port) > > So, I hope you stand with me, in making port as int; If we consider just interfacing with (the current version of) glusterfs-devel, int is the natural type of port. However, we already have a related abstraction: InetSocketAddress. If it fits, we should probably reuse it. If we decide not to reuse it now, we may still want to minimize differences to keep the door open for future reuse. InetSocketAddress's port is str. If we make GlusterInetSocketAddress's port an integer, then a future unification with InetSocketAddress will have to make port an alternate. Not impossible, but why tie our hands that way now? But let's not get bogged down in this detail now. We first have to decide whether to reuse InetSocketAddress now (more on that below). If yes, the question is moot. >> Hmmm.. As Markus suggested in v18 qemu_gluster_parseuri() is >> *parse_uri() same with *parse_json() (in 5/5) >> >> Not sure I got that. Do you mean "I renamed qemu_gluster_parseuri() to >> qemu_gluster_parse_uri() for consistency with >> qemu_gluster_parse_json()"? > > In a comment you did mentioned "parsejson isn't a word. "parse_json" > would be two :)" > > So to maintain consistency for parseuri with parsejson, I made the changes. Makes sense. > If I have over taken them, please excuse me for this :) Nah, I just didn't understand your note, and wanted to make sure I didn't miss anything. >>>> * GlusterUnixSocketAddress vs. UnixSocketAddress >>>> >>>> Identical but for the member name. Why can't we use >>>> UnixSocketAddress? >>> >>> May be you are right, it may not worth just for a member name. >> >> Can't say, yet; that's why I ask you to explain why it may be confusing. Okay, looks like GlusterUnixSocketAddress is not really necessary. >>>> * GlusterInetSocketAddress vs. InetSocketAddress >>>> >>>> Changes in GlusterInetSocketAddress over InetSocketAddress: >>>> >>>> - @port is optional >>>> >>>> Convenience feature. We can discuss making it optional in >>>> InetSocketAddress, too. >>> >>> sure. Of course, it's too late in the development cycle for making it optional *now*. If we reuse InetSocketAddress now, as is, then the QMP client has to specify the Gluster port, even though it's usually 24007. For command line and HMP, we can still make it optional. I think that's tolerable. We can investigate making it optional later. >>>> - @port has type 'uint16' instead of 'str' >>>> >>>> No support for service names. Why is that a good idea? >>> >>> I honestly do not understand tie service names to port. >>> As far all I understand at least from gluster use-case wise its just >>> an integer ranging from 0 - 65535 (mostly 24007) >>> I am happy to learn this >> >> Hope I was able to explain this above. > > yes! > >> >>>> - Lacks @to >>>> >>>> No support for trying a range of ports. I guess that simply makes >>>> no sense for a Gluster client. I guess makes no sense in some other >>>> uses of InetSocketAddress, too. Eric has proposed to split off >>>> InetSocketAddressRange off InetSocketAddress. >>> I still don't understand the essence, why we need to loop through the ports ? >> >> Best explained by looking at a use of this feature. With -display vnc, >> we start a VNC server. By default, it listens on port 5900. If this >> port isn't available, it fails like this: >> >> qemu-system-x86_64:-display vnc=:0: Failed to start VNC server: Failed to bind socket: Address already in use >> >> If you don't care about the port, you can use something like "-display >> vnc=:0,to=9" to bind the first free port in the range of 5900 to 5909. >> > > In gluster case arriving at the port that is in use, is non trivial. > Its mostly 24007 or something user choice (say 5007) which is > something unpredictable or looping for that may not worth; > > So, IMO this is not the case in gluster port pickup; Port ranges make sense for some users of InetSocketAddress, but not for others. Gluster is of the latter kind. So far, the latter kind simply uses InetSocketAddress, and either rejects @to or ignores it. The latter would be a bug. We could do the same for Gluster: if has_to, error out. Alternatively, we could split InetSocketAddressRange (with @to) off InetSocketAddress (delete @to). Makes range support visible in introspection. If we use InetSocketAddress as is now, and have gluster.c reject @to, we can still split off InetSocketAddressRange later. The external interface doesn't care whether @to is rejected as unknown parameter by QAPI or as unsupported parameter by gluster.c. Remember, the purpose of reviewing differences between GlusterInetSocketAddress and InetSocketAddressRange is to figure out whether we can use InetSocketAddress for Gluster, and if we can, whether we should. The result here is that @to doesn't preclude reuse of InetSocketAddress. We've already seen that @port doesn't preclude it, either. >>>> - Lacks @ipv4, @ipv6 >>> >>> Gluster don't support ipv6 addresses (there is some work needed in it rpc code) >>> Do you think its worth to have a control over ipv4/ipv6 just to >>> restrict from ipv6 addresses? >> >> In that case, it's not a show-stopper. When Gluster acquires IPv6 >> support, we'll need them. Until then, we can omit them. If we don't >> omit them, the gluster driver should reject "ipv4": false. >> > > So I am dropping this for now; will send a patch soon after gluster > fully supports ipv6 addressing Like @to, @ipv4 and @ipv6 don't preclude reuse of InetSocketAddress: C code can simply reject the ones it can't obey. Now let me summarize. We could do without GlusterInetSocketAddress, because: * SocketAddress's non-optional @port is a minor inconvenience which we can address in the future without breaking compatibility. * SocketAddress's string @port is a minor inconvenience for the C code using it. Keeping the external interface consistent (same type for TCP ports everywhere) is worth some inconvenience in the C code. * SocketAddress's @to complicates the external interface, but the complication exists elsewhere already. gluster.c can reject @to. We can clean up the external interface in the future without breaking compatibility. * SocketAddress's @ipv4, @ipv6 cannot be fully implemented by gluster.c, yet. gluster.c can simply reject the settings it doesn't implement, until they get implemented. Reasons for having a separate GlusterInetSocketAddress: * Slightly less code in gluster.c. I reject that reason. Keeping the interface lean is worth some extra code. Note that extra schema definitions to avoid code complications may be fine as long as they don't affect external interfaces. * Makes non-support of port ranges and IPv6 visible in introspection. That's a valid argument, but it's an argument for having Inet4SockAddress, not for GlusterInetSocketAddress. Eric, do you think there's a need for introspecting IPv6 support? Any other reasons? >>>> No control over IPv4 vs. IPv6 use. Immediate show-stopper. >>>> >>>> Can we use InetSocketAddress? >>> >>> sorry, I don't have an answer, since I was unclear in my comments above. >>> >>>> >>>> * GlusterServer vs. SocketAddress >>>> >>>> GlusterServer lacks case 'fd'. Do file descriptors make no sense >>>> here, or is it merely an implementation restriction? >>> >>> Again, Gluster doesn't support. >> >> Yes, the library we use to talk to Gluster doesn't let you pass in a >> file descriptor today. >> >> My question is whether this *could* be supported. The answer is >> probably "yes". >> >> Fd support is usually desirable for privilege separation. There, we >> want to run QEMU with the least possible privileges. Ideally no way to >> open TCP connections. Instead, the management application does the >> opening, and passes the open fd to QEMU. Makes sense because it limits >> what a malicious guest can gain by attacking QEMU. >> > > I got your point here; > > since we are clear that gluster doesn't support this at least for now; > Somehow I came to a opinion from all the points described above, we > don't need 'SocketAddress' for the same reasons that gluster needs a > tweaked 'InetSocketAddress' , so lets also keep-off the fd for now. Similar design question as for @ipv4, @ipv6: reuse of SocketAddress is possible, but the C code has to reject options it doesn't implement, i.e. "type": "fd". Non-support of fd isn't visible in introspection then. Eric, do you think there's a need for introspecting fd support? Additional design question: do we want to move away from SocketAddress's pointless nesting on the wire for new interfaces? I.e. move from something like { "type": "tcp", "data": { "host": "gluster.example.com", ... } to { "type": "tcp", "host": "gluster.example.com", ... } This isn't a Gluster question, it's a general question. Eric, what do you think? > { 'struct': 'InetSocketAddress', > 'data': { > 'host': 'str', > 'port': 'str', > '*to': 'uint16', > '*ipv4': 'bool', > '*ipv6': 'bool' } } > > tweaked version of InetSocketAddress i.e GlusterInetSocketAddress > * not require 'ipv6' > * doesn't need 'to' and > * 'port' should be optional and integer, as glfs_set_volfile_server() demands > > so, > > { 'struct': 'GlusterInetSocketAddress', > 'data': { > 'host': 'str', > '*port': 'str' }} > > Hence, > > { 'union': 'SocketAddress', > 'data': { > 'inet': 'InetSocketAddress', > 'unix': 'UnixSocketAddress', > 'fd': 'String' } } > > after removing 'fd' part which is not supported now, this look like > > { 'union': 'GlusterSocketAddress', > 'data': { > 'inet': 'GlusterInetSocketAddress', > 'unix': 'UnixSocketAddress' } } > > What do you think ? This is fine if * we decide we want a new GlusterInetSocketAddress instead of reusing InetSocketAddress, and * we decide we don't want to avoid nesting on the wire. But we haven't settled these questions. I'd like to settle them today, taking Eric's advice into account.
On 07/19/2016 05:12 AM, Markus Armbruster wrote: > Cc'ing Eric, because I'd like his advice on a couple of points. I've been following the thread, but with this specific invitation, I'll definitely chime in. >> int >> pub_glfs_set_volfile_server (struct glfs *fs, const char *transport, >> const char *host, int port) >> >> So, I hope you stand with me, in making port as int; > > If we consider just interfacing with (the current version of) > glusterfs-devel, int is the natural type of port. > > However, we already have a related abstraction: InetSocketAddress. If > it fits, we should probably reuse it. If we decide not to reuse it now, > we may still want to minimize differences to keep the door open for > future reuse. Furthermore, just because gluster code doesn't accept a string does not prevent qemu from accepting a string, looking it up in the database of named ports, and passing the corresponding number on to gluster. I am very strongly in favor of supporting named ports in the qemu interface, even if it means more work under the hood for qemu to pass on to gluster. Whether we support that by an alternate between int and string, or keep symmetry with existing interfaces being string only, is where it becomes a judgment call (and going string-only for now can always be improved later for convenience). > > InetSocketAddress's port is str. If we make GlusterInetSocketAddress's > port an integer, then a future unification with InetSocketAddress will > have to make port an alternate. Not impossible, but why tie our hands > that way now? But let's not get bogged down in this detail now. We > first have to decide whether to reuse InetSocketAddress now (more on > that below). If yes, the question is moot. > >>>>> Changes in GlusterInetSocketAddress over InetSocketAddress: >>>>> >>>>> - @port is optional >>>>> >>>>> Convenience feature. We can discuss making it optional in >>>>> InetSocketAddress, too. >>>> >>>> sure. > > Of course, it's too late in the development cycle for making it optional > *now*. > > If we reuse InetSocketAddress now, as is, then the QMP client has to > specify the Gluster port, even though it's usually 24007. For command > line and HMP, we can still make it optional. I think that's tolerable. > We can investigate making it optional later. Agree - we're too late to make it optional in 2.7, but there is nothing preventing us from making it optional in the future, and it's neither too much burden on libvirt to supply a sane default if it is mandatory, nor too hard for the command line and HMP to make it optional on top of a mandatory QMP, for the sake of human interaction. > > Port ranges make sense for some users of InetSocketAddress, but not for > others. Gluster is of the latter kind. > > So far, the latter kind simply uses InetSocketAddress, and either > rejects @to or ignores it. The latter would be a bug. > > We could do the same for Gluster: if has_to, error out. > > Alternatively, we could split InetSocketAddressRange (with @to) off > InetSocketAddress (delete @to). Makes range support visible in > introspection. Ultimately, I think we should split InetSocketAddress. But we're too late to do it in 2.7. > > If we use InetSocketAddress as is now, and have gluster.c reject @to, we > can still split off InetSocketAddressRange later. The external > interface doesn't care whether @to is rejected as unknown parameter by > QAPI or as unsupported parameter by gluster.c. > Agree - at this point, manually rejecting has_to will let us reuse the type, and would then be part of the cleanup work when 2.8 splits InetSocketAddress. > Now let me summarize. We could do without GlusterInetSocketAddress, > because: > > * SocketAddress's non-optional @port is a minor inconvenience which > we can address in the future without breaking compatibility. > > * SocketAddress's string @port is a minor inconvenience for the C code > using it. Keeping the external interface consistent (same type for > TCP ports everywhere) is worth some inconvenience in the C code. > > * SocketAddress's @to complicates the external interface, but the > complication exists elsewhere already. gluster.c can reject @to. We > can clean up the external interface in the future without breaking > compatibility. In fact, introspection will be nicer when we split InetSocketAddress to no longer include 'to', as you will then be able to precisely determine which uses support ranges. For now, management apps just have to be aware that ranges may be rejected. > > * SocketAddress's @ipv4, @ipv6 cannot be fully implemented by gluster.c, > yet. gluster.c can simply reject the settings it doesn't implement, > until they get implemented. > > Reasons for having a separate GlusterInetSocketAddress: > > * Slightly less code in gluster.c. I reject that reason. Keeping the > interface lean is worth some extra code. > > Note that extra schema definitions to avoid code complications may be > fine as long as they don't affect external interfaces. > > * Makes non-support of port ranges and IPv6 visible in introspection. > That's a valid argument, but it's an argument for having > Inet4SockAddress, not for GlusterInetSocketAddress. > > Eric, do you think there's a need for introspecting IPv6 support? I'm borderline on that one. If the error message given when attempting an IPv6 address is nice enough, then libvirt can just blindly try an IPv6 address and inform the user that qemu/gluster is too old to support it. If the error message is lousy, then being able to introspect whether IPv6 support is present would let libvirt provide a saner error message. But at the moment, I'm not convinced that introspection alone is reason to create a struct without IPv6 now, where IPv6 is added later once gluster supports it. Furthermore, a system administrator that NEEDS to use IPv6 is very likely to upgrade their system to support things before trying to use IPv6. So in this case, I'm leaning towards just reusing the type and rejecting IPv6 in the gluster code until it works. > > Similar design question as for @ipv4, @ipv6: reuse of SocketAddress is > possible, but the C code has to reject options it doesn't implement, > i.e. "type": "fd". Non-support of fd isn't visible in introspection > then. > > Eric, do you think there's a need for introspecting fd support? There's no easy way to declare one QAPI union that is a superset of another (all the same branches as before, plus these additional branches); it requires duplication of all the common branches. Arguably, we could extend QAPI if we find ourselves doing that a lot, to make it supported with less copy and paste; but manual duplication right now doesn't hurt. Here, I think introspection is probably more useful, particularly since there are security aspects in fd passing (where libvirt can pre-open the connection) that are nicer to know up front if they will work, and where it is less obvious to a system administrator that they need/want to use fd passing under the hood. IPv6 is very easy to say: "will my deployment need IPv6 addresses - if so, upgrade gluster to a version that supports IPv6". But fd passing is not so straightforward. So I think being able to introspect this one is useful - but I also think that having a union that shares the same underlying types for the common branches is still slightly nicer than having different types for the same-named branches (that is, since both the reduced union without 'fd' support and the full union will have 'unix' and 'inet' branches, both the 'unix' and 'inet' branch should use the same underlying type rather than being subtly different). > > Additional design question: do we want to move away from SocketAddress's > pointless nesting on the wire for new interfaces? I.e. move from > something like > > { "type": "tcp", "data": { "host": "gluster.example.com", ... } > > to > > { "type": "tcp", "host": "gluster.example.com", ... } > > This isn't a Gluster question, it's a general question. Eric, what do > you think? > Nested vs. flat is somewhat cosmetic - machine-generated code (like libvirt's interactions) can cope with either. On the other hand, adding a flat type now may make OTHER blockdev-add drivers easier to QAPIfy, such as nbd and sheepdog, and I kind of like the simplicity afforded by a flat type. >> { 'union': 'SocketAddress', >> 'data': { >> 'inet': 'InetSocketAddress', >> 'unix': 'UnixSocketAddress', >> 'fd': 'String' } } I also think that if we add a flat type, it would be nicer to use 'fd':'str' instead of 'fd':'String' (which is itself another pointless nesting due to backwards compatibility), at least for the version of the flat type where 'fd' is supported. >> >> after removing 'fd' part which is not supported now, this look like >> >> { 'union': 'GlusterSocketAddress', >> 'data': { >> 'inet': 'GlusterInetSocketAddress', >> 'unix': 'UnixSocketAddress' } } >> >> What do you think ? > > This is fine if > > * we decide we want a new GlusterInetSocketAddress instead of reusing > InetSocketAddress, and > > * we decide we don't want to avoid nesting on the wire. > > But we haven't settled these questions. I'd like to settle them today, > taking Eric's advice into account. I think sharing InetSocketAddress is reasonable, but having separate GlusterSocketAddress (or more generically, FlatSocketAddress that can be shared by gluster, sheepdog, and nbd), seems okay. I'm also not fussed about naming - naming it 'GlusterSocketAddress' now and renaming it 'FlatSocketAddress' later when it gets more use does not impact introspection.
Eric Blake <eblake@redhat.com> writes: > On 07/19/2016 05:12 AM, Markus Armbruster wrote: >> Cc'ing Eric, because I'd like his advice on a couple of points. > > I've been following the thread, but with this specific invitation, I'll > definitely chime in. > > >>> int >>> pub_glfs_set_volfile_server (struct glfs *fs, const char *transport, >>> const char *host, int port) >>> >>> So, I hope you stand with me, in making port as int; >> >> If we consider just interfacing with (the current version of) >> glusterfs-devel, int is the natural type of port. >> >> However, we already have a related abstraction: InetSocketAddress. If >> it fits, we should probably reuse it. If we decide not to reuse it now, >> we may still want to minimize differences to keep the door open for >> future reuse. > > Furthermore, just because gluster code doesn't accept a string does not > prevent qemu from accepting a string, looking it up in the database of > named ports, and passing the corresponding number on to gluster. I am > very strongly in favor of supporting named ports in the qemu interface, > even if it means more work under the hood for qemu to pass on to > gluster. Whether we support that by an alternate between int and > string, or keep symmetry with existing interfaces being string only, is > where it becomes a judgment call (and going string-only for now can > always be improved later for convenience). Let's do just string now. It's consistent with what we use elsewhere, and doesn't tie our hands. To avoid further delay, arbitrarily restrict the string to port number for now. >> InetSocketAddress's port is str. If we make GlusterInetSocketAddress's >> port an integer, then a future unification with InetSocketAddress will >> have to make port an alternate. Not impossible, but why tie our hands >> that way now? But let's not get bogged down in this detail now. We >> first have to decide whether to reuse InetSocketAddress now (more on >> that below). If yes, the question is moot. >> > >>>>>> Changes in GlusterInetSocketAddress over InetSocketAddress: >>>>>> >>>>>> - @port is optional >>>>>> >>>>>> Convenience feature. We can discuss making it optional in >>>>>> InetSocketAddress, too. >>>>> >>>>> sure. >> >> Of course, it's too late in the development cycle for making it optional >> *now*. >> >> If we reuse InetSocketAddress now, as is, then the QMP client has to >> specify the Gluster port, even though it's usually 24007. For command >> line and HMP, we can still make it optional. I think that's tolerable. >> We can investigate making it optional later. > > Agree - we're too late to make it optional in 2.7, but there is nothing > preventing us from making it optional in the future, and it's neither > too much burden on libvirt to supply a sane default if it is mandatory, > nor too hard for the command line and HMP to make it optional on top of > a mandatory QMP, for the sake of human interaction. Agreed then. >> Port ranges make sense for some users of InetSocketAddress, but not for >> others. Gluster is of the latter kind. >> >> So far, the latter kind simply uses InetSocketAddress, and either >> rejects @to or ignores it. The latter would be a bug. >> >> We could do the same for Gluster: if has_to, error out. >> >> Alternatively, we could split InetSocketAddressRange (with @to) off >> InetSocketAddress (delete @to). Makes range support visible in >> introspection. > > Ultimately, I think we should split InetSocketAddress. But we're too > late to do it in 2.7. Makes sense. >> If we use InetSocketAddress as is now, and have gluster.c reject @to, we >> can still split off InetSocketAddressRange later. The external >> interface doesn't care whether @to is rejected as unknown parameter by >> QAPI or as unsupported parameter by gluster.c. >> > > Agree - at this point, manually rejecting has_to will let us reuse the > type, and would then be part of the cleanup work when 2.8 splits > InetSocketAddress. > >> Now let me summarize. We could do without GlusterInetSocketAddress, >> because: >> >> * SocketAddress's non-optional @port is a minor inconvenience which >> we can address in the future without breaking compatibility. >> >> * SocketAddress's string @port is a minor inconvenience for the C code >> using it. Keeping the external interface consistent (same type for >> TCP ports everywhere) is worth some inconvenience in the C code. >> >> * SocketAddress's @to complicates the external interface, but the >> complication exists elsewhere already. gluster.c can reject @to. We >> can clean up the external interface in the future without breaking >> compatibility. > > In fact, introspection will be nicer when we split InetSocketAddress to > no longer include 'to', as you will then be able to precisely determine > which uses support ranges. For now, management apps just have to be > aware that ranges may be rejected. > >> >> * SocketAddress's @ipv4, @ipv6 cannot be fully implemented by gluster.c, >> yet. gluster.c can simply reject the settings it doesn't implement, >> until they get implemented. >> >> Reasons for having a separate GlusterInetSocketAddress: >> >> * Slightly less code in gluster.c. I reject that reason. Keeping the >> interface lean is worth some extra code. >> >> Note that extra schema definitions to avoid code complications may be >> fine as long as they don't affect external interfaces. >> >> * Makes non-support of port ranges and IPv6 visible in introspection. >> That's a valid argument, but it's an argument for having >> Inet4SockAddress, not for GlusterInetSocketAddress. >> >> Eric, do you think there's a need for introspecting IPv6 support? > > I'm borderline on that one. If the error message given when attempting > an IPv6 address is nice enough, then libvirt can just blindly try an > IPv6 address and inform the user that qemu/gluster is too old to support > it. If the error message is lousy, then being able to introspect > whether IPv6 support is present would let libvirt provide a saner error > message. But at the moment, I'm not convinced that introspection alone > is reason to create a struct without IPv6 now, where IPv6 is added later > once gluster supports it. > > Furthermore, a system administrator that NEEDS to use IPv6 is very > likely to upgrade their system to support things before trying to use > IPv6. So in this case, I'm leaning towards just reusing the type and > rejecting IPv6 in the gluster code until it works. Okay, let's reuse InetSocketAddress, and reject the following in C code: * has_to * has_ipv4 || has_ipv6 This is overkill. We only have to reject the combinations that normally result in v6, but don't with Gluster. inet_ai_family_from_address()'s function comment documents how we pick the address family. I'm proposing overkill since this is at v19, and we need to make progress. >> Similar design question as for @ipv4, @ipv6: reuse of SocketAddress is >> possible, but the C code has to reject options it doesn't implement, >> i.e. "type": "fd". Non-support of fd isn't visible in introspection >> then. >> >> Eric, do you think there's a need for introspecting fd support? > > There's no easy way to declare one QAPI union that is a superset of > another (all the same branches as before, plus these additional > branches); it requires duplication of all the common branches. Yes. > Arguably, we could extend QAPI if we find ourselves doing that a lot, to > make it supported with less copy and paste; but manual duplication right > now doesn't hurt. Agreed. > Here, I think introspection is probably more useful, particularly since > there are security aspects in fd passing (where libvirt can pre-open the > connection) that are nicer to know up front if they will work, and where > it is less obvious to a system administrator that they need/want to use > fd passing under the hood. IPv6 is very easy to say: "will my > deployment need IPv6 addresses - if so, upgrade gluster to a version > that supports IPv6". But fd passing is not so straightforward. So I > think being able to introspect this one is useful - but I also think > that having a union that shares the same underlying types for the common > branches is still slightly nicer than having different types for the > same-named branches (that is, since both the reduced union without 'fd' > support and the full union will have 'unix' and 'inet' branches, both > the 'unix' and 'inet' branch should use the same underlying type rather > than being subtly different). Makes sense. >> Additional design question: do we want to move away from SocketAddress's >> pointless nesting on the wire for new interfaces? I.e. move from >> something like >> >> { "type": "tcp", "data": { "host": "gluster.example.com", ... } >> >> to >> >> { "type": "tcp", "host": "gluster.example.com", ... } >> >> This isn't a Gluster question, it's a general question. Eric, what do >> you think? >> > > Nested vs. flat is somewhat cosmetic - machine-generated code (like > libvirt's interactions) can cope with either. On the other hand, adding > a flat type now may make OTHER blockdev-add drivers easier to QAPIfy, > such as nbd and sheepdog, and I kind of like the simplicity afforded by > a flat type. The longer I do QAPI, the more I dislike "simple" unions. >>> { 'union': 'SocketAddress', >>> 'data': { >>> 'inet': 'InetSocketAddress', >>> 'unix': 'UnixSocketAddress', >>> 'fd': 'String' } } > > I also think that if we add a flat type, it would be nicer to use > 'fd':'str' instead of 'fd':'String' (which is itself another pointless > nesting due to backwards compatibility), at least for the version of the > flat type where 'fd' is supported. Oh yes. String's documentation justifies its existence with "to be embedded in lists." Well, strList exists. >>> after removing 'fd' part which is not supported now, this look like >>> >>> { 'union': 'GlusterSocketAddress', >>> 'data': { >>> 'inet': 'GlusterInetSocketAddress', >>> 'unix': 'UnixSocketAddress' } } >>> >>> What do you think ? >> >> This is fine if >> >> * we decide we want a new GlusterInetSocketAddress instead of reusing >> InetSocketAddress, and >> >> * we decide we don't want to avoid nesting on the wire. >> >> But we haven't settled these questions. I'd like to settle them today, >> taking Eric's advice into account. > > I think sharing InetSocketAddress is reasonable, but having separate > GlusterSocketAddress (or more generically, FlatSocketAddress that can be > shared by gluster, sheepdog, and nbd), seems okay. I'm also not fussed > about naming - naming it 'GlusterSocketAddress' now and renaming it > 'FlatSocketAddress' later when it gets more use does not impact > introspection. Renaming won't be a big deal because it should affect mostly just gluster.c. So let's start with { 'enum': 'GlusterTransport', 'data': [ 'unix', 'tcp' ] } { 'union': 'GlusterServer', 'base': { 'type': 'GlusterTransport' }, 'discriminator': 'type', 'data': { 'unix': 'UnixSocketAddress', 'tcp': 'InetSocketAddress' } } I.e. the current patch with GlusterUnixSocketAddress and GlusterSocketAddress dropped. I'll discuss the next steps with Prasanna on IRC right away. This GlusterServer type can be compatibly evolved into a perfectly generic replacement for the current SocketAddress type to be used in new code. The current SocketAddress is only used by commands nbd-server-start and chardev-add.
diff --git a/block/gluster.c b/block/gluster.c index 59f77bb..ff1e783 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -15,6 +15,7 @@ #define GLUSTER_OPT_FILENAME "filename" #define GLUSTER_OPT_DEBUG "debug" +#define GLUSTER_DEFAULT_PORT 24007 #define GLUSTER_DEBUG_DEFAULT 4 #define GLUSTER_DEBUG_MAX 9 @@ -39,15 +40,6 @@ typedef struct BDRVGlusterReopenState { struct glfs_fd *fd; } BDRVGlusterReopenState; -typedef struct GlusterConf { - char *host; - int port; - char *volume; - char *path; - char *transport; - int debug_level; -} GlusterConf; - static QemuOptsList qemu_gluster_create_opts = { .name = "qemu-gluster-create-opts", @@ -91,18 +83,7 @@ static QemuOptsList runtime_opts = { }; -static void qemu_gluster_gconf_free(GlusterConf *gconf) -{ - if (gconf) { - g_free(gconf->host); - g_free(gconf->volume); - g_free(gconf->path); - g_free(gconf->transport); - g_free(gconf); - } -} - -static int parse_volume_options(GlusterConf *gconf, char *path) +static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) { char *p, *q; @@ -162,8 +143,10 @@ static int parse_volume_options(GlusterConf *gconf, char *path) * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket */ -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) +static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf, + const char *filename) { + GlusterServer *gsconf; URI *uri; QueryParams *qp = NULL; bool is_unix = false; @@ -174,13 +157,15 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) return -EINVAL; } + gconf->server = gsconf = g_new0(GlusterServer, 1); + /* transport */ if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { - gconf->transport = g_strdup("tcp"); + gsconf->type = GLUSTER_TRANSPORT_TCP; } else if (!strcmp(uri->scheme, "gluster+tcp")) { - gconf->transport = g_strdup("tcp"); + gsconf->type = GLUSTER_TRANSPORT_TCP; } else if (!strcmp(uri->scheme, "gluster+unix")) { - gconf->transport = g_strdup("unix"); + gsconf->type = GLUSTER_TRANSPORT_UNIX; is_unix = true; } else { ret = -EINVAL; @@ -207,10 +192,15 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename) ret = -EINVAL; goto out; } - gconf->host = g_strdup(qp->p[0].value); + gsconf->u.q_unix.socket = g_strdup(qp->p[0].value); } else { - gconf->host = g_strdup(uri->server ? uri->server : "localhost"); - gconf->port = uri->port; + gsconf->u.tcp.host = g_strdup(uri->server ? uri->server : "localhost"); + if (uri->port) { + gsconf->u.tcp.port = uri->port; + } else { + gsconf->u.tcp.port = GLUSTER_DEFAULT_PORT; + } + gsconf->u.tcp.has_port = true; } out: @@ -221,14 +211,14 @@ out: return ret; } -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename, - Error **errp) +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, + const char *filename, Error **errp) { struct glfs *glfs = NULL; int ret; int old_errno; - ret = qemu_gluster_parseuri(gconf, filename); + ret = qemu_gluster_parse_uri(gconf, filename); if (ret < 0) { error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" "volume/path[?socket=...]"); @@ -241,8 +231,16 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename, goto out; } - ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->host, - gconf->port); + if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) { + ret = glfs_set_volfile_server(glfs, + GlusterTransport_lookup[gconf->server->type], + gconf->server->u.q_unix.socket, 0); + } else { + ret = glfs_set_volfile_server(glfs, + GlusterTransport_lookup[gconf->server->type], + gconf->server->u.tcp.host, + gconf->server->u.tcp.port); + } if (ret < 0) { goto out; } @@ -254,15 +252,24 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename, ret = glfs_init(glfs); if (ret) { - error_setg_errno(errp, errno, - "Gluster connection failed for host=%s port=%d " - "volume=%s path=%s transport=%s", gconf->host, - gconf->port, gconf->volume, gconf->path, - gconf->transport); + if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) { + error_setg(errp, + "Gluster connection for volume: %s, path: %s " + "failed to connect on socket %s ", + gconf->volume, gconf->path, + gconf->server->u.q_unix.socket); + } else { + error_setg(errp, + "Gluster connection for volume: %s, path: %s " + "failed to connect host %s and port %d ", + gconf->volume, gconf->path, + gconf->server->u.tcp.host, gconf->server->u.tcp.port); + } /* glfs_init sometimes doesn't set errno although docs suggest that */ - if (errno == 0) + if (errno == 0) { errno = EINVAL; + } goto out; } @@ -350,7 +357,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, BDRVGlusterState *s = bs->opaque; int open_flags = 0; int ret = 0; - GlusterConf *gconf = g_new0(GlusterConf, 1); + BlockdevOptionsGluster *gconf = NULL; QemuOpts *opts; Error *local_err = NULL; const char *filename; @@ -373,7 +380,9 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, s->debug_level = GLUSTER_DEBUG_MAX; } + gconf = g_new0(BlockdevOptionsGluster, 1); gconf->debug_level = s->debug_level; + gconf->has_debug_level = true; s->glfs = qemu_gluster_init(gconf, filename, errp); if (!s->glfs) { ret = -errno; @@ -408,7 +417,9 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, out: qemu_opts_del(opts); - qemu_gluster_gconf_free(gconf); + if (gconf) { + qapi_free_BlockdevOptionsGluster(gconf); + } if (!ret) { return ret; } @@ -427,7 +438,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, int ret = 0; BDRVGlusterState *s; BDRVGlusterReopenState *reop_s; - GlusterConf *gconf = NULL; + BlockdevOptionsGluster *gconf; int open_flags = 0; assert(state != NULL); @@ -440,9 +451,9 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, qemu_gluster_parse_flags(state->flags, &open_flags); - gconf = g_new0(GlusterConf, 1); - + gconf = g_new0(BlockdevOptionsGluster, 1); gconf->debug_level = s->debug_level; + gconf->has_debug_level = true; reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp); if (reop_s->glfs == NULL) { ret = -errno; @@ -468,7 +479,9 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, exit: /* state->opaque will be freed in either the _abort or _commit */ - qemu_gluster_gconf_free(gconf); + if (gconf) { + qapi_free_BlockdevOptionsGluster(gconf); + } return ret; } @@ -570,14 +583,15 @@ static inline int qemu_gluster_zerofill(struct glfs_fd *fd, int64_t offset, static int qemu_gluster_create(const char *filename, QemuOpts *opts, Error **errp) { + BlockdevOptionsGluster *gconf; struct glfs *glfs; struct glfs_fd *fd; int ret = 0; int prealloc = 0; int64_t total_size = 0; char *tmp = NULL; - GlusterConf *gconf = g_new0(GlusterConf, 1); + gconf = g_new0(BlockdevOptionsGluster, 1); gconf->debug_level = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG, GLUSTER_DEBUG_DEFAULT); if (gconf->debug_level < 0) { @@ -585,6 +599,7 @@ static int qemu_gluster_create(const char *filename, } else if (gconf->debug_level > GLUSTER_DEBUG_MAX) { gconf->debug_level = GLUSTER_DEBUG_MAX; } + gconf->has_debug_level = true; glfs = qemu_gluster_init(gconf, filename, errp); if (!glfs) { @@ -626,7 +641,9 @@ static int qemu_gluster_create(const char *filename, } out: g_free(tmp); - qemu_gluster_gconf_free(gconf); + if (gconf) { + qapi_free_BlockdevOptionsGluster(gconf); + } if (glfs) { glfs_fini(glfs); } diff --git a/qapi/block-core.json b/qapi/block-core.json index a7fdb28..d7b5c76 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1658,13 +1658,14 @@ # @host_device, @host_cdrom: Since 2.1 # # Since: 2.0 +# @gluster: Since 2.7 ## { 'enum': 'BlockdevDriver', 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', - 'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels', - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', - 'vmdk', 'vpc', 'vvfat' ] } + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', + 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } ## # @BlockdevOptionsFile @@ -2057,6 +2058,89 @@ '*read-pattern': 'QuorumReadPattern' } } ## +# @GlusterTransport +# +# An enumeration of Gluster transport type +# +# @tcp: TCP - Transmission Control Protocol +# +# @unix: UNIX - Unix domain socket +# +# Since: 2.7 +## +{ 'enum': 'GlusterTransport', + 'data': [ 'unix', 'tcp' ] } + +## +# @GlusterUnixSocketAddress +# +# Captures a socket address in the local ("Unix socket") namespace. +# +# @socket: absolute path to socket file +# +# Since 2.7 +## +{ 'struct': 'GlusterUnixSocketAddress', + 'data': { 'socket': 'str' } } + +## +# @GlusterInetSocketAddress +# +# Captures a socket address or address range in the Internet namespace. +# +# @host: host part of the address +# +# @port: #optional port part of the address, or lowest port if @to is present +# +# Since 2.7 +## +{ 'struct': 'GlusterInetSocketAddress', + 'data': { 'host': 'str', + '*port': 'uint16' } } + +## +# @GlusterServer +# +# Captures the address of a socket +# +# Details for connecting to a gluster server +# +# @type: Transport type used for gluster connection +# +# @unix: socket file +# +# @tcp: host address and port number +# +# Since: 2.7 +## +{ 'union': 'GlusterServer', + 'base': { 'type': 'GlusterTransport' }, + 'discriminator': 'type', + 'data': { 'unix': 'GlusterUnixSocketAddress', + 'tcp': 'GlusterInetSocketAddress' } } + +## +# @BlockdevOptionsGluster +# +# Driver specific block device options for Gluster +# +# @volume: name of gluster volume where VM image resides +# +# @path: absolute path to image file in gluster volume +# +# @server: gluster server description +# +# @debug_level: #optional libgfapi log level (default '4' which is Error) +# +# Since: 2.7 +## +{ 'struct': 'BlockdevOptionsGluster', + 'data': { 'volume': 'str', + 'path': 'str', + 'server': 'GlusterServer', + '*debug_level': 'int' } } + +## # @BlockdevOptions # # Options for creating a block device. Many options are available for all @@ -2119,7 +2203,7 @@ 'file': 'BlockdevOptionsFile', 'ftp': 'BlockdevOptionsFile', 'ftps': 'BlockdevOptionsFile', -# TODO gluster: Wait for structured options + 'gluster': 'BlockdevOptionsGluster', 'host_cdrom': 'BlockdevOptionsFile', 'host_device':'BlockdevOptionsFile', 'http': 'BlockdevOptionsFile',
this patch adds 'GlusterServer' related schema in qapi/block-core.json Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- block/gluster.c | 111 +++++++++++++++++++++++++++++---------------------- qapi/block-core.json | 94 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 153 insertions(+), 52 deletions(-)