Re: [Qemu-devel] [PATCH v20 3/5] block/gluster: deprecate rdma support

[Eric Blake]

On 07/19/2016 10:57 AM, Prasanna Kumar Kalever wrote:

Grammar suggestions, can be squashed in by the maintainer (in addition
to what Markus pointed out):

> gluster volfile server fetch happens through unix and/or tcp, it doesn't
> support volfile fetch over rdma, the rdma code may actually mislead,

s/rdma, the/rdma. The/

> to make sure things do not break, for now we fallback to tcp when requested
s/to make/so to make/

> for rdma with a warning.

s/rdma with/rdma, with/

> 
> If you are wondering how this worked all these days, its the gluster libgfapi
> code which handles anything other than unix transport as socket/tcp, sad but
> true.
> 
> Also gluster doesn't support ipv6 addresses, removing the ipv6 related
> comments/docs section
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 40ee852..8a54ad4 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -12,6 +12,7 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qemu/uri.h"
> +#include "qemu/error-report.h"
>  
>  #define GLUSTER_OPT_FILENAME"filename"
>  #define GLUSTER_OPT_DEBUG   "debug"
> @@ -134,12 +135,10 @@ static int parse_volume_options(GlusterConf *gconf, 
> char *path)
>   *
>   * 'transport' specifies the transport type used to connect to gluster
>   * management daemon (glusterd). Valid transport types are
> - * tcp, unix and rdma. If a transport type isn't specified, then tcp
> - * type is assumed.
> + * tcp, unix. If a transport type isn't specified, then tcp type is assumed.

s/tcp, unix/tcp or unix/

>   *
>   * 'host' specifies the host where the volume file specification for
> - * the given volume resides. This can be either hostname, ipv4 address
> - * or ipv6 address. ipv6 address needs to be within square brackets [ ].
> + * the given volume resides. This can be either hostname, ipv4 address.

s/either hostname, ipv4 address/either a hostname or an ipv4 address/


> @@ -1048,6 +1046,12 @@ static BlockDriver bdrv_gluster_unix = {
>  .create_opts  = _gluster_create_opts,
>  };
>  
> +/* rdma is deprecated (actually never supported for volfile fetch)
> + * lets maintain for the protocol compatibility, to make sure things

s/fetch) lets maintain for/fetch). Let's maintain it for/

> + * won't break immediately for now gluster+rdma will fall back to gluster+tcp

s/immediately for now gluster+rdma/immediately. For now, gluster+rdma/

> + * protocol with Warning

s/Warning/a warning/

> + * TODO: remove gluster+rdma interface support
> + */
>  static BlockDriver bdrv_gluster_rdma = {
>  .format_name  = "gluster",
>  .protocol_name= "gluster+rdma",
> 

Reviewed-by: Eric Blake 

-- 
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 v20 3/5] block/gluster: deprecate rdma support

[Jeff Cody]

