[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] qapi: allow blockdev-add for NFS |
Date: |
Wed, 26 Oct 2016 09:23:58 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
A few drive-by comments...
Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>> 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' } }
>> +