| Message ID | 1468418269-13490-5-git-send-email-prasanna.kalever@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Interface and error message review only. Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > 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: > > Currently VM Image on gluster volume is specified like this: > > file=gluster[+tcp]://host[:port]/testvol/a.img > > Assuming we have three hosts in trusted pool with replica 3 volume > in action and unfortunately host (mentioned in the command above) went down > for some reason, since the volume is replica 3 we now have other 2 hosts > active from which we can boot the VM. > > But currently there is no mechanism to pass the other 2 gluster host > addresses to qemu. Awkward. Perhaps: Say we have three hosts in a trusted pool with replica 3 volume in action. When the host mentioned in the command above goes down for some reason, the other two hosts are still available. But there's currently no way to tell QEMU about them. > 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, > volume=testvol,path=/path/a.raw, > server.0.host=1.2.3.4, > [server.0.port=24007,] > [server.0.transport=tcp,] > server.1.host=5.6.7.8, > [server.1.port=24008,] > [server.1.transport=rdma,] Don't forget to update this line when you drop transport 'rdma'. More of the same below. > server.2.host=/var/run/glusterd.socket, > server.2.transport=unix ... > > Pattern II: > 'json:{"driver":"qcow2","file":{"driver":"gluster", > "volume":"testvol","path":"/path/a.qcow2", > "server":[{tuple0},{tuple1}, ...{tupleN}]}}' Suggest to add -drive here, for symmetry with pattern I. JSON calls the things in { ... } objects, not tuples. Let's stick to JSON terminology: [{object0},{object1}, ...]. But I think spelling things out to a similar degree as in pattern I would be clearer: -drive 'json:{"driver": "qcow2", "file": { "driver": "gluster", "volume": "testvol", "path": "/path/a.qcow2", "server": [ { "host": "1.2.3.4", "port": 24007, "transport": "tcp" }, { "host": "5.6.7.8" ... }, ... ] } ... }' > driver => 'gluster' (protocol name) > volume => name of gluster volume where our VM image resides > path => absolute path of image in gluster volume > > {tuple} => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]} > > host => host address (hostname/ipv4/ipv6 addresses/socket path) > 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|unix (default 'tcp') > > Examples: > 1. > -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, > file.server.2.host=/var/run/glusterd.socket > file.server.1.transport=unix > 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"}, > {"host":"/var/run/glusterd.socket","transport":"unix"}] } }' Ah, working examples. Good! No need to expand your abbreviated description of pattern II then, just say object instead of tuple. But if you prefer to expand it, go ahead, your choice. > This patch gives a mechanism to provide all the server addresses, which are in > replica set, so in case host1 is down VM can still boot from any of the > active hosts. > > This is equivalent to the backup-volfile-servers option supported by > mount.glusterfs (FUSE way of mounting gluster volume) > > 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> > --- > block/gluster.c | 290 ++++++++++++++++++++++++++++++++++++++++++++------- > qapi/block-core.json | 4 +- > 2 files changed, 255 insertions(+), 39 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 40dcbc1..41046f0 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -12,8 +12,15 @@ > #include "block/block_int.h" > #include "qapi/error.h" > #include "qemu/uri.h" > +#include "qemu/error-report.h" > > #define GLUSTER_OPT_FILENAME "filename" > +#define GLUSTER_OPT_VOLUME "volume" > +#define GLUSTER_OPT_PATH "path" > +#define GLUSTER_OPT_HOST "host" > +#define GLUSTER_OPT_PORT "port" > +#define GLUSTER_OPT_TRANSPORT "transport" > +#define GLUSTER_OPT_SERVER_PATTERN "server." > #define GLUSTER_OPT_DEBUG "debug" > #define GLUSTER_DEFAULT_PORT 24007 > #define GLUSTER_DEBUG_DEFAULT 4 > @@ -82,6 +89,46 @@ static QemuOptsList runtime_opts = { > }, > }; > > +static QemuOptsList runtime_json_opts = { > + .name = "gluster_json", > + .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head), > + .desc = { > + { > + .name = GLUSTER_OPT_VOLUME, > + .type = QEMU_OPT_STRING, > + .help = "name of gluster volume where VM image resides", > + }, > + { > + .name = GLUSTER_OPT_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_HOST, > + .type = QEMU_OPT_STRING, > + .help = "host address (hostname/ipv4/ipv6 addresses/socket path)", > + }, > + { > + .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 'tcp'|'rdma'|'unix' (default 'tcp')", > + }, > + { /* end of list */ } > + }, > +}; > > static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) > { > @@ -148,6 +195,7 @@ static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) > static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, > const char *filename) > { > + GlusterServer *gsconf; > URI *uri; > QueryParams *qp = NULL; > bool is_unix = false; > @@ -158,23 +206,24 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, > return -EINVAL; > } > > - gconf->server = g_new0(GlusterServer, 1); > + gconf->server = g_new0(GlusterServerList, 1); > + gconf->server->value = gsconf = g_new0(GlusterServer, 1); > > /* transport */ > if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { > - gconf->server->transport = GLUSTER_TRANSPORT_TCP; > + gsconf->transport = GLUSTER_TRANSPORT_TCP; > } else if (!strcmp(uri->scheme, "gluster+tcp")) { > - gconf->server->transport = GLUSTER_TRANSPORT_TCP; > + gsconf->transport = GLUSTER_TRANSPORT_TCP; > } else if (!strcmp(uri->scheme, "gluster+unix")) { > - gconf->server->transport = GLUSTER_TRANSPORT_UNIX; > + gsconf->transport = GLUSTER_TRANSPORT_UNIX; > is_unix = true; > } else if (!strcmp(uri->scheme, "gluster+rdma")) { > - gconf->server->transport = GLUSTER_TRANSPORT_RDMA; > + gsconf->transport = GLUSTER_TRANSPORT_RDMA; > } else { > ret = -EINVAL; > goto out; > } > - gconf->server->has_transport = true; > + gsconf->has_transport = true; > > ret = parse_volume_options(gconf, uri->path); > if (ret < 0) { > @@ -196,15 +245,15 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, > ret = -EINVAL; > goto out; > } > - gconf->server->host = g_strdup(qp->p[0].value); > + gsconf->host = g_strdup(qp->p[0].value); > } else { > - gconf->server->host = g_strdup(uri->server ? uri->server : "localhost"); > + gsconf->host = g_strdup(uri->server ? uri->server : "localhost"); > if (uri->port) { > - gconf->server->port = uri->port; > + gsconf->port = uri->port; > } else { > - gconf->server->port = GLUSTER_DEFAULT_PORT; > + gsconf->port = GLUSTER_DEFAULT_PORT; > } > - gconf->server->has_port = true; > + gsconf->has_port = true; > } > > out: > @@ -215,31 +264,26 @@ out: > return ret; > } > > -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, > - const char *filename, Error **errp) > +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, > + Error **errp) > { > - struct glfs *glfs = NULL; > + struct glfs *glfs; > int ret; > int old_errno; > - > - ret = qemu_gluster_parseuri(gconf, filename); > - if (ret < 0) { > - error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" > - "volume/path[?socket=...]"); > - errno = -ret; > - goto out; > - } > + GlusterServerList *server = NULL; > > glfs = glfs_new(gconf->volume); > if (!glfs) { > goto out; > } > > - ret = glfs_set_volfile_server(glfs, > - GlusterTransport_lookup[gconf->server->transport], > - gconf->server->host, gconf->server->port); > - if (ret < 0) { > - goto out; > + for (server = gconf->server; server; server = server->next) { > + ret = glfs_set_volfile_server(glfs, > + GlusterTransport_lookup[server->value->transport], > + server->value->host, server->value->port); > + if (ret < 0) { > + goto out; > + } > } > > ret = glfs_set_logging(glfs, "-", gconf->debug_level); > @@ -249,11 +293,13 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, > > ret = glfs_init(glfs); > if (ret) { > + error_report("Gluster connection for 'volume: %s and path: %s': ", > + gconf->volume, gconf->path); > + for (server = gconf->server; server; server = server->next) { > + error_report("failed for host %s", server->value->host); > + } > error_setg_errno(errp, errno, > - "Gluster connection failed for host=%s port=%d " > - "volume=%s path=%s transport=%s", gconf->server->host, > - gconf->server->port, gconf->volume, gconf->path, > - GlusterTransport_lookup[gconf->server->transport]); > + "Please refer to gluster logs for more info"); error_setg() and error_report() to not mix. error_report() reports to stderr or the current monitor. Okay in some contexts, wrong in others. Okay contexts include QEMU startup, and HMP commands. Wrong contexts include QMP commands, and functions with an Error **errp parameter. It's wrong in QMP commands, because the error must be sent via QMP. It's wrong in functions taking errp, because how to handle errors is the caller's decision. In particular, the caller may decide to ignore the error. The function mustn't babble about it with error_report(). In particular, when this runs in QMP context, the error sent to QMP will just be "Please refer to gluster logs for more info: %s", strerror(errno) and the real error "Gluster connection for ..." gets barfed to stderr. Aside: when error_report() is correct, use exactly one per error. You may add information with error_printf(). You should call just error_setg_errno() here, with the full error message. Since the error message consists of a fixed part followed by variable number of "failed for host" parts, you have to construct it dynamically. Perhaps with a series of g_strdup_printf(). Inefficient, but good enough for an error path. "Please refer to gluster logs for more info" is a hint. The function to add hints to an Error is error_append_hint(). Search the tree for examples. Finally, do test your error messages to make sure they come out readable! > > /* glfs_init sometimes doesn't set errno although docs suggest that */ > if (errno == 0) errno = EINVAL; Well, if that's the case, shouldn't you do this before you pass (possibly zero) errno to error_setg_errno()? > @@ -272,6 +318,176 @@ out: > return NULL; > } > > +static int qapi_enum_parse(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 i; > +} > + > +/* > + * Convert the json formatted command line into qapi. > +*/ > +static int qemu_gluster_parsejson(BlockdevOptionsGluster *gconf, "parsejson" isn't a word. "parse_json" would be two :) > + QDict *options) > +{ > + QemuOpts *opts; > + GlusterServer *gsconf; > + GlusterServerList *curr = NULL; > + QDict *backing_options = NULL; > + Error *local_err = NULL; > + char *str = NULL; > + const char *ptr; > + size_t num_servers; > + int i; > + > + /* create opts info from runtime_json_opts list */ > + opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + goto out; > + } > + > + num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN); > + if (num_servers < 1) { > + error_setg(&local_err, "qemu_gluster: please provide 'server' " > + "option with valid fields in array of tuples"); This isn't an error message, it's instructions what to do. Such instructions can be useful when they're correct, but they can't replace an error message. The error message should state what's wrong. No less, no more. Moreover, avoid prefixes like "qemu_gluster:". Usually, the fact that this is about Gluster is obvious. When it isn't, providing context is the caller's job. > + goto out; > + } > + > + ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME); > + if (!ptr) { > + error_setg(&local_err, "qemu_gluster: please provide 'volume' " > + "option"); Not an error message. > + goto out; > + } > + gconf->volume = g_strdup(ptr); > + > + ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH); > + if (!ptr) { > + error_setg(&local_err, "qemu_gluster: please provide 'path' " > + "option"); Not an error message. > + goto out; > + } > + gconf->path = g_strdup(ptr); > + > + qemu_opts_del(opts); > + > + /* create opts info from runtime_tuple_opts list */ > + for (i = 0; i < num_servers; i++) { > + opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort); > + str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i); > + qdict_extract_subqdict(options, &backing_options, str); > + qemu_opts_absorb_qdict(opts, backing_options, &local_err); > + if (local_err) { > + goto out; > + } > + qdict_del(backing_options, str); > + g_free(str); > + str = NULL; > + > + ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST); > + if (!ptr) { > + error_setg(&local_err, "qemu_gluster: server.{tuple.%d} " > + "requires 'host' option", i); > + goto out; > + } > + > + gsconf = g_new0(GlusterServer, 1); > + > + gsconf->host = g_strdup(ptr); > + > + ptr = qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT); > + /* check whether transport type specified in json command is valid */ > + gsconf->transport = qapi_enum_parse(ptr); > + if (gsconf->transport == GLUSTER_TRANSPORT__MAX) { > + error_setg(&local_err, "qemu_gluster: please set 'transport'" > + " type in tuple.%d as tcp|rdma|unix", i); Not an error message. > + qapi_free_GlusterServer(gsconf); > + goto out; > + } > + gsconf->has_transport = true; > + > + /* we don't need port number for unix transport */ > + if (gsconf->transport != GLUSTER_TRANSPORT_UNIX) { > + gsconf->port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT, > + GLUSTER_DEFAULT_PORT); > + gsconf->has_port = true; > + } > + > + if (gconf->server == NULL) { > + gconf->server = g_new0(GlusterServerList, 1); > + gconf->server->value = gsconf; > + curr = gconf->server; > + } else { > + curr->next = g_new0(GlusterServerList, 1); > + curr->next->value = gsconf; > + curr = curr->next; > + } > + > + qemu_opts_del(opts); > + } > + > + return 0; > + > +out: > + error_report_err(local_err); Aha, qemu_gluster_parsejson() reports errors with error_report_err(). > + qemu_opts_del(opts); > + if (str) { > + qdict_del(backing_options, str); > + g_free(str); > + } > + errno = EINVAL; > + return -errno; > +} > + > +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, > + const char *filename, > + QDict *options, Error **errp) > +{ > + int ret; > + > + if (filename) { > + ret = qemu_gluster_parseuri(gconf, filename); > + if (ret < 0) { > + error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" > + "volume/path[?socket=...]"); Not an error message. > + errno = -ret; > + return NULL; > + } > + } else { > + ret = qemu_gluster_parsejson(gconf, options); Anti-pattern: function that reports errors with error_report() or similar called from a function with an Error **errp parameter. In QMP context, qemu_gluster_parsejson() writes the real error to stderr, and QMP sends back only the useless "Usage: ..." error below. qemu_gluster_parsejson() needs to return the Error instead, via errp parameter. Then you can error_propagate() here, and... > + if (ret < 0) { > + error_setg(errp, "Usage: " > + "-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,]" > + "file.server.2.host=/var/run/glusterd.socket," > + "file.server.2.transport=unix ..."); ... drop this usage hint. > + errno = -ret; > + return NULL; > + } > + } > + > + return qemu_gluster_glfs_init(gconf, errp); > +} > + > static void qemu_gluster_complete_aio(void *opaque) > { > GlusterAIOCB *acb = (GlusterAIOCB *)opaque; > @@ -371,7 +587,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, > gconf = g_new0(BlockdevOptionsGluster, 1); > gconf->debug_level = s->debug_level; > gconf->has_debug_level = true; > - s->glfs = qemu_gluster_init(gconf, filename, errp); > + s->glfs = qemu_gluster_init(gconf, filename, options, errp); > if (!s->glfs) { > ret = -errno; > goto out; > @@ -442,7 +658,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, > gconf = g_new0(BlockdevOptionsGluster, 1); > gconf->debug_level = s->debug_level; > gconf->has_debug_level = true; > - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp); > + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); > if (reop_s->glfs == NULL) { > ret = -errno; > goto exit; > @@ -589,7 +805,7 @@ static int qemu_gluster_create(const char *filename, > } > gconf->has_debug_level = true; > > - glfs = qemu_gluster_init(gconf, filename, errp); > + glfs = qemu_gluster_init(gconf, filename, NULL, errp); > if (!glfs) { > ret = -errno; > goto out; > @@ -969,7 +1185,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, > @@ -997,7 +1213,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, > @@ -1053,7 +1269,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 f8b38bb..52cee7d 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2077,7 +2077,7 @@ > # > # @path: absolute path to image file in gluster volume > # > -# @server: gluster server description > +# @server: one or more gluster server descriptions > # > # @debug_level: #libgfapi log level (default '4' which is Error) > # > @@ -2086,7 +2086,7 @@ > { 'struct': 'BlockdevOptionsGluster', > 'data': { 'volume': 'str', > 'path': 'str', > - 'server': 'GlusterServer', > + 'server': [ 'GlusterServer' ], > '*debug_level': 'int' } } > > ## Incompatible change, okay because the thing got added only in the previous patch.
On Thu, Jul 14, 2016 at 5:35 PM, Markus Armbruster <armbru@redhat.com> wrote: > Interface and error message review only. > > Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > >> 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: >> >> Currently VM Image on gluster volume is specified like this: >> >> file=gluster[+tcp]://host[:port]/testvol/a.img >> >> Assuming we have three hosts in trusted pool with replica 3 volume >> in action and unfortunately host (mentioned in the command above) went down >> for some reason, since the volume is replica 3 we now have other 2 hosts >> active from which we can boot the VM. >> >> But currently there is no mechanism to pass the other 2 gluster host >> addresses to qemu. > > Awkward. Perhaps: > > Say we have three hosts in a trusted pool with replica 3 volume > in action. When the host mentioned in the command above goes down > for some reason, the other two hosts are still available. But there's > currently no way to tell QEMU about them. Will incorporate in v19 > >> 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, >> volume=testvol,path=/path/a.raw, >> server.0.host=1.2.3.4, >> [server.0.port=24007,] >> [server.0.transport=tcp,] >> server.1.host=5.6.7.8, >> [server.1.port=24008,] >> [server.1.transport=rdma,] > > Don't forget to update this line when you drop transport 'rdma'. More > of the same below. > >> server.2.host=/var/run/glusterd.socket, >> server.2.transport=unix ... >> >> Pattern II: >> 'json:{"driver":"qcow2","file":{"driver":"gluster", >> "volume":"testvol","path":"/path/a.qcow2", >> "server":[{tuple0},{tuple1}, ...{tupleN}]}}' > > Suggest to add -drive here, for symmetry with pattern I. > > JSON calls the things in { ... } objects, not tuples. Let's stick to > JSON terminology: [{object0},{object1}, ...]. But I think spelling > things out to a similar degree as in pattern I would be clearer: > > -drive 'json:{"driver": "qcow2", > "file": { "driver": "gluster", > "volume": "testvol", > "path": "/path/a.qcow2", > "server": [ > { "host": "1.2.3.4", > "port": 24007, > "transport": "tcp" }, > { "host": "5.6.7.8" > ... }, > ... ] } > ... }' IMO, this doesn't work, I have given a try. > >> driver => 'gluster' (protocol name) >> volume => name of gluster volume where our VM image resides >> path => absolute path of image in gluster volume >> >> {tuple} => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]} >> >> host => host address (hostname/ipv4/ipv6 addresses/socket path) >> 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|unix (default 'tcp') >> >> Examples: >> 1. >> -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, >> file.server.2.host=/var/run/glusterd.socket >> file.server.1.transport=unix >> 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"}, >> {"host":"/var/run/glusterd.socket","transport":"unix"}] } }' > > Ah, working examples. Good! No need to expand your abbreviated > description of pattern II then, just say object instead of tuple. But if > you prefer to expand it, go ahead, your choice. > >> This patch gives a mechanism to provide all the server addresses, which are in >> replica set, so in case host1 is down VM can still boot from any of the >> active hosts. >> >> This is equivalent to the backup-volfile-servers option supported by >> mount.glusterfs (FUSE way of mounting gluster volume) >> >> 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> >> --- >> block/gluster.c | 290 ++++++++++++++++++++++++++++++++++++++++++++------- >> qapi/block-core.json | 4 +- >> 2 files changed, 255 insertions(+), 39 deletions(-) >> >> diff --git a/block/gluster.c b/block/gluster.c >> index 40dcbc1..41046f0 100644 >> --- a/block/gluster.c >> +++ b/block/gluster.c >> @@ -12,8 +12,15 @@ >> #include "block/block_int.h" >> #include "qapi/error.h" >> #include "qemu/uri.h" >> +#include "qemu/error-report.h" >> >> #define GLUSTER_OPT_FILENAME "filename" >> +#define GLUSTER_OPT_VOLUME "volume" >> +#define GLUSTER_OPT_PATH "path" >> +#define GLUSTER_OPT_HOST "host" >> +#define GLUSTER_OPT_PORT "port" >> +#define GLUSTER_OPT_TRANSPORT "transport" >> +#define GLUSTER_OPT_SERVER_PATTERN "server." >> #define GLUSTER_OPT_DEBUG "debug" >> #define GLUSTER_DEFAULT_PORT 24007 >> #define GLUSTER_DEBUG_DEFAULT 4 >> @@ -82,6 +89,46 @@ static QemuOptsList runtime_opts = { >> }, >> }; >> >> +static QemuOptsList runtime_json_opts = { >> + .name = "gluster_json", >> + .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head), >> + .desc = { >> + { >> + .name = GLUSTER_OPT_VOLUME, >> + .type = QEMU_OPT_STRING, >> + .help = "name of gluster volume where VM image resides", >> + }, >> + { >> + .name = GLUSTER_OPT_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_HOST, >> + .type = QEMU_OPT_STRING, >> + .help = "host address (hostname/ipv4/ipv6 addresses/socket path)", >> + }, >> + { >> + .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 'tcp'|'rdma'|'unix' (default 'tcp')", >> + }, >> + { /* end of list */ } >> + }, >> +}; >> >> static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) >> { >> @@ -148,6 +195,7 @@ static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) >> static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, >> const char *filename) >> { >> + GlusterServer *gsconf; >> URI *uri; >> QueryParams *qp = NULL; >> bool is_unix = false; >> @@ -158,23 +206,24 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, >> return -EINVAL; >> } >> >> - gconf->server = g_new0(GlusterServer, 1); >> + gconf->server = g_new0(GlusterServerList, 1); >> + gconf->server->value = gsconf = g_new0(GlusterServer, 1); >> >> /* transport */ >> if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { >> - gconf->server->transport = GLUSTER_TRANSPORT_TCP; >> + gsconf->transport = GLUSTER_TRANSPORT_TCP; >> } else if (!strcmp(uri->scheme, "gluster+tcp")) { >> - gconf->server->transport = GLUSTER_TRANSPORT_TCP; >> + gsconf->transport = GLUSTER_TRANSPORT_TCP; >> } else if (!strcmp(uri->scheme, "gluster+unix")) { >> - gconf->server->transport = GLUSTER_TRANSPORT_UNIX; >> + gsconf->transport = GLUSTER_TRANSPORT_UNIX; >> is_unix = true; >> } else if (!strcmp(uri->scheme, "gluster+rdma")) { >> - gconf->server->transport = GLUSTER_TRANSPORT_RDMA; >> + gsconf->transport = GLUSTER_TRANSPORT_RDMA; >> } else { >> ret = -EINVAL; >> goto out; >> } >> - gconf->server->has_transport = true; >> + gsconf->has_transport = true; >> >> ret = parse_volume_options(gconf, uri->path); >> if (ret < 0) { >> @@ -196,15 +245,15 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, >> ret = -EINVAL; >> goto out; >> } >> - gconf->server->host = g_strdup(qp->p[0].value); >> + gsconf->host = g_strdup(qp->p[0].value); >> } else { >> - gconf->server->host = g_strdup(uri->server ? uri->server : "localhost"); >> + gsconf->host = g_strdup(uri->server ? uri->server : "localhost"); >> if (uri->port) { >> - gconf->server->port = uri->port; >> + gsconf->port = uri->port; >> } else { >> - gconf->server->port = GLUSTER_DEFAULT_PORT; >> + gsconf->port = GLUSTER_DEFAULT_PORT; >> } >> - gconf->server->has_port = true; >> + gsconf->has_port = true; >> } >> >> out: >> @@ -215,31 +264,26 @@ out: >> return ret; >> } >> >> -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, >> - const char *filename, Error **errp) >> +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, >> + Error **errp) >> { >> - struct glfs *glfs = NULL; >> + struct glfs *glfs; >> int ret; >> int old_errno; >> - >> - ret = qemu_gluster_parseuri(gconf, filename); >> - if (ret < 0) { >> - error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" >> - "volume/path[?socket=...]"); >> - errno = -ret; >> - goto out; >> - } >> + GlusterServerList *server = NULL; >> >> glfs = glfs_new(gconf->volume); >> if (!glfs) { >> goto out; >> } >> >> - ret = glfs_set_volfile_server(glfs, >> - GlusterTransport_lookup[gconf->server->transport], >> - gconf->server->host, gconf->server->port); >> - if (ret < 0) { >> - goto out; >> + for (server = gconf->server; server; server = server->next) { >> + ret = glfs_set_volfile_server(glfs, >> + GlusterTransport_lookup[server->value->transport], >> + server->value->host, server->value->port); >> + if (ret < 0) { >> + goto out; >> + } >> } >> >> ret = glfs_set_logging(glfs, "-", gconf->debug_level); >> @@ -249,11 +293,13 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, >> >> ret = glfs_init(glfs); >> if (ret) { >> + error_report("Gluster connection for 'volume: %s and path: %s': ", >> + gconf->volume, gconf->path); >> + for (server = gconf->server; server; server = server->next) { >> + error_report("failed for host %s", server->value->host); >> + } >> error_setg_errno(errp, errno, >> - "Gluster connection failed for host=%s port=%d " >> - "volume=%s path=%s transport=%s", gconf->server->host, >> - gconf->server->port, gconf->volume, gconf->path, >> - GlusterTransport_lookup[gconf->server->transport]); >> + "Please refer to gluster logs for more info"); > > error_setg() and error_report() to not mix. > > error_report() reports to stderr or the current monitor. Okay in some > contexts, wrong in others. Okay contexts include QEMU startup, and HMP > commands. Wrong contexts include QMP commands, and functions with an > Error **errp parameter. > > It's wrong in QMP commands, because the error must be sent via QMP. > > It's wrong in functions taking errp, because how to handle errors is the > caller's decision. In particular, the caller may decide to ignore the > error. The function mustn't babble about it with error_report(). > > In particular, when this runs in QMP context, the error sent to QMP will > just be > > "Please refer to gluster logs for more info: %s", strerror(errno) > > and the real error "Gluster connection for ..." gets barfed to > stderr. > > Aside: when error_report() is correct, use exactly one per error. You > may add information with error_printf(). > > You should call just error_setg_errno() here, with the full error > message. Since the error message consists of a fixed part followed by > variable number of "failed for host" parts, you have to construct it > dynamically. Perhaps with a series of g_strdup_printf(). Inefficient, > but good enough for an error path. > > "Please refer to gluster logs for more info" is a hint. The function to > add hints to an Error is error_append_hint(). Search the tree for > examples. > > Finally, do test your error messages to make sure they come out > readable! That's awesome, thanks for the keen theory I hope that I will not disappoint you in v19. > >> >> /* glfs_init sometimes doesn't set errno although docs suggest that */ >> if (errno == 0) > errno = EINVAL; > > Well, if that's the case, shouldn't you do this before you pass > (possibly zero) errno to error_setg_errno()? > >> @@ -272,6 +318,176 @@ out: >> return NULL; >> } >> >> +static int qapi_enum_parse(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 i; >> +} >> + >> +/* >> + * Convert the json formatted command line into qapi. >> +*/ >> +static int qemu_gluster_parsejson(BlockdevOptionsGluster *gconf, > > "parsejson" isn't a word. "parse_json" would be two :) taken care. > >> + QDict *options) >> +{ >> + QemuOpts *opts; >> + GlusterServer *gsconf; >> + GlusterServerList *curr = NULL; >> + QDict *backing_options = NULL; >> + Error *local_err = NULL; >> + char *str = NULL; >> + const char *ptr; >> + size_t num_servers; >> + int i; >> + >> + /* create opts info from runtime_json_opts list */ >> + opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); >> + qemu_opts_absorb_qdict(opts, options, &local_err); >> + if (local_err) { >> + goto out; >> + } >> + >> + num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN); >> + if (num_servers < 1) { >> + error_setg(&local_err, "qemu_gluster: please provide 'server' " >> + "option with valid fields in array of tuples"); > > This isn't an error message, it's instructions what to do. Such > instructions can be useful when they're correct, but they can't replace > an error message. The error message should state what's wrong. No > less, no more. > > Moreover, avoid prefixes like "qemu_gluster:". Usually, the fact that > this is about Gluster is obvious. When it isn't, providing context is > the caller's job. sure, I have modified accordingly. > >> + goto out; >> + } >> + >> + ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME); >> + if (!ptr) { >> + error_setg(&local_err, "qemu_gluster: please provide 'volume' " >> + "option"); > > Not an error message. > >> + goto out; >> + } >> + gconf->volume = g_strdup(ptr); >> + >> + ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH); >> + if (!ptr) { >> + error_setg(&local_err, "qemu_gluster: please provide 'path' " >> + "option"); > > Not an error message. > >> + goto out; >> + } >> + gconf->path = g_strdup(ptr); >> + >> + qemu_opts_del(opts); >> + >> + /* create opts info from runtime_tuple_opts list */ >> + for (i = 0; i < num_servers; i++) { >> + opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort); >> + str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i); >> + qdict_extract_subqdict(options, &backing_options, str); >> + qemu_opts_absorb_qdict(opts, backing_options, &local_err); >> + if (local_err) { >> + goto out; >> + } >> + qdict_del(backing_options, str); >> + g_free(str); >> + str = NULL; >> + >> + ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST); >> + if (!ptr) { >> + error_setg(&local_err, "qemu_gluster: server.{tuple.%d} " >> + "requires 'host' option", i); >> + goto out; >> + } >> + >> + gsconf = g_new0(GlusterServer, 1); >> + >> + gsconf->host = g_strdup(ptr); >> + >> + ptr = qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT); >> + /* check whether transport type specified in json command is valid */ >> + gsconf->transport = qapi_enum_parse(ptr); >> + if (gsconf->transport == GLUSTER_TRANSPORT__MAX) { >> + error_setg(&local_err, "qemu_gluster: please set 'transport'" >> + " type in tuple.%d as tcp|rdma|unix", i); > > Not an error message. > >> + qapi_free_GlusterServer(gsconf); >> + goto out; >> + } >> + gsconf->has_transport = true; >> + >> + /* we don't need port number for unix transport */ >> + if (gsconf->transport != GLUSTER_TRANSPORT_UNIX) { >> + gsconf->port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT, >> + GLUSTER_DEFAULT_PORT); >> + gsconf->has_port = true; >> + } >> + >> + if (gconf->server == NULL) { >> + gconf->server = g_new0(GlusterServerList, 1); >> + gconf->server->value = gsconf; >> + curr = gconf->server; >> + } else { >> + curr->next = g_new0(GlusterServerList, 1); >> + curr->next->value = gsconf; >> + curr = curr->next; >> + } >> + >> + qemu_opts_del(opts); >> + } >> + >> + return 0; >> + >> +out: >> + error_report_err(local_err); > > Aha, qemu_gluster_parsejson() reports errors with error_report_err(). oops...! > >> + qemu_opts_del(opts); >> + if (str) { >> + qdict_del(backing_options, str); >> + g_free(str); >> + } >> + errno = EINVAL; >> + return -errno; >> +} >> + >> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, >> + const char *filename, >> + QDict *options, Error **errp) >> +{ >> + int ret; >> + >> + if (filename) { >> + ret = qemu_gluster_parseuri(gconf, filename); >> + if (ret < 0) { >> + error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" >> + "volume/path[?socket=...]"); > > Not an error message. > >> + errno = -ret; >> + return NULL; >> + } >> + } else { >> + ret = qemu_gluster_parsejson(gconf, options); > > Anti-pattern: function that reports errors with error_report() or > similar called from a function with an Error **errp parameter. > > In QMP context, qemu_gluster_parsejson() writes the real error to > stderr, and QMP sends back only the useless "Usage: ..." error below. > > qemu_gluster_parsejson() needs to return the Error instead, via errp > parameter. Then you can error_propagate() here, and... > >> + if (ret < 0) { >> + error_setg(errp, "Usage: " >> + "-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,]" >> + "file.server.2.host=/var/run/glusterd.socket," >> + "file.server.2.transport=unix ..."); > > ... drop this usage hint. I still think this is needed with a slight modified version. Thanks Markus! -- Prasanna > >> + errno = -ret; >> + return NULL; >> + } >> + } >> + >> + return qemu_gluster_glfs_init(gconf, errp); >> +} >> + >> static void qemu_gluster_complete_aio(void *opaque) >> { >> GlusterAIOCB *acb = (GlusterAIOCB *)opaque; >> @@ -371,7 +587,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, >> gconf = g_new0(BlockdevOptionsGluster, 1); >> gconf->debug_level = s->debug_level; >> gconf->has_debug_level = true; >> - s->glfs = qemu_gluster_init(gconf, filename, errp); >> + s->glfs = qemu_gluster_init(gconf, filename, options, errp); >> if (!s->glfs) { >> ret = -errno; >> goto out; >> @@ -442,7 +658,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, >> gconf = g_new0(BlockdevOptionsGluster, 1); >> gconf->debug_level = s->debug_level; >> gconf->has_debug_level = true; >> - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp); >> + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); >> if (reop_s->glfs == NULL) { >> ret = -errno; >> goto exit; >> @@ -589,7 +805,7 @@ static int qemu_gluster_create(const char *filename, >> } >> gconf->has_debug_level = true; >> >> - glfs = qemu_gluster_init(gconf, filename, errp); >> + glfs = qemu_gluster_init(gconf, filename, NULL, errp); >> if (!glfs) { >> ret = -errno; >> goto out; >> @@ -969,7 +1185,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, >> @@ -997,7 +1213,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, >> @@ -1053,7 +1269,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 f8b38bb..52cee7d 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2077,7 +2077,7 @@ >> # >> # @path: absolute path to image file in gluster volume >> # >> -# @server: gluster server description >> +# @server: one or more gluster server descriptions >> # >> # @debug_level: #libgfapi log level (default '4' which is Error) >> # >> @@ -2086,7 +2086,7 @@ >> { 'struct': 'BlockdevOptionsGluster', >> 'data': { 'volume': 'str', >> 'path': 'str', >> - 'server': 'GlusterServer', >> + 'server': [ 'GlusterServer' ], >> '*debug_level': 'int' } } >> >> ## > > Incompatible change, okay because the thing got added only in the > previous patch.
diff --git a/block/gluster.c b/block/gluster.c index 40dcbc1..41046f0 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -12,8 +12,15 @@ #include "block/block_int.h" #include "qapi/error.h" #include "qemu/uri.h" +#include "qemu/error-report.h" #define GLUSTER_OPT_FILENAME "filename" +#define GLUSTER_OPT_VOLUME "volume" +#define GLUSTER_OPT_PATH "path" +#define GLUSTER_OPT_HOST "host" +#define GLUSTER_OPT_PORT "port" +#define GLUSTER_OPT_TRANSPORT "transport" +#define GLUSTER_OPT_SERVER_PATTERN "server." #define GLUSTER_OPT_DEBUG "debug" #define GLUSTER_DEFAULT_PORT 24007 #define GLUSTER_DEBUG_DEFAULT 4 @@ -82,6 +89,46 @@ static QemuOptsList runtime_opts = { }, }; +static QemuOptsList runtime_json_opts = { + .name = "gluster_json", + .head = QTAILQ_HEAD_INITIALIZER(runtime_json_opts.head), + .desc = { + { + .name = GLUSTER_OPT_VOLUME, + .type = QEMU_OPT_STRING, + .help = "name of gluster volume where VM image resides", + }, + { + .name = GLUSTER_OPT_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_HOST, + .type = QEMU_OPT_STRING, + .help = "host address (hostname/ipv4/ipv6 addresses/socket path)", + }, + { + .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 'tcp'|'rdma'|'unix' (default 'tcp')", + }, + { /* end of list */ } + }, +}; static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) { @@ -148,6 +195,7 @@ static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path) static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, const char *filename) { + GlusterServer *gsconf; URI *uri; QueryParams *qp = NULL; bool is_unix = false; @@ -158,23 +206,24 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, return -EINVAL; } - gconf->server = g_new0(GlusterServer, 1); + gconf->server = g_new0(GlusterServerList, 1); + gconf->server->value = gsconf = g_new0(GlusterServer, 1); /* transport */ if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { - gconf->server->transport = GLUSTER_TRANSPORT_TCP; + gsconf->transport = GLUSTER_TRANSPORT_TCP; } else if (!strcmp(uri->scheme, "gluster+tcp")) { - gconf->server->transport = GLUSTER_TRANSPORT_TCP; + gsconf->transport = GLUSTER_TRANSPORT_TCP; } else if (!strcmp(uri->scheme, "gluster+unix")) { - gconf->server->transport = GLUSTER_TRANSPORT_UNIX; + gsconf->transport = GLUSTER_TRANSPORT_UNIX; is_unix = true; } else if (!strcmp(uri->scheme, "gluster+rdma")) { - gconf->server->transport = GLUSTER_TRANSPORT_RDMA; + gsconf->transport = GLUSTER_TRANSPORT_RDMA; } else { ret = -EINVAL; goto out; } - gconf->server->has_transport = true; + gsconf->has_transport = true; ret = parse_volume_options(gconf, uri->path); if (ret < 0) { @@ -196,15 +245,15 @@ static int qemu_gluster_parseuri(BlockdevOptionsGluster *gconf, ret = -EINVAL; goto out; } - gconf->server->host = g_strdup(qp->p[0].value); + gsconf->host = g_strdup(qp->p[0].value); } else { - gconf->server->host = g_strdup(uri->server ? uri->server : "localhost"); + gsconf->host = g_strdup(uri->server ? uri->server : "localhost"); if (uri->port) { - gconf->server->port = uri->port; + gsconf->port = uri->port; } else { - gconf->server->port = GLUSTER_DEFAULT_PORT; + gsconf->port = GLUSTER_DEFAULT_PORT; } - gconf->server->has_port = true; + gsconf->has_port = true; } out: @@ -215,31 +264,26 @@ out: return ret; } -static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, - const char *filename, Error **errp) +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, + Error **errp) { - struct glfs *glfs = NULL; + struct glfs *glfs; int ret; int old_errno; - - ret = qemu_gluster_parseuri(gconf, filename); - if (ret < 0) { - error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" - "volume/path[?socket=...]"); - errno = -ret; - goto out; - } + GlusterServerList *server = NULL; glfs = glfs_new(gconf->volume); if (!glfs) { goto out; } - ret = glfs_set_volfile_server(glfs, - GlusterTransport_lookup[gconf->server->transport], - gconf->server->host, gconf->server->port); - if (ret < 0) { - goto out; + for (server = gconf->server; server; server = server->next) { + ret = glfs_set_volfile_server(glfs, + GlusterTransport_lookup[server->value->transport], + server->value->host, server->value->port); + if (ret < 0) { + goto out; + } } ret = glfs_set_logging(glfs, "-", gconf->debug_level); @@ -249,11 +293,13 @@ static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, ret = glfs_init(glfs); if (ret) { + error_report("Gluster connection for 'volume: %s and path: %s': ", + gconf->volume, gconf->path); + for (server = gconf->server; server; server = server->next) { + error_report("failed for host %s", server->value->host); + } error_setg_errno(errp, errno, - "Gluster connection failed for host=%s port=%d " - "volume=%s path=%s transport=%s", gconf->server->host, - gconf->server->port, gconf->volume, gconf->path, - GlusterTransport_lookup[gconf->server->transport]); + "Please refer to gluster logs for more info"); /* glfs_init sometimes doesn't set errno although docs suggest that */ if (errno == 0) @@ -272,6 +318,176 @@ out: return NULL; } +static int qapi_enum_parse(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 i; +} + +/* + * Convert the json formatted command line into qapi. +*/ +static int qemu_gluster_parsejson(BlockdevOptionsGluster *gconf, + QDict *options) +{ + QemuOpts *opts; + GlusterServer *gsconf; + GlusterServerList *curr = NULL; + QDict *backing_options = NULL; + Error *local_err = NULL; + char *str = NULL; + const char *ptr; + size_t num_servers; + int i; + + /* create opts info from runtime_json_opts list */ + opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options, &local_err); + if (local_err) { + goto out; + } + + num_servers = qdict_array_entries(options, GLUSTER_OPT_SERVER_PATTERN); + if (num_servers < 1) { + error_setg(&local_err, "qemu_gluster: please provide 'server' " + "option with valid fields in array of tuples"); + goto out; + } + + ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME); + if (!ptr) { + error_setg(&local_err, "qemu_gluster: please provide 'volume' " + "option"); + goto out; + } + gconf->volume = g_strdup(ptr); + + ptr = qemu_opt_get(opts, GLUSTER_OPT_PATH); + if (!ptr) { + error_setg(&local_err, "qemu_gluster: please provide 'path' " + "option"); + goto out; + } + gconf->path = g_strdup(ptr); + + qemu_opts_del(opts); + + /* create opts info from runtime_tuple_opts list */ + for (i = 0; i < num_servers; i++) { + opts = qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error_abort); + str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i); + qdict_extract_subqdict(options, &backing_options, str); + qemu_opts_absorb_qdict(opts, backing_options, &local_err); + if (local_err) { + goto out; + } + qdict_del(backing_options, str); + g_free(str); + str = NULL; + + ptr = qemu_opt_get(opts, GLUSTER_OPT_HOST); + if (!ptr) { + error_setg(&local_err, "qemu_gluster: server.{tuple.%d} " + "requires 'host' option", i); + goto out; + } + + gsconf = g_new0(GlusterServer, 1); + + gsconf->host = g_strdup(ptr); + + ptr = qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT); + /* check whether transport type specified in json command is valid */ + gsconf->transport = qapi_enum_parse(ptr); + if (gsconf->transport == GLUSTER_TRANSPORT__MAX) { + error_setg(&local_err, "qemu_gluster: please set 'transport'" + " type in tuple.%d as tcp|rdma|unix", i); + qapi_free_GlusterServer(gsconf); + goto out; + } + gsconf->has_transport = true; + + /* we don't need port number for unix transport */ + if (gsconf->transport != GLUSTER_TRANSPORT_UNIX) { + gsconf->port = qemu_opt_get_number(opts, GLUSTER_OPT_PORT, + GLUSTER_DEFAULT_PORT); + gsconf->has_port = true; + } + + if (gconf->server == NULL) { + gconf->server = g_new0(GlusterServerList, 1); + gconf->server->value = gsconf; + curr = gconf->server; + } else { + curr->next = g_new0(GlusterServerList, 1); + curr->next->value = gsconf; + curr = curr->next; + } + + qemu_opts_del(opts); + } + + return 0; + +out: + error_report_err(local_err); + qemu_opts_del(opts); + if (str) { + qdict_del(backing_options, str); + g_free(str); + } + errno = EINVAL; + return -errno; +} + +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf, + const char *filename, + QDict *options, Error **errp) +{ + int ret; + + if (filename) { + ret = qemu_gluster_parseuri(gconf, filename); + if (ret < 0) { + error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/" + "volume/path[?socket=...]"); + errno = -ret; + return NULL; + } + } else { + ret = qemu_gluster_parsejson(gconf, options); + if (ret < 0) { + error_setg(errp, "Usage: " + "-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,]" + "file.server.2.host=/var/run/glusterd.socket," + "file.server.2.transport=unix ..."); + errno = -ret; + return NULL; + } + } + + return qemu_gluster_glfs_init(gconf, errp); +} + static void qemu_gluster_complete_aio(void *opaque) { GlusterAIOCB *acb = (GlusterAIOCB *)opaque; @@ -371,7 +587,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict *options, gconf = g_new0(BlockdevOptionsGluster, 1); gconf->debug_level = s->debug_level; gconf->has_debug_level = true; - s->glfs = qemu_gluster_init(gconf, filename, errp); + s->glfs = qemu_gluster_init(gconf, filename, options, errp); if (!s->glfs) { ret = -errno; goto out; @@ -442,7 +658,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state, gconf = g_new0(BlockdevOptionsGluster, 1); gconf->debug_level = s->debug_level; gconf->has_debug_level = true; - reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp); + reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp); if (reop_s->glfs == NULL) { ret = -errno; goto exit; @@ -589,7 +805,7 @@ static int qemu_gluster_create(const char *filename, } gconf->has_debug_level = true; - glfs = qemu_gluster_init(gconf, filename, errp); + glfs = qemu_gluster_init(gconf, filename, NULL, errp); if (!glfs) { ret = -errno; goto out; @@ -969,7 +1185,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, @@ -997,7 +1213,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, @@ -1053,7 +1269,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 f8b38bb..52cee7d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2077,7 +2077,7 @@ # # @path: absolute path to image file in gluster volume # -# @server: gluster server description +# @server: one or more gluster server descriptions # # @debug_level: #libgfapi log level (default '4' which is Error) # @@ -2086,7 +2086,7 @@ { 'struct': 'BlockdevOptionsGluster', 'data': { 'volume': 'str', 'path': 'str', - 'server': 'GlusterServer', + 'server': [ 'GlusterServer' ], '*debug_level': 'int' } } ##
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: Currently VM Image on gluster volume is specified like this: file=gluster[+tcp]://host[:port]/testvol/a.img Assuming we have three hosts in trusted pool with replica 3 volume in action and unfortunately host (mentioned in the command above) went down for some reason, since the volume is replica 3 we now have other 2 hosts active from which we can boot the VM. But currently there is no mechanism to pass the other 2 gluster host 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, volume=testvol,path=/path/a.raw, server.0.host=1.2.3.4, [server.0.port=24007,] [server.0.transport=tcp,] server.1.host=5.6.7.8, [server.1.port=24008,] [server.1.transport=rdma,] server.2.host=/var/run/glusterd.socket, server.2.transport=unix ... Pattern II: 'json:{"driver":"qcow2","file":{"driver":"gluster", "volume":"testvol","path":"/path/a.qcow2", "server":[{tuple0},{tuple1}, ...{tupleN}]}}' driver => 'gluster' (protocol name) volume => name of gluster volume where our VM image resides path => absolute path of image in gluster volume {tuple} => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]} host => host address (hostname/ipv4/ipv6 addresses/socket path) 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|unix (default 'tcp') Examples: 1. -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, file.server.2.host=/var/run/glusterd.socket file.server.1.transport=unix 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"}, {"host":"/var/run/glusterd.socket","transport":"unix"}] } }' This patch gives a mechanism to provide all the server addresses, which are in replica set, so in case host1 is down VM can still boot from any of the active hosts. This is equivalent to the backup-volfile-servers option supported by mount.glusterfs (FUSE way of mounting gluster volume) 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> --- block/gluster.c | 290 ++++++++++++++++++++++++++++++++++++++++++++------- qapi/block-core.json | 4 +- 2 files changed, 255 insertions(+), 39 deletions(-)