Issue174 #176

FrKaram · 2018-06-30T20:15:53Z

Implemented elements requested in Issue 174

Add FlushAndPurgeOnCleanup option
Add GetFSPVersion

Before submitting this PR please review this checklist. Ideally all checkmarks should be checked upon submitting. (Use an x inside square brackets like so: [x])

Contributing: You MUST read and be willing to accept the CONTRIBUTOR AGREEMENT. The agreement gives joint copyright interests in your contributions to you and the original WinFsp author. If you have already accepted the CONTRIBUTOR AGREEMENT you do not need to do so again.
Topic branch: Avoid creating the PR off the master branch of your fork. Consider creating a topic branch and request a pull from that. This allows you to add commits to the master branch of your fork without affecting this PR.
No tabs: Consistently use SPACES everywhere. NO TABS, unless the file format requires it (e.g. Makefile).
Style: Follow the same code style as the rest of the project.
Tests: Include tests to the extent that it is possible, especially if you add a new feature.
Quality: Your design and code should be of high quality and something that you are proud of.

Added FlushAndPurgeOnCleanup

billziss-gh

Thank you for the excellent work. This looks very good. There are a few issues having to do with an extraneous .gitignore file, some spaces vs tabs issues and a couple of other minor issues. Please see my comments below.

For me to accept this PR I also need you to agree to the Contributor Agreement. This gives joint copyright to your changes to both of us and also identifies you as a contributor to this project. You only need to sign this document for your first contribution, you do not need to sign it again for additional contributions.

Thanks again for your PR!

billziss-gh · 2018-06-30T21:25:00Z

src/dotnet/FileSystemHost.cs

+        {
+            get { return 0 != (_VolumeParams.Flags & VolumeParams.FlushAndPurgeOnCleanup); }
+            set { _VolumeParams.Flags |= (value ? VolumeParams.FlushAndPurgeOnCleanup : 0); }
+        }


Looks good!

I agreed the Contributor Agreement and I was under the impression that the updated version (with my name in it) was waiting for your approval. Am I missing something there ?

Thank you for your changes. Everything looks in order.

Regarding the Contributor Agreement, I have not received any document on this or any other Pull Request.

The preferred way to do this would be to add your name to the Contributor Agreement in your fork and add it as a commit that will then be added to this Pull Request. When I merge this Pull Request into WinFsp your name will also be included in the Contributor Agreement document.

billziss-gh · 2018-06-30T21:26:25Z

src/dotnet/FileSystemHost.cs

@@ -347,6 +352,14 @@ public static Int32 SetDebugLogFile(String FileName)
        {
            return Api.SetDebugLogFile(FileName);
        }
+        /// <summary>
+        /// Return the installed version for WinFSP


Please change this to read: Return the installed version of WinFsp.

(Note WinFsp vs WinFSP spelling and fullstop at end of sentence.)

Looks good otherwise although we may want to consider renaming the method to Version. What do you think?

Shouldn't methods be called with an action verb ?
In any case, we can call it version. It's fine for me.

billziss-gh · 2018-06-30T21:28:10Z

src/dotnet/Interop.cs

@@ -43,6 +43,7 @@ internal struct VolumeParams
        internal const UInt32 PassQueryDirectoryPattern = 0x00000800;
        internal const UInt32 AlwaysUseDoubleBuffering = 0x00001000;
        internal const UInt32 PassQueryDirectoryFileName = 0x00002000;
+        internal const UInt32 FlushAndPurgeOnCleanup = 0x00004000;


Looks good!

billziss-gh · 2018-06-30T21:29:27Z

src/dotnet/Interop.cs

+                SecurityDescriptorBytes = MakeSecurityDescriptor(SecurityDescriptor);
+                FspDeleteSecurityDescriptor(SecurityDescriptor, _FspSetSecurityDescriptorPtr);
+                return SecurityDescriptorBytes;
+            }
        }


Please fix tabs vs spaces here. WinFsp uses 4 space indendation and no tabs. (Or perhaps the file was erroneously using tabs before?)

There should be no changes here.

Quite sure i used spaces ... VisualStudio auto-format must have changed that... I'll correct that

billziss-gh · 2018-06-30T21:29:45Z

src/dotnet/Interop.cs

+            fixed (Byte *R = ReplaceReparseData)
+                return _FspFileSystemCanReplaceReparsePoint(
+                    (IntPtr)C, (UIntPtr)CurrentReparseData.Length,
+                    (IntPtr)R, (UIntPtr)ReplaceReparseData.Length);
        }


Same comment as above re: spaces vs tabs.

There should be no changes here.

billziss-gh · 2018-06-30T21:33:05Z

src/dotnet/Interop.cs

+            VersionMinor = Version & 0xFFFF;
+            return new System.Version((Int32)VersionMajor, (Int32)VersionMinor);
+        }
+


Looks good overall. I do find the VersionMajor and VersionMinor variables unnecessary. Perhaps we should change this to read:

internal static Version GetFspVersion() { UInt32 Version = 0; FspVersion(out Version); return new System.Version((Int32)(Version >> 16), (Int32)(Version & 0xFFFF)); }

