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

[Fix] Bug 34: Monitor files at Windows root directory #154

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

TinaMor
Copy link
Contributor

@TinaMor TinaMor commented Aug 14, 2023

PR Description

This PR allows us to monitor files in the root directory e.g. C:\

Solution

For root directory, do not prepend, otherwise, prepend "\?" to the directory path. We validate the config file to check that when the root directory is provided, includeSubdirectories must be false. Otherwise, the application terminates with an error.

image

TL;DR

By default, the name is limited to MAX_PATH characters. To extend this limit to 32,767 wide characters, we prepend "\?" to the path in LogFileMonitor.cpp. Prepending the string "\?" does not allow access to the root directory. Maximum Path Length Limitation
In editions of Windows before Windows 10 version 1607, the maximum length for a path is MAX_PATH, which is defined as 260 characters. In later versions of Windows, changing a registry key or using the Group Policy tool is required to remove the limit. See Maximum Path Length Limitation for full details.
There is no limit on the depth of a file or directory. With the standard API you can create as many folders as you want until the actual length of the expanded path exceeds 32,767 characters. The math on that is 2¹⁵ minus 1. The limitation of 32,767 doesn’t come from NTFS. It comes from the Win32 API.

References

  1. FindFirstFileW function (fileapi.h)
  2. Naming Files, Paths, and Namespaces
  3. Maximum Path Length Limitation

@TinaMor TinaMor linked an issue Aug 14, 2023 that may be closed by this pull request
@TinaMor TinaMor force-pushed the user/chmurimi/monitor-file-at-c-34 branch 7 times, most recently from 1b93082 to f9d4a5e Compare August 17, 2023 17:36
Copy link
Contributor

@iankingori iankingori left a comment

Choose a reason for hiding this comment

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

LGTM, let’s fix the linting issues and we should be good to go

@TinaMor TinaMor force-pushed the user/chmurimi/monitor-file-at-c-34 branch 2 times, most recently from 28bd738 to eafc665 Compare August 18, 2023 08:56
@bobsira
Copy link
Contributor

bobsira commented Aug 21, 2023

The looks good to me. I've noticed a few issues while try to test your code.

  1. If I begin monitoring the C: without having a log file and create the logfile later on I keep getting LogFilesChangeHandler: Timer event even after the log file is created and I'm pushing data to that log file.
  2. If I begin monitoring the C: with a log file already there I still see the LogFilesChangeHandler: Timer event

Is this the expected behavior and I'm not sure if it is related to this PR as well?

logfilemon

@TinaMor
Copy link
Contributor Author

TinaMor commented Aug 22, 2023

@bobsira I accidentally uncommeted out code

The looks good to me. I've noticed a few issues while try to test your code.

  1. If I begin monitoring the C: without having a log file and create the logfile later on I keep getting LogFilesChangeHandler: Timer event even after the log file is created and I'm pushing data to that log file.
  2. If I begin monitoring the C: with a log file already there I still see the LogFilesChangeHandler: Timer event

Is this the expected behavior and I'm not sure if it is related to this PR as well?

logfilemon

@TinaMor TinaMor force-pushed the user/chmurimi/monitor-file-at-c-34 branch from eafc665 to a0baf65 Compare August 22, 2023 10:43
@TinaMor TinaMor force-pushed the user/chmurimi/monitor-file-at-c-34 branch from a0baf65 to d7be080 Compare August 22, 2023 11:28
Copy link
Contributor

@bobsira bobsira left a comment

Choose a reason for hiding this comment

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

LGTM!

@bobsira bobsira self-requested a review August 22, 2023 11:41
Copy link
Contributor

@CharityKathure CharityKathure left a comment

Choose a reason for hiding this comment

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

LGTM

@bobsira bobsira merged commit 33b1a27 into main Aug 22, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't monitor a file at C:
4 participants