[PATCH] support volfile fetch from multiple glusterd nodes
Raghavendra Talur
rtalur at redhat.com
Fri Oct 14 10:27:34 UTC 2016
On Thu, Oct 6, 2016 at 5:34 AM, Michael Adam <obnox at samba.org> wrote:
> 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 for the update and review Michael. The patch looks much better now.
I have verified that the patch passes all the tests and is functionally
complete.
Waiting for review and ack from others.
Thanks,
Raghavendra Talur
> > 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
> >
>
>
>
>
More information about the samba-technical
mailing list