On Tue, Jul 19, 2016 at 04:39:39PM -0400, Jeff Cody wrote:
> On Tue, Jul 19, 2016 at 10:27:32PM +0530, Prasanna Kumar Kalever wrote:
> > this patch adds 'GlusterServer' related schema in qapi/block-core.json
> >
> > Signed-off-by: Prasanna Kumar Kalever <prasanna.kale...@redhat.com>
> > ---
> > block/gluster.c | 115
> > +++++++++++++++++++++++++++++----------------------
> > qapi/block-core.json | 68 +++++++++++++++++++++++++++---
> > 2 files changed, 128 insertions(+), 55 deletions(-)
> >
> > diff --git a/block/gluster.c b/block/gluster.c
> > index 8a54ad4..c4ca59e 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -16,6 +16,7 @@
> >
> > #define GLUSTER_OPT_FILENAME "filename"
> > #define GLUSTER_OPT_DEBUG "debug"
> > +#define GLUSTER_DEFAULT_PORT 24007
> > #define GLUSTER_DEBUG_DEFAULT 4
> > #define GLUSTER_DEBUG_MAX 9
> >
> > @@ -40,15 +41,6 @@ typedef struct BDRVGlusterReopenState {
> > struct glfs_fd *fd;
> > } BDRVGlusterReopenState;
> >
> > -typedef struct GlusterConf {
> > - char *host;
> > - int port;
> > - char *volume;
> > - char *path;
> > - char *transport;
> > - int debug_level;
> > -} GlusterConf;
> > -
> >
> > static QemuOptsList qemu_gluster_create_opts = {
> > .name = "qemu-gluster-create-opts",
> > @@ -92,18 +84,7 @@ static QemuOptsList runtime_opts = {
> > };
> >
> >
> > -static void qemu_gluster_gconf_free(GlusterConf *gconf)
> > -{
> > - if (gconf) {
> > - g_free(gconf->host);
> > - g_free(gconf->volume);
> > - g_free(gconf->path);
> > - g_free(gconf->transport);
> > - g_free(gconf);
> > - }
> > -}
> > -
> > -static int parse_volume_options(GlusterConf *gconf, char *path)
> > +static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
> > {
> > char *p, *q;
> >
> > @@ -160,8 +141,10 @@ static int parse_volume_options(GlusterConf *gconf,
> > char *path)
> > * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img
> > * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
> > */
> > -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> > +static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
> > + const char *filename)
> > {
> > + GlusterServer *gsconf;
> > URI *uri;
> > QueryParams *qp = NULL;
> > bool is_unix = false;
> > @@ -172,16 +155,18 @@ static int qemu_gluster_parseuri(GlusterConf *gconf,
> > const char *filename)
> > return -EINVAL;
> > }
> >
> > + gconf->server = gsconf = g_new0(GlusterServer, 1);
> > +
> > /* transport */
> > if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> > - gconf->transport = g_strdup("tcp");
> > + gsconf->type = GLUSTER_TRANSPORT_TCP;
> > } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> > - gconf->transport = g_strdup("tcp");
> > + gsconf->type = GLUSTER_TRANSPORT_TCP;
> > } else if (!strcmp(uri->scheme, "gluster+unix")) {
> > - gconf->transport = g_strdup("unix");
> > + gsconf->type = GLUSTER_TRANSPORT_UNIX;
> > is_unix = true;
> > } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> > - gconf->transport = g_strdup("tcp");
> > + gsconf->type = GLUSTER_TRANSPORT_TCP;
> > error_report("Warning: rdma feature is not supported falling "
> > "back to tcp");
> > } else {
> > @@ -209,10 +194,14 @@ static int qemu_gluster_parseuri(GlusterConf *gconf,
> > const char *filename)
> > ret = -EINVAL;
> > goto out;
> > }
> > - gconf->host = g_strdup(qp->p[0].value);
> > + gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
> > } else {
> > - gconf->host = g_strdup(uri->server ? uri->server : "localhost");
> > - gconf->port = uri->port;
> > + gsconf->u.tcp.host = g_strdup(uri->server ? uri->server :
> > "localhost");
> > + if (uri->port) {
> > + gsconf->u.tcp.port = g_strdup_printf("%d", uri->port);
> > + } else {
> > + gsconf->u.tcp.port = g_strdup_printf("%d",
> > GLUSTER_DEFAULT_PORT);
> > + }
> > }
> >
> > out:
> > @@ -223,17 +212,18 @@ out:
> > return ret;
> > }
> >
> > -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char
> > *filename,
> > - Error **errp)
> > +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster *gconf,
> > + const char *filename, Error **errp)
> > {
> > struct glfs *glfs = NULL;
> > int ret;
> > int old_errno;
> >
> > - ret = qemu_gluster_parseuri(gconf, filename);
> > + ret = qemu_gluster_parse_uri(gconf, filename);
> > if (ret < 0) {
> > - error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
> > - "volume/path[?socket=...]");
> > + error_setg(errp, "Invalid URI");
> > + error_append_hint(errp, "Usage: file=gluster[+transport]://"
> > +
> > "[host[:port]]/volume/path[?socket=...]\n");
> > errno = -ret;
> > goto out;
> > }
> > @@ -243,8 +233,16 @@ static struct glfs *qemu_gluster_init(GlusterConf
> > *gconf, const char *filename,
> > goto out;
> > }
> >
> > - ret = glfs_set_volfile_server(glfs, gconf->transport, gconf->host,
> > - gconf->port);
> > + if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
> > + ret = glfs_set_volfile_server(glfs,
> > +
> > GlusterTransport_lookup[gconf->server->type],
>
> Formatting issue found by checkpatch; exceeds 80 char. Will fix on commit,
> no need for respin.
>
> > + gconf->server->u.q_unix.path, 0);
> > + } else {
> > + ret = glfs_set_volfile_server(glfs,
> > +
> > GlusterTransport_lookup[gconf->server->type],
>
> Formatting issue found by checkpatch; exceeds 80 char. Will fix on commit,
> no need for respin.
>
>
> > + gconf->server->u.tcp.host,
> > + atoi(gconf->server->u.tcp.port));
> > + }
> > if (ret < 0) {
> > goto out;
> > }
> > @@ -256,15 +254,22 @@ 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 host=%s port=%d "
> > - "volume=%s path=%s transport=%s", gconf->host,
> > - gconf->port, gconf->volume, gconf->path,
> > - gconf->transport);
> > + if (gconf->server->type == GLUSTER_TRANSPORT_UNIX) {
> > + error_setg(errp,
> > + "Gluster connection for volume %s, path %s failed
> > on "
> > + "socket %s ", gconf->volume, gconf->path,
> > + gconf->server->u.q_unix.path);
> > + } else {
> > + error_setg(errp,
> > + "Gluster connection for volume %s, path %s failed
> > on "
> > + "host %s and port %s ", gconf->volume, gconf->path,
> > + gconf->server->u.tcp.host,
> > gconf->server->u.tcp.port);
> > + }
> >
> > /* glfs_init sometimes doesn't set errno although docs suggest
> > that */
> > - if (errno == 0)
> > + if (errno == 0) {
> > errno = EINVAL;
> > + }
>
> Eric pointed out the errno risk here; as this is pre-existing, nothing to
> hold up this patch (can be fixed in later patch).
>
> >
> > goto out;
> > }
> > @@ -352,7 +357,7 @@ 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);
> > + BlockdevOptionsGluster *gconf = NULL;
> > QemuOpts *opts;
> > Error *local_err = NULL;
> > const char *filename;
> > @@ -375,7 +380,9 @@ static int qemu_gluster_open(BlockDriverState *bs,
> > QDict *options,
> > s->debug_level = GLUSTER_DEBUG_MAX;
> > }
> >
> > + gconf = g_new0(BlockdevOptionsGluster, 1);
> > gconf->debug_level = s->debug_level;
> > + gconf->has_debug_level = true;
> > s->glfs = qemu_gluster_init(gconf, filename, errp);
> > if (!s->glfs) {
> > ret = -errno;
> > @@ -410,7 +417,9 @@ static int qemu_gluster_open(BlockDriverState *bs,
> > QDict *options,
> >
> > out:
> > qemu_opts_del(opts);
> > - qemu_gluster_gconf_free(gconf);
> > + if (gconf) {
> > + qapi_free_BlockdevOptionsGluster(gconf);
> > + }
>
> Per Markus's suggestion, will remove conditional on commit.
>
> > if (!ret) {
> > return ret;
> > }
> > @@ -429,7 +438,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState
> > *state,
> > int ret = 0;
> > BDRVGlusterState *s;
> > BDRVGlusterReopenState *reop_s;
> > - GlusterConf *gconf = NULL;
> > + BlockdevOptionsGluster *gconf;
> > int open_flags = 0;
> >
> > assert(state != NULL);
> > @@ -442,9 +451,9 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState
> > *state,
> >
> > qemu_gluster_parse_flags(state->flags, &open_flags);
> >
> > - gconf = g_new0(GlusterConf, 1);
> > -
> > + 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);
> > if (reop_s->glfs == NULL) {
> > ret = -errno;
> > @@ -470,7 +479,9 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState
> > *state,
> >
> > exit:
> > /* state->opaque will be freed in either the _abort or _commit */
> > - qemu_gluster_gconf_free(gconf);
> > + if (gconf) {
> > + qapi_free_BlockdevOptionsGluster(gconf);
> > + }
>
> Here too.
>
> > return ret;
> > }
> >
> > @@ -572,14 +583,15 @@ static inline int qemu_gluster_zerofill(struct
> > glfs_fd *fd, int64_t offset,
> > static int qemu_gluster_create(const char *filename,
> > QemuOpts *opts, Error **errp)
> > {
> > + BlockdevOptionsGluster *gconf;
> > struct glfs *glfs;
> > struct glfs_fd *fd;
> > int ret = 0;
> > int prealloc = 0;
> > int64_t total_size = 0;
> > char *tmp = NULL;
> > - GlusterConf *gconf = g_new0(GlusterConf, 1);
> >
> > + gconf = g_new0(BlockdevOptionsGluster, 1);
> > gconf->debug_level = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
> > GLUSTER_DEBUG_DEFAULT);
> > if (gconf->debug_level < 0) {
> > @@ -587,6 +599,7 @@ static int qemu_gluster_create(const char *filename,
> > } else if (gconf->debug_level > GLUSTER_DEBUG_MAX) {
> > gconf->debug_level = GLUSTER_DEBUG_MAX;
> > }
> > + gconf->has_debug_level = true;
> >
> > glfs = qemu_gluster_init(gconf, filename, errp);
> > if (!glfs) {
> > @@ -628,7 +641,9 @@ static int qemu_gluster_create(const char *filename,
> > }
> > out:
> > g_free(tmp);
> > - qemu_gluster_gconf_free(gconf);
> > + if (gconf) {
> > + qapi_free_BlockdevOptionsGluster(gconf);
> > + }
I almost missed this one; will remove this conditional as well.
> > if (glfs) {
> > glfs_fini(glfs);
> > }
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index a7fdb28..1fa0674 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1658,13 +1658,14 @@
> > # @host_device, @host_cdrom: Since 2.1
> > #
> > # Since: 2.0
> > +# @gluster: Since 2.7
> > ##
> > { 'enum': 'BlockdevDriver',
> > 'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> > - 'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> > - 'http', 'https', 'luks', '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', 'http', 'https', 'luks', 'null-aio', 'null-co',
> > + 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp',
> > + 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> >
> > ##
> > # @BlockdevOptionsFile
> > @@ -2057,6 +2058,63 @@
> > '*read-pattern': 'QuorumReadPattern' } }
> >
> > ##
> > +# @GlusterTransport
> > +#
> > +# An enumeration of Gluster transport type
> > +#
> > +# @tcp: TCP - Transmission Control Protocol
> > +#
> > +# @unix: UNIX - Unix domain socket
> > +#
> > +# Since: 2.7
> > +##
> > +{ 'enum': 'GlusterTransport',
> > + 'data': [ 'unix', 'tcp' ] }
> > +
> > +
> > +##
> > +# @GlusterServer
> > +#
> > +# Captures the address of a socket
> > +#
> > +# Details for connecting to a gluster server
> > +#
> > +# @type: Transport type used for gluster connection
> > +#
> > +# @unix: socket file
> > +#
> > +# @tcp: host address and port number
> > +#
> > +# Since: 2.7
> > +##
> > +{ 'union': 'GlusterServer',
> > + 'base': { 'type': 'GlusterTransport' },
> > + 'discriminator': 'type',
> > + 'data': { 'unix': 'UnixSocketAddress',
> > + 'tcp': 'InetSocketAddress' } }
> > +
> > +##
> > +# @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
> > +#
> > +# @server: gluster server description
> > +#
> > +# @debug_level: #optional libgfapi log level (default '4' which is Error)
> > +#
>
> Per Eric's suggestion, will change this to debug-level on commit.
>
> > +# Since: 2.7
> > +##
> > +{ 'struct': 'BlockdevOptionsGluster',
> > + 'data': { 'volume': 'str',
> > + 'path': 'str',
> > + 'server': 'GlusterServer',
> > + '*debug_level': 'int' } }
> > +
> > +##
> > # @BlockdevOptions
> > #
> > # Options for creating a block device. Many options are available for all
> > @@ -2119,7 +2177,7 @@
> > 'file': 'BlockdevOptionsFile',
> > 'ftp': 'BlockdevOptionsFile',
> > 'ftps': 'BlockdevOptionsFile',
> > -# TODO gluster: Wait for structured options
> > + 'gluster': 'BlockdevOptionsGluster',
> > 'host_cdrom': 'BlockdevOptionsFile',
> > 'host_device':'BlockdevOptionsFile',
> > 'http': 'BlockdevOptionsFile',
> > --
> > 2.7.4
> >
>
> After fix-ups:
>
> Reviewed-by: Jeff Cody <jc...@redhat.com>
