Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben: > A few drive-by comments... > > Eric Blake <ebl...@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 <ashijeetacha...@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