Re: [Qemu-block] [Qemu-devel] [PATCH 10/15] gluster: Drop assumptions on SocketTransport names

[Markus Armbruster]

Niels de Vos <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!