Re: [Qemu-devel] [PATCH 4/4] block/gluster: add support for multiple gluster servers

[Kevin Wolf]

Am 12.11.2015 um 23:36 hat Eric Blake geschrieben:
> On 11/12/2015 03:22 AM, Prasanna Kumar Kalever wrote:
> > +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **gconf,
> > +                                      const char *filename,
> > +                                      QDict *options, Error **errp)
> > +{
> > +    int ret;
> > +
> > +    if (filename) {
> > +        ret = qemu_gluster_parseuri(gconf, filename);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Usage: 
> > file=gluster[+transport]://[host[:port]]/"
> > +                             "volume/path[?socket=...]");
> 
> Hmm, just noticing this now, even though this error message is just code
> motion.  It looks like the optional [?socket=...] part of a URI is only
> important when using gluster+unix (is it silently ignored otherwise?).
> And if it is used, you are then assigning it to the host field?
> 
> I almost wonder if GlusterServer should be a discriminated union.  That
> is, in qapi-pseudocode (won't actually compile yet, because it depends
> on features that I have queued for 2.6):
> 
> { 'union':'GlusterServer', 'base':{'transport':'GlusterTransport'},
>   'discriminator':'transport', 'data':{
>     'tcp':{'host':'str', '*port':'uint16'},
>     'unix':{'socket':'str'},
>     'rdma':{...} } }
> 
> Hmm. Qapi doesn't (yet) allow for an optional discriminator (where the
> omission of the discriminator picks a default branch) - another RFE for
> my qapi work for 2.6.

Eric, Prasanna, is this QAPI extension what we're waiting for or what is
the status of this series? Niels (CCed) was hacking on the same thing,
so maybe it's time to get this moving again.

Kevin

> Command-line wise, this would mean you could do in JSON:
> 
> 'servers':[{'transport':'tcp', 'host':'foo'},
>            {'transport':'unix', 'socket':'/path/to/bar'},
>            {'host':'blah'}]
> 
> where the third entry defaults to transport tcp.
> 
> If we think that description is better than what we proposed in 3/4,
> then it's really late to be adding it now, especially since (without
> qapi changes) we'd have a mandatory rather than optional 'transport';
> but worse if we commit to the interface of 3/4 and don't get the
> conversion made in time to the nicer interface.  At least it's okay from
> back-compat perspective to make a mandatory field become optional in
> later releases.
> 
> If it were just gluster code I was worried about, then I could live with
> the interface proposal.  But since seeing this error message is making
> me double-guess the interface correctness, and that will have an impact
> on libvirt, I'm starting to have doubts on what it means for qemu 2.5.