Prasanna Kumar Kalever <prasanna.kale...@redhat.com> writes: > 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); > + }
I'd prefer something like gsconf->u.tcp.port = g_strdup_printf("%d", uri->has_port ? uri->port : GLUSTER_DEFAULT_PORT); or with a new variable port: port = uri->has_port ? uri->port : GLUSTER_DEFAULT_PORT; gsconf->u.tcp.port = g_strdup_printf("%d", port); Not worth a respin now, we can take it as is. > } > > 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], > + gconf->server->u.q_unix.path, 0); > + } else { > + ret = glfs_set_volfile_server(glfs, > + > GlusterTransport_lookup[gconf->server->type], > + gconf->server->u.tcp.host, > + atoi(gconf->server->u.tcp.port)); atoi() is okay, because its argument can only come from qemu_gluster_parse_uri(), so atoi() can't fail. It'll become wrong when the argument comes from a QMP client. > + } > 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; > + } > > 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); > + } > 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); > + } > 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); > + } > 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' } } We might want to add comments describing the relationship to SocketAddress, but that can be done on top. > + > +## > +# @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) > +# > +# 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', Reviewed-by: Markus Armbruster <arm...@redhat.com>