On Tue, Jul 19, 2016 at 10:27:31PM +0530, Prasanna Kumar Kalever wrote:
> gluster volfile server fetch happens through unix and/or tcp, it doesn't
> support volfile fetch over rdma, the rdma code may actually mislead,
> to make sure things do not break, for now we fallback to tcp when requested
> for rdma with a warning.
> 
> If you are wondering how this worked all these days, its the gluster libgfapi
> code which handles anything other than unix transport as socket/tcp, sad but
> true.
> 
> Also gluster doesn't support ipv6 addresses, removing the ipv6 related
> comments/docs section
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 40ee852..8a54ad4 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -12,6 +12,7 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qemu/uri.h"
> +#include "qemu/error-report.h"
>  
>  #define GLUSTER_OPT_FILENAME"filename"
>  #define GLUSTER_OPT_DEBUG   "debug"
> @@ -134,12 +135,10 @@ static int parse_volume_options(GlusterConf *gconf, 
> char *path)
>   *
>   * 'transport' specifies the transport type used to connect to gluster
>   * management daemon (glusterd). Valid transport types are
> - * tcp, unix and rdma. If a transport type isn't specified, then tcp
> - * type is assumed.
> + * tcp, unix. If a transport type isn't specified, then tcp type is assumed.
>   *
>   * 'host' specifies the host where the volume file specification for
> - * the given volume resides. This can be either hostname, ipv4 address
> - * or ipv6 address. ipv6 address needs to be within square brackets [ ].
> + * the given volume resides. This can be either hostname, ipv4 address.
>   * If transport type is 'unix', then 'host' field should not be specified.
>   * The 'socket' field needs to be populated with the path to unix domain
>   * socket.
> @@ -158,11 +157,8 @@ static int parse_volume_options(GlusterConf *gconf, char 
> *path)
>   * file=gluster://1.2.3.4/testvol/a.img
>   * file=gluster+tcp://1.2.3.4/testvol/a.img
>   * file=gluster+tcp://1.2.3.4:24007/testvol/dir/a.img
> - * file=gluster+tcp://[1:2:3:4:5:6:7:8]/testvol/dir/a.img
> - * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img
>   * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img
>   * 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)
>  {
> @@ -185,7 +181,9 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  gconf->transport = g_strdup("unix");
>  is_unix = true;
>  } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -gconf->transport = g_strdup("rdma");
> +gconf->transport = g_strdup("tcp");
> +error_report("Warning: rdma feature is not supported falling "
> + "back to tcp");
>  } else {
>  ret = -EINVAL;
>  goto out;
> @@ -1048,6 +1046,12 @@ static BlockDriver bdrv_gluster_unix = {
>  .create_opts  = _gluster_create_opts,
>  };
>  
> +/* rdma is deprecated (actually never supported for volfile fetch)
> + * lets maintain for the protocol compatibility, to make sure things
> + * won't break immediately for now gluster+rdma will fall back to gluster+tcp
> + * protocol with Warning
> + * TODO: remove gluster+rdma interface support
> + */
>  static BlockDriver bdrv_gluster_rdma = {
>  .format_name  = "gluster",
>  .protocol_name= "gluster+rdma",
> -- 
> 2.7.4
> 
>

Modulo Markus's and Eric's comments:

Reviewed-by: Jeff Cody 

(I'll make the comment changes on commit)

Re: [Qemu-devel] [PATCH v20 3/5] block/gluster: deprecate rdma support

[Markus Armbruster]

Prasanna Kumar Kalever  writes:

> gluster volfile server fetch happens through unix and/or tcp, it doesn't
> support volfile fetch over rdma, the rdma code may actually mislead,
> to make sure things do not break, for now we fallback to tcp when requested
> for rdma with a warning.
>
> If you are wondering how this worked all these days, its the gluster libgfapi
> code which handles anything other than unix transport as socket/tcp, sad but
> true.
>
> Also gluster doesn't support ipv6 addresses, removing the ipv6 related
> comments/docs section
>
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/block/gluster.c b/block/gluster.c
> index 40ee852..8a54ad4 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -12,6 +12,7 @@
>  #include "block/block_int.h"
>  #include "qapi/error.h"
>  #include "qemu/uri.h"
> +#include "qemu/error-report.h"
>  
>  #define GLUSTER_OPT_FILENAME"filename"
>  #define GLUSTER_OPT_DEBUG   "debug"
> @@ -134,12 +135,10 @@ static int parse_volume_options(GlusterConf *gconf, 
> char *path)
>   *
>   * 'transport' specifies the transport type used to connect to gluster
>   * management daemon (glusterd). Valid transport types are
> - * tcp, unix and rdma. If a transport type isn't specified, then tcp
> - * type is assumed.
> + * tcp, unix. If a transport type isn't specified, then tcp type is assumed.
>   *
>   * 'host' specifies the host where the volume file specification for
> - * the given volume resides. This can be either hostname, ipv4 address
> - * or ipv6 address. ipv6 address needs to be within square brackets [ ].
> + * the given volume resides. This can be either hostname, ipv4 address.

"either hostname or ipv4 address"

Could be touched up on commit, or in a cleanup patch on top.

>   * If transport type is 'unix', then 'host' field should not be specified.
>   * The 'socket' field needs to be populated with the path to unix domain
>   * socket.
> @@ -158,11 +157,8 @@ static int parse_volume_options(GlusterConf *gconf, char 
> *path)
>   * file=gluster://1.2.3.4/testvol/a.img
>   * file=gluster+tcp://1.2.3.4/testvol/a.img
>   * file=gluster+tcp://1.2.3.4:24007/testvol/dir/a.img
> - * file=gluster+tcp://[1:2:3:4:5:6:7:8]/testvol/dir/a.img
> - * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img
>   * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img
>   * 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)
>  {
> @@ -185,7 +181,9 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, 
> const char *filename)
>  gconf->transport = g_strdup("unix");
>  is_unix = true;
>  } else if (!strcmp(uri->scheme, "gluster+rdma")) {
> -gconf->transport = g_strdup("rdma");
> +gconf->transport = g_strdup("tcp");
> +error_report("Warning: rdma feature is not supported falling "
> + "back to tcp");

"not supported, falling back"

Could be touched up on commit, or in a cleanup patch on top.

>  } else {
>  ret = -EINVAL;
>  goto out;
> @@ -1048,6 +1046,12 @@ static BlockDriver bdrv_gluster_unix = {
>  .create_opts  = _gluster_create_opts,
>  };
>  
> +/* rdma is deprecated (actually never supported for volfile fetch)
> + * lets maintain for the protocol compatibility, to make sure things
> + * won't break immediately for now gluster+rdma will fall back to gluster+tcp
> + * protocol with Warning
> + * TODO: remove gluster+rdma interface support
> + */
>  static BlockDriver bdrv_gluster_rdma = {
>  .format_name  = "gluster",
>  .protocol_name= "gluster+rdma",

This comment could use some polish, but we can do that on top.

Reviewed-by: Markus Armbruster 

[Qemu-devel] [PATCH v20 3/5] block/gluster: deprecate rdma support

[Prasanna Kumar Kalever]

gluster volfile server fetch happens through unix and/or tcp, it doesn't
support volfile fetch over rdma, the rdma code may actually mislead,
to make sure things do not break, for now we fallback to tcp when requested
for rdma with a warning.

If you are wondering how this worked all these days, its the gluster libgfapi
code which handles anything other than unix transport as socket/tcp, sad but
true.

Also gluster doesn't support ipv6 addresses, removing the ipv6 related
comments/docs section

Signed-off-by: Prasanna Kumar Kalever 
---
 block/gluster.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 40ee852..8a54ad4 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -12,6 +12,7 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qemu/uri.h"
+#include "qemu/error-report.h"
 
 #define GLUSTER_OPT_FILENAME"filename"
 #define GLUSTER_OPT_DEBUG   "debug"
@@ -134,12 +135,10 @@ static int parse_volume_options(GlusterConf *gconf, char 
*path)
  *
  * 'transport' specifies the transport type used to connect to gluster
  * management daemon (glusterd). Valid transport types are
- * tcp, unix and rdma. If a transport type isn't specified, then tcp
- * type is assumed.
+ * tcp, unix. If a transport type isn't specified, then tcp type is assumed.
  *
  * 'host' specifies the host where the volume file specification for
- * the given volume resides. This can be either hostname, ipv4 address
- * or ipv6 address. ipv6 address needs to be within square brackets [ ].
+ * the given volume resides. This can be either hostname, ipv4 address.
  * If transport type is 'unix', then 'host' field should not be specified.
  * The 'socket' field needs to be populated with the path to unix domain
  * socket.
@@ -158,11 +157,8 @@ static int parse_volume_options(GlusterConf *gconf, char 
*path)
  * file=gluster://1.2.3.4/testvol/a.img
  * file=gluster+tcp://1.2.3.4/testvol/a.img
  * file=gluster+tcp://1.2.3.4:24007/testvol/dir/a.img
- * file=gluster+tcp://[1:2:3:4:5:6:7:8]/testvol/dir/a.img
- * file=gluster+tcp://[1:2:3:4:5:6:7:8]:24007/testvol/dir/a.img
  * file=gluster+tcp://host.domain.com:24007/testvol/dir/a.img
  * 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)
 {
@@ -185,7 +181,9 @@ static int qemu_gluster_parseuri(GlusterConf *gconf, const 
char *filename)
 gconf->transport = g_strdup("unix");
 is_unix = true;
 } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-gconf->transport = g_strdup("rdma");
+gconf->transport = g_strdup("tcp");
+error_report("Warning: rdma feature is not supported falling "
+ "back to tcp");
 } else {
 ret = -EINVAL;
 goto out;
@@ -1048,6 +1046,12 @@ static BlockDriver bdrv_gluster_unix = {
 .create_opts  = _gluster_create_opts,
 };
 
+/* rdma is deprecated (actually never supported for volfile fetch)
+ * lets maintain for the protocol compatibility, to make sure things
+ * won't break immediately for now gluster+rdma will fall back to gluster+tcp
+ * protocol with Warning
+ * TODO: remove gluster+rdma interface support
+ */
 static BlockDriver bdrv_gluster_rdma = {
 .format_name  = "gluster",
 .protocol_name= "gluster+rdma",
-- 
2.7.4