| Message ID | 1477337274-7939-3-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 24.10.2016 um 21:27 hat Ashijeet Acharya geschrieben: > Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to > support blockdev-add for NFS network protocol driver. Also make a new > struct NFSServer to support tcp connection. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 52 insertions(+), 4 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 9d797b8..3ab028d 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1714,9 +1714,9 @@ > { 'enum': 'BlockdevDriver', > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', > + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > ## > # @BlockdevOptionsFile > @@ -2212,6 +2212,54 @@ > '*top-id': 'str' } } > > ## > +# @NFSServer > +# > +# Captures the address of the socket > +# > +# @type: transport type used for NFS (only TCP supported) > +# > +# @host: host part of the address > +# > +# Since 2.8 > +## > +{ 'struct': 'NFSServer', > + 'data': { 'type': 'str', > + 'host': 'str' } } > + > +## > +# @BlockdevOptionsNfs > +# > +# Driver specific block device option for NFS > +# > +# @server: host address > +# > +# @path: path of the image on the host > +# > +# @uid: #optional UID value to use when talking to the server > +# > +# @gid: #optional GID value to use when talking to the server > +# > +# @tcp-syncnt: #optional number of SYNs during the session establishment > +# > +# @readahead: #optional set the readahead size in bytes > +# > +# @pagecache: #optional set the pagecache size in bytes > +# > +# @debug: #optional set the NFS debug level (max 2) > +# > +# Since 2.8 > +## > +{ 'struct': 'BlockdevOptionsNfs', > + 'data': { 'server': 'NFSServer', Patch 1 doesn't handle "server.type" and "server.host", so does this actually work? > + 'path': 'str', > + '*uid': 'int', > + '*gid': 'int', > + '*tcp-syncnt': 'int', > + '*readahead': 'int', > + '*pagecache': 'int', > + '*debug': 'int' } } > + > +## > # @BlockdevOptionsCurl > # > # Driver specific block device options for the curl backend. Kevin
On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: > Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to > support blockdev-add for NFS network protocol driver. Also make a new > struct NFSServer to support tcp connection. > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > --- > qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 52 insertions(+), 4 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 9d797b8..3ab028d 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1714,9 +1714,9 @@ > { 'enum': 'BlockdevDriver', > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', > + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } Missing a comment that 'nfs' is since 2.8. > ## > +# @NFSServer > +# > +# Captures the address of the socket > +# > +# @type: transport type used for NFS (only TCP supported) > +# > +# @host: host part of the address > +# > +# Since 2.8 > +## > +{ 'struct': 'NFSServer', > + 'data': { 'type': 'str', Please make this an enum, instead of an open-coded string. It's okay if the enum only has one value 'tcp' for now; but using an enum will make it introspectable if we later add a second transport, unlike what we get with an open-coded string. Must 'type' be mandatory if it must always be 'tcp'? > + 'host': 'str' } } > + > +## > +# @BlockdevOptionsNfs > +# > +# Driver specific block device option for NFS > +# > +# @server: host address > +# > +# @path: path of the image on the host > +# > +# @uid: #optional UID value to use when talking to the server > +# > +# @gid: #optional GID value to use when talking to the server Do we want to allow string names in addition to numeric uid/gid values? I'm not sure if NFS has name-based id mapping, but it's food for thought on whether we need to use an alternate type here (alternate between integer id and string name), or leave this as is. > +# > +# @tcp-syncnt: #optional number of SYNs during the session establishment Would tcp-syn-count be any more legible? What is the default when omitted? > +# > +# @readahead: #optional set the readahead size in bytes What's the default when omitted? > +# > +# @pagecache: #optional set the pagecache size in bytes Default? > +# > +# @debug: #optional set the NFS debug level (max 2) Presumably default 0? > +# > +# Since 2.8 > +## > +{ 'struct': 'BlockdevOptionsNfs', > + 'data': { 'server': 'NFSServer', > + 'path': 'str', > + '*uid': 'int', > + '*gid': 'int', > + '*tcp-syncnt': 'int', > + '*readahead': 'int', > + '*pagecache': 'int', > + '*debug': 'int' } } > +
A few drive-by comments... Eric Blake <eblake@redhat.com> writes: > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to >> support blockdev-add for NFS network protocol driver. Also make a new >> struct NFSServer to support tcp connection. >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> --- >> qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 52 insertions(+), 4 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 9d797b8..3ab028d 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1714,9 +1714,9 @@ >> { 'enum': 'BlockdevDriver', >> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >> 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >> - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >> - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', >> - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', >> + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', >> + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > Missing a comment that 'nfs' is since 2.8. > >> ## >> +# @NFSServer >> +# >> +# Captures the address of the socket >> +# >> +# @type: transport type used for NFS (only TCP supported) >> +# >> +# @host: host part of the address >> +# >> +# Since 2.8 >> +## >> +{ 'struct': 'NFSServer', >> + 'data': { 'type': 'str', > > Please make this an enum, instead of an open-coded string. It's okay if > the enum only has one value 'tcp' for now; but using an enum will make > it introspectable if we later add a second transport, unlike what we get > with an open-coded string. Yes. When a JSON string has a compile-time fixed set of values, 'str' is generally wrong. > Must 'type' be mandatory if it must always be 'tcp'? > >> + 'host': 'str' } } >> + >> +## >> +# @BlockdevOptionsNfs >> +# >> +# Driver specific block device option for NFS >> +# >> +# @server: host address >> +# >> +# @path: path of the image on the host >> +# >> +# @uid: #optional UID value to use when talking to the server >> +# >> +# @gid: #optional GID value to use when talking to the server > > Do we want to allow string names in addition to numeric uid/gid values? > I'm not sure if NFS has name-based id mapping, but it's food for thought > on whether we need to use an alternate type here (alternate between > integer id and string name), or leave this as is. As far as I know, NFS4 supports user and group names. On Linux, see rpc.idmapd(8). How the name support affects C code I can't say. If it's transparent, i.e. you simply use local UID/GID, and the mapping happens below the hood, then we'd have to translate string values to local UID/GID by the usual means. Looks like a minor convenience feature on first glance. However, QMP is a *network* protocol. A remote client can't easily do this translation. Consider a GUI like virt-manager: I guess we'd rather support user and group names there. But if we do, and QMP doesn't, either virt-manager or libvirt need to map to numeric IDs. Easy enough when running on the host, probably impractical when not. If we permit string values, are @uid and @gid still appropriate names? The user name is not the user ID, it just maps to it. >> +# >> +# @tcp-syncnt: #optional number of SYNs during the session establishment > > Would tcp-syn-count be any more legible? We generally write out things in long hand in the QAPI schema. > What is the default when omitted? Whenever you write #optional, you must explain the default. When the default is a fixed value, specify it. When the system picks a default, state that, and think hard about what you need to specify on how the system picks. >> +# >> +# @readahead: #optional set the readahead size in bytes @read-ahead > What's the default when omitted? > >> +# >> +# @pagecache: #optional set the pagecache size in bytes @page-cache > Default? > >> +# >> +# @debug: #optional set the NFS debug level (max 2) > > Presumably default 0? @BlockdevOptionsGluster calls this @debug-level. >> +# >> +# Since 2.8 >> +## >> +{ 'struct': 'BlockdevOptionsNfs', >> + 'data': { 'server': 'NFSServer', >> + 'path': 'str', >> + '*uid': 'int', >> + '*gid': 'int', >> + '*tcp-syncnt': 'int', >> + '*readahead': 'int', >> + '*pagecache': 'int', >> + '*debug': 'int' } } >> +
Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben: > A few drive-by comments... > > Eric Blake <eblake@redhat.com> writes: > > > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: > >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to > >> support blockdev-add for NFS network protocol driver. Also make a new > >> struct NFSServer to support tcp connection. > >> > >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > >> --- > >> qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- > >> 1 file changed, 52 insertions(+), 4 deletions(-) > >> > >> diff --git a/qapi/block-core.json b/qapi/block-core.json > >> index 9d797b8..3ab028d 100644 > >> --- a/qapi/block-core.json > >> +++ b/qapi/block-core.json > >> @@ -1714,9 +1714,9 @@ > >> { 'enum': 'BlockdevDriver', > >> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > >> 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > >> - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > >> - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > >> - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > >> + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', > >> + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > >> + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > > > Missing a comment that 'nfs' is since 2.8. > > > >> ## > >> +# @NFSServer > >> +# > >> +# Captures the address of the socket > >> +# > >> +# @type: transport type used for NFS (only TCP supported) > >> +# > >> +# @host: host part of the address > >> +# > >> +# Since 2.8 > >> +## > >> +{ 'struct': 'NFSServer', > >> + 'data': { 'type': 'str', > > > > Please make this an enum, instead of an open-coded string. It's okay if > > the enum only has one value 'tcp' for now; but using an enum will make > > it introspectable if we later add a second transport, unlike what we get > > with an open-coded string. > > Yes. When a JSON string has a compile-time fixed set of values, 'str' > is generally wrong. > > > Must 'type' be mandatory if it must always be 'tcp'? > > > >> + 'host': 'str' } } > >> + > >> +## > >> +# @BlockdevOptionsNfs > >> +# > >> +# Driver specific block device option for NFS > >> +# > >> +# @server: host address > >> +# > >> +# @path: path of the image on the host > >> +# > >> +# @uid: #optional UID value to use when talking to the server > >> +# > >> +# @gid: #optional GID value to use when talking to the server > > > > Do we want to allow string names in addition to numeric uid/gid values? > > I'm not sure if NFS has name-based id mapping, but it's food for thought > > on whether we need to use an alternate type here (alternate between > > integer id and string name), or leave this as is. > > As far as I know, NFS4 supports user and group names. On Linux, see > rpc.idmapd(8). > > How the name support affects C code I can't say. If it's transparent, > i.e. you simply use local UID/GID, and the mapping happens below the > hood, then we'd have to translate string values to local UID/GID by the > usual means. Looks like a minor convenience feature on first glance. > However, QMP is a *network* protocol. A remote client can't easily do > this translation. > > Consider a GUI like virt-manager: I guess we'd rather support user and > group names there. But if we do, and QMP doesn't, either virt-manager > or libvirt need to map to numeric IDs. Easy enough when running on the > host, probably impractical when not. > > If we permit string values, are @uid and @gid still appropriate names? > The user name is not the user ID, it just maps to it. > > >> +# > >> +# @tcp-syncnt: #optional number of SYNs during the session establishment > > > > Would tcp-syn-count be any more legible? > > We generally write out things in long hand in the QAPI schema. > > > What is the default when omitted? > > Whenever you write #optional, you must explain the default. When > the default is a fixed value, specify it. When the system picks a > default, state that, and think hard about what you need to specify on > how the system picks. > > >> +# > >> +# @readahead: #optional set the readahead size in bytes > > @read-ahead > > > What's the default when omitted? > > > >> +# > >> +# @pagecache: #optional set the pagecache size in bytes > > @page-cache > > > Default? > > > >> +# > >> +# @debug: #optional set the NFS debug level (max 2) > > > > Presumably default 0? > > @BlockdevOptionsGluster calls this @debug-level. Are all of these renames really worth having to support two option names for each option in the driver? I'm not sure if I'm convinced of this. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben: >> A few drive-by comments... >> >> Eric Blake <eblake@redhat.com> writes: >> >> > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: >> >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to >> >> support blockdev-add for NFS network protocol driver. Also make a new >> >> struct NFSServer to support tcp connection. >> >> >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> >> --- >> >> qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- >> >> 1 file changed, 52 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> >> index 9d797b8..3ab028d 100644 >> >> --- a/qapi/block-core.json >> >> +++ b/qapi/block-core.json >> >> @@ -1714,9 +1714,9 @@ >> >> { 'enum': 'BlockdevDriver', >> >> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >> >> 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >> >> - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >> >> - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', >> >> - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> >> + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', >> >> + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', >> >> + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> > >> > Missing a comment that 'nfs' is since 2.8. >> > >> >> ## >> >> +# @NFSServer >> >> +# >> >> +# Captures the address of the socket >> >> +# >> >> +# @type: transport type used for NFS (only TCP supported) >> >> +# >> >> +# @host: host part of the address >> >> +# >> >> +# Since 2.8 >> >> +## >> >> +{ 'struct': 'NFSServer', >> >> + 'data': { 'type': 'str', >> > >> > Please make this an enum, instead of an open-coded string. It's okay if >> > the enum only has one value 'tcp' for now; but using an enum will make >> > it introspectable if we later add a second transport, unlike what we get >> > with an open-coded string. >> >> Yes. When a JSON string has a compile-time fixed set of values, 'str' >> is generally wrong. >> >> > Must 'type' be mandatory if it must always be 'tcp'? >> > >> >> + 'host': 'str' } } >> >> + >> >> +## >> >> +# @BlockdevOptionsNfs >> >> +# >> >> +# Driver specific block device option for NFS >> >> +# >> >> +# @server: host address >> >> +# >> >> +# @path: path of the image on the host >> >> +# >> >> +# @uid: #optional UID value to use when talking to the server >> >> +# >> >> +# @gid: #optional GID value to use when talking to the server >> > >> > Do we want to allow string names in addition to numeric uid/gid values? >> > I'm not sure if NFS has name-based id mapping, but it's food for thought >> > on whether we need to use an alternate type here (alternate between >> > integer id and string name), or leave this as is. >> >> As far as I know, NFS4 supports user and group names. On Linux, see >> rpc.idmapd(8). >> >> How the name support affects C code I can't say. If it's transparent, >> i.e. you simply use local UID/GID, and the mapping happens below the >> hood, then we'd have to translate string values to local UID/GID by the >> usual means. Looks like a minor convenience feature on first glance. >> However, QMP is a *network* protocol. A remote client can't easily do >> this translation. >> >> Consider a GUI like virt-manager: I guess we'd rather support user and >> group names there. But if we do, and QMP doesn't, either virt-manager >> or libvirt need to map to numeric IDs. Easy enough when running on the >> host, probably impractical when not. >> >> If we permit string values, are @uid and @gid still appropriate names? >> The user name is not the user ID, it just maps to it. >> >> >> +# >> >> +# @tcp-syncnt: #optional number of SYNs during the session establishment >> > >> > Would tcp-syn-count be any more legible? >> >> We generally write out things in long hand in the QAPI schema. >> >> > What is the default when omitted? >> >> Whenever you write #optional, you must explain the default. When >> the default is a fixed value, specify it. When the system picks a >> default, state that, and think hard about what you need to specify on >> how the system picks. >> >> >> +# >> >> +# @readahead: #optional set the readahead size in bytes >> >> @read-ahead >> >> > What's the default when omitted? >> > >> >> +# >> >> +# @pagecache: #optional set the pagecache size in bytes >> >> @page-cache >> >> > Default? >> > >> >> +# >> >> +# @debug: #optional set the NFS debug level (max 2) >> > >> > Presumably default 0? >> >> @BlockdevOptionsGluster calls this @debug-level. > > Are all of these renames really worth having to support two option > names for each option in the driver? I'm not sure if I'm convinced of > this. I don't see "two option names for each option" in this patch, so please explain.
Am 26.10.2016 um 10:40 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben: > >> A few drive-by comments... > >> > >> Eric Blake <eblake@redhat.com> writes: > >> > >> > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: > >> >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to > >> >> support blockdev-add for NFS network protocol driver. Also make a new > >> >> struct NFSServer to support tcp connection. > >> >> > >> >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > >> >> --- > >> >> qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- > >> >> 1 file changed, 52 insertions(+), 4 deletions(-) > >> >> > >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json > >> >> index 9d797b8..3ab028d 100644 > >> >> --- a/qapi/block-core.json > >> >> +++ b/qapi/block-core.json > >> >> @@ -1714,9 +1714,9 @@ > >> >> { 'enum': 'BlockdevDriver', > >> >> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > >> >> 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > >> >> - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > >> >> - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > >> >> - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > >> >> + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', > >> >> + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > >> >> + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > >> > > >> > Missing a comment that 'nfs' is since 2.8. > >> > > >> >> ## > >> >> +# @NFSServer > >> >> +# > >> >> +# Captures the address of the socket > >> >> +# > >> >> +# @type: transport type used for NFS (only TCP supported) > >> >> +# > >> >> +# @host: host part of the address > >> >> +# > >> >> +# Since 2.8 > >> >> +## > >> >> +{ 'struct': 'NFSServer', > >> >> + 'data': { 'type': 'str', > >> > > >> > Please make this an enum, instead of an open-coded string. It's okay if > >> > the enum only has one value 'tcp' for now; but using an enum will make > >> > it introspectable if we later add a second transport, unlike what we get > >> > with an open-coded string. > >> > >> Yes. When a JSON string has a compile-time fixed set of values, 'str' > >> is generally wrong. > >> > >> > Must 'type' be mandatory if it must always be 'tcp'? > >> > > >> >> + 'host': 'str' } } > >> >> + > >> >> +## > >> >> +# @BlockdevOptionsNfs > >> >> +# > >> >> +# Driver specific block device option for NFS > >> >> +# > >> >> +# @server: host address > >> >> +# > >> >> +# @path: path of the image on the host > >> >> +# > >> >> +# @uid: #optional UID value to use when talking to the server > >> >> +# > >> >> +# @gid: #optional GID value to use when talking to the server > >> > > >> > Do we want to allow string names in addition to numeric uid/gid values? > >> > I'm not sure if NFS has name-based id mapping, but it's food for thought > >> > on whether we need to use an alternate type here (alternate between > >> > integer id and string name), or leave this as is. > >> > >> As far as I know, NFS4 supports user and group names. On Linux, see > >> rpc.idmapd(8). > >> > >> How the name support affects C code I can't say. If it's transparent, > >> i.e. you simply use local UID/GID, and the mapping happens below the > >> hood, then we'd have to translate string values to local UID/GID by the > >> usual means. Looks like a minor convenience feature on first glance. > >> However, QMP is a *network* protocol. A remote client can't easily do > >> this translation. > >> > >> Consider a GUI like virt-manager: I guess we'd rather support user and > >> group names there. But if we do, and QMP doesn't, either virt-manager > >> or libvirt need to map to numeric IDs. Easy enough when running on the > >> host, probably impractical when not. > >> > >> If we permit string values, are @uid and @gid still appropriate names? > >> The user name is not the user ID, it just maps to it. > >> > >> >> +# > >> >> +# @tcp-syncnt: #optional number of SYNs during the session establishment > >> > > >> > Would tcp-syn-count be any more legible? > >> > >> We generally write out things in long hand in the QAPI schema. > >> > >> > What is the default when omitted? > >> > >> Whenever you write #optional, you must explain the default. When > >> the default is a fixed value, specify it. When the system picks a > >> default, state that, and think hard about what you need to specify on > >> how the system picks. > >> > >> >> +# > >> >> +# @readahead: #optional set the readahead size in bytes > >> > >> @read-ahead > >> > >> > What's the default when omitted? > >> > > >> >> +# > >> >> +# @pagecache: #optional set the pagecache size in bytes > >> > >> @page-cache > >> > >> > Default? > >> > > >> >> +# > >> >> +# @debug: #optional set the NFS debug level (max 2) > >> > > >> > Presumably default 0? > >> > >> @BlockdevOptionsGluster calls this @debug-level. > > > > Are all of these renames really worth having to support two option > > names for each option in the driver? I'm not sure if I'm convinced of > > this. > > I don't see "two option names for each option" in this patch, so please > explain. You're right. I was assuming that this patch just exposes existing command line options as they are. But before patch 1 of this series, the options aren't actual command line options, but just the query parameter names in URIs. As we have to put these into the QDict explicitly anyway in nfs_parse_uri(), we can still make a choice on the name of the options there. So I support renaming things before we expose them. I'll reply again to your original proposal for the exact naming. Kevin
Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben: > A few drive-by comments... > > Eric Blake <eblake@redhat.com> writes: > > > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: > >> Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to > >> support blockdev-add for NFS network protocol driver. Also make a new > >> struct NFSServer to support tcp connection. > >> > >> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > >> --- > >> qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- > >> 1 file changed, 52 insertions(+), 4 deletions(-) > >> > >> diff --git a/qapi/block-core.json b/qapi/block-core.json > >> index 9d797b8..3ab028d 100644 > >> --- a/qapi/block-core.json > >> +++ b/qapi/block-core.json > >> @@ -1714,9 +1714,9 @@ > >> { 'enum': 'BlockdevDriver', > >> 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > >> 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > >> - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > >> - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > >> - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > >> + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', > >> + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > >> + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > > > Missing a comment that 'nfs' is since 2.8. > > > >> ## > >> +# @NFSServer > >> +# > >> +# Captures the address of the socket > >> +# > >> +# @type: transport type used for NFS (only TCP supported) > >> +# > >> +# @host: host part of the address > >> +# > >> +# Since 2.8 > >> +## > >> +{ 'struct': 'NFSServer', > >> + 'data': { 'type': 'str', > > > > Please make this an enum, instead of an open-coded string. It's okay if > > the enum only has one value 'tcp' for now; but using an enum will make > > it introspectable if we later add a second transport, unlike what we get > > with an open-coded string. > > Yes. When a JSON string has a compile-time fixed set of values, 'str' > is generally wrong. > > > Must 'type' be mandatory if it must always be 'tcp'? > > > >> + 'host': 'str' } } > >> + > >> +## > >> +# @BlockdevOptionsNfs > >> +# > >> +# Driver specific block device option for NFS > >> +# > >> +# @server: host address > >> +# > >> +# @path: path of the image on the host > >> +# > >> +# @uid: #optional UID value to use when talking to the server > >> +# > >> +# @gid: #optional GID value to use when talking to the server > > > > Do we want to allow string names in addition to numeric uid/gid values? > > I'm not sure if NFS has name-based id mapping, but it's food for thought > > on whether we need to use an alternate type here (alternate between > > integer id and string name), or leave this as is. > > As far as I know, NFS4 supports user and group names. On Linux, see > rpc.idmapd(8). > > How the name support affects C code I can't say. If it's transparent, > i.e. you simply use local UID/GID, and the mapping happens below the > hood, then we'd have to translate string values to local UID/GID by the > usual means. Looks like a minor convenience feature on first glance. > However, QMP is a *network* protocol. A remote client can't easily do > this translation. > > Consider a GUI like virt-manager: I guess we'd rather support user and > group names there. But if we do, and QMP doesn't, either virt-manager > or libvirt need to map to numeric IDs. Easy enough when running on the > host, probably impractical when not. > > If we permit string values, are @uid and @gid still appropriate names? > The user name is not the user ID, it just maps to it. I guess we can make it @user and @group now, and then leave it an int for now so that making it an alternate later is introspectable? > >> +# > >> +# @tcp-syncnt: #optional number of SYNs during the session establishment > > > > Would tcp-syn-count be any more legible? Looks good to me. > We generally write out things in long hand in the QAPI schema. > > > What is the default when omitted? > > Whenever you write #optional, you must explain the default. When > the default is a fixed value, specify it. When the system picks a > default, state that, and think hard about what you need to specify on > how the system picks. > > >> +# > >> +# @readahead: #optional set the readahead size in bytes > > @read-ahead I think this is generally considered a single word. But @readahead-size. > > What's the default when omitted? > > > >> +# > >> +# @pagecache: #optional set the pagecache size in bytes > > @page-cache @page-cache-size > > Default? > > > >> +# > >> +# @debug: #optional set the NFS debug level (max 2) > > > > Presumably default 0? > > @BlockdevOptionsGluster calls this @debug-level. I wouldn't mind either way, but if there's precedence, let's do the same for consistency. Kevin
Am 25.10.2016 um 23:16 hat Eric Blake geschrieben: > On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: > > Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to > > support blockdev-add for NFS network protocol driver. Also make a new > > struct NFSServer to support tcp connection. > > > > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> > > --- > > qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 52 insertions(+), 4 deletions(-) > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index 9d797b8..3ab028d 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -1714,9 +1714,9 @@ > > { 'enum': 'BlockdevDriver', > > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > > 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', > > - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', > > - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > > - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', > > + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', > > + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > > Missing a comment that 'nfs' is since 2.8. > > > ## > > +# @NFSServer > > +# > > +# Captures the address of the socket > > +# > > +# @type: transport type used for NFS (only TCP supported) > > +# > > +# @host: host part of the address > > +# > > +# Since 2.8 > > +## > > +{ 'struct': 'NFSServer', > > + 'data': { 'type': 'str', > > Please make this an enum, instead of an open-coded string. It's okay if > the enum only has one value 'tcp' for now; but using an enum will make > it introspectable if we later add a second transport, unlike what we get > with an open-coded string. > > Must 'type' be mandatory if it must always be 'tcp'? I think the idea here was to make the wire format compatible with SocketAddress so we could later extend it. So it any case, it should be 'inet' rather than 'tcp'. Using an enum is probably a good idea, too. Kevin
On Wed, Oct 26, 2016 at 3:19 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 25.10.2016 um 23:16 hat Eric Blake geschrieben: >> On 10/24/2016 02:27 PM, Ashijeet Acharya wrote: >> > Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to >> > support blockdev-add for NFS network protocol driver. Also make a new >> > struct NFSServer to support tcp connection. >> > >> > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> >> > --- >> > qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- >> > 1 file changed, 52 insertions(+), 4 deletions(-) >> > >> > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > index 9d797b8..3ab028d 100644 >> > --- a/qapi/block-core.json >> > +++ b/qapi/block-core.json >> > @@ -1714,9 +1714,9 @@ >> > { 'enum': 'BlockdevDriver', >> > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', >> > 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', >> > - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', >> > - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', >> > - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> > + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', >> > + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', >> > + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> >> Missing a comment that 'nfs' is since 2.8. >> >> > ## >> > +# @NFSServer >> > +# >> > +# Captures the address of the socket >> > +# >> > +# @type: transport type used for NFS (only TCP supported) >> > +# >> > +# @host: host part of the address >> > +# >> > +# Since 2.8 >> > +## >> > +{ 'struct': 'NFSServer', >> > + 'data': { 'type': 'str', >> >> Please make this an enum, instead of an open-coded string. It's okay if >> the enum only has one value 'tcp' for now; but using an enum will make >> it introspectable if we later add a second transport, unlike what we get >> with an open-coded string. >> >> Must 'type' be mandatory if it must always be 'tcp'? > > I think the idea here was to make the wire format compatible with > SocketAddress so we could later extend it. So it any case, it should be > 'inet' rather than 'tcp'. > > Using an enum is probably a good idea, too. Making it an enum and converting it to a flat union like the way gluster does and set tcp (or inet as you prefer) to something like NFSSocketAddress which contains only host for now? That way we will just have to expand the enum in the future. Ashijeet > > Kevin
diff --git a/qapi/block-core.json b/qapi/block-core.json index 9d797b8..3ab028d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1714,9 +1714,9 @@ { 'enum': 'BlockdevDriver', 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', - 'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co', - 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', - 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } + 'host_device', 'http', 'https', 'luks', 'nfs', 'null-aio', + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', + 'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } ## # @BlockdevOptionsFile @@ -2212,6 +2212,54 @@ '*top-id': 'str' } } ## +# @NFSServer +# +# Captures the address of the socket +# +# @type: transport type used for NFS (only TCP supported) +# +# @host: host part of the address +# +# Since 2.8 +## +{ 'struct': 'NFSServer', + 'data': { 'type': 'str', + 'host': 'str' } } + +## +# @BlockdevOptionsNfs +# +# Driver specific block device option for NFS +# +# @server: host address +# +# @path: path of the image on the host +# +# @uid: #optional UID value to use when talking to the server +# +# @gid: #optional GID value to use when talking to the server +# +# @tcp-syncnt: #optional number of SYNs during the session establishment +# +# @readahead: #optional set the readahead size in bytes +# +# @pagecache: #optional set the pagecache size in bytes +# +# @debug: #optional set the NFS debug level (max 2) +# +# Since 2.8 +## +{ 'struct': 'BlockdevOptionsNfs', + 'data': { 'server': 'NFSServer', + 'path': 'str', + '*uid': 'int', + '*gid': 'int', + '*tcp-syncnt': 'int', + '*readahead': 'int', + '*pagecache': 'int', + '*debug': 'int' } } + +## # @BlockdevOptionsCurl # # Driver specific block device options for the curl backend. @@ -2269,7 +2317,7 @@ # TODO iscsi: Wait for structured options 'luks': 'BlockdevOptionsLUKS', # TODO nbd: Should take InetSocketAddress for 'host'? -# TODO nfs: Wait for structured options + 'nfs': 'BlockdevOptionsNfs', 'null-aio': 'BlockdevOptionsNull', 'null-co': 'BlockdevOptionsNull', 'parallels': 'BlockdevOptionsGenericFormat',
Introduce new object 'BlockdevOptionsNFS' in qapi/block-core.json to support blockdev-add for NFS network protocol driver. Also make a new struct NFSServer to support tcp connection. Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com> --- qapi/block-core.json | 56 ++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 4 deletions(-)