[PATCH] support volfile fetch from multiple glusterd nodes
Günther Deschner
gd at samba.org
Fri Oct 14 10:41:54 UTC 2016
Looks great now, RB+ and pushed to autobuild.
Thanks!
Guenther
On 14/10/16 12:27, Raghavendra Talur wrote:
> 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
>>>
>>
>>
>>
>>
--
Günther Deschner GPG-ID: 8EE11688
Red Hat gdeschner at redhat.com
Samba Team gd at samba.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 201 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161014/69ec58a2/signature.sig>
More information about the samba-technical
mailing list