Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema

2015-11-13 Thread Markus Armbruster 
Prasanna Kumar Kalever  writes:

> this patch adds GlusterConf to qapi/block-core.json
>
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c  | 104 
> +--
>  qapi/block-core.json |  60 +++--
>  2 files changed, 109 insertions(+), 55 deletions(-)
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 425fdab..bbefe43 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1375,13 +1375,14 @@
>  # Drivers that are supported in block device operations.
>  #
>  # @host_device, @host_cdrom: Since 2.1
> +# @gluster: Since 2.5
>  #
>  # Since: 2.0
>  ##
>  { 'enum': 'BlockdevDriver',
>'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
> -'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -'http', 'https', 'null-aio', 'null-co', 'parallels',
> +'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> +'host_device', 'http', 'https', 'null-aio', 'null-co', 
> 'parallels',
>  'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
>  'vmdk', 'vpc', 'vvfat' ] }
>  
> @@ -1797,6 +1798,59 @@
>  '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @GlusterTransport
> +#
> +# An enumeration of Gluster transport type
> +#
> +# @tcp:   TCP   - Transmission Control Protocol
> +#
> +# @unix:  UNIX  - Unix domain socket
> +#
> +# @rdma:  RDMA  - Remote direct memory access
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'GlusterTransport', 'data': [ 'tcp', 'unix', 'rdma'] }
> +
> +##
> +# @GlusterServer
> +#
> +# Details for connecting to a gluster server
> +#
> +# @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 management
> +#   daemon (default 'tcp')
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GlusterServer',
> +  'data': { 'host': 'str',
> +'*port': 'int',
> +'*transport': 'GlusterTransport' } }

Are you reinventing SocketAddress?

Differences:

* You support transport rdma.

* You don't have a way to control IPv4/IPv6.  Why?

* You don't support file descriptor passing.  Why?

Should this be something like

   { 'union': 'GlusterServer',
  'data': {
... all the applicable members of SocketAddress ...
'rdma': 'RdmaAddress'
   }

?

> +
> +##
> +# @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
> +#
> +# @servers:  gluster server description
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsGluster',
> +  'data': { 'volume': 'str',
> +'path': 'str',
> +'server': 'GlusterServer' } }
> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.
> @@ -1816,7 +1870,7 @@
>'file':   'BlockdevOptionsFile',
>'ftp':'BlockdevOptionsFile',
>'ftps':   'BlockdevOptionsFile',
> -# TODO gluster: Wait for structured options
> +  'gluster':'BlockdevOptionsGluster',
>'host_cdrom': 'BlockdevOptionsFile',
>'host_device':'BlockdevOptionsFile',
>'http':   'BlockdevOptionsFile',

Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema

2015-11-12 Thread Eric Blake 
On 11/12/2015 03:22 AM, Prasanna Kumar Kalever wrote:
> this patch adds GlusterConf to qapi/block-core.json

Missing a vNN in the subject line.  I think we're up to v14?  But it
doesn't affect what 'git am' will do.

> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c  | 104 
> +--
>  qapi/block-core.json |  60 +++--
>  2 files changed, 109 insertions(+), 55 deletions(-)
> 

Modulo Jeff's findings,

> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..615f28b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c

> -typedef struct GlusterConf {
> -char *host;
> -int port;
> -char *volume;
> -char *path;
> -char *transport;
> -} GlusterConf;
> -
> -

So this is the struct being replaced by qapi BlockdevOptionsGluster.
/me jumps ahead to [1] in my review, before continuing here...

I'm back. Looks like your qapi struct matches this nicely, with the
possible exception of what happens if we try to avoid churn by
using/enforcing a 1-element array now rather than converting to array in
patch 4.

> @@ -143,8 +127,10 @@ static int parse_volume_options(GlusterConf *gconf, char 
> *path)
>   * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
>   * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
>   */
> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
> + const char *filename)

I'm not sure from looking at just the signature why you changed from
*gconf to **pgconf; maybe that sort of conversion would have been worth
mentioning in the commit message (a good rule of thumb - if the change
isn't blatantly obvious, then calling it out in the commit message will
prepare reviewers for it).

> @@ -190,13 +180,23 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  ret = -EINVAL;
>  goto out;
>  }
> -gconf->host = g_strdup(qp->p[0].value);
> +gconf->server->host = g_strdup(qp->p[0].value);
>  } else {
> -gconf->host = g_strdup(uri->server ? uri->server : "localhost");
> -gconf->port = uri->port;
> +gconf->server->host = g_strdup(uri->server ? uri->server : 
> "localhost");
> +if (uri->port) {
> +gconf->server->port = uri->port;
> +} else {
> +gconf->server->port = GLUSTER_DEFAULT_PORT;
> +}
> +gconf->server->has_port = true;
>  }
>  
> +*pgconf = gconf;

