Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mountinfo: GetMountsFromReader() remove workaround for kernel < 3.10 #83

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

follow-up to #81

In Linux <= 3.9 mounting a cifs with spaces in a share name (like "//srv/My Docs")
may end up having a space in the last field of mountinfo (like "unc=//serv/My Docs").

Since kernel 3.10-rc1, cifs option "unc=" is ignored, so spaces should not appear.

Given that all major distros are now on at least kernel 3.10, and (e.g.) docker
has not supported older kernel versions for 5 Years 1, it should be save to
remove this special handling.

@@ -504,66 +499,6 @@ func TestParseFedoraMountinfoFilterFields(t *testing.T) {
}
}

func TestParseMountinfoWithSpaces(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how removing this test is relevant to this commit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reiterate the comment above -- this test case is NOT about pre-3.10 bug of having extra spaces in a cifs mount entry where share name has spaces.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah ^^^ (or do we have other tests where we test fields with spaces)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test case is NOT about pre-3.10 bug of having extra spaces in a cifs mount entry where share name has spaces.

Oh! Right, only the second entry is about the unc=<path with spaces> issue; the others are escaped

IIUC from the comment, kernel 3.10-rc1 should ignore that unc option, so I can just remove that part.

Do we know of other ways for spaces to get into the last column, or should 3.10-rc1 and up guarantee there not to be?

(or do we have other tests where we test fields with spaces)?

@kolyshkin hm.. not sure if we have; I'll add back this test, but update it to only remove the unc= bits.

@thaJeztah thaJeztah force-pushed the mountinfo_remove_kernel_3.9_support branch from 8832f96 to 955e12d Compare August 23, 2021 16:29
@kolyshkin
Copy link
Collaborator

We should probably do this once Go < 1.23 (which raises the Linux kernel requirement to >= 3.16 or so) is no longer supported.

@thaJeztah thaJeztah force-pushed the mountinfo_remove_kernel_3.9_support branch from 955e12d to ea3f2d8 Compare July 16, 2024 17:24
In Linux <= 3.9 mounting a cifs with spaces in a share name (like "//srv/My Docs")
_may_ end up having a space in the last field of mountinfo (like "unc=//serv/My Docs").

Since kernel 3.10-rc1, cifs option "unc=" is ignored, so spaces should not appear.

Given that all major distros are now on at least kernel 3.10, and (e.g.) docker
has not supported older kernel versions for 5 Years [1], it should be save to
remove this special handling.

[1]: moby/moby@51b23d8

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the mountinfo_remove_kernel_3.9_support branch from ea3f2d8 to 1456f0a Compare August 8, 2024 08:04
@thaJeztah
Copy link
Member Author

We should probably do this once Go < 1.23 (which raises the Linux kernel requirement to >= 3.16 or so) is no longer supported.

Slightly wondering if we need to wait for that; Kernel 3.9 (or older) means;

  • Debian 7 Wheezy (kernel 3.2); EOL since 2018
  • Ubuntu 13.04 (kernel 3.8); EOL since 2014
  • CentOS 6 / RHEL 6 / Fedora 10 (kernel 2.6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants