[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] block/gluster: add support for multiple glu
From: |
Prasanna Kumar Kalever |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] block/gluster: add support for multiple gluster servers |
Date: |
Tue, 20 Oct 2015 03:23:18 -0400 (EDT) |
On Tuesday, October 20, 2015 1:38:37 AM, Eric Blake wrote:
> On 10/19/2015 06:13 AM, Prasanna Kumar Kalever wrote:
> > This patch adds a way to specify multiple volfile servers to the gluster
> > block backend of QEMU with tcp|rdma transport types and their port numbers.
>
> When sending a multi-patch series, it is best to also include a 0/3
> cover letter. Git can be configured to do this automatically with:
> git config format.coverLetter auto
>
Thanks for the tip Eric :)
[...]
> > +#define GLUSTER_DEFAULT_PORT 24007
> > +
> > typedef struct GlusterAIOCB {
> > int64_t size;
> > int ret;
> > @@ -24,22 +34,72 @@ typedef struct BDRVGlusterState {
> > struct glfs_fd *fd;
> > } BDRVGlusterState;
> >
> > -typedef struct GlusterConf {
> > +typedef struct GlusterServerConf {
> > char *server;
> > int port;
> > + char *transport;
>
> How do you know how many transport tuples are present? I'd expect a size
> argument somewhere.
>
Its based on users choice I don't want to make it static
> > +} GlusterServerConf;
[...]
> > @@ -117,16 +178,19 @@ static int qemu_gluster_parseuri(GlusterConf *gconf,
> > const char *filename)
> > return -EINVAL;
> > }
> >
> > + gconf = g_new0(GlusterConf, 1);
> > + gconf->gsconf = g_new0(GlusterServerConf, 1);
>
> Wow - you are hard-coding things to exactly one server. The subject
> line of the patch claims multiple gluster servers, but I don't see
> anything that supports more than one. So something needs to be fixed up
> (if this patch is just renaming things, and a later patch adds support
> for more than one, that's okay - but it needs to be described that way).
>
[1] I think you need to check 'qemu_gluster_parsejson' function for multiple
gluster servers
usage which parse JSON syntax with multiple tuples, 'qemu_gluster_parseuri'
function is to
parse URI syntax only and that supports single server usage only (kept for
compatibility)
> > +static int qemu_gluster_parsejson(GlusterConf **pgconf, QDict *options)
> > +{
[...]
> > +#
> > +# Since: 2.5
> > +##
> > +{ 'struct': 'GlusterTuple',
> > + 'data': { 'host': 'str',
> > + '*port': 'int',
> > + '*transport': 'GlusterTransport' } }
> > +
> > +##
> > +# @BlockdevOptionsGluster
> > +#
> > +# Driver specific block device options for Gluster
> > +#
> > +# @volume: name of gluster volume where our VM image resides
> > +#
> > +# @path: absolute path to image file in gluster volume
> > +#
> > +# @servers: holds multiple tuples of {host, transport, port}
>
> For this patch, it looks like it holds exactly one tuple. But it looks
> like you plan to support multiple tuples later on; maybe a better
> wording is:
>
> @servers: one or more gluster host descriptions (host, port, and transport)
>
... [1] should clarify your understanding, but yes still I will bind to your
comment
[...]
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
-Prasanna
- [Qemu-devel] [PATCH 1/3] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2015/10/19
- [Qemu-devel] [PATCH 2/3] block/gluster: rename [server, volname, image] -> [host, volume, path], Prasanna Kumar Kalever, 2015/10/19
- [Qemu-devel] [PATCH 3/3] block/gluster: code cleanup, Prasanna Kumar Kalever, 2015/10/19
- Re: [Qemu-devel] [PATCH 1/3] block/gluster: add support for multiple gluster servers, Eric Blake, 2015/10/19
- Re: [Qemu-devel] [PATCH 1/3] block/gluster: add support for multiple gluster servers,
Prasanna Kumar Kalever <=