Okay, now I see where the change in signature comes into play - you want
to return a new allocation to the user, but only on success.  But I'm
still not necessarily convinced that you need it.  See more at [3] below.

> +
>  out:
> +if (ret < 0) {
> +qapi_free_BlockdevOptionsGluster(gconf);
> +}
>  if (qp) {
>  query_params_free(qp);
>  }
> @@ -204,14 +204,15 @@ out:
>  return ret;
>  }

Okay, this parseuri conversion is sane.  It will need tweaking in patch
4 to deal with gconf->server becoming a list rather than a single
server, but as long as both patches go in, we should be okay.

>  
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char 
> *filename,
> -  Error **errp)
> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
> +  const char *filename, Error **errp)
>  {
> -struct glfs *glfs = NULL;
> +struct glfs *glfs;

Jeff already spotted that the change here is spurious.

>  int ret;
>  int old_errno;
> +BlockdevOptionsGluster *gconf;
>  
> -ret = qemu_gluster_parseuri(gconf, filename);
> +ret = qemu_gluster_parseuri(, filename);
>  if (ret < 0) {
>  error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>   "volume/path[?socket=...]");
> @@ -224,8 +225,9 @@ 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);
> +ret = glfs_set_volfile_server(glfs,
> +  
> GlusterTransport_lookup[gconf->server->transport],

Line longer than 80 characters; I might have used an intermediate const
char * variable to cut down on the length. But as long as it gets past
scripts/checkpatch.pl, I won't insist on a reformat.

> +  gconf->server->host, gconf->server->port);

Ouch - since you aren't validating that gconf->server->port fits in 16
bits, you may be passing something so large that it silently wraps around.

>  if (ret < 0) {
>  goto out;
>  }
> @@ -242,10 +244,10 @@ static struct glfs *qemu_gluster_init(GlusterConf 
> *gconf, const char *filename,
>

Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema

2015-11-12 Thread Jeff Cody 
On Thu, Nov 12, 2015 at 03:52:07PM +0530, Prasanna Kumar Kalever wrote:
> this patch adds GlusterConf to qapi/block-core.json
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c  | 104 
> +--
>  qapi/block-core.json |  60 +++--
>  2 files changed, 109 insertions(+), 55 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..615f28b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -11,6 +11,10 @@
>  #include "block/block_int.h"
>  #include "qemu/uri.h"
>  
> +#define GLUSTER_OPT_FILENAME"filename"
> +#define GLUSTER_DEFAULT_PORT24007
> +
> +
>  typedef struct GlusterAIOCB {
>  int64_t size;
>  int ret;
> @@ -29,15 +33,6 @@ typedef struct BDRVGlusterReopenState {
>  struct glfs_fd *fd;
>  } BDRVGlusterReopenState;
>  
> -typedef struct GlusterConf {
> -char *host;
> -int port;
> -char *volume;
> -char *path;
> -char *transport;
> -} GlusterConf;
> -
> -
>  static QemuOptsList qemu_gluster_create_opts = {
>  .name = "qemu-gluster-create-opts",
>  .head = QTAILQ_HEAD_INITIALIZER(qemu_gluster_create_opts.head),
> @@ -61,7 +56,7 @@ static QemuOptsList runtime_opts = {
>  .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>  .desc = {
>  {
> -.name = "filename",
> +.name = GLUSTER_OPT_FILENAME,
>  .type = QEMU_OPT_STRING,
>  .help = "URL to the gluster image",
>  },
> @@ -70,18 +65,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;
>  
> @@ -143,8 +127,10 @@ static int parse_volume_options(GlusterConf *gconf, char 
> *path)
>   * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
>   * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
>   */
> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
> + const char *filename)
>  {
> +BlockdevOptionsGluster *gconf;
>  URI *uri;
>  QueryParams *qp = NULL;
>  bool is_unix = false;
> @@ -155,20 +141,24 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  return -EINVAL;
>  }
>  
> +gconf = g_new0(BlockdevOptionsGluster, 1);
> +gconf->server = g_new0(GlusterServer, 1);
> +
>  /* transport */
>  if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> -gconf->transport = g_strdup("tcp");
> +gconf->server->transport = GLUSTER_TRANSPORT_TCP;
>  } else if (!strcmp(uri->scheme, "gluster+tcp")) {
> -gconf->transport = g_strdup("tcp");
> +gconf->server->transport = GLUSTER_TRANSPORT_TCP;
>  } else if (!strcmp(uri->scheme, "gluster+unix")) {
> -gconf->transport = g_strdup("unix");
> +gconf->server->transport = GLUSTER_TRANSPORT_UNIX;
>  is_unix = true;
>  } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -gconf->transport = g_strdup("rdma");
> +gconf->server->transport = GLUSTER_TRANSPORT_RDMA;
>  } else {
>  ret = -EINVAL;
>  goto out;
>  }
> +gconf->server->has_transport = true;
>  
>  ret = parse_volume_options(gconf, uri->path);
>  if (ret < 0) {
> @@ -190,13 +180,23 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  ret = -EINVAL;
>  goto out;
>  }
> -gconf->host = g_strdup(qp->p[0].value);
> +gconf->server->host = g_strdup(qp->p[0].value);
>  } else {
> -gconf->host = g_strdup(uri->server ? uri->server : "localhost");
> -gconf->port = uri->port;
> +gconf->server->host = g_strdup(uri->server ? uri->server : 
> "localhost");
> +if (uri->port) {
> +gconf->server->port = uri->port;
> +} else {
> +gconf->server->port = GLUSTER_DEFAULT_PORT;
> +}
> +gconf->server->has_port = true;
>  }
>  
> +*pgconf = gconf;
> +
>  out:
> +if (ret < 0) {
> +qapi_free_BlockdevOptionsGluster(gconf);
> +}
>  if (qp) {
>  query_params_free(qp);
>  }
> @@ -204,14 +204,15 @@ out:
>  return ret;
>  }
>  
> -static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char 
> *filename,
> -  Error **errp)
> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
> +

Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema

