[PATCH] support volfile fetch from multiple glusterd nodes
Michael Adam
obnox at samba.org
Wed Oct 5 23:03:32 UTC 2016
Talur,
after going over the patch togehter, here is the resulting
add-on patch to be squashed with your patch. It streamlines
the logic a bit, moves variables into scope and makes the
function use the new DBG_FOO macros.
It builds on my box, but please test!
Thanks - Michael
On 2016-10-05 at 16:10 +0530, Raghavendra Talur wrote:
> Please let me know if the patch needs any changes, will be happy to make it
> better.
>
> Thanks,
> Raghavendra Talur
>
> On Tue, Sep 27, 2016 at 12:31 PM, Raghavendra Talur <rtalur at redhat.com>
> wrote:
>
> > Thank you for the valuable comments.
> > I have attached new version of the patch. It has been tested with the
> > following input
> > glusterfs:volfile_server = rtalur [::1] [::1]:24007 tcp+
> > rtalur.local:24007 192.168.21.6 unix+ unix+/run/glusterd.socket 192.168.21.2
> >
> > I have replied to Jeremy's comments inline.
> >
> > Ira,
> > 1. Now index() isn't called multiple times for same parse as suggested.
> > 2. About IPv6 parse, I think the case you mention is also handled.
> > Check the test string above.
> > 3. Moved the strtok_r into the while clause and eliminated the duplicated
> > call to strtok_r as suggested.
> >
> > On Thu, Sep 8, 2016 at 4:08 AM, Jeremy Allison <jra at samba.org> wrote:
> >
> >> On Wed, Sep 07, 2016 at 03:22:57PM +0530, Raghavendra Talur wrote:
> >> > Hi All,
> >> >
> >> > This patch allows one to list multiple Glusterd nodes from
> >> vfs_glusterfs to
> >> > connect to. This prevents single point of failure when initiating
> >> > connections.
> >> >
> >> > Please review.
> >>
> >> Needs some work I'm afraid. Comments inline:
> >>
> >> >
> >> > +static int vfs_gluster_set_volfile_servers(glfs_t *fs,
> >> > + const char *volfile_servers)
> >> > +{
> >> > + char *servers;
> >> > + char *server;
> >> > + char *saveptr;
> >> > + char *port_index;
> >> > + char *transport;
> >> > + char *host;
> >> > + char *ip_start;
> >> > + char *ip_end;
> >> > + int port = 0;
> >> > + int ip_len;
> >> > + int server_count = 0;
> >> > + int server_success = 0;
> >> > + int ret = -1;
> >> > + TALLOC_CTX *tmp_ctx;
> >>
> >> ^^^^^^^^^^^^^^^^^^^^^^^^
> >> Try and initialize pointers to NULL.
> >>
> > Done.
> >
> >
> >>
> >> > +
> >> > + tmp_ctx = talloc_new(NULL);
> >>
> >> Use talloc_frame() here for tmp context.
> >>
> >> > + if (tmp_ctx == NULL) {
> >> > + return -1;
> >> > + }
> >> > +
> >> > +
> >> > + servers = talloc_strdup(tmp_ctx, volfile_servers);
> >>
> >> No check for talloc returning NULL - all talloc calls
> >> must be NULL checked.
> >>
> >
> > Done.
> >
> >
> >>
> >> > + DEBUG(4, ("servers list"
> >> > + "%s\n", servers));
> >> > +
> >> > + server = strtok_r(servers, " \t", &saveptr);
> >>
> >> Instead of tokenizing yourself, use next_token_talloc(),
> >> designed to do exactly this.
> >>
> >
> > Thanks, done.
> >
> >>
> >> > + while (server != NULL) {
> >> > +
> >> > + server_count++;
> >> > + DEBUG(4, ("server %d"
> >> > + "%s\n", server_count, server));
> >> > +
> >> > + /* Determine the transport type */
> >> > + if(strncmp(server, "unix+", 5) == 0) {
> >> > + port = 0;
> >> > + transport = talloc_strdup(tmp_ctx, "unix");
> >> > + host = talloc_strdup(tmp_ctx, server + 5);
> >> > + } else {
> >> > + transport = talloc_strdup(tmp_ctx, "tcp");
> >> > + if (strncmp(server, "tcp+", 4) == 0) {
> >> > + ip_start = &server[4];
> >> > + } else {
> >> > + ip_start = server;
> >> > + }
> >> > +
> >> > + /* Account for IPv6 */
> >> > + if ((ip_start[0] == '[') &&
> >> > + (index(ip_start, ']') != NULL)) {
> >> > + ip_end = index(ip_start, ']') - 1;
> >> > + ip_start++;
> >> > + port_index = index(ip_end,':');
> >> > + if (port_index == NULL) {
> >> > + port = 0;
> >> > + } else {
> >> > + port = atoi(port_index + 1);
> >> > + }
> >> > + host = talloc_strndup(tmp_ctx, ip_start,
> >> > + ip_end - ip_start
> >> + 1);
> >> > + } else {
> >> > + port_index = index(ip_start,':');
> >> > + if (port_index == NULL) {
> >> > + port = 0;
> >> > + host = talloc_strdup(tmp_ctx,
> >> > + ip_start);
> >> > + } else {
> >> > + port = atoi(port_index + 1);
> >> > + host = talloc_strndup(tmp_ctx,
> >> > + ip_start,
> >> > + port_index
> >> - ip_start);
> >> > + }
> >> > + }
> >> > + }
> >>
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> Don't tokenize IPv4/IPv6 yourself, use interpret_string_addr()
> >> to tokenize into a struct sockaddr_storage (will fail if not
> >> in standard format) then print into canonical form to use.
> >>
> >
> > Unfortunately, this function is not capable of parsing if
> > IPv4/IPv6/Hostname have port next to them. I simplified the parsing
> > mechanism in the code and double checked that glfs_ API does all the
> > required checks to ensure that the IPs/Hostnames are in standard format. I
> > hope that is sufficient.
> >
> >
> >> > +
> >> > + DEBUG(4, ("Calling set volfile server with params "
> >> > + "transport=%s, host=%s, port=%d\n", transport,
> >> > + host, port));
> >> > +
> >> > + ret = glfs_set_volfile_server(fs, transport, host, port);
> >> > + if (ret < 0) {
> >> > + DEBUG(2, ("Failed to set volfile_server "
> >> > + "transport=%s, host=%s, port=%d\n",
> >> transport,
> >> > + host, port));
> >> > + } else {
> >> > + server_success++;
> >> > + ret = 0;
> >> > + }
> >> > +
> >> > + server = strtok_r(NULL, " \t", &saveptr);
> >> > + }
> >> > +
> >> > + if (server_success < server_count) {
> >> > + DEBUG(2, ("Failed to set %d out of %d servers ",
> >> > + server_count - server_success, server_count));
> >> > + }
> >> > +
> >> > + TALLOC_FREE(tmp_ctx);
> >> > + return ret;
> >> > +}
> >> > +
> >> > /* Disk Operations */
> >> >
> >> > static int vfs_gluster_connect(struct vfs_handle_struct *handle,
> >> > const char *service,
> >> > const char *user)
> >> > {
> >> > - const char *volfile_server;
> >> > + const char *volfile_servers;
> >> > const char *volume;
> >> > char *logfile;
> >> > int loglevel;
> >> > @@ -177,15 +280,16 @@ static int vfs_gluster_connect(struct
> >> vfs_handle_struct *handle,
> >> > ret = -1;
> >> > goto done;
> >> > }
> >> > - logfile = lp_parm_talloc_string(tmp_ctx, SNUM(handle->conn),
> >> "glusterfs",
> >> > - "logfile", NULL);
> >> > + logfile = lp_parm_talloc_string(tmp_ctx, SNUM(handle->conn),
> >> > + "glusterfs", "logfile", NULL);
> >> >
> >> > loglevel = lp_parm_int(SNUM(handle->conn), "glusterfs",
> >> "loglevel", -1);
> >> >
> >> > - volfile_server = lp_parm_const_string(SNUM(handle->conn),
> >> "glusterfs",
> >> > - "volfile_server", NULL);
> >> > - if (volfile_server == NULL) {
> >> > - volfile_server = DEFAULT_VOLFILE_SERVER;
> >> > + volfile_servers = lp_parm_talloc_string(tmp_ctx,
> >> SNUM(handle->conn),
> >> > + "glusterfs",
> >> "volfile_server",
> >> > + NULL);
> >> > + if (volfile_servers == NULL) {
> >> > + volfile_servers = DEFAULT_VOLFILE_SERVER;
> >> > }
> >> >
> >> > volume = lp_parm_const_string(SNUM(handle->conn), "glusterfs",
> >> "volume",
> >> > @@ -205,9 +309,10 @@ static int vfs_gluster_connect(struct
> >> vfs_handle_struct *handle,
> >> > goto done;
> >> > }
> >> >
> >> > - ret = glfs_set_volfile_server(fs, "tcp", volfile_server, 0);
> >> > + ret = vfs_gluster_set_volfile_servers(fs, volfile_servers);
> >> > if (ret < 0) {
> >> > - DEBUG(0, ("Failed to set volfile_server %s\n",
> >> volfile_server));
> >> > + DEBUG(0, ("Failed to set volfile_servers from list %s\n",
> >> > + volfile_servers));
> >> > goto done;
> >> > }
> >> >
> >> > @@ -250,17 +355,16 @@ static int vfs_gluster_connect(struct
> >> vfs_handle_struct *handle,
> >> > goto done;
> >> > }
> >> > done:
> >> > - talloc_free(tmp_ctx);
> >> > if (ret < 0) {
> >> > if (fs)
> >> > glfs_fini(fs);
> >> > - return -1;
> >> > } else {
> >> > - DEBUG(0, ("%s: Initialized volume from server %s\n",
> >> > - volume, volfile_server));
> >> > + DEBUG(0, ("%s: Initialized volume from servers %s\n",
> >> > + volume, volfile_servers));
> >> > handle->data = fs;
> >> > - return 0;
> >> > }
> >> > + talloc_free(tmp_ctx);
> >> > + return ret;
> >> > }
> >> >
> >> > static void vfs_gluster_disconnect(struct vfs_handle_struct *handle)
> >> > --
> >> > 2.7.4
> >> >
> >>
> >>
> > Thanks,
> > Raghavendra Talur
> >
-------------- next part --------------
From dcb249c9fbedd3f06b9ccae385a91613fce73d38 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 6 Oct 2016 00:40:49 +0200
Subject: [PATCH] SQ w/ talur
---
source3/modules/vfs_glusterfs.c | 62 +++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c
index 0499f4c..9fb84fc 100644
--- a/source3/modules/vfs_glusterfs.c
+++ b/source3/modules/vfs_glusterfs.c
@@ -162,22 +162,20 @@ static int vfs_gluster_set_volfile_servers(glfs_t *fs,
const char *volfile_servers)
{
char *server = NULL;
- char *port_index = NULL;
- char *ip_end = NULL;
- char *transport = NULL;
- char *host = NULL;
- int port = 0;
int server_count = 0;
int server_success = 0;
int ret = -1;
TALLOC_CTX *frame = talloc_stackframe();
- struct sockaddr_storage server_ss;
- DEBUG(4, ("servers list %s\n", volfile_servers));
+ DBG_INFO("servers list %s\n", volfile_servers);
while (next_token_talloc(frame, &volfile_servers, &server, " \t")) {
+ char *transport = NULL;
+ char *host = NULL;
+ int port = 0;
+
server_count++;
- DEBUG(4, ("server %d %s\n", server_count, server));
+ DBG_INFO("server %d %s\n", server_count, server);
/* Determine the transport type */
if (strncmp(server, "unix+", 5) == 0) {
@@ -193,27 +191,31 @@ static int vfs_gluster_set_volfile_servers(glfs_t *fs,
goto out;
}
} else {
- if (strncmp(server, "tcp+", 4) == 0)
- server = server + 4;
+ char *p = NULL;
+ char *port_index = NULL;
+
+ if (strncmp(server, "tcp+", 4) == 0) {
+ server += 4;
+ }
/* IPv6 is enclosed in []
* ':' before ']' is part of IPv6
* ':' after ']' indicates port
*/
+ p = server;
if (server[0] == '[') {
- server = server + 1;
- if (ip_end = index(server, ']')) {
- ip_end--;
- } else {
+ server++;
+ p = index(server, ']');
+ if (p == NULL) {
/* Malformed IPv6 */
continue;
}
- port_index = index(ip_end,':');
- ip_end[1] = '\0';
- } else {
- port_index = index(server,':');
+ p[0] = '\0';
+ p++;
}
+ port_index = index(p, ':');
+
if (port_index == NULL) {
port = 0;
} else {
@@ -232,15 +234,15 @@ static int vfs_gluster_set_volfile_servers(glfs_t *fs,
}
}
- DEBUG(4, ("Calling set volfile server with params "
- "transport=%s, host=%s, port=%d\n", transport,
- host, port));
+ DBG_INFO("Calling set volfile server with params "
+ "transport=%s, host=%s, port=%d\n", transport,
+ host, port);
ret = glfs_set_volfile_server(fs, transport, host, port);
if (ret < 0) {
- DEBUG(2, ("Failed to set volfile_server "
- "transport=%s, host=%s, port=%d (%s)\n",
- transport, host, port, strerror(errno)));
+ DBG_WARNING("Failed to set volfile_server "
+ "transport=%s, host=%s, port=%d (%s)\n",
+ transport, host, port, strerror(errno));
} else {
server_success++;
}
@@ -250,8 +252,8 @@ out:
if (server_count == 0) {
ret = -1;
} else if (server_success < server_count) {
- DEBUG(2, ("Failed to set %d out of %d servers parsed\n",
- server_count - server_success, server_count));
+ DBG_WARNING("Failed to set %d out of %d servers parsed\n",
+ server_count - server_success, server_count);
ret = 0;
}
@@ -309,8 +311,8 @@ static int vfs_gluster_connect(struct vfs_handle_struct *handle,
ret = vfs_gluster_set_volfile_servers(fs, volfile_servers);
if (ret < 0) {
- DEBUG(0, ("Failed to set volfile_servers from list %s\n",
- volfile_servers));
+ DBG_ERR("Failed to set volfile_servers from list %s\n",
+ volfile_servers);
goto done;
}
@@ -357,8 +359,8 @@ done:
if (fs)
glfs_fini(fs);
} else {
- DEBUG(0, ("%s: Initialized volume from servers %s\n",
- volume, volfile_servers));
+ DBG_ERR("%s: Initialized volume from servers %s\n",
+ volume, volfile_servers);
handle->data = fs;
}
talloc_free(tmp_ctx);
--
2.7.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161006/72b16f64/signature.sig>
More information about the samba-technical
mailing list