Message ID | 1488491046-2549-11-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
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>
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 --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); }
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(-)