2015-11-12 Thread Eric Blake 
On 11/12/2015 03:22 AM, Prasanna Kumar Kalever wrote:
> this patch adds GlusterConf to qapi/block-core.json
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c  | 104 
> +--
>  qapi/block-core.json |  60 +++--
>  2 files changed, 109 insertions(+), 55 deletions(-)

One more comment:

> -static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
> +static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
> + const char *filename)
>  {
> +BlockdevOptionsGluster *gconf;
>  URI *uri;
>  QueryParams *qp = NULL;
>  bool is_unix = false;
> @@ -155,20 +141,24 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  return -EINVAL;

If we hit this early return, then *pgconf was never assigned...


> +static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
> +  const char *filename, Error **errp)
>  {
> -struct glfs *glfs = NULL;
> +struct glfs *glfs;
>  int ret;
>  int old_errno;
> +BlockdevOptionsGluster *gconf;

but here, gconf is uninitialized,

>  
> -ret = qemu_gluster_parseuri(gconf, filename);
> +ret = qemu_gluster_parseuri(, filename);
>  if (ret < 0) {
>  error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"
>   "volume/path[?socket=...]");

which means we can goto out with it uninitialized...

> @@ -224,8 +225,9 @@ 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);
> +ret = glfs_set_volfile_server(glfs,
> +  
> GlusterTransport_lookup[gconf->server->transport],
> +  gconf->server->host, gconf->server->port);
>  if (ret < 0) {
>  goto out;
>  }

...vs. here where we can goto out with it initialized.

So whatever solution you use to plug the leak must be careful to not
free uninitialized memory.  Easiest solution - initialize gconf to NULL
before qemu_gluster_parseuri (or else go back to a *gconf parameter
rather than **pgconf).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema

2015-11-12 Thread Eric Blake 
On 11/12/2015 03:22 AM, Prasanna Kumar Kalever wrote:
> this patch adds GlusterConf to qapi/block-core.json
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c  | 104 
> +--
>  qapi/block-core.json |  60 +++--
>  2 files changed, 109 insertions(+), 55 deletions(-)
> 

Pointing it out here for completeness, even though I first stumbled on
it when reviewing 4/4:

> @@ -190,13 +180,23 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  ret = -EINVAL;
>  goto out;
>  }
> -gconf->host = g_strdup(qp->p[0].value);
> +gconf->server->host = g_strdup(qp->p[0].value);

This is abusing the 'host' field of GlusterServer to track a socket
path, and ignores the fact that port is meaningless for a
gluster+unix:// connection.

> @@ -224,8 +225,9 @@ 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);
> +ret = glfs_set_volfile_server(glfs,
> +  
> GlusterTransport_lookup[gconf->server->transport],
> +  gconf->server->host, gconf->server->port);

At least gluster itself has the same overloaded abuse of terminology;
I'm hoping that a port of 0 is okay when requesting a "unix"
volfile_server.  [I don't know, because I didn't read the docs for
glfs_set_volfile_server()]

> +##
> +# @GlusterServer
> +#
> +# Details for connecting to a gluster server
> +#
> +# @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 management
> +#   daemon (default 'tcp')
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'GlusterServer',
> +  'data': { 'host': 'str',
> +'*port': 'int',
> +'*transport': 'GlusterTransport' } }

And my idea on patch 4/4 was that converting this from simple struct to
flat union might be a more realistic view of things (if transport is
'unix', there can't be a port; and rather than abusing the name 'host'
we could use the name 'socket'; similarly for 'rdma') - but without
additional qapi support, I don't know that we can have an optional
'transport' and still have a discriminated union in time for 2.5.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 3/4] block/gluster: using new qapi schema

