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

[Prasanna Kalever <pkalever@redhat.com>]

On Wed, Mar 23, 2016 at 5:46 PM, Prasanna Kalever <pkalever@redhat.com> wrote:
> On Fri, Feb 5, 2016 at 6:47 PM, Prasanna Kumar Kalever
> <pkalever@redhat.com> 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

[ Adding Eric in 'to:' list ]

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