[PATCH] support volfile fetch from multiple glusterd nodes
Raghavendra Talur
rtalur at redhat.com
Tue Sep 27 07:01:46 UTC 2016
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 --------------
A non-text attachment was scrubbed...
Name: 0001-support-volfile-fetch-from-multiple-glusterd-nodes.patch
Type: text/x-patch
Size: 7255 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160927/6c6706f6/0001-support-volfile-fetch-from-multiple-glusterd-nodes.bin>
More information about the samba-technical
mailing list