Conversation
WalkthroughAdds .NET Standard 2.0 support to the NLog.Targets.AtomicFile package by introducing a Windows-specific low-level file stream creation method via P/Invoke, updating the project file to target netstandard2.0, and implementing conditional compilation logic to route file operations appropriately across NETSTANDARD, NETFRAMEWORK, and platform variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj`:
- Around line 37-39: The MSBuild property conditions for EnableTrimAnalyzer,
IsTrimmable, and IsAotCompatible incorrectly include netstandard2.0; update the
Condition expressions on the EnableTrimAnalyzer, IsTrimmable, and
IsAotCompatible property elements so they apply only to net8.0 (or net8.0+),
e.g. remove "OR '$(TargetFramework)' == 'netstandard2.0'" or change the
condition to target net8.0+ (so these properties are not set for
netstandard2.0).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
f475f53 to
8ae7ef4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 121-129: The FileSystemAclExtensions.CreateDirectory call is
malformed and the temp FileStream is opened before ensuring the directory
exists; fix by moving the directory-creation call before the NETSTANDARD2_0
FileStream creation and close the CreateDirectory call properly, then call
FileSystemAclExtensions.Create to open the temp file. Specifically, in
AtomicFileTarget.cs ensure FileSystemAclExtensions.CreateDirectory(path,
fileSecurity) (or the correct overload) is invoked and completed before
executing the NETSTANDARD2_0 block that does using FileStream _ = new(...); keep
the fileSecurity = new FileSecurity() assignment under NETSTANDARD2_0 but only
after directory creation, and replace the stray "return
FileSystemAclExtensions.Create(" line with a proper call to
FileSystemAclExtensions.Create(path, fileSecurity) to obtain the file stream.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
2 times, most recently
from
ddf1c99 to
9e45853
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj (2)
4-4:netstandard2.0target addition is good; consider removing stalenet45blocks.Line 4 correctly adds
netstandard2.0for this PR objective. Sincenet45is not inTargetFrameworks, thenet45-conditioned groups (Lines 48-51 and Lines 75-78) are now dead config and can be removed for clarity.♻️ Optional cleanup diff
- <PropertyGroup Condition=" '$(TargetFramework)' == 'net45' "> - <Title>NLog.Targets.AtomicFile for .NET Framework 4.5</Title> - <DisableImplicitFrameworkReferences>true</DisableImplicitFrameworkReferences> - </PropertyGroup> @@ - <ItemGroup Condition=" '$(TargetFramework)' == 'net45' "> - <Reference Include="System" /> - <Reference Include="System.Core" /> - </ItemGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj` at line 4, The project file still contains conditional groups that target net45 even though net45 was removed from the TargetFrameworks; locate the PropertyGroup/ItemGroup elements conditioned on net45 (e.g., any Group with Condition="'$(TargetFramework)' == 'net45'" or similar) and delete those stale net45-specific blocks so the csproj no longer contains dead configuration tied to net45 while keeping the added netstandard2.0 entry intact.
98-100: Verify dependency version baseline alignment with your maintenance policy.The pinned versions—
System.IO.FileSystem.AccessControl5.0.0 andMono.Posix.NETStandard1.0.0—are the current stable versions and are compatible withnetstandard2.0. No known security advisories affect either version.However, both packages are from significantly older release timelines:
System.IO.FileSystem.AccessControl5.0.0 (Nov 2020) is tied to the .NET 5 era, which reached end-of-life in May 2022. Note that its APIs are Windows-only and will raisePlatformNotSupportedExceptionon non-Windows platforms.Mono.Posix.NETStandard1.0.0 (Mar 2018) has a newer prerelease available (5.20.1-preview from June 2020).If your policy requires dependencies to be from actively supported frameworks or more recent releases, consider evaluating upgrades or verifying that these older versions remain acceptable for your maintenance baseline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj` around lines 98 - 100, The csproj pins older packages (PackageReference System.IO.FileSystem.AccessControl 5.0.0 and Mono.Posix.NETStandard 1.0.0) that may be outside your maintenance baseline; update or justify them by either bumping to a currently supported stable release (or the latest compatible prerelease for Mono.Posix if your policy allows), or explicitly document and gate usage where needed (e.g., ensure System.IO.FileSystem.AccessControl is only used on Windows or wrap with TargetFramework/conditional references); locate and update the PackageReference entries in the project file (the ItemGroup conditioned on '$(TargetFramework)' == 'netstandard2.0') and run cross-platform tests to confirm no PlatformNotSupportedException and that the newer package versions remain compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj`:
- Line 4: The project file still contains conditional groups that target net45
even though net45 was removed from the TargetFrameworks; locate the
PropertyGroup/ItemGroup elements conditioned on net45 (e.g., any Group with
Condition="'$(TargetFramework)' == 'net45'" or similar) and delete those stale
net45-specific blocks so the csproj no longer contains dead configuration tied
to net45 while keeping the added netstandard2.0 entry intact.
- Around line 98-100: The csproj pins older packages (PackageReference
System.IO.FileSystem.AccessControl 5.0.0 and Mono.Posix.NETStandard 1.0.0) that
may be outside your maintenance baseline; update or justify them by either
bumping to a currently supported stable release (or the latest compatible
prerelease for Mono.Posix if your policy allows), or explicitly document and
gate usage where needed (e.g., ensure System.IO.FileSystem.AccessControl is only
used on Windows or wrap with TargetFramework/conditional references); locate and
update the PackageReference entries in the project file (the ItemGroup
conditioned on '$(TargetFramework)' == 'netstandard2.0') and run cross-platform
tests to confirm no PlatformNotSupportedException and that the newer package
versions remain compatible.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NLog.Targets.AtomicFile/AtomicFileTarget.cs
|
Please help me out with the incomplete tasks. What is a stacked PR and which docstrings should be added/modified? Are there Unit tests that can run for a specific target? Point me at the right direction. |
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
9e45853 to
844348a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 133-137: The unconditional call to
fileInfo.SetAccessControl(fileSecurity) overwrites existing files' DACLs; change
the logic in AtomicFileTarget.cs so SetAccessControl(fileSecurity) is only
applied when the file was just created (i.e., after fileInfo.Create() succeeded)
and not for opened existing files—use the result of the create operation or
check file existence (fileInfo.Exists or a createdNew flag) to conditionally
call SetAccessControl, leaving existing files' ACLs intact; ensure you still set
access control for newly created files but remove/restrict the unconditional
call around the fileInfo.SetAccessControl invocation.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
844348a to
fade5ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 121-139: Remove the TOCTOU check that sets resetAccessControl
based on fileInfo.Exists before creating the file; instead open the file first
(fileInfo.Create(...) -> fileStream) and then decide whether to call
fileInfo.SetAccessControl(fileSecurity) by detecting if this Create actually
created the file (e.g., inspect the opened handle via fileStream.SafeFileHandle
and use GetFileInformationByHandle/GetFileInformationByHandleEx or refresh
fileInfo and compare CreationTimeUtc within a narrow time window) so the
decision is made from the opened file state rather than a prior Exists() check;
keep references to fileSecurity, fileStream, resetAccessControl, fileInfo.Create
and fileInfo.SetAccessControl when implementing the change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
2 times, most recently
from
3c31e37 to
09a73a5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (2)
135-136: RedundantSetAccessControlcall.The
fileSecurityis already passed tofileInfo.Create()at line 127, which applies the security descriptor during file creation. The subsequentSetAccessControlcall applies the sameFileSecurityobject again, which is redundant for newly created files.♻️ Consider removing the redundant call
fileStream = fileInfo.Create( FileMode.CreateNew, systemRights, fileShare, bufferSize: 1, FileOptions.None, fileSecurity: fileSecurity); - - // inherit permissions from parent directory - fileInfo.SetAccessControl(fileSecurity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 135 - 136, Remove the redundant FileInfo.SetAccessControl call: when creating the file with fileInfo.Create(...) passing fileSecurity (in AtomicFileTarget.cs) the security descriptor is already applied, so delete the subsequent fileInfo.SetAccessControl(fileSecurity) invocation to avoid applying the same FileSecurity twice; keep the Create(...) usage that supplies fileSecurity and ensure no other code relies on the extra SetAccessControl call.
138-147: Consider narrowing the exception catch to handle only "file exists" scenario.The
catch (IOException)block is intended to handle the case where the file already exists (sinceCreateNewthrowsIOExceptionwhen the file exists). However,IOExceptionis broad and can also be thrown for other reasons (disk full, network errors, permission issues, etc.). In those cases, retrying withFileMode.Appendmay not be the correct behavior and could mask the real error.Consider catching only the specific error code for "file already exists" or checking the
HResult:♻️ Proposed refinement
- catch (IOException) + catch (IOException ex) when (ex.HResult == unchecked((int)0x80070050)) // ERROR_FILE_EXISTS { fileStream = fileInfo.Create(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 138 - 147, The catch block currently swallows all IOExceptions when calling fileInfo.Create (after trying CreateNew) and falls back to FileMode.Append; change it to catch(IOException ex) and only handle the "file already exists" case—otherwise rethrow the exception. Implement a narrow check (e.g., test ex.HResult or convert to a Win32Exception and compare NativeErrorCode to ERROR_ALREADY_EXISTS/ERROR_FILE_EXISTS) and only call fileInfo.Create(..., FileMode.Append, ...) when that check passes; for other IO errors simply rethrow the exception to avoid masking disk/permission/network failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 135-136: Remove the redundant FileInfo.SetAccessControl call: when
creating the file with fileInfo.Create(...) passing fileSecurity (in
AtomicFileTarget.cs) the security descriptor is already applied, so delete the
subsequent fileInfo.SetAccessControl(fileSecurity) invocation to avoid applying
the same FileSecurity twice; keep the Create(...) usage that supplies
fileSecurity and ensure no other code relies on the extra SetAccessControl call.
- Around line 138-147: The catch block currently swallows all IOExceptions when
calling fileInfo.Create (after trying CreateNew) and falls back to
FileMode.Append; change it to catch(IOException ex) and only handle the "file
already exists" case—otherwise rethrow the exception. Implement a narrow check
(e.g., test ex.HResult or convert to a Win32Exception and compare
NativeErrorCode to ERROR_ALREADY_EXISTS/ERROR_FILE_EXISTS) and only call
fileInfo.Create(..., FileMode.Append, ...) when that check passes; for other IO
errors simply rethrow the exception to avoid masking disk/permission/network
failures.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
09a73a5 to
e685e35
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
e685e35 to
6044b39
Compare
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
7a488a7 to
0a915a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cs (1)
45-45: Prefer internal visibility for this helper enum.Line 45 exposes
Win32FileAttributespublicly, but it appears to be internal interop plumbing for this assembly.🧩 Proposed visibility change
- public enum Win32FileAttributes : int + internal enum Win32FileAttributes : int🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cs` at line 45, The enum Win32FileAttributes is declared public but is internal interop plumbing; change its visibility from public to internal by updating the declaration of the Win32FileAttributes enum so it is internal instead of public (ensure any internal callers in the assembly continue to compile and remove/avoid exposing this type from the public API). Use the Win32FileAttributes identifier to locate and update the enum declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cs`:
- Line 42: The XML doc comment in Win32FileAttributes (file
Win32FileAttributes.cs) references an obsolete MSDN URL; update that href to the
current Microsoft Learn CreateFile documentation (e.g., the CreateFile function
page on learn.microsoft.com) so the summary link points to the modern Microsoft
Learn CreateFile reference instead of the old msdn.microsoft.com address.
---
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cs`:
- Line 45: The enum Win32FileAttributes is declared public but is internal
interop plumbing; change its visibility from public to internal by updating the
declaration of the Win32FileAttributes enum so it is internal instead of public
(ensure any internal callers in the assembly continue to compile and
remove/avoid exposing this type from the public API). Use the
Win32FileAttributes identifier to locate and update the enum declaration.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cssrc/NLog.Targets.AtomicFile/Internal/Win32FileNativeMethods.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
0a915a2 to
8fccbdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
136-136: Consider using theownsHandleparameter to simplify cleanup.The
FileStreamconstructor overloadFileStream(SafeFileHandle, FileAccess, int)defaults to owning the handle. SinceFileStreamwill dispose the handle on success, the catch block is only needed for failures betweenCreateFileandFileStreamconstruction—which is correct. However, making ownership explicit improves clarity.Optional: Make handle ownership explicit
- return new FileStream(handle, FileAccess.Write, bufferSize: 1); + return new FileStream(handle, FileAccess.Write, bufferSize: 1, isAsync: false);Or use the constructor that explicitly shows ownership is transferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` at line 136, The FileStream construction in AtomicFileTarget (the return new FileStream(handle, FileAccess.Write, bufferSize: 1) line) should explicitly transfer ownership of the SafeFileHandle; change to an overload that makes ownership explicit so disposal semantics are clear — e.g. either call the IntPtr overload with ownsHandle: true via new FileStream(handle.DangerousGetHandle(), FileAccess.Write, ownsHandle: true) or, if your target framework provides a SafeFileHandle overload that accepts an ownership flag, use that overload to pass ownership to FileStream instead of relying on the default. Ensure you still only close the SafeFileHandle on error paths when FileStream construction fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 119-143: The Windows NETSTANDARD2_0 branch that calls
Win32FileNativeMethods.CreateFile (in AtomicFileTarget.cs, the code that returns
new FileStream(handle...)) needs to handle missing parent directories when
CreateDirs is true: after CreateFile returns an invalid handle, check
Marshal.GetLastWin32Error() for ERROR_PATH_NOT_FOUND (or PathNotFound constant)
and if CreateDirs is true, call Directory.CreateDirectory on
Path.GetDirectoryName(filePath) and retry the CreateFile call once before
throwing; ensure any intermediate handle is closed in the catch/finally as
currently done and preserve existing behavior for other errors.
---
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Line 136: The FileStream construction in AtomicFileTarget (the return new
FileStream(handle, FileAccess.Write, bufferSize: 1) line) should explicitly
transfer ownership of the SafeFileHandle; change to an overload that makes
ownership explicit so disposal semantics are clear — e.g. either call the IntPtr
overload with ownsHandle: true via new FileStream(handle.DangerousGetHandle(),
FileAccess.Write, ownsHandle: true) or, if your target framework provides a
SafeFileHandle overload that accepts an ownership flag, use that overload to
pass ownership to FileStream instead of relying on the default. Ensure you still
only close the SafeFileHandle on error paths when FileStream construction fails.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/Internal/Win32FileAttributes.cssrc/NLog.Targets.AtomicFile/Internal/Win32FileNativeMethods.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
| #if NETSTANDARD2_0 | ||
| Microsoft.Win32.SafeHandles.SafeFileHandle? handle = null; | ||
| try | ||
| { | ||
| handle = Win32FileNativeMethods.CreateFile( | ||
| filePath, | ||
| Win32FileNativeMethods.FileAccess.AppendData | Win32FileNativeMethods.FileAccess.Synchronize, | ||
| (int) fileShare, | ||
| IntPtr.Zero, | ||
| Win32FileNativeMethods.CreationDisposition.OpenAlways, | ||
| (int) Win32FileAttributes.Normal | (int) Win32FileNativeMethods.Win32SecurityFlags.SecuritySqosPresent | (int) Win32FileNativeMethods.Win32SecurityFlags.SecurityAnonymous, | ||
| IntPtr.Zero); | ||
|
|
||
| if (handle.IsInvalid) { | ||
| System.Runtime.InteropServices.Marshal.ThrowExceptionForHR(System.Runtime.InteropServices.Marshal.GetHRForLastWin32Error()); | ||
| } | ||
|
|
||
| return new FileStream(handle, FileAccess.Write, bufferSize: 1); | ||
| } | ||
| catch | ||
| { | ||
| if ((handle != null) && (!handle.IsClosed)) | ||
| handle.Close(); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Missing directory creation handling in NETSTANDARD2_0 path.
The CreateFile P/Invoke will fail if the parent directory doesn't exist. Unlike the Unix path (lines 167-171) which handles CreateDirs, this Windows path has no directory creation logic.
When CreateDirs is true and the directory doesn't exist, CreateFile returns INVALID_HANDLE_VALUE with ERROR_PATH_NOT_FOUND. The current code will throw, but the retry loop (lines 64-86) only catches IOException and doesn't create directories.
Suggested fix
`#if` NETSTANDARD2_0
+ if (CreateDirs)
+ {
+ var dirName = Path.GetDirectoryName(filePath);
+ if (!string.IsNullOrEmpty(dirName) && !Directory.Exists(dirName))
+ Directory.CreateDirectory(dirName);
+ }
+
Microsoft.Win32.SafeHandles.SafeFileHandle? handle = null;
try
{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 119 - 143, The
Windows NETSTANDARD2_0 branch that calls Win32FileNativeMethods.CreateFile (in
AtomicFileTarget.cs, the code that returns new FileStream(handle...)) needs to
handle missing parent directories when CreateDirs is true: after CreateFile
returns an invalid handle, check Marshal.GetLastWin32Error() for
ERROR_PATH_NOT_FOUND (or PathNotFound constant) and if CreateDirs is true, call
Directory.CreateDirectory on Path.GetDirectoryName(filePath) and retry the
CreateFile call once before throwing; ensure any intermediate handle is closed
in the catch/finally as currently done and preserve existing behavior for other
errors.
There was a problem hiding this comment.
The behavior has not changed with the new code. The Windows code path always threw DirectoryNotFoundException in which case there is no retry.
The new code uses System.Runtime.InteropServices.Marshal.ThrowExceptionForHR(System.Runtime.InteropServices.Marshal.GetHRForLastWin32Error()); which will convert ERROR_PATH_NOT_FOUND into DirectoryNotFoundException.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Marshal.ThrowExceptionForHR ERROR_PATH_NOT_FOUND DirectoryNotFoundException C#
💡 Result:
Marshal.ThrowExceptionForHR(...) isn’t the cause—it’s just converting an HRESULT returned by a Windows/COM call into a .NET exception. ERROR_PATH_NOT_FOUND / 0x80070003 means Windows couldn’t find the directory in the path you passed (“The system cannot find the path specified”), which commonly surfaces as DirectoryNotFoundException. [1][2][3]
What to check / fix in C#
-
Log the HRESULT and the exact path being used
try { /* call that triggers the HR */ } catch (Exception ex) { Console.WriteLine(ex); // if you have the hr: // Console.WriteLine(new System.ComponentModel.Win32Exception(hr).Message); }
-
Make sure the path is absolute and normalized
var full = Path.GetFullPath(path); // resolves relative paths against current working dir
-
Verify every parent directory exists (not just the last segment)
if (!Directory.Exists(full)) throw new DirectoryNotFoundException(full);
-
Create the directory before writing/copying into it
Directory.CreateDirectory(Path.GetDirectoryName(full)!);
-
Common gotchas
- Relative paths behave differently under IIS/Windows Services/tests (different working directory).
- UNC/mapped drives: services often can’t see mapped drives; use
\\server\share\.... - Permissions: you might be able to read some parts but not enumerate/create in others.
- Paths that appear to exist can still fail if a parent folder was deleted/renamed between enumeration and use (race condition), which is why robust code re-checks/handles exceptions. [3]
If you paste the line of code that calls ThrowExceptionForHR (or the API you P/Invoke/COM-call) and the path value at runtime, I can point to the specific failure point.
Sources: [1] [2] [3]
@michailf You're absolutely correct! ERROR_PATH_NOT_FOUND (0x80070003) does map to DirectoryNotFoundException via ThrowExceptionForHR, and the retry loop immediately re-throws DirectoryNotFoundException at line 69-70, so there's no retry for missing directories. The NETSTANDARD2_0 code maintains the same behavior as the existing NETFRAMEWORK Windows path.
However, I notice an inconsistency: the Unix code path (lines 167-171) does handle CreateDirs:
if (fd == -1 && Mono.Unix.Native.Stdlib.GetLastError() == Mono.Unix.Native.Errno.ENOENT && CreateDirs)
{
var dirName = Path.GetDirectoryName(filePath);
if (dirName != null && !Directory.Exists(dirName))
Directory.CreateDirectory(dirName);
fd = Mono.Unix.Native.Syscall.open(filePath, openFlags, permissions);
}So on Unix with CreateDirs=true, missing directories are automatically created, but on Windows they're not. Is this platform difference intentional, or should Windows also respect the CreateDirs property?
There was a problem hiding this comment.
@snakefoot What do you want to do about this? Seems like a bug but not related to this PR.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think the logic you have implemented with ThrowExceptionForHR looks good. It is mostly because I know very little about Linux-API and Mono, that CreateUnixStream has the special logic for handling directory creation.
The CodeRabbit is not an authority, but more an assistant that makes observations. Some valid, some not.
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
8fccbdb to
274e066
Compare
✅ Actions performedReviews resumed. |
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
274e066 to
a529132
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
148-148: Consider documenting the SQOS flags rationale.The
SecuritySqosPresent | SecurityAnonymousflags are a security best practice to prevent impersonation attacks. While primarily relevant for named pipes, including them for regular files is harmless and defensive. A brief comment explaining this choice would help future maintainers understand the intent.📝 Optional documentation
(int) fileShare, IntPtr.Zero, NativeMethods.CreationDisposition.OpenAlways, + // SQOS flags prevent security impersonation attacks (defensive for regular files) (int) NativeMethods.Win32FileAttributes.Normal | (int) NativeMethods.Win32SecurityFlags.SecuritySqosPresent | (int) NativeMethods.Win32SecurityFlags.SecurityAnonymous, IntPtr.Zero);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` at line 148, Add a short inline comment near the flags combination in AtomicFileTarget (where NativeMethods.Win32FileAttributes.Normal is OR'd with NativeMethods.Win32SecurityFlags.SecuritySqosPresent and SecurityAnonymous) explaining that SecuritySqosPresent | SecurityAnonymous are included defensively to prevent impersonation/SQOS-related attacks (originating from named-pipe guidance) and are harmless for regular files; reference the specific flags by name (SecuritySqosPresent, SecurityAnonymous) so future maintainers understand the security rationale.src/NLog.Targets.AtomicFile/Internal/Win32FileNativeMethods.cs (1)
163-165: Redundant#if !NET35directive.This file is only compiled when
NETSTANDARDis defined (line 34), andNET35is never defined alongsideNETSTANDARD. TheDefaultDllImportSearchPathsattribute will always be included, making this conditional unnecessary.🧹 Optional cleanup
[DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)] -#if !NET35 [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] -#endif public static extern Microsoft.Win32.SafeHandles.SafeFileHandle CreateFile(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/Internal/Win32FileNativeMethods.cs` around lines 163 - 165, Remove the redundant preprocessor conditional around the DefaultDllImportSearchPaths attribute: delete the surrounding "#if !NET35" and corresponding "#endif" so the [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] attribute remains unconditionally applied to the P/Invoke declarations (e.g., the attribute above the Win32FileNativeMethods methods).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Line 148: Add a short inline comment near the flags combination in
AtomicFileTarget (where NativeMethods.Win32FileAttributes.Normal is OR'd with
NativeMethods.Win32SecurityFlags.SecuritySqosPresent and SecurityAnonymous)
explaining that SecuritySqosPresent | SecurityAnonymous are included defensively
to prevent impersonation/SQOS-related attacks (originating from named-pipe
guidance) and are harmless for regular files; reference the specific flags by
name (SecuritySqosPresent, SecurityAnonymous) so future maintainers understand
the security rationale.
In `@src/NLog.Targets.AtomicFile/Internal/Win32FileNativeMethods.cs`:
- Around line 163-165: Remove the redundant preprocessor conditional around the
DefaultDllImportSearchPaths attribute: delete the surrounding "#if !NET35" and
corresponding "#endif" so the
[DefaultDllImportSearchPaths(DllImportSearchPath.System32)] attribute remains
unconditionally applied to the P/Invoke declarations (e.g., the attribute above
the Win32FileNativeMethods methods).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/Internal/Win32FileNativeMethods.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csproj
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
a529132 to
d53a323
Compare
| int dwShareMode, | ||
| IntPtr lpSecurityAttributes, | ||
| CreationDisposition dwCreationDisposition, | ||
| int dwFlagsAndAttributes, |
There was a problem hiding this comment.
Can int dwFlagsAndAttributes be changed to Win32FileAttributes dwFlagsAndAttributes ?
There was a problem hiding this comment.
Not sure, I combine Win32FileAttributes with Win32SecurityFlags
There was a problem hiding this comment.
I can keep it as uint or can combine into a single enum Win32FlagsAndAttributes
There was a problem hiding this comment.
Then keep it as uint. I like to keep the name Win32FileAttributes, since others are using the same name.
michailf
force-pushed
the
netstandard2.0_for_AtomicFile
branch
from
d53a323 to
98c07e1
Compare
|
Thank you for making all the small adjustments. Think the code looks good. Guess final work will happen in #6100 ? |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cs (1)
143-157: InconsistentFileModecomparison: bitwise vs equality.
FileModeis not a[Flags]enum, so the bitwise check on line 144 is semantically incorrect:if ((fileMode & FileMode.Append) != 0) // line 144 - bitwise (incorrect pattern) ... if (fileMode == FileMode.Append) // line 154 - equality (correct pattern)Currently this works because the method is only called with
FileMode.Append, but(FileMode.Create & FileMode.Append) != 0would also be true (2 & 6 = 2), which could cause subtle bugs if this method is reused.Suggested fix for consistency
- if ((fileMode & FileMode.Append) != 0) + if (fileMode == FileMode.Append) { dwDesiredAccess |= NativeMethods.Win32FileAccess.FILE_APPEND_DATA; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 143 - 157, The code in AtomicFileTarget.cs uses a bitwise check against the non‑[Flags] enum FileMode (the condition "if ((fileMode & FileMode.Append) != 0)"), which is incorrect and inconsistent with the later equality check; change the bitwise test to an equality test (if (fileMode == FileMode.Append)) so both checks use proper FileMode equality when determining if Append is requested (update the branch that sets dwDesiredAccess to use fileMode == FileMode.Append and leave the subsequent normalization that maps FileMode.Append to FileMode.OpenOrCreate intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 143-157: The code in AtomicFileTarget.cs uses a bitwise check
against the non‑[Flags] enum FileMode (the condition "if ((fileMode &
FileMode.Append) != 0)"), which is incorrect and inconsistent with the later
equality check; change the bitwise test to an equality test (if (fileMode ==
FileMode.Append)) so both checks use proper FileMode equality when determining
if Append is requested (update the branch that sets dwDesiredAccess to use
fileMode == FileMode.Append and leave the subsequent normalization that maps
FileMode.Append to FileMode.OpenOrCreate intact).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/Internal/NativeMethods.cs
|
|
Superseeded by #6100 |
3 participants
My attempt to add .NET Standard 2.0 support.
Should resolve #6098
Unit tests #6100