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

cmd mountlib: adding remount option for linux systems - fixes #6488 #7802

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KDreynolds
Copy link
Contributor

What is the purpose of this change?

Added a --remount option as requested in issue #6488 . option only works on linux systems. Updated documentation to reflect this optional flag.

Was the change discussed in an issue or in the forum before?

Issue #6488

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

@KDreynolds
Copy link
Contributor Author

Took a crack at addressing issue #6488

Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

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

Apologies for the delay reviewing this.

This is looking great - thank you.

I put some runtime.GOOS suggestions inline.

@@ -161,6 +163,7 @@ func AddFlags(flagSet *pflag.FlagSet) {
flags.BoolVarP(flagSet, &Opt.NetworkMode, "network-mode", "", Opt.NetworkMode, "Mount as remote network drive, instead of fixed disk drive (supported on Windows only)", "Mount")
// Unix only
flags.DurationVarP(flagSet, &Opt.DaemonWait, "daemon-wait", "", Opt.DaemonWait, "Time to wait for ready mount from daemon (maximum time on Linux, constant sleep time on OSX/BSD) (not supported on Windows)", "Mount")
flagSet.BoolVarP(&Opt.Remount, "remount", "", false, "Remount a file system")
Copy link
Member

Choose a reason for hiding this comment

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

I suggest you wrap this in a if runtime.GOOS == "linux" { } then the flag will only appear for linux systems.

// Ensure sensible defaults
m.SetVolumeName(m.MountOpt.VolumeName)
m.SetDeviceName(m.MountOpt.DeviceName)

// Handle the --remount flag by attempting to unmount the mountpoint first
if m.MountOpt.Remount {
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably put if runtime.GOOS == linux && m.MountOpt.Remount { here.

The compiler is smart enough to evaluate runtime.GOOS == linux at compile time then figure out it doesn't need to compile any of the stuff in the if statement so it saves bloating the binaries where this doesn't work.

@KDreynolds
Copy link
Contributor Author

ok made the updates, not sure about this Race test that is failing on the go1.20 build, let me know if I can do anything to fix this.

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

2 participants