[PATCH] support volfile fetch from multiple glusterd nodes
Jeremy Allison
jra at samba.org
Wed Sep 7 22:38:51 UTC 2016
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
>
More information about the samba-technical
mailing list