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

RFC: Enhance automount to wait for volume to be available #359

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions GVFS/GVFS.Common/FileSystem/IPlatformFileSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

/// 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);
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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.


/// <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();
}
}
25 changes: 25 additions & 0 deletions GVFS/GVFS.Common/FileSystem/IVolumeStateWatcher.cs
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
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

@mjcheetham mjcheetham Oct 10, 2018

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'?

Copy link
Contributor

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.

{
/// <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();
}
}
36 changes: 36 additions & 0 deletions GVFS/GVFS.Common/FileSystem/VolumeStateChangedEventArgs.cs
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; }
}
}
2 changes: 2 additions & 0 deletions GVFS/GVFS.Common/NamedPipes/NamedPipeMessages.cs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ public class Response : BaseResponse<GetActiveRepoListRequest>
{
public List<string> RepoList { get; set; }

public List<string> InvalidRepoList { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Expand Down
7 changes: 7 additions & 0 deletions GVFS/GVFS.Common/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,13 @@ public static DateTime GetLastRebootTime()
return DateTime.Now - uptime;
}

[DllImport("kernel32.dll", CharSet = CharSet.Unicode)]
Copy link
Member

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,

[return: MarshalAs(UnmanagedType.Bool)]
public static extern bool GetVolumePathName(
string volumeName,
StringBuilder volumePathName,
uint bufferLength);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit - couple things.

  1. Is volumePathName the output here? And the return value indicates success/failure? If yes, then may be this could be renamed to TryGetVolumePathName()?
  2. 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, …)?

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down
29 changes: 28 additions & 1 deletion GVFS/GVFS.Platform.Mac/MacFileSystem.cs
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;

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

// TODO(Mac): Check if the user has set HFS+/APFS to case sensitive

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);
Expand Down
27 changes: 27 additions & 0 deletions GVFS/GVFS.Platform.Mac/MacVolumeStateWatcher.cs
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));
}
}
}
67 changes: 67 additions & 0 deletions GVFS/GVFS.Platform.Windows/BitLockerHelpers.cs
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
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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 const string VolumeEncryptionNamespace = @"root\cimv2\security\MicrosoftVolumeEncryption";
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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}'";
Copy link
Contributor

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?

Copy link
Member Author

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 __InstanceModificationEvents.. 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.

Copy link
Contributor

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.


using (var searcher = new ManagementObjectSearcher(VolumeEncryptionNamespace, queryString))
Copy link
Member

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

{
ManagementObjectCollection results = searcher.Get();
ManagementObject mo = results.OfType<ManagementObject>().FirstOrDefault();
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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.

if (mo != null)
{
// Check the protection status
// ProtectionStatus == 0 means the drive is not protected by BitLocker and therefore cannot be locked
Copy link
Contributor

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.

Copy link
Member Author

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.

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)
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

{
return false;
}
}
}
}
2 changes: 2 additions & 0 deletions GVFS/GVFS.Platform.Windows/GVFS.Platform.Windows.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
<Compile Include="$(BuildOutputDir)\CommonAssemblyVersion.cs">
<Link>CommonAssemblyVersion.cs</Link>
</Compile>
<Compile Include="BitLockerHelpers.cs" />
<Compile Include="DiskLayoutUpgrades\DiskLayout10to11Upgrade_NewOperationType.cs" />
<Compile Include="DiskLayoutUpgrades\DiskLayout11to12Upgrade_SharedLocalCache.cs" />
<Compile Include="DiskLayoutUpgrades\DiskLayout12to13Upgrade_FolderPlaceholder.cs" />
Expand Down Expand Up @@ -101,6 +102,7 @@
<Compile Include="WindowsPlatform.cs" />
<Compile Include="WindowsPlatform.Shared.cs" />
<Compile Include="WindowsFileSystem.cs" />
<Compile Include="WindowsVolumeStateWatcher.cs" />
</ItemGroup>
<ItemGroup>
<None Include="packages.config">
Expand Down
9 changes: 1 addition & 8 deletions GVFS/GVFS.Platform.Windows/ProjFSFilter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

{
errorMessage = "Could not get volume path name";
tracer.RelatedError($"{nameof(TryAttach)}:{errorMessage}");
Expand Down Expand Up @@ -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);
}
}
}
37 changes: 36 additions & 1 deletion GVFS/GVFS.Platform.Windows/WindowsFileSystem.cs
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
Expand Down Expand Up @@ -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);
Copy link
Contributor

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.

Copy link
Member Author

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*)?

Copy link
Contributor

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.

}

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);
Copy link
Contributor

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

}

public IVolumeStateWatcher CreateVolumeStateWatcher()
{
// TODO: Extract the polling interval to a configuration value?
return new WindowsVolumeStateWatcher(TimeSpan.FromSeconds(15));
Copy link
Contributor

@alameenshah alameenshah Oct 10, 2018

Choose a reason for hiding this comment

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

  1. 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?

  2. After all the un-available repos get mounted eventually, do we shutdown this timer?

Copy link
Member

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

}

public bool TryGetNormalizedPath(string path, out string normalizedPath, out string errorMessage)
{
return WindowsFileSystem.TryGetNormalizedPathImplementation(path, out normalizedPath, out errorMessage);
Expand Down
Loading