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

Add dependencies to the VirtioFsSvc service #30

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

Conversation

jwatt
Copy link
Contributor

@jwatt jwatt commented Jan 5, 2022

The service has two dependencies (WinFsp.Launcher and VirtioFsDrv) as
noted, for example, at:

https://virtio-fs.gitlab.io/howto-windows.html
virtio-win/kvm-guest-drivers-windows#550 (comment)

@ovirt-infra
Copy link
Collaborator

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

  1. Type ci test please on this PR to trigger automated tests for it.
  2. Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically. ( keep in mind this list might be overwritten if the job XML is refreshed, for permanent whitelisting, please follow Installer fails to start VdService on Windows 2016 after virt-v2v conversion #3 option )
  3. If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

The service has two dependencies (WinFsp.Launcher and VirtioFsDrv) as
noted, for example, at:

https://virtio-fs.gitlab.io/howto-windows.html
virtio-win/kvm-guest-drivers-windows#550 (comment)

Signed-off-by: Jonathan Watt <[email protected]>
Copy link

@viktor-prutyanov viktor-prutyanov left a comment

Choose a reason for hiding this comment

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

Hi,

VirtioFsSvc doesn't depend actually on WinFsp.Launcher, because VirtioFsSvc is started by Service Manager, not WinFsp.Launcher.

For example, this configuration (only VirtioFsDrv as a dependecy) works OK:

C:\Users\Administrator>sc qc VirtioFsSvc
SERVICE_NAME: VirtioFsSvc
        TYPE               : 10  WIN32_OWN_PROCESS
        START_TYPE         : 2   AUTO_START
        ERROR_CONTROL      : 1   NORMAL
        BINARY_PATH_NAME   : C:\Users\Administrator\kvm-guest-drivers-windows\viofs\Install\Win10\amd64\virtiofs.exe
        DISPLAY_NAME       : Virtio FS Service
        DEPENDENCIES       : VirtioFsDrv
        SERVICE_START_NAME : LocalSystem

So, looks like WinFsp.Launcher is not necessary.

@jwatt
Copy link
Contributor Author

jwatt commented Jan 16, 2022

Thanks for taking a look at this pull request, @viktor-prutyanov.

Maybe @hammerg or @xiagao can comment since they have both previously said that WinFsp.Launcher is a dependency (hammerg's comment) (xiagao's comment).

virtiofs is built on top of WinFsp, as you'll know, and my (possibly incorrect) understanding from the discussion on some virtiofs issues is that if WinFsp isn't fully ready when VirtioFsSvc is started, then virtiofs sharing may (or may not - racey?) fail for certain tasks (such as opening files in subdirectories of the shared directory, I think). To fix that, VirtioFsSvc needs to be manually restarted after boot. So I'm guessing marking WinFsp.Launcher as a dependency isn't because it's needed to launch VirtioFsSvc, but instead it may be a convenient way to have Service Manager wait until it's safe before it starts VirtioFsSvc.

@xiagao
Copy link

xiagao commented Jan 18, 2022

Thanks for taking a look at this pull request, @viktor-prutyanov.

Maybe @hammerg or @xiagao can comment since they have both previously said that WinFsp.Launcher is a dependency (hammerg's comment) (xiagao's comment).

Actually I config VirtioFsSvc according to https://virtio-fs.gitlab.io/howto-windows.html .
And I tried without any dependecy, virtiofs still works.

virtiofs is built on top of WinFsp, as you'll know, and my (possibly incorrect) understanding from the discussion on some virtiofs issues is that if WinFsp isn't fully ready when VirtioFsSvc is started, then virtiofs sharing may (or may not - racey?) fail for certain tasks (such as opening files in subdirectories of the shared directory, I think). To fix that, VirtioFsSvc needs to be manually restarted after boot. So I'm guessing marking WinFsp.Launcher as a dependency isn't because it's needed to launch VirtioFsSvc, but instead it may be a convenient way to have Service Manager wait until it's safe before it starts VirtioFsSvc.

@viktor-prutyanov
Copy link

Hi @jwatt

Do you plan to send a new version of the patch?

@@ -25,6 +25,7 @@
Name="VirtioFsSvc"
DisplayName="VirtIO-FS Service"
Description="Enables Windows virtual machines to access directories on the host that have been shared with them using virtiofs."
Dependencies="WinFsp.Launcher/VirtioFsDrv"

Choose a reason for hiding this comment

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

Suggested change
Dependencies="WinFsp.Launcher/VirtioFsDrv"
Dependencies="VirtioFsDrv"

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.

4 participants