From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzK4D-0003RY-QG for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:00:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzK49-0000Hv-0Q for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:00:13 -0400 Date: Wed, 26 Oct 2016 10:59:59 +0200 From: Kevin Wolf Message-ID: <20161026085959.GF4758@noname.str.redhat.com> References: <1477337274-7939-1-git-send-email-ashijeetacharya@gmail.com> <1477337274-7939-3-git-send-email-ashijeetacharya@gmail.com> <1cf114e6-aa47-677c-43aa-06240e852c26@redhat.com> <8760ofr2sx.fsf@dusky.pond.sub.org> <20161026080632.GB4758@noname.str.redhat.com> <87zilrmrjd.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87zilrmrjd.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-block@nongnu.org, jcody@redhat.com, pl@kamp.de, qemu-devel@nongnu.org, mreitz@redhat.com, Ashijeet Acharya Am 26.10.2016 um 10:40 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 26.10.2016 um 09:23 hat Markus Armbruster geschrieben: > >> A few drive-by comments... > >> > >> Eric Blake 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 > >> >> --- > >> >> 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