| Message ID | 1443443772-7630-1-git-send-email-prasanna.kalever@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <kwolf@redhat.com> and > "Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > --- [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
Am 07.10.2015 um 11:09 hat Peter Krempa geschrieben: > On Mon, Sep 28, 2015 at 18:06:12 +0530, Prasanna Kumar Kalever wrote: > > 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' I agree, let's keep the names short so they are easy to read, type and remember. I especially agree with changing 'server' into 'host' because that makes it consistent with InetSocketAddress. Kevin
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 <kwolf@redhat.com> and > > "Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support > > > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > > --- > > [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 >
[ trimmed the CC list for this ] On Wed, Oct 07, 2015 at 06:15:59 -0400, Prasanna Kalever wrote: > Hi Peter & Kevin, > > Thanks for your detailed review comments. I shall try to incorporate these changes as a next patch-set. > > - Prasanna Kumar Kalever > > Please don't top post on technical lists ... and ... > > > 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: ... trim parts of messages that are no longer relevant. http://www.idallen.com/topposting.html
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 */ + 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=...]"); - 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); + 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); /* glfs_init sometimes doesn't set errno although docs suggest that */ if (errno == 0) errno = EINVAL; - 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; + + 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); + 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); + if (num_servers < 1) { + error_setg(&local_err, "\n\n ********* qemu_gluster: " + "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"); + 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"); + 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; + } + str = g_malloc(40); + snprintf(str, 40, GLUSTER_OPT_READ_PATTERN"%d.", i); + qdict_extract_subqdict(options, &backing_options, str); + g_free(str); + qemu_opts_absorb_qdict(opts, backing_options, &local_err); + if (local_err) { + error_report_err(local_err); + goto out; + } + gconf[i].server = (char *) qemu_opt_get(opts, GLUSTER_OPT_SERVER); + if (!gconf[i].server) { + error_setg(&local_err, "\n\n ********* qemu_gluster: " + "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)); + if (ret < 0) { + error_setg(&local_err, "\n\n ********* qemu_gluster: " + "please set 'transport' type in tuple.%d as tcp or rdma " + "*********\n", i); + error_report_err(local_err); + goto out; + } else { + if (ret == GLUSTER_TRANSPORT_TCP) { + gconf[i].transport = (char *)"tcp"; + } else { + gconf[i].transport = (char *)"rdma"; + } + } + 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); + } + *pgconf = gconf; + return num_servers; + +out: + ret = -EINVAL; + errno = -ret; + qemu_opts_del(opts); + return ret; +} + +static struct glfs *qemu_gluster_opts_init(GlusterConf **pgconf, + QDict *options, Error **errp) +{ + struct glfs *glfs = NULL; + int num_servers = 0; + + num_servers = qemu_gluster_parseopts(pgconf, options); + if (num_servers < 1) { + error_setg(errp, "\n" + "\n#Usage1:\n" + "-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); + if (!glfs) { + goto out; + } + return glfs; + +out: + return NULL; +} + static void qemu_gluster_complete_aio(void *opaque) { GlusterAIOCB *acb = (GlusterAIOCB *)opaque; @@ -254,20 +601,6 @@ static void gluster_finish_aiocb(struct glfs_fd *fd, ssize_t ret, void *arg) qemu_bh_schedule(acb->bh); } -/* TODO Convert to fine grained options */ -static QemuOptsList runtime_opts = { - .name = "gluster", - .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), - .desc = { - { - .name = "filename", - .type = QEMU_OPT_STRING, - .help = "URL to the gluster image", - }, - { /* end of list */ } - }, -}; - static void qemu_gluster_parse_flags(int bdrv_flags, int *open_flags) { assert(open_flags != NULL); @@ -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; + 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); + } 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); + } 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; @@ -718,7 +1058,7 @@ static BlockDriver bdrv_gluster = { .format_name = "gluster", .protocol_name = "gluster", .instance_size = sizeof(BDRVGlusterState), - .bdrv_needs_filename = true, + .bdrv_needs_filename = false, .bdrv_file_open = qemu_gluster_open, .bdrv_reopen_prepare = qemu_gluster_reopen_prepare, .bdrv_reopen_commit = qemu_gluster_reopen_commit, @@ -745,7 +1085,7 @@ static BlockDriver bdrv_gluster_tcp = { .format_name = "gluster", .protocol_name = "gluster+tcp", .instance_size = sizeof(BDRVGlusterState), - .bdrv_needs_filename = true, + .bdrv_needs_filename = false, .bdrv_file_open = qemu_gluster_open, .bdrv_reopen_prepare = qemu_gluster_reopen_prepare, .bdrv_reopen_commit = qemu_gluster_reopen_commit, @@ -799,7 +1139,7 @@ static BlockDriver bdrv_gluster_rdma = { .format_name = "gluster", .protocol_name = "gluster+rdma", .instance_size = sizeof(BDRVGlusterState), - .bdrv_needs_filename = true, + .bdrv_needs_filename = false, .bdrv_file_open = qemu_gluster_open, .bdrv_reopen_prepare = qemu_gluster_reopen_prepare, .bdrv_reopen_commit = qemu_gluster_reopen_commit, diff --git a/qapi/block-core.json b/qapi/block-core.json index bb2189e..8aedef8 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1380,10 +1380,10 @@ ## { 'enum': 'BlockdevDriver', 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device', - 'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'parallels', - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx', - 'vmdk', 'vpc', 'vvfat' ] } + 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom', + 'host_device', 'host_floppy', 'http', 'https', 'null-aio', + 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', + 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } ## # @BlockdevOptionsBase @@ -1794,6 +1794,56 @@ '*read-pattern': 'QuorumReadPattern' } } ## +# @GlusterTransport +# +# An enumeration of Gluster transport type +# +# @tcp: TCP - Transmission Control Protocol +# +# @rdma: RDMA - Remote direct memory access +# +# Since: 2.5 +## +{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'rdma' ] } + +## +# @GlusterTuple +# +# Gluster tuple set +# +# @server: server address (hostname/ipv4/ipv6 addresses) +# +# @port: port number on which glusterd is listening. (default 24007) +# +# @transport: #transport type used to connect to gluster management daemon +# it can be tcp|rdma (default 'tcp') +# +# Since: 2.5 +## +{ 'struct': 'GlusterTuple', + 'data': { 'server': 'str', + '*port': 'int', + '*transport': 'GlusterTransport' } } + +## +# @BlockdevOptionsGluster +# +# Driver specific block device options for Gluster +# +# @volname: name of gluster volume where our VM image resides +# +# @image-path: absolute path to image file in gluster volume +# +# @volfile-servers: holds multiple tuples of {server, transport, port} +# +# Since: 2.5 +## +{ 'struct': 'BlockdevOptionsGluster', + 'data': { 'volname': 'str', + 'image-path': 'str', + 'volfile-servers': [ 'GlusterTuple' ] } } + +## # @BlockdevOptions # # Options for creating a block device. @@ -1813,7 +1863,7 @@ 'file': 'BlockdevOptionsFile', 'ftp': 'BlockdevOptionsFile', 'ftps': 'BlockdevOptionsFile', -# TODO gluster: Wait for structured options + 'gluster': 'BlockdevOptionsGluster', 'host_cdrom': 'BlockdevOptionsFile', 'host_device':'BlockdevOptionsFile', 'host_floppy':'BlockdevOptionsFile',
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, [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"}] } }' 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) 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 <kwolf@redhat.com> and "Deepak C Shetty" <deepakcs@redhat.com> for inputs and all their support Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- v1: multiple server addresses but common port number and transport type pattern: URI syntax with query (?) delimitor syntax: file=gluster[+transport-type]://server1:24007/testvol/a.img\ ?backup-volfile-servers=server2&backup-volfile-servers=server3 v2: multiple server addresses each have their own port number, but all use common transport type pattern: URI syntax with query (?) delimiter syntax: file=gluster[+transport-type]://[server[:port]]/testvol/a.img\ [?backup-volfile-servers=server1[:port]\ &backup-volfile-servers=server2[:port]] v3: multiple server addresses each have their own port number and transport type pattern: changed to json syntax: 'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol", "image-path":"/path/a.qcow2","backup-volfile-servers": [{"server":"1.2.3.4","port":"24007","transport":"tcp"}, {"server":"4.5.6.7","port":"24008","transport":"rdma"}] } }' v4, v5: address comments from "Eric Blake" <eblake@redhat.com> v5: renamed option "backup-volfile-servers" as "volfile-servers" pattern: json syntax: '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"}] } }' --- block/gluster.c | 428 +++++++++++++++++++++++++++++++++++++++++++++------ qapi/block-core.json | 60 +++++++- 2 files changed, 439 insertions(+), 49 deletions(-)