-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
/// Check if a path exists as a subpath of the specified directory. | ||
/// </summary> | ||
/// <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 commentThe 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 commentThe 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 commentThe 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. |
||
|
||
/// <summary> | ||
/// Get the path to the volume root that the given path. | ||
/// </summary> | ||
/// <param name="path">File path to find the volume for.</param> | ||
/// <returns>Path to the root of the volume.</returns> | ||
string GetVolumeRoot(string path); | ||
|
||
/// <summary> | ||
/// Check if the volume for the given path is available and ready for use. | ||
/// </summary> | ||
/// <param name="path">Path to any directory or file on a volume.</param> | ||
/// <remarks> | ||
/// A volume might be unavailable for multiple reasons, including: | ||
/// <para/> | ||
/// - the volume resides on a removable device which is not present | ||
/// <para/> | ||
/// - the volume is not mounted in the operating system | ||
/// <para/> | ||
/// - the volume is encrypted or locked | ||
/// </remarks> | ||
/// <returns>True if the volume is available, false otherwise.</returns> | ||
bool IsVolumeAvailable(string path); | ||
|
||
/// <summary> | ||
/// Create an <see cref="IVolumeStateWatcher"/> which monitors for changes to the state of volumes on the system. | ||
/// </summary> | ||
/// <returns>Volume watcher</returns> | ||
IVolumeStateWatcher CreateVolumeStateWatcher(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
using System; | ||
|
||
namespace GVFS.Common.FileSystem | ||
{ | ||
/// <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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
{ | ||
/// <summary> | ||
/// Raised when the state of a volume has changed, such as becoming available. | ||
/// </summary> | ||
event EventHandler<VolumeStateChangedEventArgs> VolumeStateChanged; | ||
|
||
/// <summary> | ||
/// Start watching for changes to the states of volumes on the system. | ||
/// </summary> | ||
void Start(); | ||
|
||
/// <summary> | ||
/// Stop watching for changes to the states of volumes on the system. | ||
/// </summary> | ||
void Stop(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
using System; | ||
|
||
namespace GVFS.Common.FileSystem | ||
{ | ||
public enum VolumeStateChangeType | ||
{ | ||
/// <summary> | ||
/// The volume is now available and ready to use. | ||
/// </summary> | ||
VolumeAvailable, | ||
|
||
/// <summary> | ||
/// The volume is no longer available. | ||
/// </summary> | ||
VolumeUnavailable, | ||
} | ||
|
||
public class VolumeStateChangedEventArgs : EventArgs | ||
{ | ||
public VolumeStateChangedEventArgs(string volumePath, VolumeStateChangeType changeType) | ||
{ | ||
this.VolumePath = volumePath; | ||
this.ChangeType = changeType; | ||
} | ||
|
||
/// <summary> | ||
/// Path to the root of the volume that has changed. | ||
/// </summary> | ||
public string VolumePath { get; } | ||
|
||
/// <summary> | ||
/// Type of change. | ||
/// </summary> | ||
public VolumeStateChangeType ChangeType { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary? |
||
|
||
public static Response FromMessage(Message message) | ||
{ | ||
return JsonConvert.DeserializeObject<Response>(message.Body); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
[return: MarshalAs(UnmanagedType.Bool)] | ||
public static extern bool GetVolumePathName( | ||
string volumeName, | ||
StringBuilder volumePathName, | ||
uint bufferLength); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - couple things.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. oh...ok |
||
[DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)] | ||
private static extern bool MoveFileEx( | ||
string existingFileName, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
using GVFS.Common; | ||
using GVFS.Common; | ||
using GVFS.Common.FileSystem; | ||
using System; | ||
using System.IO; | ||
using System.Runtime.InteropServices; | ||
|
||
|
@@ -33,6 +34,32 @@ public void ChangeMode(string path, int mode) | |
Chmod(path, mode); | ||
} | ||
|
||
public bool IsPathUnderDirectory(string directoryPath, string path) | ||
{ | ||
// TODO(Mac): Check if the user has set HFS+/APFS to case sensitive | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's worth adding a TODO for this. If HFS+/APFS is set to case sensitive a lot of stuff will break. @sanoursa what do you think? |
||
// TODO(Mac): Normalize paths | ||
// Note: this may be called with paths or volumes which do not exist/are not mounted | ||
return path.StartsWith(directoryPath, StringComparison.OrdinalIgnoreCase); | ||
} | ||
|
||
public string GetVolumeRoot(string path) | ||
{ | ||
// TODO(Mac): Query the volume mount points and check if the path is under any of those | ||
// For now just assume everything is under the system root. | ||
return "/"; | ||
} | ||
|
||
public bool IsVolumeAvailable(string path) | ||
{ | ||
// TODO(Mac): Perform any additional checks for locked or encrypted volumes | ||
return Directory.Exists(path) || File.Exists(path); | ||
} | ||
|
||
public IVolumeStateWatcher CreateVolumeStateWatcher() | ||
{ | ||
return new MacVolumeStateWatcher(); | ||
} | ||
|
||
public bool TryGetNormalizedPath(string path, out string normalizedPath, out string errorMessage) | ||
{ | ||
return MacFileSystem.TryGetNormalizedPathImplementation(path, out normalizedPath, out errorMessage); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
using System; | ||
using GVFS.Common.FileSystem; | ||
|
||
namespace GVFS.Platform.Mac | ||
{ | ||
public class MacVolumeStateWatcher : IVolumeStateWatcher | ||
{ | ||
public event EventHandler<VolumeStateChangedEventArgs> VolumeStateChanged; | ||
|
||
public void Start() | ||
{ | ||
} | ||
|
||
public void Stop() | ||
{ | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
} | ||
|
||
protected void RaiseVolumeStateChanged(string volumePath, VolumeStateChangeType changeType) | ||
{ | ||
this.VolumeStateChanged?.Invoke(this, new VolumeStateChangedEventArgs(volumePath, changeType)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
using System; | ||
using System.Linq; | ||
using System.Management; | ||
|
||
namespace GVFS.Platform.Windows | ||
{ | ||
internal static class BitLockerHelpers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I can fold this into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That will just bloat the responsibilities of that other class :-) |
||
{ | ||
public const string VolumeEncryptionNamespace = @"root\cimv2\security\MicrosoftVolumeEncryption"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where did There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
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 commentThe 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 commentThe 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 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 commentThe 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. |
||
|
||
using (var searcher = new ManagementObjectSearcher(VolumeEncryptionNamespace, queryString)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
{ | ||
ManagementObjectCollection results = searcher.Get(); | ||
ManagementObject mo = results.OfType<ManagementObject>().FirstOrDefault(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 Will change to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. A constant There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'. |
||
if (mo.Properties["ProtectionStatus"].Value is uint protectedInt && protectedInt == 0) | ||
{ | ||
isLocked = false; | ||
return true; | ||
} | ||
|
||
// Check the lock status | ||
// Lock status is not a property and must be retrieved by the GetLockStatus method | ||
var args = new object[1]; | ||
if (TryInvokeMethod(mo, "GetLockStatus", args) && args[0] is uint lockedInt) | ||
{ | ||
// Unlocked | ||
if (lockedInt == 0) | ||
{ | ||
isLocked = false; | ||
return true; | ||
} | ||
|
||
// Locked | ||
if (lockedInt == 1) | ||
{ | ||
isLocked = true; | ||
return true; | ||
} | ||
} | ||
} | ||
} | ||
|
||
isLocked = false; | ||
return false; | ||
} | ||
|
||
private static bool TryInvokeMethod(ManagementObject mo, string methodName, object[] args) | ||
{ | ||
try | ||
{ | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Fail gracefully, trace exceptions. I mentioned that I was missing the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
{ | ||
return false; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are several classes named I needed to use this P/Invoke call outside of this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, i missed that |
||
{ | ||
errorMessage = "Could not get volume path name"; | ||
tracer.RelatedError($"{nameof(TryAttach)}:{errorMessage}"); | ||
|
@@ -626,13 +626,6 @@ public static extern uint FilterAttach( | |
string instanceName, | ||
uint createdInstanceNameLength = 0, | ||
string createdInstanceName = null); | ||
|
||
[DllImport("kernel32.dll", CharSet = CharSet.Unicode)] | ||
[return: MarshalAs(UnmanagedType.Bool)] | ||
public static extern bool GetVolumePathName( | ||
string volumeName, | ||
StringBuilder volumePathName, | ||
uint bufferLength); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
using GVFS.Common; | ||
using System; | ||
using System.Text; | ||
using GVFS.Common; | ||
using GVFS.Common.FileSystem; | ||
|
||
namespace GVFS.Platform.Windows | ||
|
@@ -29,6 +31,39 @@ public void ChangeMode(string path, int mode) | |
{ | ||
} | ||
|
||
public bool IsPathUnderDirectory(string directoryPath, string path) | ||
{ | ||
// TODO: Normalize paths | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. Referring back to my question about why The way to think of the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
} | ||
|
||
public string GetVolumeRoot(string path) | ||
{ | ||
var volumePathName = new StringBuilder(GVFSConstants.MaxPath); | ||
if (NativeMethods.GetVolumePathName(path, volumePathName, GVFSConstants.MaxPath)) | ||
{ | ||
return volumePathName.ToString(); | ||
} | ||
|
||
return null; | ||
} | ||
|
||
public bool IsVolumeAvailable(string path) | ||
{ | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. This also seems like it's not particular to any platform |
||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
public bool TryGetNormalizedPath(string path, out string normalizedPath, out string errorMessage) | ||
{ | ||
return WindowsFileSystem.TryGetNormalizedPathImplementation(path, 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.
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.