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

[Kevin Wolf]

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.

Kevin

pgpwGqju0jX3g.pgp
Description: PGP signature