[PATCH] support volfile fetch from multiple glusterd nodes
Raghavendra Talur
rtalur at redhat.com
Wed Oct 5 10:40:43 UTC 2016
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
>
More information about the samba-technical
mailing list