Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS

[Kevin Wolf <kwolf@redhat.com>]

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