2015-11-12 Thread Prasanna Kumar Kalever 
this patch adds GlusterConf to qapi/block-core.json

Signed-off-by: Prasanna Kumar Kalever 
---
 block/gluster.c  | 104 +--
 qapi/block-core.json |  60 +++--
 2 files changed, 109 insertions(+), 55 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index ededda2..615f28b 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -11,6 +11,10 @@
 #include "block/block_int.h"
 #include "qemu/uri.h"
 
+#define GLUSTER_OPT_FILENAME"filename"
+#define GLUSTER_DEFAULT_PORT24007
+
+
 typedef struct GlusterAIOCB {
 int64_t size;
 int ret;
@@ -29,15 +33,6 @@ typedef struct BDRVGlusterReopenState {
 struct glfs_fd *fd;
 } BDRVGlusterReopenState;
 
-typedef struct GlusterConf {
-char *host;
-int port;
-char *volume;
-char *path;
-char *transport;
-} GlusterConf;
-
-
 static QemuOptsList qemu_gluster_create_opts = {
 .name = "qemu-gluster-create-opts",
 .head = QTAILQ_HEAD_INITIALIZER(qemu_gluster_create_opts.head),
@@ -61,7 +56,7 @@ static QemuOptsList runtime_opts = {
 .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
 .desc = {
 {
-.name = "filename",
+.name = GLUSTER_OPT_FILENAME,
 .type = QEMU_OPT_STRING,
 .help = "URL to the gluster image",
 },
@@ -70,18 +65,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;
 
@@ -143,8 +127,10 @@ static int parse_volume_options(GlusterConf *gconf, char 
*path)
  * file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glusterd.socket
  * file=gluster+rdma://1.2.3.4:24007/testvol/a.img
  */
-static int qemu_gluster_parseuri(GlusterConf *gconf, const char *filename)
+static int qemu_gluster_parseuri(BlockdevOptionsGluster **pgconf,
+ const char *filename)
 {
+BlockdevOptionsGluster *gconf;
 URI *uri;
 QueryParams *qp = NULL;
 bool is_unix = false;
@@ -155,20 +141,24 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
const char *filename)
 return -EINVAL;
 }
 
+gconf = g_new0(BlockdevOptionsGluster, 1);
+gconf->server = g_new0(GlusterServer, 1);
+
 /* transport */
 if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-gconf->transport = g_strdup("tcp");
+gconf->server->transport = GLUSTER_TRANSPORT_TCP;
 } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-gconf->transport = g_strdup("tcp");
+gconf->server->transport = GLUSTER_TRANSPORT_TCP;
 } else if (!strcmp(uri->scheme, "gluster+unix")) {
-gconf->transport = g_strdup("unix");
+gconf->server->transport = GLUSTER_TRANSPORT_UNIX;
 is_unix = true;
 } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-gconf->transport = g_strdup("rdma");
+gconf->server->transport = GLUSTER_TRANSPORT_RDMA;
 } else {
 ret = -EINVAL;
 goto out;
 }
+gconf->server->has_transport = true;
 
 ret = parse_volume_options(gconf, uri->path);
 if (ret < 0) {
@@ -190,13 +180,23 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
const char *filename)
 ret = -EINVAL;
 goto out;
 }
-gconf->host = g_strdup(qp->p[0].value);
+gconf->server->host = g_strdup(qp->p[0].value);
 } else {
-gconf->host = g_strdup(uri->server ? uri->server : "localhost");
-gconf->port = uri->port;
+gconf->server->host = g_strdup(uri->server ? uri->server : 
"localhost");
+if (uri->port) {
+gconf->server->port = uri->port;
+} else {
+gconf->server->port = GLUSTER_DEFAULT_PORT;
+}
+gconf->server->has_port = true;
 }
 
+*pgconf = gconf;
+
 out:
+if (ret < 0) {
+qapi_free_BlockdevOptionsGluster(gconf);
+}
 if (qp) {
 query_params_free(qp);
 }
@@ -204,14 +204,15 @@ out:
 return ret;
 }
 
-static struct glfs *qemu_gluster_init(GlusterConf *gconf, const char *filename,
-  Error **errp)
+static struct glfs *qemu_gluster_init(BlockdevOptionsGluster **pgconf,
+  const char *filename, Error **errp)
 {
-struct glfs *glfs = NULL;
+struct glfs *glfs;
 int ret;
 int old_errno;
+BlockdevOptionsGluster *gconf;
 
-ret = qemu_gluster_parseuri(gconf, filename);
+ret = qemu_gluster_parseuri(, filename);
 if (ret < 0) {
 error_setg(errp, "Usage: file=gluster[+transport]://[host[:port]]/"