File system watcher #20

sbusch42 · 2018-11-25T12:20:42Z

This is our first take on implementing a file system watcher interface. It can be merged as soon as the Windows implementation of LocalFileWatcher has been implemented.

Missing features, nice-to-have:

Implement windows/LocalFileWatcher
Research and implement macOS backend
The linux backend is in the posix directory, but it's not posix, so it should be in a linux directory, use null implementation for other *nix systems

…r linux

…irectory

… FS (not relevant, yet)

source/cppfs/include/cppfs/cppfs.h

source/cppfs/source/posix/LocalFileWatcher.cpp

source/cppfs/source/FileEventHandler.cpp

source/cppfs/source/posix/LocalFileWatcher.cpp

… is subject of an event)

sbusch42 · 2018-11-25T15:34:35Z

I guess we should also support some more events like changing file permissions or renaming files

sbusch42 · 2018-11-25T15:47:47Z

ref #3

More complete windows implementation

sbusch42

Some questions mainly about the windows backend.

source/cppfs/source/windows/LocalFileWatcher.cpp

sbusch42 · 2018-12-03T19:09:09Z

source/cppfs/source/windows/LocalFileWatcher.cpp

+    if (!hEvent)
+        throw std::runtime_error("failed creating wait event");
+
+    auto lw = std::make_shared<LocalWatcher>();


Are the shared pointers here ever shared between several objects, or are they unique? (seems to me that way). Maybe we can use unique_ptr, or even just plain handles?

shared_ptr is used because you can specify the delete functions for the object, which in case of Windows HANDLE is the ::CloseHandle function. Out of pure convenience, and to avoid dangling handles.

sbusch42 · 2018-12-03T19:10:54Z

source/cppfs/source/windows/LocalFileWatcher.cpp