billziss-gh · 2018-06-30T21:33:20Z

tst/memfs-dotnet/Program.cs

@@ -70,7 +70,7 @@ public FileInfo GetFileInfo()
            {
                FileInfo FileInfo = MainFileNode.FileInfo;
                FileInfo.FileAttributes &= ~(UInt32)FileAttributes.Directory;
-                    /* named streams cannot be directories */
+                /* named streams cannot be directories */
                FileInfo.AllocationSize = this.FileInfo.AllocationSize;


Same comment as above re: spaces vs tabs.

There should be no changes here.

billziss-gh · 2018-06-30T21:35:30Z

tst/memfs-dotnet/Program.cs

@@ -255,6 +255,7 @@ public override Int32 Init(Object Host0)
            Host.NamedStreams = true;
            Host.PostCleanupWhenModifiedOnly = true;
            Host.PassQueryDirectoryFileName = true;
+            Host.FlushAndPurgeOnCleanup = true;


I would ask that we remove this line, because it changes the default behavior of WinFsp .NET file systems. Thanks to your work, It is now easy to have a file system set this property when it needs it.

Sure. It was a test that I didn't remove ...

billziss-gh · 2018-06-30T21:35:52Z

tst/memfs-dotnet/Program.cs

+                            argtos(Args, ref I, ref VolumePrefix);
+                            break;
+                        default:
+                            throw new CommandLineUsageException();
                    }


Same comment as above re: spaces vs tabs.

There should be no changes here.

billziss-gh · 2018-06-30T21:36:13Z

.gitignore

+################################################################################
+
+*.db
+*.opendb


This .gitignore file is not needed. Please remove.

I had to add it because of locked file.
I was pushing via Visual Studio. I will push from command line and won't need that file anymore

FrKaram · 2018-07-01T11:54:56Z

Seems to me that I managed to correct all those indendation issues...
The only remaining changes are the "real" ones

billziss-gh · 2018-07-03T09:42:35Z

@FrKaram thank you. This looks great. I am currently on the road so I do not have easy access to a computer, but I will try to accept this PR tonight or tomorrow.

FrKaram · 2018-07-03T09:48:36Z

Well that's my first PR on an open-source project which is not a project of mine.
That's great ! :-)

billziss-gh · 2018-07-05T20:55:53Z

@FrKaram I have finally merged this into master. Thank you again.

dotnet: add FlushAndPurgeOnCleanup and FspVersion

FrKaram added 2 commits June 28, 2018 23:01

Added FlushAndPurgeOnCleanup property in .NET wrapper

d0f5ea6

Added FpsVersion as a static method in FileSystemHost

fb8cb8a

Added FlushAndPurgeOnCleanup

billziss-gh requested changes Jun 30, 2018

View reviewed changes

FrKaram added 8 commits July 1, 2018 13:29

Changes following PR remarks

a7424c9

Wrongly remove gitignore

916b4f5

Fixed indentation

88516f3

Fixed identation

8422e81

Fixed Indentation

c9c62b1

Correct VS auto-format

3513f0d

Indentation again

c73f709

Indentation again

2056766

FrKaram and others added 2 commits July 3, 2018 10:01

Updated Contributor Agreement

fd817e3

Update Contributors.asciidoc

ee4145e

billziss-gh approved these changes Jul 3, 2018

View reviewed changes

billziss-gh merged commit 6ffddf3 into winfsp:master Jul 5, 2018

billziss-gh mentioned this pull request Jul 5, 2018

Add missing properties and helper in the C# wrapper #174

Closed

Noire001 pushed a commit to Noire001/winfsp that referenced this pull request Sep 25, 2022

Merge pull request winfsp#176 from FrKaram/Issue174

27327b7

dotnet: add FlushAndPurgeOnCleanup and FspVersion

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue174 #176

Issue174 #176

FrKaram commented Jun 30, 2018 •

edited

billziss-gh left a comment •

edited

billziss-gh Jun 30, 2018

FrKaram Jul 1, 2018 •

edited

billziss-gh Jul 2, 2018

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018

FrKaram Jul 1, 2018

billziss-gh Jun 30, 2018

billziss-gh Jun 30, 2018 •

edited

FrKaram Jul 1, 2018

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018

FrKaram Jul 1, 2018

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018

FrKaram Jul 1, 2018

FrKaram commented Jul 1, 2018

billziss-gh commented Jul 3, 2018

FrKaram commented Jul 3, 2018

billziss-gh commented Jul 5, 2018

Issue174 #176

Issue174 #176

Conversation

FrKaram commented Jun 30, 2018 • edited

billziss-gh left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrKaram Jul 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh Jun 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh Jun 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh Jun 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh Jun 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billziss-gh Jun 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrKaram commented Jul 1, 2018

billziss-gh commented Jul 3, 2018

FrKaram commented Jul 3, 2018

billziss-gh commented Jul 5, 2018

FrKaram commented Jun 30, 2018 •

edited

billziss-gh left a comment •

edited

FrKaram Jul 1, 2018 •

edited

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018 •

edited

billziss-gh Jun 30, 2018 •

edited