Skip to content

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 14, 2021

This switch was implementing the exact same check as is done in
strings.Join(); https://cs.opensource.google/go/go/+/refs/tags/go1.16.7:src/strings/strings.go;l=421-427

func Join(elems []string, sep string) string {
	switch len(elems) {
	case 0:
		return ""
	case 1:
		return elems[0]
	}

This only difference removing this switch makes is that it will assign
an empty string to p.Optional if no optional fields are present, but
given that an empty string is the default value for p.Optional, this
should make no difference.

This also revealed an off-by-one bug:

Before this change:

  • we return an error if sepIdx == 5 (earlier check)
  • for sepIdx == 6 we skipped (in this switch)
  • for 7, we take field 6 (equivalent to fields[6:7], or fields[6:sepIdx])
  • for the "default" (else), we use fields[6:sepIdx-1], which for (e.g.) sepIndex=8
    would do the same as sepIdx=7, so we'd drop one value.

@thaJeztah thaJeztah marked this pull request as draft August 14, 2021 14:35
case sepIdx == 7:
p.Optional = fields[6]
default:
p.Optional = strings.Join(fields[6:sepIdx-1], " ")
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there's an off-by-one bug here; removing this switch caused it to panic;

--- FAIL: TestMountedBy (0.01s)
panic: runtime error: slice bounds out of range [6:5] [recovered]
	panic: runtime error: slice bounds out of range [6:5]
  • we return an error if sepIdx == 5 (earlier check)
  • for sepIdx == 6 we skipped (in this switch)
  • for 7, we take field 6 (equivalent to fields[6:7], or fields[6:sepIdx])
  • for the "default" (else), we use fields[6:sepIdx-1], which for (e.g.) sepIndex=8 would do the same as sepIdx=7, so we'd drop one value

@thaJeztah thaJeztah marked this pull request as ready for review August 14, 2021 15:05
@thaJeztah
Copy link
Member Author

@kolyshkin PTAL

@thaJeztah thaJeztah changed the title mountinfo: GetMountsFromReader() remove redundant switch mountinfo: GetMountsFromReader() remove redundant switch, fix dropped "optional" fields Aug 14, 2021
@thaJeztah
Copy link
Member Author

Looking further, I noticed that TestParseMountinfoExtraCases was not checking the Optional field; fixing that test showed that it was indeed broken without this patch, so I included those commits in this PR

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

@AkihiroSuda ptal

… fields

This switch was implementing the exact same check as is done in
strings.Join(); https://cs.opensource.google/go/go/+/refs/tags/go1.16.7:src/strings/strings.go;l=421-427

This only difference removing this switch makes is that it will assign
an empty string to p.Optional if no optional fields are present, but
given that an empty string is the default value for p.Optional, this
should make no difference.

This also revealed an off-by-one bug:

Before this change:

- we return an error if `sepIdx == 5` (earlier check)
- for `sepIdx == 6` we skipped (in this switch)
- for `7`, we take field 6 (equivalent to `fields[6:7]`, or `fields[6:sepIdx]`)
- for the "default" (else), we use `fields[6:sepIdx-1]`, which for (e.g.) `sepIndex=8`
  would do the same as `sepIdx=7`, so we'd drop one value.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@AkihiroSuda AkihiroSuda merged commit 76a42cd into moby:master Aug 23, 2021
@thaJeztah thaJeztah deleted the mountinfo_simplify branch August 23, 2021 16:21
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.

3 participants