+                NULL);
+            if (ret != 0) {
+                std::string fname(fileName, fileName + ret);
+                std::replace(fname.begin(), fname.end(), '\\', '/');


No need to convert path delimiters, FilePath already takes care of that

sbusch42 · 2018-12-03T19:14:03Z

source/cppfs/include/cppfs/windows/LocalFileWatcher.h

+
+protected:
+    std::shared_ptr<LocalFileSystem> m_fs;            ///< File system that created this watcher
+    std::shared_ptr<void>            m_waitStopEvent; ///< Event to stop watch function


These are not shared in any way, so it should be unique_ptr instead of shared_ptr. But also that smart pointer does not do anything, the handle has been released by specific API-functions anyway, so I would suggest to just use void* here.

sbusch42 · 2018-12-03T19:15:22Z

source/cppfs/source/posix/LocalFileWatcher.cpp

+                 if (event->mask & IN_CREATE) eventType = FileCreated;
+            else if (event->mask & IN_DELETE) eventType = FileRemoved;
+            else if (event->mask & IN_MODIFY) eventType = FileModified;
+            else if (event->mask & IN_ATTRIB) eventType = FileModified;


I think we should add a new type FileAttribChanged instead of implicitly merging them.

sbusch42 · 2018-12-03T19:16:08Z

source/cppfs/source/posix/LocalFileWatcher.cpp

+    uint32_t flags = 0;
+    if (events & FileCreated)  flags |= IN_CREATE;
+    if (events & FileRemoved)  flags |= IN_DELETE;
+    if (events & FileModified) flags |= (IN_MODIFY | IN_ATTRIB);


I think we should add a new type FileAttribChanged instead of implicitly merging them.

source/examples/cppfs-watch/main.cpp

scheibel · 2018-12-09T22:02:51Z

source/cppfs/include/cppfs/null/NullFileWatcher.h

+*    This class is used for file systems that do not support
+*    file system watchers, such as remote file systems.
+*/
+class CPPFS_API NullFileWatcher : public AbstractFileWatcherBackend


I do like the null object pattern.
However, I have the impression that our remaining code base uses nullptr in similiar cases.
Does using this pattern simplify our code base?

I used it in some of my older projects with the private implementation pattern (which we also use here). It makes it simpler because one can just assume that the implementation object is always valid and doesn't have to check it in each function. But you're right, we're checking m_backend in all functions to far, so maybe we should stick to that pattern (use nullptr instead of null-implementation)

scheibel · 2018-12-09T22:04:54Z

source/cppfs/source/AbstractFileWatcherBackend.cpp

+
+AbstractFileWatcherBackend::AbstractFileWatcherBackend(FileWatcher * fileWatcher)
+: m_fileWatcher(fileWatcher)
+{


The documentation states that m_fileWatcher may never be nullptr.
I suggest to add the corresponding assertion here.

source/cppfs/include/cppfs/FileHandle.h

source/cppfs/include/cppfs/fs.h

scheibel · 2018-12-09T22:13:30Z

source/cppfs/include/cppfs/windows/LocalFileWatcher.h

+    *  @brief
+    *    Watcher entry
+    */
+    struct Watcher {


Is it possible to reorder the members?
I suggest from large to small.
For this case, this may result in something like this:

struct Watcher { std::shared_ptr<void> handle; std::shared_ptr<void> platform; FileHandle fileHandle; RecursiveMode recursive; unsigned int events; }

Rationale: hopefully reduce object size. Probably we should measure this properly. Using the attribute packed may be another option.

source/cppfs/source/FileWatcher.cpp

source/cppfs/source/fs.cpp

scheibel · 2018-12-09T22:19:26Z

source/cppfs/source/posix/LocalFileWatcher.cpp

+    uint32_t flags = 0;
+    if (events & FileCreated)  flags |= IN_CREATE;
+    if (events & FileRemoved)  flags |= IN_DELETE;
+    if (events & FileModified) flags |= (IN_MODIFY | IN_ATTRIB);


source/cppfs/source/windows/LocalFileSystem.cpp

source/cppfs/source/windows/LocalFileWatcher.cpp

…ystem, remove configuration option for remote login

…atched

…instead

… flag (off by default)

deleting of directories

sbusch42 added 11 commits November 24, 2018 16:03

Fix comments in FileIterator

9e2a31b

Make filesystem accessible through interfaces (FileHandle, FileIterator)

0b91777

Create structure for file system watchers and first implementation fo…

299d797

…r linux

Remove clone() from file watcher interface

9d08a58

Create file event handlers (analog to FileVisitor)

89e4dd8

Finish file watcher interface

a8b0d3d

Add mode to watch files/directories recursively

ea51205

Clean up file watcher interface

85a6575

Clean up file watcher interface

2f068b1

Create convenience function for creating a watcher on a single file/d…

79b88ec

…irectory

Fix FileHandle::watch() in case of other file systems than the system…

2fad581

… FS (not relevant, yet)

sbusch42 requested a review from scheibel November 25, 2018 12:20

scheibel approved these changes Nov 25, 2018

View reviewed changes

source/cppfs/include/cppfs/cppfs.h Show resolved Hide resolved

source/cppfs/source/posix/LocalFileWatcher.cpp Outdated Show resolved Hide resolved

source/cppfs/source/FileEventHandler.cpp Outdated Show resolved Hide resolved

scheibel reviewed Nov 25, 2018

View reviewed changes

source/cppfs/source/posix/LocalFileWatcher.cpp Outdated Show resolved Hide resolved

sbusch42 added 2 commits November 25, 2018 16:25

Remove bit field logic where not necessary

773cb63

Fix handling empty name field (happens when the watched object itself…

9d2cf4e

… is subject of an event)

robiwano and others added 4 commits November 26, 2018 16:15

Adds windows file watcher, updates watch function prototype

ca00053

Windows add of folder to watch is now thread safe

022be82

Merge pull request #22 from robiwano/watcher

f24bba7

More complete windows implementation

Small style related changes

e363bb0

sbusch42 commented Dec 3, 2018

View reviewed changes

scheibel reviewed Dec 9, 2018

View reviewed changes

sbusch42 added 6 commits January 6, 2019 12:44

cppfs-watch: As file watchers are only available for the local file s…

f8b16fd

…ystem, remove configuration option for remote login

cppfs-watch: use single threading in example

f3c27ff

Clarify in interface and documentation that only directories can be w…

d8cf3ce

…atched

Remove null implementation of FileWatcher, check for backend nullptr …

60c8fb1

…instead

Move LocalFileWatcher to linux directory, as it is specific for Linux

b45312a

Implement new event type FileAttribChanged

b55db4a

sbusch42 added 5 commits January 6, 2019 15:39

cppfs-watch: allow multiple paths to be specified and add --recursive…

4ff0fa4

… flag (off by default)

Fix compilation on Windows

cd2fabd

Windows backend: add handling renaming of files and creating and

dc0ebb5

deleting of directories

Review, clean up and comment Windows file watcher implementation

68c41aa

Adjust contributors list

e2f78b6

sbusch42 changed the title ~~[WIP] File system watcher~~ File system watcher Jan 6, 2019

sbusch42 merged commit a5e7840 into master Jan 6, 2019

sbusch42 deleted the watcher branch January 6, 2019 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File system watcher #20

File system watcher #20

sbusch42 commented Nov 25, 2018 •

edited

Loading

sbusch42 commented Nov 25, 2018

sbusch42 commented Nov 25, 2018

sbusch42 left a comment

sbusch42 Dec 3, 2018

robiwano Dec 14, 2018

sbusch42 Dec 3, 2018

sbusch42 Dec 3, 2018

sbusch42 Dec 3, 2018

robiwano Dec 14, 2018

sbusch42 Dec 3, 2018

scheibel Dec 9, 2018

scheibel Dec 9, 2018

sbusch42 Dec 9, 2018

scheibel Dec 9, 2018

scheibel Dec 9, 2018

scheibel Dec 9, 2018

scheibel Dec 9, 2018

File system watcher #20

File system watcher #20

Conversation

sbusch42 commented Nov 25, 2018 • edited Loading

sbusch42 commented Nov 25, 2018

sbusch42 commented Nov 25, 2018

sbusch42 left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbusch42 commented Nov 25, 2018 •

edited

Loading