-
Notifications
You must be signed in to change notification settings - Fork 451
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
RFC: Enhance automount to wait for volume to be available #359
Conversation
Enhance the auto-mounting logic to watch for new volumes to be attached, and to wait for BitLocker drives to be unlocked before mounting. Add ability to list the state of all known repositories using the cmd: `gvfs service --list-all`. TODO: tests!
private readonly IDictionary<string, bool> cryptoVolumeLockStatuses = | ||
new ConcurrentDictionary<string, bool>(StringComparer.OrdinalIgnoreCase); | ||
|
||
public WindowsVolumeStateWatcher(TimeSpan pollingInterval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably take an ITracer
.
@@ -6,34 +6,34 @@ | |||
|
|||
namespace GVFS.Service | |||
{ | |||
public class GVFSMountProcess : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name confused me to no-end! 🤦♂️ I thought this represented a mount process instance itself, but it's really a sort of factory for processes right? I originally added a StopMount(string repoRoot)
method to this along with the rename which would be invoked on VolumeUnavailable
but removed that logic in favour of open question 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this represented a mount process instance itself, but it's really a sort of factory for processes right?
I'm not sure that I'd say it's a factory (as it doesn't produce any objects) but you have the right idea. This class is a helper for calling the gvfs
CLI.
Part of the naming confusion might be because prior to calling gvfs mount
this class would start GVFS.Mount.exe
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave the name unchanged. I can understand the confusion, but it's analogous to the .NET Process
class, or our own GitProcess
class. The class is not the process - it's code for interacting with the process.
|
||
mounter.Start(); | ||
|
||
// TODO: this.repoRegistry.TraceStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracing of current state the repositories after logon should probably still happen, but do we want to trace the full status after each 'auto mount' or just at logon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to trace enough that we know what each auto-mount mounted (or attempted to mount and failed)
@@ -42,14 +43,7 @@ public void Run() | |||
{ | |||
if (!this.IsValidRepo(repoRoot)) | |||
{ | |||
if (!this.registry.TryRemoveRepo(repoRoot, out errorMessage)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the line which is 'cleaning up' missing repositories on read. I don't think we should be clearing away entries on read? A 'clean up registrations' message would be more explicit, and could also do the check (volume exists && repo not exists
) listed in open question 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still be nice to clean up old entries without requiring user interaction, and so your suggestion here:
and could also do the check (volume exists && repo not exists) listed in open question 2.
Sounds like a good option.
|
||
namespace GVFS.Service | ||
{ | ||
public class RepoAutoMounter : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently there is no Mac daemon equivalent to the Windows service, but when there is in the future, this class is platform independent.
break; | ||
case VolumeStateChangeType.VolumeUnavailable: | ||
// There is no need to do anything here to stop any potentially orphaned mount processes | ||
// since they will self-terminate if the volume is removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment lies. Mount processes do not self-terminate.. but should they? Open question 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do self-terminate the next time they try to do some IO and it fails.
{ | ||
if (!normalizedEnlistmentRootPath.Equals(registration.EnlistmentRoot, StringComparison.OrdinalIgnoreCase)) | ||
string normalizedPath; | ||
if (GVFSPlatform.Instance.FileSystem.TryGetNormalizedPath(registration.EnlistmentRoot, out normalizedPath, out errorMessage)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryGetNormalizedPath
will fail if the path doesn't actually exist. For repositories on volumes which are still locked by BitLocker this means we can read the registry entries! Not sure why we'd ever get non-normalised paths in the registry, but changed this to only bother trying if the volume is actually available so we can read all the registry entries.
// TODO #1043088: We need to respect the elevation level of the original mount | ||
if (this.mountProcessManager.StartMount(enlistmentRoot)) | ||
{ | ||
this.SendNotification("GVFS AutoMount", "The following GVFS repo is now mounted:\n{0}", enlistmentRoot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: these toast notifications (existing code) from "GVFS.Service.UI" never actually seemed to show for me, even on released versions of VFS for Git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea they have been very problematic and inconsistent for us. I suspect something changed in the platform and we're no longer using their APIs correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to keep the comments high level. Overall the approach looks good, but I think there are some features you've included in your changes that might be outside the scope of auto-mounting when volumes become available.
{ | ||
internal static class BitLockerHelpers | ||
{ | ||
public const string VolumeEncryptionNamespace = @"root\cimv2\security\MicrosoftVolumeEncryption"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did VolumeEncryptionNamespace
come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
string queryString = $"SELECT * FROM Win32_EncryptableVolume WHERE DriveLetter = '{driveName}'"; | ||
|
||
using (var searcher = new ManagementObjectSearcher(VolumeEncryptionNamespace, queryString)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: in the VFS for Git source code we use explicit types rather than var
@@ -53,7 +53,7 @@ public static bool TryAttach(ITracer tracer, string enlistmentRoot, out string e | |||
try | |||
{ | |||
StringBuilder volumePathName = new StringBuilder(GVFSConstants.MaxPath); | |||
if (!NativeMethods.GetVolumePathName(enlistmentRoot, volumePathName, GVFSConstants.MaxPath)) | |||
if (!Common.NativeMethods.GetVolumePathName(enlistmentRoot, volumePathName, GVFSConstants.MaxPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need Common
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several classes named NativeMethods
thoughout the codebase.
This callsite used to use the inner-class private static class NativeMethods
which had the GetVolumePath
definition.
I needed to use this P/Invoke call outside of this ProjFSFilter
class so I moved it to the GVFS.Common.NativeMethods
class instead. Using just NativeMethods.blah
here refers to the inner-class (which still has the FilterAttach
P/Invoke method) whereas I want to reference GVFS.Common.NativeMethods
instead now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, i missed that NativeMethods
was still a private class here. The P/Invokes are kind of a mess right now, long term I don't think we want any in GVFS.Common
(as they're all platform specific) but no need to clean that up now.
Default = false, | ||
Required = false, | ||
HelpText = "Prints a list of all known repos and their statuses")] | ||
public bool ListAll { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding list-all
seems a bit unrelated to the main feature of this PR (auto-mounting after unlocking a drive). Was there something that required it?
If not, I'd recommend taking out the changes to support list-all
so that it's easier to focus on the changes specifically needed for auto-mounting after unlocking a locked drive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels more like a debugging feature than an end user feature anyway, so I would take it out. For debugging, we can just inspect the file directly.
@@ -164,6 +164,13 @@ public static DateTime GetLastRebootTime() | |||
return DateTime.Now - uptime; | |||
} | |||
|
|||
[DllImport("kernel32.dll", CharSet = CharSet.Unicode)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your change, and even though we're not currently using it no for consistency add SetLastError = true,
@@ -26,6 +28,7 @@ public class GVFSService : ServiceBase | |||
private string serviceName; | |||
private string serviceDataLocation; | |||
private RepoRegistry repoRegistry; | |||
private IDictionary<int, RepoAutoMounter> repoMounters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does
repoMounters
need to be thread safe? - I'm not sure we're gaining anything by using
IDictionary
here, can we just use the underlying type ofrepoMounters
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even clear that we need a dictionary here. From the current usage, a List would be fine, but it seems that we could even get away with nothing at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to have multiple concurrent sessions from different users (imagine a terminal services server allowing multiple interactive sessions).
The current design (and even in the 'register for volume available and fire once' design too) means we should stop listening for volume changes for a user's repos once they've ended their session. We need a way to keep track of that 'listen for volume changes for this session' so we can stop it.
if (this.repoMounters.TryGetValue(sessionId, out mounter)) | ||
{ | ||
mounter.Stop(); | ||
mounter.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove the mounter from repoMounters
|
||
mounter.Start(); | ||
|
||
// TODO: this.repoRegistry.TraceStatus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to trace enough that we know what each auto-mount mounted (or attempted to mount and failed)
{ | ||
private readonly ManagementEventWatcher volumeWatcher; | ||
private readonly ManagementEventWatcher cryptoVolumeWatcher; | ||
private readonly IDictionary<string, bool> cryptoVolumeLockStatuses = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same minor comment here ConcurrentDictionary<string, bool> cryptoVolumeLockStatuses
instead of IDictionary<string, bool> cryptoVolumeLockStatuses
|
||
// Create a mount process factory for this session/user | ||
this.mountProcessManager = new GVFSMountProcessManager(this.tracer, sessionId); | ||
this.userSid = this.mountProcessManager.CurrentUser.Identity.User?.Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you ever seen this.mountProcessManager.CurrentUser.Identity.User
be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not seen it to be null, but some helpful(?) tooling indicated this property could possibly be null.
It appears the WindowsIdentity.User
property is null
only if the identity is the 'anonymous' user. Sessions could be created for anonymous users (file share connections open to 'everyone' perhaps?).. not sure if the fact we only act for SessionChangeReason.SessionLogon
in OnSessionChanged
means we don't see anonymous user sessions.
I can remove the ?.
if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would this do if User is null? What does it mean for us to store a userSid of 0? How will that behave when we try to mount later?
The issue isn't so much the presence of the ?
as it is that this will likely lead to cascading failures down the line. If you're going to handle null users here, then you also need to do something other than carry on as if it succeeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user is null, then we'd cascade the nulls and have a (string)null
SID. The call later when mounting to get the list of registered, active repositories for that user's SID would return no repositories (it filters the collection by string.Compare(repo.OwnerSID, ownerSID, ..)
.
I guess this is a silent event that we might want to know about (that we have a null/anonymous user). I'll remove the ?.
to revert to the existing behaviour?
I think this could probably be its own issue\PR. @sanoursa, what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick style pass so far. I'm still working on understanding the design, more feedback to come on that.
@@ -8,5 +8,42 @@ public interface IPlatformFileSystem | |||
void CreateHardLink(string newLinkFileName, string existingFileName); | |||
bool TryGetNormalizedPath(string path, out string normalizedPath, out string errorMessage); | |||
void ChangeMode(string path, int mode); | |||
|
|||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: As you can see, we don't tend to have many xml docs on our methods. I know this is a subject that we could debate and have well-informed opinions on both sides, but in this codebase our approach has been to favor descriptive method and param names over xml docs. We still do add the comments where they describe something that's non-obvious, but in this example here, the comments are only duplicating the info that is already present in the method signature.
Basically, I would drop all of the comments in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I was under the impression that it was policy for all public
types and methods in product assemblies to be fully documented. Is VFS for Git a supported product? Should all VFS for Git assemblies not be considered public (even if they physically are) and carry no compatibility guarentees? Should these types be made internal instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of any such policy, and I don't think it's useful to litter the codebase with boilerplate comments. Also we're creating a product, not an API, and no valid deployment will ever mix/match different versions of our dll's, so no there are no compatibility guarantees.
/// <param name="directoryPath">Directory path.</param> | ||
/// <param name="path">Path to query.</param> | ||
/// <returns>True if <see cref="path"/> exists as a subpath of <see cref="directoryPath"/>, false otherwise.</returns> | ||
bool IsPathUnderDirectory(string directoryPath, string path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this method exist here in this interface? Isn't this a platform-agnostic question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, although case sensitivity when comparing path segments is file system (and transitively OS) dependent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that is a valid concern. However, note that we are currently very unprepared to deal with case sensitive file systems. Note that even putting it here in this interface doesn't solve that problem, since it's not a per-OS question but rather a per-file-system-and-machine-configuration question. We will have to deal with this as a broader issue since we are now porting to Linux as well, but it is outside the scope of this PR.
using (var searcher = new ManagementObjectSearcher(VolumeEncryptionNamespace, queryString)) | ||
{ | ||
ManagementObjectCollection results = searcher.Get(); | ||
ManagementObject mo = results.OfType<ManagementObject>().FirstOrDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid abbreviated variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I usually don't use abbreviated variable names, but based on this prior art I thought it was acceptable for ManagementBaseObject
and ManagementObject
to be mbo
/mo
.
Will change to win32CryptoVolume
or similar. Should the other instances of mbo
short var names (such as in WindowPhysicalDiskInfo.cs) be expanded too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain I asked for no abbreviations on that other code as well :-). I do like your suggested rename, as it describe the thing the object represents, rather than its type in the API.
// We can't use the existing TryGetNormalizedPathImplementation method | ||
// because it relies on actual calls to the disk to check if directories exist. | ||
// This may be called with paths or volumes which do not actually exist. | ||
return path.StartsWith(directoryPath, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referring back to my question about why IsPathUnderDirectory
was added to the interface - the implementations of this method do not seem to be doing anything platform-specific, so they do not seem to belong here.
The way to think of the GVFSPlatform
and the interfaces that it uses are that they represent the capabilities of the OS that we rely on. Resolving a path to a volume root is a good example, because what we require is that we can resolve to a root, and each platform will do that differently. Normalizing a path is also a good example. But given those two primitives, there is no need for a platform-specific method to do just a string comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is case sensitivity of the underlying file system not a platform dependent property? HFS+ on Mac (and I guess NTFS on Windows) can support case sensitive modes or directories.. or is the stake in the ground that VFS for Git will never support case sensitive file systems (Linux/ext*)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it is not a platform-dependent property, where platform means OS. On the same OS, a file system can be case sensitive or insensitive. We currently have no pattern to deal with this, but will need to add one soon.
{ | ||
// No paths 'exist' on locked BitLocker volumes so it is sufficent to just | ||
// check if the directory/file exists using the framework APIs. | ||
return System.IO.Directory.Exists(path) || System.IO.File.Exists(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems like it's not particular to any platform
this.cryptoVolumeWatcher.EventArrived += this.OnCryptoVolumeEvent; | ||
} | ||
|
||
public event EventHandler<VolumeStateChangedEventArgs> VolumeStateChanged; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What benefit are we getting here from using a C# event vs just a callback delegate? I don't think we want/need to support multiple listeners, right? Events tend to make diagnostics a little harder, so I would constrain this down if we don't need the flexibility.
@@ -6,34 +6,34 @@ | |||
|
|||
namespace GVFS.Service | |||
{ | |||
public class GVFSMountProcess : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave the name unchanged. I can understand the confusion, but it's analogous to the .NET Process
class, or our own GitProcess
class. The class is not the process - it's code for interacting with the process.
Default = false, | ||
Required = false, | ||
HelpText = "Prints a list of all known repos and their statuses")] | ||
public bool ListAll { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels more like a debugging feature than an end user feature anyway, so I would take it out. For debugging, we can just inspect the file directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the piece-meal review. More coming.
/// <summary> | ||
/// Monitors the system for changes to the state of any disk volume, notifying subscribers when a change occurs. | ||
/// </summary> | ||
public interface IVolumeStateWatcher : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new interface and 3 methods seems like overkill for the actual functionality that this is providing. Would we get everything we need from IPlatformFileSystem.RegisterForVolumeAvailable(Action<string> callback)
where the string is the path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you imagine this to be a 'fire once/self unregister' callback? Or return some cookie/handle as part of the registration process to be used when unregistering otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fire once makes the most sense to me, but I've realized that we just have different assumptions about what this feature is trying to do. See more on my upcoming note.
/// <summary> | ||
/// Monitors the system for changes to the state of any disk volume, notifying subscribers when a change occurs. | ||
/// </summary> | ||
public interface IVolumeStateWatcher : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think we actually care about volumes becoming un available, so we don't want to keep firing events if a volume comes up, goes down, comes up, goes down... I would think about constraining this design so that we make a single attempt to automount per repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a user who has a laptop (with a bitlockered drive attached to the docking station), undocks to go for a meeting, and then redocks not be considered for automounting? (I might imagine MacBook users with limited drive space might have external drives for their large-VFS-for-Git-required repos.)
Is 'automounting' === 'try and mount once automatically' or === 'keep this repo mounted automatiaclly'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today's behavior is: one and only one best effort attempt to automount after initial login. I don't think we should expand the scope of that feature in this PR, but should instead focus on extending that same behavior to BitLockered drives.
Expanding the intent of the feature could also be a fine thing to do, but it carries a lot more risk (we have nothing today that will automount without either an explicit user gesture or a fatal error, and nothing that will automount at any time other than initial login) and so it could lead to surprising interactions for users and issues that are difficult to diagnose. So going after those other scenarios could be nice, but will require a much more thorough design to ensure we don't create any unexpected behaviors.
@@ -300,6 +300,8 @@ public class Response : BaseResponse<GetActiveRepoListRequest> | |||
{ | |||
public List<string> RepoList { get; set; } | |||
|
|||
public List<string> InvalidRepoList { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
|
||
namespace GVFS.Platform.Windows | ||
{ | ||
internal static class BitLockerHelpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names like "Helper" and "Manager" are code smells for classes that do too much. Can we be more specific about what this class is responsible for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fold this into the WindowsVolumeStateWatcher
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will just bloat the responsibilities of that other class :-)
|
||
public static bool TryGetVolumeLockStatus(string driveName, out bool isLocked) | ||
{ | ||
string queryString = $"SELECT * FROM Win32_EncryptableVolume WHERE DriveLetter = '{driveName}'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have seen in the past that WMI queries were unreliable (when we were trying to use this approach to detect git processes that terminated without calling our hooks). How reliable is this going to be for detecting volumes getting unlocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I have found reading the current BitLocker state it to be very reliable.
Watching for events (in class WindowsVolumeStateWatcher
) has also been reliable.
Events for Win32_VolumeChangeEvent
monitor for general drive added/removed; this was pretty instant.
Events for __InstanceModificationEvent
to Win32_EncryptableVolume
monitor for BitLocker volume status changes; these are a little slower due to the filtering required in the query over all __InstanceModificationEvent
s.. we don't need to respond immediately within microseconds to an unlock however do we?
AFAIK this is the only exposed API we have to query BitLocker information and to watch/wait for changes to lock/unlock state, without having the BitLocker filter driver expose information and events like PrjFlt does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem we had with the WMI queries in the past is that they worked 99% of the time but would occasionally not deliver an event. I strongly suspect you'll have the same problem here.
If we continue to treat automount as a best effort one-time behavior after initial login, I think that lack of reliability is tolerable. If we're trying to continuously monitor volumes and react, this becomes a lot more problematic.
if (mo != null) | ||
{ | ||
// Check the protection status | ||
// ProtectionStatus == 0 means the drive is not protected by BitLocker and therefore cannot be locked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A constant NotBitLockered = 0
would make the code more clear, and remove the need for this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the part of the comment that a drive being unprotected means it cannot be locked is still useful for those reading in the future not knowing the difference between 'protection' and 'lock'.
I'll extract the integers as private constants with commented links to the relevant source docs.
{ | ||
return mo.InvokeMethod(methodName, args) is uint result && result == 0; | ||
} | ||
catch (Exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your plan for handling/logging/failing on exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail gracefully, trace exceptions. I mentioned that I was missing the ITracer
from this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really asking what it means to fail gracefully :-). Catching all exceptions (even with logging) is more akin to succeeding blindly. We tend to favor a fail-fast approach, but it's a little different here since this code is not handling any repo data and is less likely to get itself into a bad state.
string volumeName, | ||
StringBuilder volumePathName, | ||
uint bufferLength); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - couple things.
- Is volumePathName the output here? And the return value indicates success/failure? If yes, then may be this could be renamed to TryGetVolumePathName()?
- Usage of both “Path” and “Name” in method & argument names look confusing to me. If it means the full path to a mounted volume, then may be “Path” is enough? Like TryGetVolumePath(.., out string path, …)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a new method, it's just mapping an existing Win32 API into C# code. So we need to stick with the names/signatures of that existing method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh...ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My general feedback is that some of the code is overly complex, but the root cause is that we have different assumptions about the goals of this feature. I would suggest scoping down this PR to just extend the current "best effort one-time automount after login" behavior to drives that are BitLockered. That should simplify the code down quite a bit, but more importantly, create far fewer moving parts that could lead to unexpected failures for users.
public IVolumeStateWatcher CreateVolumeStateWatcher() | ||
{ | ||
// TODO: Extract the polling interval to a configuration value? | ||
return new WindowsVolumeStateWatcher(TimeSpan.FromSeconds(15)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
If we are going to be polling anyways, wouldn't it be simpler to just iterate through the yet-to-be-mounted repository list and check if the Enlistment root path became available and readable using regular FileSystem API's in the Timer callback? Thats all that we need to know, right? Or does the volumeWatcher and cryptoVolumeWatcher objects offer anything more?
-
After all the un-available repos get mounted eventually, do we shutdown this timer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach @alameenshah is proposing in the first suggestion is interesting. It would remove the reliance in WMI and may result in a simpler implementation that also could be cross-platform from the start. What does that approach miss, @mjcheetham ?
/cc @sanoursa
I'm going to close this PR for now. We're going to pick up the work in a subsequent PR, but the approach will change enough that I want to start fresh there. |
Not ready for merge. Just a draft/RFC to get some initial feedback on the approach.
(there are still some TODOs in the code, and a lack of tests! These will follow later 😄)
Enhance the auto-mounting logic to watch for new volumes to be attached (such as external drives), and to wait for BitLocker drives to be unlocked before mounting.
This changes the notion of 'auto mount' from a single action that runs on logon to an on-going process of mounting repositories as they become 'available'. At first logon we still attempt to mount all registered repositories as before.
I've used the term 'available'/'unavailable' in this change as it captures both the missing external drive case, as well as the encypted volume cases (such as BitLocker on Windows and any equivalents on macOS). I've used the term 'volume' rather than 'drive' as you can also mount drives as NTFS folder paths on Windows, and in macOS where volumes can be mounted anywhere under "/".
Added ability to list the state of all known repositories using the cmd:
gvfs service --list-all
, to compliment the existinggvfs service --list-mounted
option which lists only the mounted repositories.Open questions:
We can now detect when a volume is removed. Currently if you remove a volume the mount process continues to run and is reported as 'mounted' (this is the existing behaviour).
Should we try and kill the mount processes for repositories on volumes that have been removed/locked? Should the mount process itself be responsible for killing itself or should the service send a "kill" message?
1.1. If we kill the process on
VolumeUnavilable
, and then you quickly lock and then unlock a volume (or plug/unplug an external drive) then do we want the removal to be considered a 'transient'; should we wait a fixed time (10 seconds) before killing the mount process?1.2. We can't send the 'unmount' message because this does two things we don't want: a) unregister the repository, b) read/write from disk! it's too late to unmount.. we can only 'die quickly'.
Currently if a repository does not exist and we're trying to mount it (at logon) we remove it from the repo-registry file (assuming it's been deleted). This might not be the case if a volume is locked, or has not been mounted yet. In this PR I've removed the 'remove repo on missing path' code, but this might lead to lots of redundant entries over time with no clean-up.
We could consider a heuristic that if the volume of the repository does not exist then do NOT remove the registration, but if the volume does exist and the enlistment does not then it has likely been deleted and should be unregistered.
The only way to watch for BitLocker lock/unlock events is using the
ManagementEventWatcher
, which polls WMI. What polling interval should be used (I picked 10 seconds arbitrarily)? Too short and you end up using CPU. Too long and we increase the gap between the volume being unlocked and the call togvfs mount
.[Future]: Users might want to select some repositories to not auto-mount. Currently they can do this by expliclty running
gvfs unmount
which sets "IsActive" to false in the registration - we don't ever try and auto-mount it.