Menu
▾
▴
log4perl-devel
|
From: Dennis G. <dg...@re...> - 2004-05-13 00:37:15
Attachments:
input_separator.patch
|
The config_read method assumes that the input separator ($/) is set to "\n" when it reads in the config file. The attached patch sets the input separator so that the code doesn't break if the assumption is false. -- Dennis |
|
From: Mike S. <m...@pe...> - 2004-05-13 07:00:46
|
Dennis Gregorovic wrote on 5/12/2004, 5:35 PM: > The config_read method assumes that the input separator ($/) is set to > "\n" when it reads in the config file. The attached patch sets the > input separator so that the code doesn't break if the assumption is > false. Not sure I'm following ... why is it assuming $/ is set to "\n"? Also, please let me know which OS you're seeing a problem on. -- -- Mike Mike Schilli m...@pe... |
|
From: Dennis G. <dg...@re...> - 2004-05-13 14:42:27
|
In this line: @text =3D <TEXT>; it is assuming that $/ is set to "\n", which would result in @text containing an entry for each line in the config file. If $/ is undefined, then @text winds up with a single entry, which contains the entire contents of the config file. When this happens, the parse method of PropertyConfigurator fails. I'm seeing this in Perl 5.8.3 on FC1. -- Dennis On Thu, 2004-05-13 at 02:59, Mike Schilli wrote: > Dennis Gregorovic wrote on 5/12/2004, 5:35 PM: >=20 > > The config_read method assumes that the input separator ($/) is set to > > "\n" when it reads in the config file. The attached patch sets the > > input separator so that the code doesn't break if the assumption is > > false. >=20 > Not sure I'm following ... why is it assuming $/ is set to "\n"? Also,=20 > please let me know which OS you're seeing a problem on. >=20 > -- > -- Mike > Mike Schilli > m...@pe... |
|
From: Mike S. <m...@pe...> - 2004-05-13 17:08:51
|
Dennis Gregorovic wrote on 5/13/2004, 7:41 AM:
> In this line:
>
> @text = <TEXT>;
>
> it is assuming that $/ is set to "\n", which would result in @text
> containing an entry for each line in the config file.
Ah, ok, now I understand. However, tinkering with $/ without limiting
its scope is considered bad style. "perldoc perlvar" shows:
But the following code is quite bad:
open my $fh, "foo" or die $!;
undef $/; # enable slurp mode
my $content = <$fh>;
close $fh;
since some other module, may want to read data from some file in the
default "line mode", so if the code we have just presented has been
executed, the global value of $/ is now changed for any other code
running inside the same Perl interpreter.
Usually when a variable is localized you want to make sure that this
change affects the shortest scope possible. So unless you are already
inside some short "{}" block, you should create one yourself. For
example:
my $content = '';
open my $fh, "foo" or die $!;
{
local $/;
$content = <$fh>;
}
close $fh;
So, regarding Log4perl: unless anyone has any strong opinion about this,
I'd rather leave it as it is for now.
--
-- Mike
Mike Schilli
m...@pe...
|
|
From: Dennis G. <dg...@re...> - 2004-05-13 17:30:20
Attachments:
input_separator.3.patch
|
In my patch I added a block around the code and made $/ local.
However, now that I look back at the patch I see that I have the files
in reverse order, so I see the confusion. Sorry about that. I've
attached a new patch with the files in the correct order.
-- Dennis
On Thu, 2004-05-13 at 13:07, Mike Schilli wrote:
> Dennis Gregorovic wrote on 5/13/2004, 7:41 AM:
>
> > In this line:
> >
> > @text = <TEXT>;
> >
> > it is assuming that $/ is set to "\n", which would result in @text
> > containing an entry for each line in the config file.
>
> Ah, ok, now I understand. However, tinkering with $/ without limiting
> its scope is considered bad style. "perldoc perlvar" shows:
>
> But the following code is quite bad:
>
> open my $fh, "foo" or die $!;
> undef $/; # enable slurp mode
> my $content = <$fh>;
> close $fh;
>
> since some other module, may want to read data from some file in the
> default "line mode", so if the code we have just presented has been
> executed, the global value of $/ is now changed for any other code
> running inside the same Perl interpreter.
>
> Usually when a variable is localized you want to make sure that this
> change affects the shortest scope possible. So unless you are already
> inside some short "{}" block, you should create one yourself. For
> example:
>
> my $content = '';
> open my $fh, "foo" or die $!;
> {
> local $/;
> $content = <$fh>;
> }
> close $fh;
>
> So, regarding Log4perl: unless anyone has any strong opinion about this,
> I'd rather leave it as it is for now.
>
> --
> -- Mike
> Mike Schilli
> m...@pe...
>
|
|
From: Kevin G. <ke...@go...> - 2004-05-13 18:09:24
|
Mike Schilli wrote:
> Dennis Gregorovic wrote on 5/13/2004, 7:41 AM:
> > it is assuming that $/ is set to "\n", which would result in @text
> > containing an entry for each line in the config file.
>
> Ah, ok, now I understand. However, tinkering with $/ without limiting
> its scope is considered bad style. "perldoc perlvar" shows:
Mike, I think the problem Dennis is pointing out is that a user might do
something like this:
open (F, 'somefile');
$/ = undef;
$somefile = <F>; #slurp the whole file
Log::Log4perl::init('myconfig.conf');
In which case our init will fail since the user has (however
ill-advisedly) tinkered with $/, and we are assuming $/ is still "\n"
Dennis' proposal protects us from the user's tinkering.
--
Happy Trails . . .
Kevin M. Goess
(and Anne and Frank)
904 Carmel Ave.
Albany, CA 94706
(510) 525-5217
|
|
From: Mike S. <m...@pe...> - 2004-05-13 18:41:17
|
Kevin Goess wrote on 5/13/2004, 11:09 AM: > In which case our init will fail since the user has (however > ill-advisedly) tinkered with $/, and we are assuming $/ is still "\n" Right, that's the behaviour which the manpage discourages. I'm not sure if modules typically catch this or are supposed to catch this. But if you guys want it in, I'm ok with it if it doesn't break anything. If you're sure that setting $/ = "\n" is compatible with all OSes, I'll apply Dennis' patch. -- -- Mike Mike Schilli m...@pe... |
|
From: Jim C. <jc...@di...> - 2004-05-13 19:26:43
|
Mike Schilli wrote: >Kevin Goess wrote on 5/13/2004, 11:09 AM: > > > In which case our init will fail since the user has (however > > ill-advisedly) tinkered with $/, and we are assuming $/ is still "\n" > > even if he properly localized it, the init-call could be inside the block, and thus be broken. I think its best to protect the user as his patch does. >Right, that's the behaviour which the manpage discourages. I'm not sure >if modules typically catch this or are supposed to catch this. But if >you guys want it in, I'm ok with it if it doesn't break anything. If >you're sure that setting $/ = "\n" is compatible with all OSes, I'll >apply Dennis' patch. > > > you might have probs with windows - \r\n might work in htat case. tho im not a windoze guy. >-- >-- Mike >Mike Schilli >m...@pe... > > > |
|
From: Dennis G. <dg...@re...> - 2004-05-13 20:00:29
|
I just did a quick search through the core modules of 5.8.3 and it appears that in most (but not all) of the cases where they read in a file handle in a list context, they set $/ first. CGI.pm and CPAN.pm are a couple examples. Also, http://perldoc.com/perl5.8.4/pod/perlport.html#Newlines indicates that $/ =3D "\n" is cross-platform:=20 Perl uses \n to represent the "logical" newline, where what is logical may depend on the platform in use. By the way, I've been using Log4j for a while on a project at work and was very excited to see Log4perl. It's pretty awesome. Thanks -- Dennis On Thu, 2004-05-13 at 14:40, Mike Schilli wrote: > Kevin Goess wrote on 5/13/2004, 11:09 AM: >=20 > > In which case our init will fail since the user has (however > > ill-advisedly) tinkered with $/, and we are assuming $/ is still "\n" >=20 > Right, that's the behaviour which the manpage discourages. I'm not sure=20 > if modules typically catch this or are supposed to catch this. But if=20 > you guys want it in, I'm ok with it if it doesn't break anything. If=20 > you're sure that setting $/ =3D "\n" is compatible with all OSes, I'll=20 > apply Dennis' patch. >=20 > -- > -- Mike > Mike Schilli > m...@pe... |
|
From: Kevin G. <ke...@go...> - 2004-05-13 20:27:19
|
Yes, and the default value of $/ is \n, so setting it back to to \n shouldn't break anything that wasn't broken before. Applying the patch makes sense to me. Dennis Gregorovic wrote: > I just did a quick search through the core modules of 5.8.3 and it > appears that in most (but not all) of the cases where they read in a > file handle in a list context, they set $/ first. CGI.pm and CPAN.pm > are a couple examples. > > Also, http://perldoc.com/perl5.8.4/pod/perlport.html#Newlines indicates > that $/ = "\n" is cross-platform: > > Perl uses \n to represent the "logical" newline, where what is > logical may depend on the platform in use. > > By the way, I've been using Log4j for a while on a project at work and > was very excited to see Log4perl. It's pretty awesome. > > Thanks > -- Dennis > > On Thu, 2004-05-13 at 14:40, Mike Schilli wrote: > >>Kevin Goess wrote on 5/13/2004, 11:09 AM: >> >> > In which case our init will fail since the user has (however >> > ill-advisedly) tinkered with $/, and we are assuming $/ is still "\n" >> >>Right, that's the behaviour which the manpage discourages. I'm not sure >>if modules typically catch this or are supposed to catch this. But if >>you guys want it in, I'm ok with it if it doesn't break anything. If >>you're sure that setting $/ = "\n" is compatible with all OSes, I'll >>apply Dennis' patch. >> >>-- >>-- Mike >>Mike Schilli >>m...@pe... > > m -- Happy Trails . . . Kevin M. Goess (and Anne and Frank) 904 Carmel Ave. Albany, CA 94706 (510) 525-5217 |
|
From: Mike S. <m...@pe...> - 2004-05-13 21:42:50
|
Dennis Gregorovic wrote on 5/13/2004, 12:59 PM: > I just did a quick search through the core modules of 5.8.3 and it > appears that in most (but not all) of the cases where they read in a > file handle in a list context, they set $/ first. CGI.pm and CPAN.pm > are a couple examples. Excellent, thanks for digging that up. I've applied your patch, it'll be part of 0.45. Sorry for any confusion I might have caused! -- -- Mike Mike Schilli m...@pe... |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X