[PATCH] support volfile fetch from multiple glusterd nodes
Michael Adam
obnox at samba.org
Thu Oct 6 00:04:27 UTC 2016
On 2016-10-06 at 01:03 +0200, Michael Adam wrote:
> 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!
Forgot to say: With these changes squashed, and barring
functional verification, I am fine with the patch, i.e.
reviewed-by me.
And thanks for the contribution! :-)
Cheers - Michael
> 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
> > >
> 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/57cd54a1/signature.sig>
More information about the samba-technical
mailing list