You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4net-dev@logging.apache.org by Jo...@rohde-schwarz.com on 2016/08/22 10:27:15 UTC
Fix for FileAppender's TextWriter creation in OpenFile
Hi,
`FileAppender` has a vritual overload for the Method `SetQWForFiles` that
takes a `Stream` and creates a StreamWriter from it. According to the API
documentation
"""
This method can be overridden by sub classes that want to wrap the
<see cref="Stream"/> in some way, for example to encrypt the output
data using a <c>System.Security.Cryptography.CryptoStream</c>.
"""
Unfortunately, it doesn't get called. Instead it is inlined in `OpenFile`,
so derived classes have no chance in wrapping the stream before it is
passed to the writer.
This trivial patch changes this.
---------------------8<--------------8<-----------------
--- C:/temp/FileAppender.cs-revBASE.svn003.tmp.cs Sat Aug 13
18:57:44 2016
+++ C:/Users/BAEHR/Documents/log4net-trunk/src/Appender/FileAppender.cs
Mon Aug 22 11:59:41 2016
@@ -1382 +1382 @@ namespace log4net.Appender
- SetQWForFiles(new
StreamWriter(m_stream, m_encoding));
+ SetQWForFiles(m_stream);
---------------------8<--------------8<-----------------
Best Regards,
Jonas
Re: Fix for FileAppender's TextWriter creation in OpenFile
Posted by Stefan Bodewig <bo...@apache.org>.
On 2016-08-22, <Jo...@rohde-schwarz.com> wrote:
> `FileAppender` has a vritual overload for the Method `SetQWForFiles` that
> takes a `Stream` and creates a StreamWriter from it. According to the API
> documentation
> """
> This method can be overridden by sub classes that want to wrap the
> <see cref="Stream"/> in some way, for example to encrypt the output
> data using a <c>System.Security.Cryptography.CryptoStream</c>.
> """
> Unfortunately, it doesn't get called. Instead it is inlined in `OpenFile`,
> so derived classes have no chance in wrapping the stream before it is
> passed to the writer.
> This trivial patch changes this.
Patch applied, many thanks
Stefan
Re: Fix for FileAppender's TextWriter creation in OpenFile
Posted by Jo...@rohde-schwarz.com.
Dominik Psenner <dp...@gmail.com> wrote on 09.09.2016 08:54:24:
> Von: Dominik Psenner <dp...@gmail.com>
> An: log4net-dev@logging.apache.org
> Datum: 09.09.2016 08:54
> Betreff: Re: Fix for FileAppender's TextWriter creation in OpenFile
>
> Is
> EventLogAppenderTest.ActivateOptionsDisablesAppenderIfSourceDoesntExist
> broken without your changes too?
The test fails also without my changes. From the comments on this special
test case I assume the pass/fail has mainly to do with some Windows
version and administrator privilege combinations.
> On 2016-08-22 14:10, Jonas.Baehr@rohde-schwarz.com wrote:
> Hi Dominik,
>
> Although there are no dedicated tests for the file appender as such,
> I did run the test suite at the time I changed the code. The recent
> changes indtroduced with r1756284 ".NET Core improvements by Peter
> Jas - closes #30" broke a lot of tests becuse of calling
> `ToUpperInvariant` in string objects that can be null. I'll start
> another thread about that issue.
> With that fixed, only a single test
>
`EventLogAppenderTest.ActivateOptionsDisablesAppenderIfSourceDoesntExist`
> fails here, but this has nothing to do with the file appender change.
>
> Notably the RollingFileAppender tests run without issues after my
change.
>
> Regards,
> Jonas
>
>
>
> Von: Dominik Psenner <dp...@apache.org>
> An: log4net-dev@logging.apache.org
> Datum: 22.08.2016 13:30
> Betreff: Re: Fix for FileAppender's TextWriter creation in
OpenFile
>
>
>
> Hi Jonas,
> thanks for your patch! It looks sensible. Have you run the tests against
it?
> Cheers,Dominik
> On 2016-08-22 12:27, Jonas.Baehr@rohde-schwarz.com wrote:
> Hi,
>
> `FileAppender` has a vritual overload for the Method `SetQWForFiles`
> that takes a `Stream` and creates a StreamWriter from it. According
> to the API documentation
> """
> This method can be overridden by sub classes that want to wrap the
> <see cref="Stream"/> in some way, for example to encrypt the output
> data using a <c>System.Security.Cryptography.CryptoStream</c>.
> """
>
> Unfortunately, it doesn't get called. Instead it is inlined in
> `OpenFile`, so derived classes have no chance in wrapping the stream
> before it is passed to the writer.
> This trivial patch changes this.
>
> ---------------------8<--------------8<-----------------
> --- C:/temp/FileAppender.cs-revBASE.svn003.tmp.cs Sat Aug 13
> 18:57:44 2016
> +++ C:/Users/BAEHR/Documents/log4net-trunk/src/Appender/
> FileAppender.cs Mon Aug 22 11:59:41 2016
> @@ -1382 +1382 @@ namespace log4net.Appender
> - SetQWForFiles(new
> StreamWriter(m_stream, m_encoding));
> +
SetQWForFiles(m_stream);
> ---------------------8<--------------8<-----------------
>
> Best Regards,
> Jonas
>
Re: Fix for FileAppender's TextWriter creation in OpenFile
Posted by Dominik Psenner <dp...@gmail.com>.
Is
EventLogAppenderTest.ActivateOptionsDisablesAppenderIfSourceDoesntExist
broken without your changes too?
On 2016-08-22 14:10, Jonas.Baehr@rohde-schwarz.com wrote:
> Hi Dominik,
>
> Although there are no dedicated tests for the file appender as such, I
> did run the test suite at the time I changed the code. The recent
> changes indtroduced with r1756284 ".NET Core improvements by Peter Jas
> - closes #30" broke a lot of tests becuse of calling
> `ToUpperInvariant` in string objects that can be null. I'll start
> another thread about that issue.
> With that fixed, only a single test
> `EventLogAppenderTest.ActivateOptionsDisablesAppenderIfSourceDoesntExist`
> fails here, but this has nothing to do with the file appender change.
>
> Notably the RollingFileAppender tests run without issues after my change.
>
> Regards,
> Jonas
>
>
>
> Von: Dominik Psenner <dp...@apache.org>
> An: log4net-dev@logging.apache.org
> Datum: 22.08.2016 13:30
> Betreff: Re: Fix for FileAppender's TextWriter creation in OpenFile
> ------------------------------------------------------------------------
>
>
>
> Hi Jonas,
>
> thanks for your patch! It looks sensible. Have you run the tests
> against it?
>
> Cheers,Dominik
>
> On 2016-08-22 12:27, _Jonas.Baehr@rohde-schwarz.com_
> <ma...@rohde-schwarz.com>wrote:
> Hi,
>
> `FileAppender` has a vritual overload for the Method `SetQWForFiles`
> that takes a `Stream` and creates a StreamWriter from it. According to
> the API documentation
> """
> This method can be overridden by sub classes that want to wrap the
> <see cref="Stream"/> in some way, for example to encrypt the output
> data using a <c>System.Security.Cryptography.CryptoStream</c>.
> """
>
> Unfortunately, it doesn't get called. Instead it is inlined in
> `OpenFile`, so derived classes have no chance in wrapping the stream
> before it is passed to the writer.
> This trivial patch changes this.
>
> ---------------------8<--------------8<-----------------
> --- C:/temp/FileAppender.cs-revBASE.svn003.tmp.cs Sat Aug 13 18:57:44
> 2016
> +++
> C:/Users/BAEHR/Documents/log4net-trunk/src/Appender/FileAppender.cs
> Mon Aug 22 11:59:41 2016
> @@ -1382 +1382 @@ namespace log4net.Appender
> - SetQWForFiles(new StreamWriter(m_stream, m_encoding));
> + SetQWForFiles(m_stream);
> ---------------------8<--------------8<-----------------
>
> Best Regards,
> Jonas
>
>
Re: Fix for FileAppender's TextWriter creation in OpenFile
Posted by Jo...@rohde-schwarz.com.
Hi Dominik,
Although there are no dedicated tests for the file appender as such, I did
run the test suite at the time I changed the code. The recent changes
indtroduced with r1756284 ".NET Core improvements by Peter Jas - closes
#30" broke a lot of tests becuse of calling `ToUpperInvariant` in string
objects that can be null. I'll start another thread about that issue.
With that fixed, only a single test
`EventLogAppenderTest.ActivateOptionsDisablesAppenderIfSourceDoesntExist`
fails here, but this has nothing to do with the file appender change.
Notably the RollingFileAppender tests run without issues after my change.
Regards,
Jonas
Von: Dominik Psenner <dp...@apache.org>
An: log4net-dev@logging.apache.org
Datum: 22.08.2016 13:30
Betreff: Re: Fix for FileAppender's TextWriter creation in OpenFile
Hi Jonas,
thanks for your patch! It looks sensible. Have you run the tests against
it?
Cheers,Dominik
On 2016-08-22 12:27, Jonas.Baehr@rohde-schwarz.com wrote:
Hi,
`FileAppender` has a vritual overload for the Method `SetQWForFiles` that
takes a `Stream` and creates a StreamWriter from it. According to the API
documentation
"""
This method can be overridden by sub classes that want to wrap the
<see cref="Stream"/> in some way, for example to encrypt the output
data using a <c>System.Security.Cryptography.CryptoStream</c>.
"""
Unfortunately, it doesn't get called. Instead it is inlined in `OpenFile`,
so derived classes have no chance in wrapping the stream before it is
passed to the writer.
This trivial patch changes this.
---------------------8<--------------8<-----------------
--- C:/temp/FileAppender.cs-revBASE.svn003.tmp.cs Sat Aug 13
18:57:44 2016
+++ C:/Users/BAEHR/Documents/log4net-trunk/src/Appender/FileAppender.cs
Mon Aug 22 11:59:41 2016
@@ -1382 +1382 @@ namespace log4net.Appender
- SetQWForFiles(new
StreamWriter(m_stream, m_encoding));
+ SetQWForFiles(m_stream);
---------------------8<--------------8<-----------------
Best Regards,
Jonas
Re: Fix for FileAppender's TextWriter creation in OpenFile
Posted by Dominik Psenner <dp...@apache.org>.
Hi Jonas,
thanks for your patch! It looks sensible. Have you run the tests against it?
Cheers,Dominik
On 2016-08-22 12:27, Jonas.Baehr@rohde-schwarz.com wrote:
> Hi,
>
> `FileAppender` has a vritual overload for the Method `SetQWForFiles`
> that takes a `Stream` and creates a StreamWriter from it. According to
> the API documentation
> """
> This method can be overridden by sub classes that want to wrap the
> <see cref="Stream"/> in some way, for example to encrypt the output
> data using a <c>System.Security.Cryptography.CryptoStream</c>.
> """
>
> Unfortunately, it doesn't get called. Instead it is inlined in
> `OpenFile`, so derived classes have no chance in wrapping the stream
> before it is passed to the writer.
> This trivial patch changes this.
>
> ---------------------8<--------------8<-----------------
> --- C:/temp/FileAppender.cs-revBASE.svn003.tmp.cs Sat Aug 13
> 18:57:44 2016
> +++
> C:/Users/BAEHR/Documents/log4net-trunk/src/Appender/FileAppender.cs
> Mon Aug 22 11:59:41 2016
> @@ -1382 +1382 @@ namespace log4net.Appender
> - SetQWForFiles(new StreamWriter(m_stream, m_encoding));
> + SetQWForFiles(m_stream);
> ---------------------8<--------------8<-----------------
>
> Best Regards,
> Jonas