| Message ID | 1468947453-5433-5-git-send-email-prasanna.kalever@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > this patch adds 'GlusterServer' related schema in qapi/block-core.json > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@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 <armbru@redhat.com>
One more thing... Prasanna Kumar Kalever <prasanna.kalever@redhat.com> writes: > this patch adds 'GlusterServer' related schema in qapi/block-core.json > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@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 [...] > @@ -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); > + } qapi_free_FOO(NULL) is safe. Let's drop the conditional. Could be done on commit, or as a follow-up cleanup. > if (glfs) { > glfs_fini(glfs); > } [...] R-by stands.
On 07/19/2016 10:57 AM, Prasanna Kumar Kalever wrote: > this patch adds 'GlusterServer' related schema in qapi/block-core.json > > Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> > --- > block/gluster.c | 115 +++++++++++++++++++++++++++++---------------------- > qapi/block-core.json | 68 +++++++++++++++++++++++++++--- > 2 files changed, 128 insertions(+), 55 deletions(-) > > @@ -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); > + } You are throwing away the errno information that used to be reported here. Is that intentional? > > /* glfs_init sometimes doesn't set errno although docs suggest that */ > - if (errno == 0) > + if (errno == 0) { > errno = EINVAL; > + } Pre-existing, but this is too late for messing with errno. You are not guaranteed that errno is unchanged by error_setg_errno(). You aren't even guaranteed that errno == 0 after glfs_init() if it wasn't 0 before entry. You probably want a separate patch that reorders this to: errno = 0; ret = glfs_init(glfs); if (ret) { if (!errno) { errno = EINVAL; } error_setg... > +++ b/qapi/block-core.json > @@ -1658,13 +1658,14 @@ > # @host_device, @host_cdrom: Since 2.1 > # > # Since: 2.0 > +# @gluster: Since 2.7 I would stick this line a bit earlier: # @host_device, @host_cdrom: Since 2.1 # @gluster: Since 2.7 # # Since: 2.0 > @@ -2057,6 +2058,63 @@ > '*read-pattern': 'QuorumReadPattern' } } > > ## > +# @GlusterTransport > +# > +# An enumeration of Gluster transport type Maybe s/type/types/ > +# > +# @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) s/debug_level/debug-level/ - all new QAPI interfaces should prefer '-' over '_' (the generated C code is the same, though) > +# > +# 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', > Between my findings and Markus', it's up to the maintainer whether to take this as-is and then fix up the problems with a followup patch, or whether we need a v21. Reviewed-by: Eric Blake <eblake@redhat.com>
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.kalever@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); > + } > 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 <jcody@redhat.com>
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.kalever@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 <jcody@redhat.com>
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], + 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)); + } 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' } } + +## +# @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',
this patch adds 'GlusterServer' related schema in qapi/block-core.json Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- block/gluster.c | 115 +++++++++++++++++++++++++++++---------------------- qapi/block-core.json | 68 +++++++++++++++++++++++++++--- 2 files changed, 128 insertions(+), 55 deletions(-)