Re: [PATCH cifs-utils] mount.cifs: handle multiple ip addresses per hostname

["Aurélien Aptel" <aaptel@suse.com>]

I would put in the commit msg that this requires recent kernel.

We should probably merge kernel code first so we can reference it here
in the commit msg, and say in the man page when did the kernel change.

There can be cases where cifs-utils is more recent than kernel and
mount.cifs will pass all the ip instead of trying them before passing
the good one to the kernel but since it's an old kernel it won't try
them all. We could add an option to enable new behavior or check the
kernel version from within mount.cifs.. thoughts?

Paulo Alcantara <pc@cjr.nz> writes:
>  
> +static void set_ip_params(char *options, size_t options_size, char *addrlist)
> +{
> +	char *s = addrlist + strlen(addrlist), *q = s;
> +	char tmp;
> +
> +	do {
> +		for (; s >= addrlist && *s != ','; s--);
> +		tmp = *q;
> +		*q = '\0';
> +		strlcat(options, *options ? ",ip=" : "ip=", options_size);
> +		strlcat(options, s + 1, options_size);
> +		*q = tmp;
> +	} while (q = s--, s >= addrlist);
> +}

I think you should write this in a clearer way and comment, this is hard
to read. What does addrlist value looks like for example? Why are we
copying things backward?
Why is the last 'q' in the while condition instead of the end of the while block?

I was going to say should we return errors if we truncate the ips, but
none of the mount.cifs.c code checks for truncation so I guess we can
ignore.

> +
>  int main(int argc, char **argv)
>  {
>  	int c;
> @@ -2043,7 +2058,6 @@ int main(int argc, char **argv)
>  	char *mountpoint = NULL;
>  	char *options = NULL;
>  	char *orig_dev = NULL;
> -	char *currentaddress, *nextaddress;
>  	char *value = NULL;
>  	char *ep = NULL;
>  	int rc = 0;
> @@ -2201,20 +2215,10 @@ assemble_retry:
>  			goto mount_exit;
>  	}
>  
> -	currentaddress = parsed_info->addrlist;
> -	nextaddress = strchr(currentaddress, ',');
> -	if (nextaddress)
> -		*nextaddress++ = '\0';
> -
>  mount_retry:
>  	options[0] = '\0';
> -	if (!currentaddress) {
> -		fprintf(stderr, "Unable to find suitable address.\n");
> -		rc = parsed_info->nofail ? 0 : EX_FAIL;
> -		goto mount_exit;
> -	}
> -	strlcpy(options, "ip=", options_size);
> -	strlcat(options, currentaddress, options_size);
> +
> +	set_ip_params(options, options_size, parsed_info->addrlist);
>  
>  	strlcat(options, ",unc=\\\\", options_size);
>  	strlcat(options, parsed_info->host, options_size);
> @@ -2266,17 +2270,9 @@ mount_retry:
>  		switch (errno) {
>  		case ECONNREFUSED:
>  		case EHOSTUNREACH:
> -			if (currentaddress) {
> -				fprintf(stderr, "mount error(%d): could not connect to %s",
> -					errno, currentaddress);
> -			}
> -			currentaddress = nextaddress;
> -			if (currentaddress) {
> -				nextaddress = strchr(currentaddress, ',');
> -				if (nextaddress)
> -					*nextaddress++ = '\0';
> -			}
> -			goto mount_retry;
> +			fprintf(stderr, "mount error(%d): could not connect to: %s", errno, parsed_info->addrlist);
> +			rc = parsed_info->nofail ? 0 : EX_FAIL;
> +			break;
>  		case ENODEV:
>  			fprintf(stderr,
>  				"mount error: %s filesystem not supported by the system\n", cifs_fstype);

Ok, so now we pass all IPs to mount() and we don't retry.
I would add a comment before the break to say that the kernel will try
all the IPs so if we get these errors none of them worked.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)