| Message ID | 20210511163952.11670-1-pc@cjr.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [cifs-utils] mount.cifs: handle multiple ip addresses per hostname | expand |
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,
On Tue, May 11, 2021 at 12:46 PM Aurélien Aptel <aaptel@suse.com> wrote: > > > 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? We could at least warn on older cifs.ko (which would be unlikely to work unless someone backported features but didn't backport the updated version #) by checking for the version number (2.32? 2.33?) "cat /proc/fs/cifs/DebugData | grep Version" shows the version number easily info (there are other ways to retrieve it as well)
Aurélien Aptel <aaptel@suse.com> writes: > I would put in the commit msg that this requires recent kernel. Agreed. > 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. Agreed. > 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. Good point! Yes, we should handle both cases. > We could add an option to enable new behavior or check the kernel > version from within mount.cifs.. thoughts? I don't like the idea of checking the version because the running kernel might not have the expected patches. Perhaps a new option would be better... I'll think more about it. > 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. That's horrible, indeed. I'll definitely make it readable in next version. > 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. IIRC, yes.
Hi Paulo, Are you planning to post another version of this patch? If there is already one HI which I missed, please let me know. -- Best regards, Pavel Shilovsky вт, 11 мая 2021 г. в 11:20, Paulo Alcantara <pc@cjr.nz>: > > Aurélien Aptel <aaptel@suse.com> writes: > > > I would put in the commit msg that this requires recent kernel. > > Agreed. > > > 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. > > Agreed. > > > 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. > > Good point! Yes, we should handle both cases. > > > We could add an option to enable new behavior or check the kernel > > version from within mount.cifs.. thoughts? > > I don't like the idea of checking the version because the running kernel > might not have the expected patches. > > Perhaps a new option would be better... I'll think more about it. > > > 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. > > That's horrible, indeed. I'll definitely make it readable in next > version. > > > 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. > > IIRC, yes.
Hi Pavel, Pavel Shilovsky <piastryyy@gmail.com> writes: > Are you planning to post another version of this patch? If there is > already one HI which I missed, please let me know. Not for now, thanks. I would have to repost the multip support in cifs.ko and rebase it against current for-next. Then figure out a way properly handle the different kernel versions and/or compatibility features in cifs-utils so we can decide whether or not let the kernel retry all ip addresses at mount time. I'll let you know.
diff --git a/mount.cifs.c b/mount.cifs.c index 7f898bbd215a..988e56b649a2 100644 --- a/mount.cifs.c +++ b/mount.cifs.c @@ -2036,6 +2036,21 @@ restore_privs: return rc; } +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); +} + 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); diff --git a/mount.cifs.rst b/mount.cifs.rst index 9d4446f035b6..89fb5c902f79 100644 --- a/mount.cifs.rst +++ b/mount.cifs.rst @@ -166,10 +166,10 @@ dir_mode=arg If the server does not support the CIFS Unix extensions this overrides the default mode for directories. -ip=arg|addr=arg - sets the destination IP address. This option is set automatically if - the server name portion of the requested UNC name can be resolved so - rarely needs to be specified by the user. +ip=arg|addr=arg[,ip=arg2|addr=arg2,...] + Sets a maximum number of 16 destination IP addresses. This option is + set automatically if the server name portion of the requested UNC + name can be resolved so rarely needs to be specified by the user. domain=arg|dom=arg|workgroup=arg Sets the domain (workgroup) of the user. If no domains are given,
Support passing multiple 'ip=' options to specify all ip addresses a server name was resolved to. Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> --- mount.cifs.c | 44 ++++++++++++++++++++------------------------ mount.cifs.rst | 8 ++++---- 2 files changed, 24 insertions(+), 28 deletions(-)