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

[Prasanna Kalever]

On Fri, Feb 5, 2016 at 6:47 PM, Prasanna Kumar Kalever
<address@hidden> wrote:
>
> 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

Eric, Kevin, any updates on union discriminator related patches,
any hope for taking these patches in 2.6 ?

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