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

[Feature]: QEMU Machine should support Windows Source paths for 9pfs on Windows hosts #17098

Closed
arixmkii opened this issue Jan 12, 2023 · 3 comments · Fixed by #17113
Closed
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@arixmkii
Copy link
Contributor

Feature request description

There is a work in progress 9pfs support implementation for QEMU. There is a working demo of it #13006 (comment)

Currently it is impossible to Init a machine on Windows hosts with FS mounts configured via command line, because of the special meaning of : after drive letter in Windows paths. Current parser just splits string using : and is not in any way platform aware:

paths := strings.SplitN(volume, ":", 3)

Additionally there should be conversion mechanism predefined to generate some reasonable Unix path based on Windows source path. There is a tool in cygwin/msys2 called "cygpath" to convert Windows paths to Unix equivalents for their subsystem, but here it looks like unnecessary dependency.

Suggest potential solution

Solutions would be provided separately for the parser and for path conversion

Parser

The simplest way would be to change : to os.PathListSeparator, which is ":' on Unixes and ";" on Windows and is used when they define multiple records inside PATH environment variable. The downside of this is that there will be need to document and communicate that parsing rules differs between platforms.

Alternative would be to implement OS awareness like checking that there is a drive letter in Windows source path and combine it with path after (so, like split into one more, but then glue the first two to go to the previous state).

Not related to this feature, but it looks like it is better to add sanity checks that paths are absolute, because allowing relative paths complicates the things significantly (also, I don't think they would work, may be it is just unsupported by definition and doesn't need coverage - user will be responsible for the input).

Path conversion

		if runtime.GOOS == "windows" {
			target = "/" + strings.ReplaceAll(target, string(os.PathSeparator), "/")
			target = strings.ReplaceAll(target, ":", "")
		}

Looks good enough:

  1. convert all PathSeparators to Unix, to represent the hierarchy;
  2. append root, because we need absolute pat;
  3. remove ":" after drive letter.

Example:
"C:\Temp\SubDir" would become "/C/Temp/SubDir"

Have you considered any alternatives?

Don't implement FS mounts on Windows hosts, but it cuts out a feature, which actually works (or will work, because now one needs to build from sources to enable it).

Additional context

Extracted from the larger scope of #13006

@arixmkii arixmkii added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 12, 2023
@arixmkii
Copy link
Contributor Author

Example how it was implemented for the demo mentioned above arixmkii@330c74c

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@arixmkii
Copy link
Contributor Author

#17113 implementing this was approved, but the merge was delay to after 4.4.0 release time point. I can rebase it, if needed.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant