From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42571) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zr5kr-0005eh-5w for qemu-devel@nongnu.org; Tue, 27 Oct 2015 11:01:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zr5kk-0000eF-OC for qemu-devel@nongnu.org; Tue, 27 Oct 2015 11:01:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47373) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zr5kk-0000e4-HS for qemu-devel@nongnu.org; Tue, 27 Oct 2015 11:01:34 -0400 References: <1445951162-3280-1-git-send-email-prasanna.kalever@redhat.com> From: Eric Blake Message-ID: <562F91C7.2000700@redhat.com> Date: Tue, 27 Oct 2015 09:01:27 -0600 MIME-Version: 1.0 In-Reply-To: <1445951162-3280-1-git-send-email-prasanna.kalever@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="pa9MUsbkVk62O9vGKNqeBSuxrdFNqws7J" Subject: Re: [Qemu-devel] [PATCH v10 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, deepakcs@redhat.com, bharata@linux.vnet.ibm.com, rtalur@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --pa9MUsbkVk62O9vGKNqeBSuxrdFNqws7J Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/27/2015 07:06 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 No 0/3 cover letter? Even if only one patch of a series changes, it's still nice to have a cover letter explaining where to find the rest of the series. > Problem: >=20 > Currenly VM Image on gluster volume is specified like this: s/Currenly/Currently/ [...] > 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 > This patch depends on a recent fix in libgfapi raised as part of this w= ork: > http://review.gluster.org/#/c/12114/ >=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 > v1: Need a '---' line before v1. Everything above the line is okay for the commit message, everything after the line is informational for reviewers but doesn't belong in qemu.git (and 'git am' will automatically strip comments after the --- separator). A year from now, we won't care how many versions it took you to get to the version that was committed. > +++ b/block/gluster.c > -typedef struct GlusterConf { > +typedef struct GlusterServerConf { > char *host; > int port; > + char *transport; > +} GlusterServerConf; Why are you defining this type, instead of reusing 'GlusterTuple' that gets auto-defined for you by your changes to block-core.json? >=20 > +typedef struct GlusterConf { > char *volume; > char *path; > - char *transport; > + int num_servers; size_t is better than int when counting items in an array. > + GlusterServerConf *gsconf; Naming it 'num_servers' vs. 'gsconf' is confusing. In fact, why are you even defining this type? Why not just directly use 'BlockdevOptionsGluster' that you defined by your changes to block-core.json? Of course, the generated type uses a single GlusterTupleList (a linked list with NULL terminator) rather than a pair of size and GlusterTuple[] for tracking the multiple servers, but directly using the qapi types now will save you some conversion efforts down the road. > +static QemuOptsList runtime_tuple_opts =3D { > + .name =3D "gluster_tuple", > + .head =3D QTAILQ_HEAD_INITIALIZER(runtime_tuple_opts.head), > + .desc =3D { > + { > + .name =3D GLUSTER_OPT_HOST, > + .type =3D QEMU_OPT_STRING, > + .help =3D "host address (hostname/ipv4/ipv6 addresses)", > + }, > + { > + .name =3D GLUSTER_OPT_PORT, > + .type =3D QEMU_OPT_NUMBER, > + .help =3D "port number on which glusterd listen (default 2= 4007)", s/listen/is listening/ > + }, > + { > + .name =3D GLUSTER_OPT_TRANSPORT, > + .type =3D QEMU_OPT_STRING, > + .help =3D "transport type 'tcp' or 'rdma' (default 'tcp')"= , > + }, > + { /* end of list */ } > + }, > +}; > + > =20 > static void qemu_gluster_gconf_free(GlusterConf *gconf) > { Unneeded. Just use the auto-generated qapi_BlockdevOptionsGluster_free() that you got by adding and using the appropriate types in block-core.json. > @@ -155,16 +218,20 @@ static int qemu_gluster_parseuri(GlusterConf *gco= nf, const char *filename) > return -EINVAL; > } > =20 > + gconf =3D g_new0(GlusterConf, 1); > + gconf->num_servers =3D 1; > + gconf->gsconf =3D g_new0(GlusterServerConf, gconf->num_servers); If you use the qapi types, as I've suggested above, then you will need to be working with the list of servers as a linked list rather than an array. > + > /* transport */ > if (!uri->scheme || !strcmp(uri->scheme, "gluster")) { > - gconf->transport =3D g_strdup("tcp"); > + gconf->gsconf[0].transport =3D g_strdup("tcp"); > } else if (!strcmp(uri->scheme, "gluster+tcp")) { > - gconf->transport =3D g_strdup("tcp"); > + gconf->gsconf[0].transport =3D g_strdup("tcp"); > } else if (!strcmp(uri->scheme, "gluster+unix")) { > - gconf->transport =3D g_strdup("unix"); > + gconf->gsconf[0].transport =3D g_strdup("unix"); > is_unix =3D true; > } else if (!strcmp(uri->scheme, "gluster+rdma")) { > - gconf->transport =3D g_strdup("rdma"); > + gconf->gsconf[0].transport =3D g_strdup("rdma"); The generated qapi code also contains convenience functions from mapping from a finite set of strings to a set of enum values, although I don't see "unix" listed in your set of values for 'GlusterTransport'. > +/* > +* > +* Basic command line syntax looks like: > +* > +* Pattern I: > +* Pattern II: > +* > +* Examples: > +* Pattern I: > +* > +* Pattern II: > +* 'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol= ", > +* > +*/ > +static int qemu_gluster_parsejson(GlusterConf **pgconf, QDict *options= ) > +{ Wow. Overly long comment. It was fine in the commit message, but I don't think you need quite so much filler here. > + } else { > + ret =3D qemu_gluster_parsejson(gconf, options); > + if (ret < 0) { > + error_setg(errp, "Wrong Usage!"); Drop the '!'. None of our other error messages need that much emphasis. > +++ b/qapi/block-core.json > @@ -1375,15 +1375,16 @@ > # > # @host_device, @host_cdrom, @host_floppy: Since 2.1 > # @host_floppy: deprecated since 2.3 > +# @gluster: Since 2.5 > # > # Since: 2.0 > ## > { 'enum': 'BlockdevDriver', > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop', > - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',= > - 'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'pa= rallels', > - 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'v= hdx', > - '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' ] } > =20 > ## > # @BlockdevOptionsBase > @@ -1794,6 +1795,57 @@ > '*read-pattern': 'QuorumReadPattern' } } > =20 > ## > +# @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' ] } As mentioned above, isn't this missing 'unix'? > + > +## > +# @GlusterTuple > +# > +# Gluster tuple set > +# > +# @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': 'GlusterTuple', > + 'data': { 'host': 'str', > + '*port': 'int', > + '*transport': 'GlusterTransport' } } I'm still not sold that 'GlusterTuple' is the best name for this type, but it doesn't affect ABI and I don't have any better suggestions off-han= d. > + > +## > +# @BlockdevOptionsGluster > +# > +# Driver specific block device options for Gluster > +# > +# @volume: name of gluster volume where our VM image resides s/our // > +# > +# @path: absolute path to image file in gluster volume > +# > +# @servers: one or more gluster server descriptions (host, port, and = transport) > +# > +# Since: 2.5 > +## > +{ 'struct': 'BlockdevOptionsGluster', > + 'data': { 'volume': 'str', > + 'path': 'str', > + 'servers': [ 'GlusterTuple' ] } } This one looks okay, as far as I can tell. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --pa9MUsbkVk62O9vGKNqeBSuxrdFNqws7J 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/ iQEcBAEBCAAGBQJWL5HHAAoJEKeha0olJ0NqZ7oH/2CoPRHd9KZjb/+mcHr7eUbz xUIT2HkpctnEPOyx6UrY9xn460TbKu4IPkA6dcTo7XPPxVC7k0a5qhTUaZtOe/sJ DONUS/aEpyYWIQ+PdjxLJ7wWGc56x3IB40veGuv9R6tP7t2WSG65k84qAV5lW/OE cIv2nOj3TdfGOkl+f4VnnZeMYoxDnHTqNObnapD/oi13MvHZBWRSamkG3e/eskZ1 QG/uFihqpOaldjnXvgNn9a/uNHwJoIxRQjQhZ9l05CIVzBByX+4nwblklB2OhCFD CM+m/YC7VRwplKQBH/Qfmpe4CzxuvMdC0KkwdPmgTJ9VGDcwFG3mbzYRSfsc5dg= =qfKi -----END PGP SIGNATURE----- --pa9MUsbkVk62O9vGKNqeBSuxrdFNqws7J--