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

[Prasanna Kumar Kalever <pkalever@redhat.com>]

On Thursday, February 4, 2016 6:52:15 PM Kevin Wolf Wrote:
> 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, correct me if I am wrong, union discriminator support is not yet added
into qemu, I am waiting for this. I spoke to Eric Blake regarding the same may be
a month ago from now.

-Prasanna

> 
> 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.
> 
>