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

mount: some refactor and improved GoDoc about Windows support #31

Merged
merged 2 commits into from Oct 1, 2020

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 18, 2020

Commit 0c569e0 (#26) added a fast-path for Linux, but inadvertendly changed the behavior on Windows, which previously handled RecursiveUnmount() as a no-op, and now panicked.

This patch makes RecursiveUnmount() a no-op again on Windows.

[Kir: fixed commit/PR reference]

@thaJeztah
Copy link
Member Author

Relates to moby/moby#41458 (comment)

ping @kolyshkin @cpuguy83 PTAL

@kolyshkin
Copy link
Collaborator

Thanks! Is it possible to add some CI job to prevent this kinds of mishap in the future?

@kolyshkin
Copy link
Collaborator

It looks like Unmount() and RecursiveUnmount() should behave in a similar way. Currently, they both panic on Windows, and it makes sense to me, as they should not be called on Windows. This commit makes them behave differently.

Perhaps we can make both no-op? This will

  • make this patch much smaller;
  • make the behavior consistent between the two.

@thaJeztah
Copy link
Member Author

@kolyshkin updated, but I noticed that before 0c569e0, Unmount() already produced a panic on Windows, so I'm a bit on the fence on updating.

Actually thinking now if we should accept the breaking change and update downstream code or not 🤔 (the package is still pre v1.0, so from that perspective, it should be ok)

@kolyshkin
Copy link
Collaborator

If you can just change unmountBare (and unmount) for Windows to return nil that will have the same effect with much smaller set of changes needed.

Yet we need to decide first whether we want them both to be no-op or panic. I don't have a strong preference but would like to hear other opinions.

@kolyshkin
Copy link
Collaborator

moby/moby#41458 (comment) it seems that the panic behavior is better than no-op. In these two cases, it clearly tells the test is not working (and it should not work on Windows).

So, I'm in favor of panic.

@thaJeztah thaJeztah changed the title Make RecursiveUnmount() a no-op on Windows Some refactor and improved GoDoc about Windows support Sep 21, 2020
mount/mount.go Outdated
Comment on lines 27 to 43
// RecursiveUnmount is not implemented on Windows.
func RecursiveUnmount(target string) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

So, alternatively, I guess we could not implement at all on Windows (move this to a _linux.go or _unix.go wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think the rename + build flags will be slightly cleaner.

@kolyshkin
Copy link
Collaborator

What I'd like to see here is a minimal patch. Moving lots of code around makes it harder to dig through git history, and unless there is a good reason to, I'd rather not do it.

Maybe I'm wrong and this is unavoidable.

@thaJeztah
Copy link
Member Author

The situation currently is that we have separate files for platform-specific implementations, but there's code in files that doesn't work on the platform it's enabled on.

So, yes, I agree that a smaller diff would be good, but it looks like code is in the wrong files.

@thaJeztah
Copy link
Member Author

I see @kolyshkin preferred "Yeah, I think the rename + build flags will be slightly cleaner."; let me have a look what that looks like.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the dont_panic branch 2 times, most recently from 585c7f1 to 832bd36 Compare October 1, 2020 16:07
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@kolyshkin pushed a commit with those changes; if you think this looks good, I'll squash

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

@kolyshkin kolyshkin merged commit 832ae17 into moby:master Oct 1, 2020
@thaJeztah thaJeztah deleted the dont_panic branch October 1, 2020 19:51
@thaJeztah thaJeztah changed the title Some refactor and improved GoDoc about Windows support mount: some refactor and improved GoDoc about Windows support Nov 10, 2020
@thaJeztah thaJeztah mentioned this pull request Nov 10, 2020
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.

None yet

3 participants