diff mbox

[10/15] gluster: Drop assumptions on SocketTransport names

Message ID 1488491046-2549-11-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster March 2, 2017, 9:44 p.m. UTC
qemu_gluster_glfs_init() passes the names of QAPI enumeration type
SocketTransport to glfs_set_volfile_server().  Works, because they
were chosen to match.  But the coupling is artificial.  Use the
appropriate literal strings instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/gluster.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Niels de Vos March 3, 2017, 6:40 a.m. UTC | #1
On Thu, Mar 02, 2017 at 10:44:01PM +0100, Markus Armbruster wrote:
> qemu_gluster_glfs_init() passes the names of QAPI enumeration type
> SocketTransport to glfs_set_volfile_server().  Works, because they
> were chosen to match.  But the coupling is artificial.  Use the
> appropriate literal strings instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/gluster.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 56b4abe..7236d59 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -412,8 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>  
>      for (server = gconf->server; server; server = server->next) {
>          if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
> -            ret = glfs_set_volfile_server(glfs,
> -                                   GlusterTransport_lookup[server->value->type],
> +            ret = glfs_set_volfile_server(glfs, "unix",
>                                     server->value->u.q_unix.path, 0);
>          } else {
>              if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
> @@ -423,8 +422,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>                  errno = EINVAL;
>                  goto out;
>              }
> -            ret = glfs_set_volfile_server(glfs,
> -                                   GlusterTransport_lookup[server->value->type],
> +            ret = glfs_set_volfile_server(glfs, "tcp",
>                                     server->value->u.tcp.host,
>                                     (int)port);
>          }
> -- 
> 2.7.4

Instead of the strings for "unix" and "tcp", I would have liked
#define's. Unfortunately it seems that these are not available in public
headers :-/

If this is easier to understand, I don't have any objections.

Reviewed-by: Niels de Vos <ndevos@redhat.com>
Markus Armbruster March 3, 2017, 7:31 a.m. UTC | #2
Niels de Vos <ndevos@redhat.com> writes:

> On Thu, Mar 02, 2017 at 10:44:01PM +0100, Markus Armbruster wrote:
>> qemu_gluster_glfs_init() passes the names of QAPI enumeration type
>> SocketTransport to glfs_set_volfile_server().  Works, because they
>> were chosen to match.  But the coupling is artificial.  Use the
>> appropriate literal strings instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/gluster.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 56b4abe..7236d59 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -412,8 +412,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>  
>>      for (server = gconf->server; server; server = server->next) {
>>          if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
>> -            ret = glfs_set_volfile_server(glfs,
>> -                                   GlusterTransport_lookup[server->value->type],
>> +            ret = glfs_set_volfile_server(glfs, "unix",
>>                                     server->value->u.q_unix.path, 0);
>>          } else {
>>              if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
>> @@ -423,8 +422,7 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
>>                  errno = EINVAL;
>>                  goto out;
>>              }
>> -            ret = glfs_set_volfile_server(glfs,
>> -                                   GlusterTransport_lookup[server->value->type],
>> +            ret = glfs_set_volfile_server(glfs, "tcp",
>>                                     server->value->u.tcp.host,
>>                                     (int)port);
>>          }
>> -- 
>> 2.7.4
>
> Instead of the strings for "unix" and "tcp", I would have liked
> #define's. Unfortunately it seems that these are not available in public
> headers :-/

Symbols from glfs.h would be ideal, yes.  But well-known string literals
aren't too bad.

> If this is easier to understand, I don't have any objections.

It's easier to read, but that's not my chief reason.  My chief reason is
that the values glfs_set_volfile_server() accepts are glfs's business,
while the names of the QAPI enumeration constants are QMP's business.
Coupling the two is inappropriate.

Decoupling them enables the renaming of the enumeration constant in
PATCH 14.

> Reviewed-by: Niels de Vos <ndevos@redhat.com>

Thanks!
diff mbox

Patch

diff --git a/block/gluster.c b/block/gluster.c
index 56b4abe..7236d59 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -412,8 +412,7 @@  static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 
     for (server = gconf->server; server; server = server->next) {
         if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
-            ret = glfs_set_volfile_server(glfs,
-                                   GlusterTransport_lookup[server->value->type],
+            ret = glfs_set_volfile_server(glfs, "unix",
                                    server->value->u.q_unix.path, 0);
         } else {
             if (parse_uint_full(server->value->u.tcp.port, &port, 10) < 0 ||
@@ -423,8 +422,7 @@  static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                 errno = EINVAL;
                 goto out;
             }
-            ret = glfs_set_volfile_server(glfs,
-                                   GlusterTransport_lookup[server->value->type],
+            ret = glfs_set_volfile_server(glfs, "tcp",
                                    server->value->u.tcp.host,
                                    (int)port);
         }