[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 1/1] block/gluster: add support for multiple
|
From: |
Prasanna Kalever |
|
Subject: |
Re: [Qemu-devel] [PATCH v5 1/1] block/gluster: add support for multiple gluster backup volfile servers |
|
Date: |
Wed, 7 Oct 2015 06:15:59 -0400 (EDT) |
Hi Peter & Kevin,
Thanks for your detailed review comments. I shall try to incorporate these
changes as a next patch-set.
- Prasanna Kumar Kalever
> On Mon, Sep 28, 2015 at 18:06:12 +0530, 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.
> >
> > Problem:
> >
> > Currenly VM Image on gluster volume is specified like this:
> >
> > file=gluster[+tcp]://server1[:port]/testvol/a.img
> >
> > Assuming we have have three servers in trustred pool with replica 3 volume
> > in action and unfortunately server1 (mentioned in the command above) went
> > down
> > for some reason, since the volume is replica 3 we now have other 2 servers
> > active from which we can boot the VM.
> >
> > But currently there is no mechanism to pass the other 2 gluster server
> > addresses to qemu.
> >
> > Solution:
> >
> > New way of specifying VM Image on gluster volume with volfile servers:
> > (We still support old syntax to maintain backward compatibility)
> >
> > Basic command line syntax looks like:
> >
> > Pattern I:
> > -drive driver=gluster,
> > volname=testvol,image-path=/path/a.raw,
> > volfile-servers.0.server=1.2.3.4,
>
> I still think 'volfile-servers' should be just 'server'. I don't
> understand why it needs to contain anything else. See below for
> suggestions ...
>
> > [volfile-servers.0.port=24007,]
> > [volfile-servers.0.transport=tcp,]
> > volfile-servers.1.server=5.6.7.8,
> > [volfile-servers.1.port=24008,]
> > [volfile-servers.1.transport=rdma,] ...
> >
> > Pattern II:
> > 'json:{"driver":"qcow2","file":{"driver":"gluster",
> > "volname":"testvol","image-path":"/path/a.qcow2",
> > "volfile-servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
> >
> > driver => 'gluster' (protocol name)
> > volname => name of gluster volume where our VM image resides
> > image-path => is the absolute path of image in gluster volume
> >
> > {tuple} => {"server":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
> >
> > server => server address (hostname/ipv4/ipv6 addresses)
> > port => port number on which glusterd is listening. (default
> > 24007)
> > tranport => transport type used to connect to gluster management
> > daemon,
> > it can be tcp|rdma (default 'tcp')
> >
> > Examples:
> > 1.
> > -drive driver=qcow2,file.driver=gluster,
> > file.volname=testvol,file.image-path=/path/a.qcow2,
> > file.volfile-servers.0.server=1.2.3.4,
> > file.volfile-servers.0.port=24007,
> > file.volfile-servers.0.transport=tcp,
> > file.volfile-servers.1.server=5.6.7.8,
> > file.volfile-servers.1.port=24008,
> > file.volfile-servers.1.transport=rdma
> > 2.
> > 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
> > "image-path":"/path/a.qcow2","volfile-servers":
> > [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
> > {"server":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
>
> -drive driver=qcow2,file.driver=gluster,
> file.volume=testvol,
> file.path=/path/a.qcow2,
> file.server.0.host=1.2.3.4,
> file.server.0.port=24007,
> file.server.0.transport=tcp,
> file.server.1.host=5.6.7.8,
> file.server.1.port=24008,
> file.server.1.transport=rdma
>
> I'm suggesting the above naming scheme.
> So:
> 'path' instead of 'image-path'
> 'volume' instead of 'volname'
> 'server' instead of 'volfile-servers'
> 'host' instead of 'server'
>
> 2.
> 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
> "path":"/path/a.qcow2","server":
> [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
> {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
>
> >
> > This patch gives a mechanism to provide all the server addresses which are
> > in
> > replica set, so in case server1 is down VM can still boot from any of the
> > active servers.
> >
> > This is equivalent to the volfile-servers option supported by
> > mount.glusterfs (FUSE way of mounting gluster volume)
>
> I don't think qemu needs to follow mount.glusterfs in naming.
>
> >
> > This patch depends on a recent fix in libgfapi raised as part of this work:
> > http://review.gluster.org/#/c/12114/
> >
> > Credits: Sincere thanks to Kevin Wolf <address@hidden> and
> > "Deepak C Shetty" <address@hidden> for inputs and all their support
> >
> > Signed-off-by: Prasanna Kumar Kalever <address@hidden>
> > ---
>
> [snip]
>
> > diff --git a/block/gluster.c b/block/gluster.c
> > index 1eb3a8c..63c3dcb 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -11,6 +11,15 @@
> > #include "block/block_int.h"
> > #include "qemu/uri.h"
> >
> > +#define GLUSTER_OPT_FILENAME "filename"
> > +#define GLUSTER_OPT_VOLNAME "volname"
> > +#define GLUSTER_OPT_IMAGE_PATH "image-path"
> > +#define GLUSTER_OPT_SERVER "server"
> > +#define GLUSTER_OPT_PORT "port"
> > +#define GLUSTER_OPT_TRANSPORT "transport"
> > +#define GLUSTER_OPT_READ_PATTERN "volfile-servers."
> > +
> > +
> > typedef struct GlusterAIOCB {
> > int64_t size;
> > int ret;
> > @@ -43,6 +52,60 @@ static void qemu_gluster_gconf_free(GlusterConf *gconf)
> > }
> > }
> >
> > +static QemuOptsList runtime_opts = {
> > + .name = "gluster",
> > + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> > + .desc = {
> > + {
> > + .name = GLUSTER_OPT_FILENAME,
> > + .type = QEMU_OPT_STRING,
> > + .help = "URL to the gluster image",
> > + },
> > + { /* end of list */ }
> > + },
> > +};
> > +
> > +static QemuOptsList runtime_json_opts = {
> > + .name = "gluster_json",
> > + .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head),
> > + .desc = {
> > + {
> > + .name = GLUSTER_OPT_VOLNAME,
> > + .type = QEMU_OPT_STRING,
> > + .help = "name of gluster volume where our VM image resides",
> > + },
> > + {
> > + .name = GLUSTER_OPT_IMAGE_PATH,
> > + .type = QEMU_OPT_STRING,
> > + .help = "absolute path to image file in gluster volume",
> > + },
> > + { /* end of list */ }
> > + },
> > +};
> > +
> > +static QemuOptsList runtime_tuple_opts = {
> > + .name = "gluster_tuple",
> > + .head = QTAILQ_HEAD_INITIALIZER(runtime_tuple_opts.head),
> > + .desc = {
> > + {
> > + .name = GLUSTER_OPT_SERVER,
> > + .type = QEMU_OPT_STRING,
> > + .help = "server address (hostname/ipv4/ipv6 addresses)",
> > + },
> > + {
> > + .name = GLUSTER_OPT_PORT,
> > + .type = QEMU_OPT_NUMBER,
> > + .help = "port number on which glusterd is listening(default
> > 24007)",
> > + },
> > + {
> > + .name = GLUSTER_OPT_TRANSPORT,
> > + .type = QEMU_OPT_STRING,
> > + .help = "transport type used to connect to glusterd(default
> > tcp)",
> > + },
> > + { /* end of list */ }
> > + },
> > +};
> > +
> > static int parse_volume_options(GlusterConf *gconf, char *path)
> > {
> > char *p, *q;
> > @@ -105,6 +168,7 @@ static int parse_volume_options(GlusterConf *gconf,
> > char *path)
> > * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
> > * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
> > */
> > +
>
> Spurious whitespace change.
>
> > static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> > {
> > URI *uri;
> > @@ -166,30 +230,25 @@ out:
> > return ret;
> > }
> >
> > -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char
> > *filename,
> > - Error **errp)
> > +static struct glfs *qemu_gluster_glfs_init(GlusterConf *gconf, int
> > num_servers,
> > + Error **errp)
> > {
> > struct glfs *glfs = NULL;
> > - int ret;
> > int old_errno;
> > -
> > - ret = qemu_gluster_parseuri(gconf, filename);
> > - if (ret < 0) {
> > - error_setg(errp, "Usage:
> > file=gluster[+transport]://[server[:port]]/"
> > - "volname/image[?socket=...]");
>
> [3] (see below)
>
> > - errno = -ret;
> > - goto out;
> > - }
> > + int ret = 0;
> > + int i = 0;
> >
> > glfs = glfs_new(gconf->volname);
> > if (!glfs) {
> > goto out;
> > }
> >
> > - ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->server,
> > - gconf->port);
> > - if (ret < 0) {
> > - goto out;
> > + for (i = 0; i < num_servers; i++) {
> > + ret = glfs_set_volfile_server(glfs, gconf[i].transport,
> > gconf[i].server,
> > + gconf[i].port);
>
> This adds back the pre-existing strange alignment of the code it removed
> before.
>
> > + if (ret < 0) {
> > + goto out;
> > + }
> > }
> >
> > /*
> > @@ -204,15 +263,12 @@ static struct glfs *qemu_gluster_init(GlusterConf
> > *gconf, const char *filename,
> > ret = glfs_init(glfs);
> > if (ret) {
> > error_setg_errno(errp, errno,
> > - "Gluster connection failed for server=%s port=%d
> > "
> > - "volume=%s image=%s transport=%s", gconf->server,
> > - gconf->port, gconf->volname, gconf->image,
> > - gconf->transport);
> > + "Gluster connection failed for volname=%s image=%s",
> > + gconf->volname, gconf->image);
>
> The above error message doesn't mention any server at all. I think it
> should contain at least the first server. Also the alignment of the code
> got strange.
>
> >
> > /* glfs_init sometimes doesn't set errno although docs suggest
> > that */
> > if (errno == 0)
> > errno = EINVAL;
> > -
>
> Removal of the empty line decreases readability.
>
> > goto out;
> > }
> > return glfs;
> > @@ -226,6 +282,297 @@ out:
> > return NULL;
> > }
> >
> > +
> > +static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char
> > *filename,
> > + Error **errp)
> > +{
> > + struct glfs *glfs = NULL;
> > + int ret;
>
> This function should allocate 'gconf' rather than have it passed. [5]
>
> > +
> > + ret = qemu_gluster_parseuri(gconf, filename);
> > + if (ret < 0) {
> > + error_setg(errp, "Usage:
> > file=gluster[+transport]://[server[:port]]/"
> > + "volname/image[?socket=...]");
> > + errno = -ret;
> > + goto out;
> > + }
> > +
> > + glfs = qemu_gluster_glfs_init(gconf, 1, errp);
>
> Everyting below can be replaced by "return qemu_gluster_glfs_init(.."
>
> > + if (!glfs) {
> > + goto out;
> > + }
> > + return glfs;
> > +
> > +out:
> > + return NULL;
> > +}
> > +
> > +static int parse_transport_option(const char *opt)
> > +{
> > + int i;
> > +
> > + if (!opt) {
> > + /* Set tcp as default */
> > + return GLUSTER_TRANSPORT_TCP;
> > + }
> > +
> > + for (i = 0; i < GLUSTER_TRANSPORT_MAX; i++) {
> > + if (!strcmp(opt, GlusterTransport_lookup[i])) {
> > + return i;
> > + }
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +/*
> > +*
> > +* Basic command line syntax looks like:
> > +*
> > +* Pattern I:
> > +* -drive driver=gluster,
> > +* volname=testvol,file.image-path=/path/a.raw,
> > +* volfile-servers.0.server=1.2.3.4,
> > +* [volfile-servers.0.port=24007,]
> > +* [volfile-servers.0.transport=tcp,]
> > +* volfile-servers.1.server=5.6.7.8,
> > +* [volfile-servers.1.port=24008,]
> > +* [volfile-servers.1.transport=rdma,] ...
> > +*
> > +* Pattern II:
> > +* 'json:{"driver":"qcow2","file":{"driver":"gluster",
> > +* "volname":"testvol","image-path":"/path/a.qcow2",
> > +* "volfile-servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
> > +*
> > +*
> > +* driver => 'gluster' (protocol name)
> > +* volname => name of gluster volume where our VM image resides
> > +* image-path => is the absolute path of image in gluster volume
> > +*
> > +* {tuple} =>
> > {"server":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
> > +*
> > +* server => server address (hostname/ipv4/ipv6 addresses)
> > +* port => port number on which glusterd is listening. (default
> > 24007)
> > +* tranport => transport type used to connect to gluster management
> > daemon,
> > +* it can be tcp|rdma (default 'tcp')
> > +*
> > +*
> > +* Examples:
> > +* Pattern I:
> > +* -drive driver=qcow2,file.driver=gluster,
> > +* file.volname=testvol,file.image-path=/path/a.qcow2,
> > +* file.volfile-servers.0.server=1.2.3.4,
> > +* file.volfile-servers.0.port=24007,
> > +* file.volfile-servers.0.transport=tcp,
> > +* file.volfile-servers.1.server=5.6.7.8,
> > +* file.volfile-servers.1.port=24008,
> > +* file.volfile-servers.1.transport=rdma, ...
> > +*
> > +* -drive driver=qcow2,file.driver=gluster,
> > +* file.volname=testvol,file.image-path=/path/a.qcow2,
> > +* file.volfile-servers.0.server=1.2.3.4,
> > +* file.volfile-servers.1.server=5.6.7.8, ...
> > +*
> > +* -drive driver=qcow2,file.driver=gluster,
> > +* file.volname=testvol,file.image-path=/path/a.qcow2,
> > +* file.volfile-servers.0.server=1.2.3.4,
> > +* file.volfile-servers.0.port=24007,
> > +* file.volfile-servers.1.server=5.6.7.8,
> > +* file.volfile-servers.1.port=24008, ...
> > +*
> > +* -drive driver=qcow2,file.driver=gluster,
> > +* file.volname=testvol,file.image-path=/path/a.qcow2,
> > +* file.volfile-servers.0.server=1.2.3.4,
> > +* file.volfile-servers.0.transport=tcp,
> > +* file.volfile-servers.1.server=5.6.7.8,
> > +* file.volfile-servers.1.transport=rdma, ...
> > +*
> > +* Pattern II:
> > +* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
> > +* "image-path":"/path/a.qcow2","volfile-servers":
> > +* [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
> > +* {"server":"4.5.6.7","port":"24008","transport":"rdma"}, ...]}}'
> > +*
> > +* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
> > +* "image-path":"/path/a.qcow2","volfile-servers":
> > +* [{"server":"1.2.3.4"},
> > +* {"server":"4.5.6.7"}, ...]}}'
> > +*
> > +*
> > +* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
> > +* "image-path":"/path/a.qcow2","volfile-servers":
> > +* [{"server":"1.2.3.4","port":"24007"},
> > +* {"server":"4.5.6.7","port":"24008"},
> > ...]}}'
> > +*
> > +*
> > +* 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
> > +* "image-path":"/path/a.qcow2","volfile-servers":
> > +* [{"server":"1.2.3.4","transport":"tcp"},
> > +* {"server":"4.5.6.7","transport":"rdma"},
> > ...]}}'
> > +*
> > +* Just for better readability pattern II is kept as:
> > +* json:
> > +* {
> > +* "driver":"qcow2",
> > +* "file":{
> > +* "driver":"gluster",
> > +* "volname":"testvol",
> > +* "image-path":"/path/a.qcow2",
> > +* "volfile-servers":[
> > +* {
> > +* "server":"1.2.3.4",
> > +* "port":"24007",
> > +* "transport":"tcp"
> > +* },
> > +* {
> > +* "server":"5.6.7.8",
> > +* "port":"24008",
> > +* "transport":"rdma"
> > +* }
> > +* ]
> > +* }
> > +* }
> > +*
> > +*/
> > +
> > +static int qemu_gluster_parseopts(GlusterConf **pgconf, QDict *options)
> > +{
> > + QemuOpts *opts;
> > + GlusterConf *gconf = NULL;
> > + QDict *backing_options;
> > + Error *local_err = NULL;
> > + char *str = NULL;
> > + int num_servers = 0;
> > + int ret = 0;
> > + int i = 0;
> > +
> > + /* parse options in dict */
> > + opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
> > + qemu_opts_absorb_qdict(opts, options, &local_err);
> > + if (local_err) {
> > + error_report_err(local_err);
> > + goto out;
> > + }
> > + num_servers = qdict_array_entries(options, GLUSTER_OPT_READ_PATTERN);
>
> [1]
>
> The function above isn't really optimal in way it's written. It
> basically does the same as the loop below that is using it by formatting
> the prefix and a number and tries to find the maximum. The code could
> avoid using it by basically merging it with the loop below.
>
> > + if (num_servers < 1) {
> > + error_setg(&local_err, "\n\n ********* qemu_gluster: "
>
> [2]
>
> These error messages are really unusual. Most of the error messages
> contain no newlines at the beginning and no stars. I think it should be
> kept consistent.
>
> Additionally, possible early errors from qemu are read back to libvirt
> and displayed to the users, so this would make also some libvirt error
> messages really ugly.
>
> > + "please provide 'volfile-servers' option "
> > + "with valid fields in array of tuples *********\n");
> > + error_report_err(local_err);
> > + goto out;
> > + }
> > +
> > + gconf = g_new0(GlusterConf, num_servers);
> > +
> > + gconf->volname = (char *) qemu_opt_get(opts, GLUSTER_OPT_VOLNAME);
> > + if (!gconf->volname) {
> > + error_setg(&local_err, "\n\n ********* qemu_gluster: "
> > + "please provide 'volname'option *********\n");
>
> [2]
>
> > + error_report_err(local_err);
> > + goto out;
> > + }
> > + gconf->image = (char *) qemu_opt_get(opts, GLUSTER_OPT_IMAGE_PATH);
> > + if (!gconf->image) {
> > + error_setg(&local_err, "\n\n ********* qemu_gluster: "
> > + "please provide 'image-path' option *********\n");
>
> [2]
>
> > + error_report_err(local_err);
> > + goto out;
> > + }
> > + opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort);
> > + for (i = 0; i < num_servers; i++) {
> > + if (i > 0) {
> > + gconf[i].volname = gconf->volname;
> > + gconf[i].image = gconf->image;
>
> [4]
>
> So this looks weird. struct GlusterConf has all the fields and you
> basically make an array of the structs includig of duplicated entries
> that can't be different for servers.
>
> I think what you want is 'GlusterServerConf' struct that will be as a
> sub-struct of 'GlusterConf' which will contain just information relevant
> to the volume file servers.
>
> > + }
> > + str = g_malloc(40);
> > + snprintf(str, 40, GLUSTER_OPT_READ_PATTERN"%d.", i);
> > + qdict_extract_subqdict(options, &backing_options, str);
> > + g_free(str);
>
> This buffer could theoretically be reused.
>
> > + qemu_opts_absorb_qdict(opts, backing_options, &local_err);
> > + if (local_err) {
> > + error_report_err(local_err);
> > + goto out;
> > + }
>
> One unfortunate drawback of the way I've described in [1] would be that
> the gconf array would need to be resized here.
>
>
> > + gconf[i].server = (char *) qemu_opt_get(opts, GLUSTER_OPT_SERVER);
> > + if (!gconf[i].server) {
> > + error_setg(&local_err, "\n\n ********* qemu_gluster: "
>
> [2]
>
> > + "volfile-servers.{tuple.%d} requires 'server' "
> > + "option *********\n", i);
> > + error_report_err(local_err);
> > + goto out;
> > + }
> > + ret = parse_transport_option(qemu_opt_get(opts,
> > GLUSTER_OPT_TRANSPORT));
>
> So this converts the 'transport' field string to a number describing the
> protocol ...
>
> > + if (ret < 0) {
> > + error_setg(&local_err, "\n\n ********* qemu_gluster: "
>
> [2]
>
> > + "please set 'transport' type in tuple.%d as tcp or
> > rdma "
> > + "*********\n", i);
> > + error_report_err(local_err);
> > + goto out;
> > + } else {
>
> ... and here you convert it back to a string?!
>
> > + if (ret == GLUSTER_TRANSPORT_TCP) {
> > + gconf[i].transport = (char *)"tcp";
> > + } else {
> > + gconf[i].transport = (char *)"rdma";
> > + }
>
> A rather weird way. I don't see a reason to go back and forth to integer
> values. You either need a string later or a integer option.
>
> > + }
> > + gconf[i].port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT,
> > 24007);
> > +
> > + /*
> > + * reset current tuple opts to NULL/0, so that in case if the next
> > tuple
> > + * misses any of (server, tranport, port) options there is no
> > chance of
> > + * copying from current set.
> > + */
> > + qemu_opt_set_number(opts, GLUSTER_OPT_PORT, 0, &error_abort);
> > + qemu_opt_set(opts, GLUSTER_OPT_TRANSPORT, NULL, &error_abort);
> > + qemu_opt_set(opts, GLUSTER_OPT_SERVER, NULL, &error_abort);
>
> This seems strange too. If the code that is parsing it is correct there
> should be no need to remove the fields which can possibly mask other
> bugs or legitimate usage.
>
> > + }
> > + *pgconf = gconf;
> > + return num_servers;
> > +
> > +out:
> > + ret = -EINVAL;
> > + errno = -ret;
> > + qemu_opts_del(opts);
> > + return ret;
>
> This can be written as:
>
> out:
> errno = EINVAL;
> qemu_opts_del(opts);
> return -EINVAL;
>
> So that you don't confuse readers of the function by reusing 'ret'.
>
> > +}
> > +
> > +static struct glfs *qemu_gluster_opts_init(GlusterConf **pgconf,
> > + QDict *options, Error **errp)
> > +{
> > + struct glfs *glfs = NULL;
> > + int num_servers = 0;
>
> No need to initialize any of those.
>
> > +
> > + num_servers = qemu_gluster_parseopts(pgconf, options);
> > + if (num_servers < 1) {
> > + error_setg(errp, "\n"
> > + "\n#Usage1:\n"
>
> The string again looks very suspicious with so many newlines in front.
> The previous version didn't have a newline in front of the string nor in
> the back [3].
>
> > + "-drive driver=qcow2,file.driver=gluster,"
> > +
> > "file.volname=testvol,file.image-path=/path/a.qcow2,"
> > + "file.volfile-servers.0.server=1.2.3.4,"
> > + "[file.volfile-servers.0.port=24007,]"
> > + "[file.volfile-servers.0.transport=tcp,]"
> > + "file.volfile-servers.1.server=5.6.7.8,"
> > + "[file.volfile-servers.1.port=24008,]"
> > + "[file.volfile-servers.1.transport=rdma,] ...\n"
> > +
> > "\n#Usage2:\n'json:{\"driver\":\"qcow2\",\"file\":"
> > + "{\"driver\":\"gluster\",\"volname\":\""
> > + "testvol\",\"image-path\":\"/path/a.qcow2\","
> > + "\"volfile-servers\":[{\"server\":\"1.2.3.4\","
> > + "\"port\":\"24007\",\"transport\":\"tcp\"},"
> > + "{\"server\":\"4.5.6.7\",\"port\":\"24007\","
> > + "\"transport\":\"rdma\"}, ...]}}'\n");
> > + goto out;
> > + }
> > + glfs = qemu_gluster_glfs_init(*pgconf, num_servers, errp);
>
> Everything below can be replaced by just "return qemu_gluster_glfs_init ..."
>
> > + if (!glfs) {
> > + goto out;
> > + }
> > + return glfs;
> > +
> > +out:
> > + return NULL;
> > +}
> > +
>
> [snip]
>
> > @@ -291,11 +624,12 @@ static int qemu_gluster_open(BlockDriverState *bs,
> > QDict *options,
> > BDRVGlusterState *s = bs->opaque;
> > int open_flags = 0;
> > int ret = 0;
> > - GlusterConf *gconf = g_new0(GlusterConf, 1);
> > + GlusterConf *gconf = NULL;
> > QemuOpts *opts;
> > Error *local_err = NULL;
> > - const char *filename;
> > + const char *filename = NULL;
>
> You don't need to initialize 'filename';
>
> >
> > + qdict_flatten(options);
> > opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> > qemu_opts_absorb_qdict(opts, options, &local_err);
> > if (local_err) {
> > @@ -305,8 +639,12 @@ static int qemu_gluster_open(BlockDriverState *bs,
> > QDict *options,
> > }
> >
> > filename = qemu_opt_get(opts, "filename");
> > -
> > - s->glfs = qemu_gluster_init(gconf, filename, errp);
> > + if (filename) {
> > + gconf = g_new0(GlusterConf, 1);
> > + s->glfs = qemu_gluster_init(gconf, filename, errp);
>
> qemu_gluster_init can be made to allocate gconf the same way as
> qemu_gluster_opts_init, so that there's no difference in calling
> semantics. See [5]
>
> > + } else {
> > + s->glfs = qemu_gluster_opts_init(&gconf, options, errp);
> > + }
> > if (!s->glfs) {
> > ret = -errno;
> > goto out;
> > @@ -321,7 +659,11 @@ static int qemu_gluster_open(BlockDriverState *bs,
> > QDict *options,
> >
> > out:
> > qemu_opts_del(opts);
> > - qemu_gluster_gconf_free(gconf);
> > + if (filename) {
> > + qemu_gluster_gconf_free(gconf);
> > + } else {
> > + g_free(gconf);
> > + }
>
> Fixing [4] will actually make this hunk really simpler.
>
> > if (!ret) {
> > return ret;
> > }
> > @@ -339,7 +681,6 @@ typedef struct BDRVGlusterReopenState {
> > struct glfs_fd *fd;
> > } BDRVGlusterReopenState;
> >
> > -
> > static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
> > BlockReopenQueue *queue, Error
> > **errp)
> > {
> > @@ -401,7 +742,6 @@ static void qemu_gluster_reopen_commit(BDRVReopenState
> > *state)
> > return;
> > }
> >
> > -
> > static void qemu_gluster_reopen_abort(BDRVReopenState *state)
> > {
> > BDRVGlusterReopenState *reop_s = state->opaque;
>
> Both hunks above are spurious whitespace change.
>
> Peter
>