[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema |
Date: |
Tue, 19 Jul 2016 16:39:39 -0400 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> ---
> 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 <address@hidden>
- [Qemu-devel] [PATCH v20 1/5] block/gluster: rename [server, volname, image] -> [host, volume, path], (continued)
- [Qemu-devel] [PATCH v20 1/5] block/gluster: rename [server, volname, image] -> [host, volume, path], Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 2/5] block/gluster: code cleanup, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 3/5] block/gluster: deprecate rdma support, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 4/5] block/gluster: using new qapi schema, Prasanna Kumar Kalever, 2016/07/19
- [Qemu-devel] [PATCH v20 5/5] block/gluster: add support for multiple gluster servers, Prasanna Kumar Kalever, 2016/07/19
- Re: [Qemu-devel] [PATCH v20 0/5] block/gluster: add support for multiple gluster servers, Jeff Cody, 2016/07/19