From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43982) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwBSE-0003KD-N1 for qemu-devel@nongnu.org; Tue, 10 Nov 2015 11:07:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwBSB-0006Tl-Ab for qemu-devel@nongnu.org; Tue, 10 Nov 2015 11:07:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwBSA-0006SF-Vs for qemu-devel@nongnu.org; Tue, 10 Nov 2015 11:07:27 -0500 References: <1447146556-7328-1-git-send-email-prasanna.kalever@redhat.com> <1447146556-7328-4-git-send-email-prasanna.kalever@redhat.com> From: Eric Blake Message-ID: <56421638.2050305@redhat.com> Date: Tue, 10 Nov 2015 09:07:20 -0700 MIME-Version: 1.0 In-Reply-To: <1447146556-7328-4-git-send-email-prasanna.kalever@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3BxUqHCaRv07KGOXwtsxAOIhtWeDVDSB5" Subject: Re: [Qemu-devel] [PATCH v13 3/3] block/gluster: add support for multiple gluster servers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Prasanna Kumar Kalever , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pkrempa@redhat.com, stefanha@gmail.com, jcody@redhat.com, deepakcs@redhat.com, bharata@linux.vnet.ibm.com, rtalur@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --3BxUqHCaRv07KGOXwtsxAOIhtWeDVDSB5 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/10/2015 02:09 AM, Prasanna Kumar Kalever wrote: > This patch adds a way to specify multiple volfile servers to the gluste= r > block backend of QEMU with tcp|rdma transport types and their port numb= ers. >=20 [...] > 2. > 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",= > "path":"/path/a.qcow2","servers": > [{"host":"1.2.3.4","port":"24007","transport":"tcp"}, > {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }' >=20 > 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. >=20 > This is equivalent to the backup-volfile-servers option supported by > mount.glusterfs (FUSE way of mounting gluster volume) >=20 > Credits: Sincere thanks to Kevin Wolf and > "Deepak C Shetty" for inputs and all their suppor= t >=20 > Signed-off-by: Prasanna Kumar Kalever > --- > v10: > fix mem-leak as per Peter Krempa review comments >=20 > v11: > using qapi-types* defined structures as per "Eric Blake" > review comments. >=20 > v12: > fix crash caused in qapi_free_BlockdevOptionsGluster >=20 > v13: > address comments from "Jeff Cody" I had some other comments against v10 that I don't see addressed yet: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06377.html > --- > block/gluster.c | 468 +++++++++++++++++++++++++++++++++++++++++++= +------- > qapi/block-core.json | 60 ++++++- > 2 files changed, 461 insertions(+), 67 deletions(-) >=20 > diff --git a/block/gluster.c b/block/gluster.c > index ededda2..8939072 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -11,6 +11,19 @@ > #include "block/block_int.h" > #include "qemu/uri.h" > =20 > +#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_SERVERS_PATTERN "servers." > + > +#define GLUSTER_DEFAULT_PORT 24007 > + > +#define MAX_SERVERS "10000" Why is this a string rather than an integer? > + > + > typedef struct GlusterAIOCB { > int64_t size; > int ret; > @@ -29,15 +42,6 @@ typedef struct BDRVGlusterReopenState { > struct glfs_fd *fd; > } BDRVGlusterReopenState; > =20 > -typedef struct GlusterConf { > - char *host; > - int port; > - char *volume; > - char *path; > - char *transport; > -} GlusterConf; > - This patch feels pretty big. It may be smarter to break it into two pieces - one that adds GlusterConf to qapi/block-core.json and replaces existing uses of this definition to the qapi type but with no changes in semantics; and the other that then extends things to add support for multiple servers (so that we aren't trying to do too much in one patch). > @@ -143,8 +176,11 @@ static int parse_volume_options(GlusterConf *gconf= , char *path) > * file=3Dgluster+unix:///testvol/dir/a.img?socket=3D/tmp/glusterd.soc= ket > * file=3Dgluster+rdma://1.2.3.4:24007/testvol/a.img > */ > -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filen= ame) > +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf, > + const char *filename) > { > + BlockdevOptionsGluster *gconf; > + GlusterServer *gsconf; > URI *uri; > QueryParams *qp =3D NULL; > bool is_unix =3D false; > @@ -155,20 +191,24 @@ static int qemu_gluster_parseuri(GlusterConf *gco= nf, const char *filename) > return -EINVAL; > } > =20 > + gconf =3D g_new0(BlockdevOptionsGluster, 1); > + gsconf =3D g_new0(GlusterServer, 1); gconf and gsconf are both allocated here... > + > /* transport */ > if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { > - gconf->transport =3D g_strdup("tcp"); > + gsconf->transport =3D GLUSTER_TRANSPORT_TCP; > } else if (!strcmp(uri->scheme, "gluster+tcp")) { > - gconf->transport =3D g_strdup("tcp"); > + gsconf->transport =3D GLUSTER_TRANSPORT_TCP; > } else if (!strcmp(uri->scheme, "gluster+unix")) { > - gconf->transport =3D g_strdup("unix"); > + gsconf->transport =3D GLUSTER_TRANSPORT_UNIX; > is_unix =3D true; > } else if (!strcmp(uri->scheme, "gluster+rdma")) { > - gconf->transport =3D g_strdup("rdma"); > + gsconf->transport =3D GLUSTER_TRANSPORT_RDMA; > } else { > ret =3D -EINVAL; > goto out; =2E..but you can error here... > } > + gsconf->has_transport =3D true; > =20 > ret =3D parse_volume_options(gconf, uri->path); > if (ret < 0) { > @@ -190,13 +230,27 @@ static int qemu_gluster_parseuri(GlusterConf *gco= nf, const char *filename) > ret =3D -EINVAL; > goto out; > } > - gconf->host =3D g_strdup(qp->p[0].value); > + gsconf->host =3D g_strdup(qp->p[0].value); > } else { > - gconf->host =3D g_strdup(uri->server ? uri->server : "localhos= t"); > - gconf->port =3D uri->port; > + gsconf->host =3D g_strdup(uri->server ? uri->server : "localho= st"); > + if (uri->port) { > + gsconf->port =3D uri->port; > + } else { > + gsconf->port =3D GLUSTER_DEFAULT_PORT; > + } > + gsconf->has_port =3D true; > } > =20 > + gconf->servers =3D g_new0(GlusterServerList, 1); > + gconf->servers->value =3D gsconf; > + gconf->servers->next =3D NULL; Dead assignment (gconf->servers->next is already NULL because of g_new0).= > + > + *pgconf =3D gconf; > + > out: > + if (ret < 0) { > + qapi_free_BlockdevOptionsGluster(gconf); > + } > if (qp) { > query_params_free(qp); > } > @@ -204,30 +258,26 @@ out: > return ret; > } =2E..which means you leak gsconf if you hit an error. > =20 > -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *= filename, > - Error **errp) > +static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gco= nf, > + Error **errp) > { > - struct glfs *glfs =3D NULL; > + struct glfs *glfs; > int ret; > int old_errno; > - > - ret =3D qemu_gluster_parseuri(gconf, filename); > - if (ret < 0) { > - error_setg(errp, "Usage: file=3Dgluster[+transport]://[host[:p= ort]]/" > - "volume/path[?socket=3D...]"); > - errno =3D -ret; > - goto out; > - } > + GlusterServerList *server; > =20 > glfs =3D glfs_new(gconf->volume); > if (!glfs) { > goto out; > } > =20 > - ret =3D glfs_set_volfile_server(glfs, gconf->transport, gconf->hos= t, > - gconf->port); > - if (ret < 0) { > - goto out; > + for (server =3D gconf->servers; server !=3D NULL; server =3D serve= r->next) { It's okay to use 'server;' rather than 'server !=3D NULL;' as the loop condition. Matter of personal style. > + ret =3D glfs_set_volfile_server(glfs, > + GlusterTransport_lookup[server->= value->transport], > + server->value->host, server->val= ue->port); port and transport are optional; which means you should probably be checking has_port and has_transport before blindly using them (unless you made sure that ALL initialization paths set things to sane defaults when the user omitted the arguments). > + if (ret < 0) { > + goto out; > + } > } > =20 > /* > @@ -242,10 +292,9 @@ static struct glfs *qemu_gluster_init(GlusterConf = *gconf, const char *filename, > ret =3D glfs_init(glfs); > if (ret) { > error_setg_errno(errp, errno, > - "Gluster connection failed for host=3D%s port= =3D%d " > - "volume=3D%s path=3D%s transport=3D%s", gconf= ->host, > - gconf->port, gconf->volume, gconf->path, > - gconf->transport); > + "Error: Gluster connection failed for given h= osts " Don't start messages with "Error: ", error_setg_errno() already does that for you. > + "volume:'%s' path:'%s' host1: %s", gconf->vol= ume, Inconsistent on whether there is a space after ':'. "given hosts volume" sounds odd. > + gconf->path, gconf->servers->value->host); > =20 > /* glfs_init sometimes doesn't set errno although docs suggest= that */ > if (errno =3D=3D 0) > @@ -264,6 +313,300 @@ out: > return NULL; > } > =20 > +static int parse_transport_option(const char *opt) > +{ > + int i; > + > + if (!opt) { > + /* Set tcp as default */ > + return GLUSTER_TRANSPORT_TCP; > + } > + > + for (i =3D 0; i < GLUSTER_TRANSPORT_MAX; i++) { > + if (!strcmp(opt, GlusterTransport_lookup[i])) { > + return i; > + } > + } > + > + return -EINVAL; > +} > + > +/* > +* > +* Basic command line syntax looks like: > +* You have 110 lines from /* to */. In v10, I already mentioned that this comment is probably 100 lines too long. You do NOT need to repeat the syntax, examples, or even more-readable example here; having them in the commit body was enough. Someone that knows how to read qapi will be able to deduce what this function is doing if you were to simplify to just this: /* * Convert the command line into qapi. */ > +* > +*/ > +static int qemu_gluster_parsejson(BlockdevOptionsGluster **pgconf, > + QDict *options) > +{ > + QemuOpts *opts; > + BlockdevOptionsGluster *gconf =3D NULL; > + GlusterServer *gsconf; > + GlusterServerList **prev; > + GlusterServerList *curr =3D NULL; > + QDict *backing_options =3D NULL; > + Error *local_err =3D NULL; > + char *str =3D NULL; > + const char *ptr; > + size_t num_servers; > + size_t buff_size; > + int i; > + > + > + /* create opts info from runtime_json_opts list */ Why two blank lines? > + opts =3D qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abor= t); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + goto out; > + } > + > + gconf =3D g_new0(BlockdevOptionsGluster, 1); > + > + num_servers =3D qdict_array_entries(options, GLUSTER_OPT_SERVERS_P= ATTERN); > + if (num_servers < 1) { > + error_setg(&local_err, "Error: qemu_gluster: please provide 's= ervers' " Again, error messages created with error_setg() need not start with "Error: ". A good thing to try when you are adding an error message for a command line parsing scenario is to try and come up with the command line that would trigger the error to see if the result looks sane. > + "option with valid fields in array of t= uples"); > + goto out; > + } > + > + ptr =3D qemu_opt_get(opts, GLUSTER_OPT_VOLUME); > + if (!ptr) { > + error_setg(&local_err, "Error: qemu_gluster: please provide 'v= olume' " > + "option"); > + goto out; > + } > + gconf->volume =3D g_strdup(ptr); > + > + ptr =3D qemu_opt_get(opts, GLUSTER_OPT_PATH); > + if (!ptr) { > + error_setg(&local_err, "Error: qemu_gluster: please provide 'p= ath' " More "Error: " prefixes. I'll quit pointing them out. > + "option"); > + goto out; > + } > + gconf->path =3D g_strdup(ptr); > + > + qemu_opts_del(opts); > + > + /* create opts info from runtime_tuple_opts list */ > + buff_size =3D strlen(GLUSTER_OPT_SERVERS_PATTERN) + strlen(MAX_SER= VERS) + 2; > + str =3D g_malloc(buff_size); > + for (i =3D 0; i < num_servers; i++) { > + opts =3D qemu_opts_create(&runtime_tuple_opts, NULL, 0, &error= _abort); > + g_assert(snprintf(str, buff_size, > + GLUSTER_OPT_SERVERS_PATTERN"%d.", i) < buff_= size); Gross - you have side effects inside g_assert(). (Absolute bug if you do that inside plain assert(); possibly excusable in g_assert() but still not good practice). If I were writing this, then instead of futzing around with snprintf, I'd just use g_strdup_printf() to malloc the appropriately sized string without worrying about sizing myself, and be sure I didn't leak things. > + 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); > + > + ptr =3D qemu_opt_get(opts, GLUSTER_OPT_HOST); > + if (!ptr) { > + error_setg(&local_err, "Error: qemu_gluster: servers.{tupl= e.%d} " > + "requires 'host' option", i); > + goto out; > + } > + > + gsconf =3D g_new0(GlusterServer, 1); gsconf is allocated here... > + > + gsconf->host =3D g_strdup(ptr); > + > + ptr =3D qemu_opt_get(opts, GLUSTER_OPT_TRANSPORT); > + /* check whether transport type specified in json command is v= alid */ > + if (parse_transport_option(ptr) < 0) { > + error_setg(&local_err, "Error: qemu_gluster: please set 't= ransport'" > + " type in tuple.%d as tcp or rdma",= i); > + goto out; =2E..but if you error here... > + } > + /* only if valid transport i.e. either of tcp|rdma is specifie= d */ > + gsconf->transport =3D parse_transport_option(ptr); Why are you calling parse_transport_option() twice? > + gsconf->has_transport =3D true; > + > + gsconf->port =3D qemu_opt_get_number(opts, GLUSTER_OPT_PORT, > + GLUSTER_DEFAULT_PO= RT); Indentation is off. > + gsconf->has_port =3D true; > + > + if (gconf->servers =3D=3D NULL) { > + gconf->servers =3D g_new0(GlusterServerList, 1); > + gconf->servers->value =3D gsconf; > + curr =3D gconf->servers; > + } else { > + prev =3D &curr->next; > + curr =3D g_new0(GlusterServerList, 1); > + curr->value =3D gsconf; > + *prev =3D curr; > + } > + curr->next =3D NULL; > + > + qemu_opts_del(opts); > + } > + > + *pgconf =3D gconf; > + g_free(str); > + return 0; > + > +out: > + error_report_err(local_err); > + qapi_free_BlockdevOptionsGluster(gconf); > + qemu_opts_del(opts); > + if (str) { > + qdict_del(backing_options, str); > + g_free(str); > + } > + errno =3D EINVAL; > + return -errno; > +} =2E..then gsconf is leaked. > + > +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **gconf, > + const char *filename, > + QDict *options, Error **errp) > +{ > + int ret; > + > + if (filename) { > + ret =3D qemu_gluster_parseuri(gconf, filename); > + if (ret < 0) { > + error_setg(errp, "Usage: file=3Dgluster[+transport]://[hos= t[:port]]/" > + "volume/path[?socket=3D...]"); > + errno =3D -ret; > + return NULL; > + } > + } else { > + ret =3D qemu_gluster_parsejson(gconf, options); > + if (ret < 0) { > + error_setg(errp, "Wrong Usage."); Don't end error_setg() messages with a period. And Talking In All Camel Case Is Odd. > + error_append_hint(errp, > + "Usage1: " > + "-drive driver=3Dqcow2,file.driver=3Dglus= ter," > + "file.volume=3Dtestvol,file.path=3D/path/= a.qcow2," > + "file.servers.0.host=3D1.2.3.4," > + "[file.servers.0.port=3D24007,]" > + "[file.servers.0.transport=3Dtcp,]" > + "file.servers.1.host=3D5.6.7.8," > + "[file.servers.1.port=3D24008,]" > + "[file.servers.1.transport=3Drdma,] ...")= ; > + error_append_hint(errp, > + "\nUsage2: " > + "'json:{\"driver\":\"qcow2\",\"file\":" > + "{\"driver\":\"gluster\",\"volume\":\"" > + "testvol\",\"path\":\"/path/a.qcow2\"," > + "\"servers\":[{\"host\":\"1.2.3.4\"," > + "\"port\":\"24007\",\"transport\":\"tcp\"= }," > + "{\"host\":\"4.5.6.7\",\"port\":\"24007\"= ," > + "\"transport\":\"rdma\"}, ...]}}'"); Rather long. I think a single hint is long enough; you don't need to display the json:{} usage. > @@ -523,7 +863,7 @@ static int qemu_gluster_create(const char *filename= , > } else if (!strcmp(tmp, "full") && gluster_supports_zerofill()) { > prealloc =3D 1; > } else { > - error_setg(errp, "Invalid preallocation mode: '%s'" > + error_setg(errp, "Error: Invalid preallocation mode: '%s'" > " or GlusterFS doesn't support zerofill API",= tmp); Spurious hunk. > +++ b/qapi/block-core.json > @@ -1375,13 +1375,14 @@ > # Drivers that are supported in block device operations. > # > # @host_device, @host_cdrom: Since 2.1 > +# @gluster: Since 2.5 Sadly, I think we've found enough issues that this will not make 2.5 hard freeze deadline, so at this point, you should change things to state 2.6. > + > +## > +# @GlusterServer > +# > +# Gluster tuple set Not very descriptive. Better might be 'Details for connecting to a gluster server'. > +# > +# @host: host address (hostname/ipv4/ipv6 addresses) > +# > +# @port: #optional port number on which glusterd is listening > +# (default 24007) > +# > +# @transport: #optional transport type used to connect to gluster man= agement > +# daemon (default 'tcp') > +# > +# Since: 2.5 > +## > +{ 'struct': 'GlusterServer', > + 'data': { 'host': 'str', > + '*port': 'int', > + '*transport': 'GlusterTransport' } } I really do think it might be easier to split this into two patches; one that introduces GlusterServer in the .json and converts existing uses to it, and the other that introduces BlockdevOptionsGluster along with its ability to use a GlusterServerList. > + > +## > +# @BlockdevOptionsGluster > +# > +# Driver specific block device options for Gluster > +# > +# @volume: name of gluster volume where VM image resides > +# > +# @path: absolute path to image file in gluster volume > +# > +# @servers: one or more gluster server descriptions (host, port, and = transport) 80 columns; might be nice to keep things at 79 or less. In fact, since GlusterServer is already documented, you could get away with: # @servers: one or more gluster server descriptions > +# > +# Since: 2.5 > +## > +{ 'struct': 'BlockdevOptionsGluster', > + 'data': { 'volume': 'str', > + 'path': 'str', > + 'servers': [ 'GlusterServer' ] } } > + > +## > # @BlockdevOptions > # > # Options for creating a block device. > @@ -1816,7 +1870,7 @@ > 'file': 'BlockdevOptionsFile', > 'ftp': 'BlockdevOptionsFile', > 'ftps': 'BlockdevOptionsFile', > -# TODO gluster: Wait for structured options > + 'gluster': 'BlockdevOptionsGluster', > 'host_cdrom': 'BlockdevOptionsFile', > 'host_device':'BlockdevOptionsFile', > 'http': 'BlockdevOptionsFile', >=20 Overall, I think we are probably on the right track for the QMP interface; but since blockdev-add is NOT stable yet for 2.5, it won't hurt to wait to get this in until 2.6, to make sure we have plenty of time; and it would also be nice to make sure we get nbd, nfs, rbd, sheepdog all supported in the same release; possibly by sharing common types instead of introducing GlusterServer as a one-off type. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --3BxUqHCaRv07KGOXwtsxAOIhtWeDVDSB5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWQhY4AAoJEKeha0olJ0NqkGUH/36P0i7ASKNOiwAk74BrbsZ/ JjHOF+XFiOpAy6JZzKRDc5LLMlpHmfAO0hSa/HjfTnX20ul/N3QW0P3KTEjJeLj0 OgC5uOCP3jors9By+42imhM9fxyOW2KDlTnVe9PCCY3lRFOBHfgnMjIYUoYctH5L SkyOhG0uKXroqp24YDTfIBrJ2oXs65bzim/cSVoeRqAV8KXsQNCPEVL9D0rtqM6g ovB+r+Rvk67DuPyPWb3zvJegUyvKJVFngUeQ26dq7+VrXexqJ7fZnREoXNfkEVrw l/iVw+LlnvZrymTerOS2m9I/zAG/dE11dx7SAgMALCVG0r2NE7LUSVDY7o9ICzU= =Gn17 -----END PGP SIGNATURE----- --3BxUqHCaRv07KGOXwtsxAOIhtWeDVDSB5--