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

[Ashijeet Acharya]

On Wed, Oct 26, 2016 at 3:19 PM, Kevin Wolf <address@hidden> 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 <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.
>>
>> 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