Port System.Diagnostics.EventLog to .NET Core #24344
Conversation
|
@Anipik, It will cover your contributions to all .NET Foundation-managed open source projects. |
|
Note for code reviewers: there are no tests for this yet. This gets it in the build. So we do not want to do any refactoring at this stage. |
| # Visual Studio 15 | ||
| VisualStudioVersion = 15.0.26919.1 | ||
| MinimumVisualStudioVersion = 10.0.40219.1 | ||
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "System.EventLog", "..\src\System.EventLog.csproj", "{432779B9-3CBD-4871-A7DC-D8A192319DBD}" |
There was a problem hiding this comment.
you can add the ref project also
| using System.Runtime.InteropServices; | ||
| using System.Text; |
There was a problem hiding this comment.
What I suggest doing with this file is: leave it unchanged. Make a new file next to it named Interop.FormatMessage_SafeLibraryHandle.cs. Into that put what you have here, ie., the FormatMessage that takes SafeLibraryHandle.
This will mean you can revert all the changes to the other C# projects that add SafeLibraryHandle and FreeLibrary.
There was a problem hiding this comment.
There is more refactoring needed (copy the helper GetMessage code into there from EventLog code, and eliminate GetMessage(IntPtr moduleHandle changing its only existing caller to use the SafeLibraryHandle one) but that can happen later. My suggestion above will make it simpler for now.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build"> | ||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Debug|AnyCPU'" /> |
There was a problem hiding this comment.
Replace these two lines to match the Configurations.props, ie.,
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netfx-Debug|AnyCPU'" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netfx-Release|AnyCPU'" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netstandard-Debug|AnyCPU'" />
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netstandard-Release|AnyCPU'" />There was a problem hiding this comment.
@Anipik a tip, you can use the thumbs up (click the smiley button) to acknowledge feedback without sending a mail each time to everyone subscribed to the issue or repo :)
| @@ -0,0 +1,13 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build"> | |||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> | |||
There was a problem hiding this comment.
add this
<PropertyGroup>
<IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netfx'">true</IsPartialFacadeAssembly>
</PropertyGroup>
| <data name="TooManyReplacementStrings" xml:space="preserve"> | ||
| <value>The maximum allowed number of replacement strings is 255.</value> | ||
| </data> | ||
| <data name="Win2000Required" xml:space="preserve"> |
There was a problem hiding this comment.
Remove Win2000Required and WinNTRequired strings. Remove the CheckEnvironment and CheckNtenvironment functions where they were used. Remove all calls to those functions.
Basically this was about checking whether it was running on Windows 95/98 or Windows 2000 etc. We are years past caring about that. We only run on Windows 7+. We don't check for that.
| <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <PackageConfigurations> | ||
| netcoreapp; |
There was a problem hiding this comment.
Please change netcoreapp to netcoreapp-Windows_NT because this code is all Windows specific.
Unix will get the netstandard one.
| <PropertyGroup> | ||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Debug|AnyCPU'" /> |
There was a problem hiding this comment.
Please replace these two lines with 6 lines for the src/Configurations.props, ie., one for netcoreapp-Windows_NT-Debug|AnyCPU, etc.
| public const int BACKWARDS_READ = 0x8; | ||
| public const int ERROR_EVENTLOG_FILE_CHANGED = 1503; | ||
| public const int ERROR_FILE_NOT_FOUND = 2; | ||
|
|
| { | ||
| internal SafeEventLogReadHandle() : base(true) { } | ||
|
|
||
| #pragma warning disable BCL0015�// Disable Pinvoke analyzer errors. |
There was a problem hiding this comment.
Instead of BCL0015, please create a baseline file src\System.EventLog\src\PinvokeAnalyzerExceptionList.analyzerdata.netcoreapp
and in it put these one per line like
advapi32.dll!OpenEventLog
advapi32.dll!CloseEventLog
Here's an example:
https://github.com/dotnet/corefx/blob/master/src/System.Console/src/PinvokeAnalyzerExceptionList.analyzerdata.netcoreapp
I know I said I didn't care much about it, but today I'm dealing with the fallout of missing one of these in a different context, so I would like to be more rigorous about using baseline files instead (those are harder to overlook)
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
replaced using regex in all files
| private static extern bool CloseEventLog(IntPtr hEventLog); | ||
| #pragma warning restore BCL0015 | ||
|
|
||
| override protected bool ReleaseHandle() |
There was a problem hiding this comment.
nit, should be protected override not override protected. see
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md bullet 5
There was a problem hiding this comment.
search elsewhere for other cases of override protected please
| return newException; | ||
| } | ||
|
|
||
| internal static int CurrentEnvironment |
There was a problem hiding this comment.
As mentioned above please eliminate this whole property and fix its callers as if it was always returning W2kEnvironment (this is the newest for them)
| internal static Win32Exception CreateSafeWin32Exception(int error) | ||
| { | ||
| Win32Exception newException = null; | ||
| SecurityPermission securityPermission = new SecurityPermission(PermissionState.Unrestricted); |
There was a problem hiding this comment.
remove securitypermission entirely. the try/finally below will be removable then also
| internal static class SharedUtils | ||
| { | ||
|
|
||
| internal const int UnknownEnvironment = 0; |
There was a problem hiding this comment.
Remove all 5 of these ints because you're getting rid of the checks and just assuming W2kEnvironment.
| { | ||
| internal static class SharedUtils | ||
| { | ||
|
|
There was a problem hiding this comment.
nit, extra line. You can use regexes to find this kind of thing if you like
| private static bool SafeWaitForMutexOnce(Mutex mutexIn, ref Mutex mutexOut) | ||
| { | ||
| bool ret; | ||
| RuntimeHelpers.PrepareConstrainedRegions(); |
There was a problem hiding this comment.
Remove all use of RuntimeHelpers.PrepareConstrainedRegions(); and Thread.Begin/EndCriticalRegion();
| string dllDir = ""; | ||
| RegistryKey baseKey = null; | ||
| RegistryKey complusReg = null; | ||
| RegistryPermission registryPermission = new RegistryPermission(PermissionState.Unrestricted); |
There was a problem hiding this comment.
remove RegistryPermission entirely from the code please, and all calls on it.
| public static extern bool ReportEvent(SafeHandle hEventLog, short type, ushort category, | ||
| uint eventID, byte[] userSID, short numStrings, int dataLen, HandleRef strings, | ||
| byte[] rawData); | ||
| #pragma warning disable BCL0015 // Disable Pinvoke analyzer errors. |
There was a problem hiding this comment.
As above, please remove BCL0015 and use a baseline file instead.
| #pragma warning disable BCL0015 // Disable Pinvoke analyzer errors. | ||
| [DllImport(Interop.Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)] | ||
| public static extern bool ClearEventLog(SafeHandle hEventLog, HandleRef lpctstrBackupFileName); | ||
| [DllImport(Interop.Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)] |
There was a problem hiding this comment.
Please put a blank line between each pair of lines so it's mor readable
Yes it is possible. I would have to take a look at this specific scenario for uapaot restore. |
|
@dotnet/dnceng can you please look at this one? |
|
@jcagme @sunandabalu Seen again |
|
@dotnet/dnceng here's another .. tests worked fine, but wouldn't upload |
|
@safern I see the issue and opened dotnet/buildtools#1724 for you |
|
@danmosemsft I have made the changes you mentioned. |
|
@ChulHul FYI in relation to the ticket you are opening. This has happened at least 3 times today. |
|
Yay! Up next : finish tests, and packaging. |
|
Was curious if there's still an effort made for porting the |
|
@huangsam also it would be better to open a new issue to ask not post on an old PR. |
|
@danmosemsft @brianrob I created https://github.com/dotnet/corefx/issues/24852 to direct the conversation to the right channels. |
* Adding sources for System.Diagnostics.EventLog * Adding some tests for System.Diagnostics.EventLog
|
This is built for the old Event Log API (ReadEventLog, OpenEventLog etc.). Since .NET core is not intended for Windows XP, this could have been written with the newer API (EvtOpenLog). The new API supports evt and evtx files along with channels. Using this API means newer logs such as Windows Update Client can't be read as it is defined as a channel. Edit: Note that the .NET Framework has a perfectly good implementation using the Vista+ API. |
|
@danmosemsft It just seems like there is a lot of overlap between the EventLog in this port, and the EventLogReader in System.Diagnostics.Eventing. The System.Diagnostics.Eventing is quite a bit more complex due to the nature of the Vista+ Event Log API's capabilities (bookmarks, channels, XML rendering etc.), but it seems portable. |
|
OK. I'm curious to know why we didn't port it, and whether we can.. |
|
@danmosemsft I've read through the code for portability, and it is pretty much just a direct port, except for a few missing attributes which were removed in .NET Core. There are some minor changes related to API (uint to int, moved parameter etc.) as well. |
|
@Genbox could you please open an issue proposing the port? We can discuss there. |
|
Assuming you're interested in port to Windows -- there is already an issue proposing port ot Linux #24852 |
* Adding sources for System.Diagnostics.EventLog * Adding some tests for System.Diagnostics.EventLog Commit migrated from dotnet/corefx@e6084de
Adding Eventlog to .Net Core