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

When the default user is defined, the wsl.conf is updated accordingly #82

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

crramirez
Copy link

No description provided.

@ghost
Copy link

ghost commented Oct 29, 2020

CLA assistant check
All CLA requirements met.

@Biswa96
Copy link

Biswa96 commented Oct 29, 2020

  • The [user] section may not be available in wsl.conf in older Windows 10 versions. A Windows version check may be useful?
  • CreateProcess return value is not checked.
  • RunProcess can be a static function here.
  • WslApiLoader::WslLaunch may be used instead of RunProcess wrapper.

@crramirez
Copy link
Author

crramirez commented Oct 29, 2020

Hello @Biswa96,

Thank you for your comments. My answers:

  • The [user] section may not be available in wsl.conf in older Windows 10 versions. A Windows version check may be useful?
    Yes I will include that
  • CreateProcess return value is not checked.
    Yes, because I think that this can be ignored. But help me think, what should happen if it fails, show a warning to the user? Maybe
  • RunProcess can be a static function here.
    Ok
  • WslApiLoader::WslLaunch may be used instead of RunProcess wrapper.
    Ok, I didn't use it, and it was my first option but it runs the distro with the default user. The first time it is ok because the default one is root. But the second time it is a limited user that won't be able to modify wsl.conf. I could add a sudoers exception but I think it is too intrusive.

@crramirez
Copy link
Author

@Biswa96 I made a code clean up to the PR; please check it. I figure that I addressed all your concerns.

It won't hurt to have [user] not available in older versions; WSL won't use it in older ones. But when the user upgrades Windows, it will be already there configured.

Regards,
Carlos

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