[PATCH] support volfile fetch from multiple glusterd nodes

[Jeremy Allison]

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.

> +
> +	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.

> +	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.

> +	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.

> +
> +		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
>