Bug #90218 for Sys-Syslog: Handle getprotobyn{ame,umber} not being available

Sun Nov 10 16:40:02 2013 fraserbn [...] gmail.com - Ticket created

Subject:

Handle getprotobyn{ame,umber} not being available

On Perls built without those functions (either because the system lacks them, ala Android, or because they were undefined during Configure), calling these results in an exception, so they should either be protected by an eval {} or only called as 'if ($Config{func}) { func() }'. The attached patch implements the latter. diff --git a/cpan/Sys-Syslog/Syslog.pm b/cpan/Sys-Syslog/Syslog.pm index 75a1832..dbceb13 100644 --- a/cpan/Sys-Syslog/Syslog.pm +++ b/cpan/Sys-Syslog/Syslog.pm @@ -3,6 +3,7 @@ use strict; use warnings; use warnings::register; use Carp; +use Config; use Exporter qw< import >; use File::Basename; use POSIX qw< strftime setlocale LC_TIME >; @@ -11,7 +12,7 @@ require 5.005; { no strict 'vars'; - $VERSION = '0.33'; + $VERSION = '0.33_1'; %EXPORT_TAGS = ( standard => [qw(openlog syslog closelog setlogmask)], @@ -429,7 +430,6 @@ sub syslog { $buf = $message; } else { - require Config; my $whoami = $ident; $whoami .= "[$$]" if $options{pid}; @@ -628,7 +628,10 @@ sub connect_log { sub connect_tcp { my ($errs) = @_; - my $proto = getprotobyname('tcp'); + my $proto; + if ( $Config{d_getpbyname} ) { + $proto = getprotobyname('tcp'); + } if (!defined $proto) { push @$errs, "getprotobyname failed for tcp"; return 0; @@ -676,7 +679,10 @@ sub connect_tcp { sub connect_udp { my ($errs) = @_; - my $proto = getprotobyname('udp'); + my $proto; + if ( $Config{d_getpbyname} ) { + $proto = getprotobyname('udp'); + } if (!defined $proto) { push @$errs, "getprotobyname failed for udp"; return 0;

Wed Nov 20 19:23:44 2013 SAPER [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #90218] Handle getprotobyn{ame,umber} not being available
Date:	Thu, 21 Nov 2013 01:23:26 +0100
To:	bug-Sys-Syslog [...] rt.cpan.org
From:	Sébastien Aperghis-Tramoni <saper [...] cpan.org>

Brian Fraser wrote via RT: Show quoted text

> On Perls built without those functions (either because the system lacks them, ala Android, or because they were undefined during Configure), calling these results in an exception, so they should either be protected by an eval {} or only called as 'if ($Config{func}) { func() }'.

Urk! This is awful! I know I'm micro-optimizing here, but I wonder if eval-ing isn't both faster and visually easier to swallow. In terms of generated opcodes at least, my $proto = eval { getprotobyname('tcp') }; produces less opcodes than my $proto = getprotobyname('tcp') if $Config{d_getpbyname}; which also produces less opcodes than my $proto; if ( $Config{d_getpbyname} ) { $proto = getprotobyname('tcp'); } I'll benchmark these to check which is faster, but in terms of readability, I really prefer eval. -- Sébastien Aperghis-Tramoni Close the world, txEn eht nepO.

Wed Nov 20 19:23:45 2013 The RT System itself - Status changed from 'new' to 'open'

Wed Nov 20 21:16:52 2013 fraserbn [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #90218] Handle getprotobyn{ame,umber} not being available
Date:	Thu, 21 Nov 2013 00:16:39 -0200
To:	bug-Sys-Syslog [...] rt.cpan.org
From:	Brian Fraser <fraserbn [...] gmail.com>

On Wed, Nov 20, 2013 at 9:23 PM, Sébastien Aperghis-Tramoni via RT < bug-Sys-Syslog@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=90218 > > > Brian Fraser wrote via RT: >

> > On Perls built without those functions (either because the system lacks

> them, ala Android, or because they were undefined during Configure), > calling these results in an exception, so they should either be protected > by an eval {} or only called as 'if ($Config{func}) { func() }'. > > Urk! This is awful! > I know I'm micro-optimizing here, but I wonder if eval-ing isn't both > faster and visually easier to swallow. > In terms of generated opcodes at least, > > my $proto = eval { getprotobyname('tcp') }; > > produces less opcodes than > > my $proto = getprotobyname('tcp') if $Config{d_getpbyname}; > > which also produces less opcodes than > > my $proto; > if ( $Config{d_getpbyname} ) { > $proto = getprotobyname('tcp'); > } > > I'll benchmark these to check which is faster, but in terms of > readability, I really prefer eval. >

Yeah, the eval version is much nicer to read. However, consider that the long version with the if can be optimized away by perl so that only the 'my $proto;' is ever run. The my $proto = ... if ...; version might give deprecation warnings too, because it's unintentionally creating a state variable. Also, the less opcodes of eval might not be as less as you imagine; off the top of my head, you'd be dealing with entertry, dotry, possibly a croak+longjmp+$SIG{__DIE__}, plus the risk that __DIE__ handler is not checking for $^S, and then leavetry. Perhaps this would be better? my $proto = $Config{d_getpbyname} ? getprotobyname('tcp') : undef;

Fri Nov 22 03:26:03 2013 SAPER [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #90218] Handle getprotobyn{ame,umber} not being available
Date:	Fri, 22 Nov 2013 09:25:42 +0100
To:	bug-Sys-Syslog [...] rt.cpan.org
From:	Sébastien Aperghis-Tramoni <saper [...] cpan.org>

Brian Fraser wrote via RT: Show quoted text

> Sébastien Aperghis-Tramoni wrote via RT: >

>> Brian Fraser wrote via RT: >>

>>> On Perls built without those functions (either because the system lacks >>> them, ala Android, or because they were undefined during Configure), >>> calling these results in an exception, so they should either be protected >>> by an eval {} or only called as 'if ($Config{func}) { func() }'.

>> >> Urk! This is awful! >> I know I'm micro-optimizing here, but I wonder if eval-ing isn't both >> faster and visually easier to swallow. >> In terms of generated opcodes at least, >> >> my $proto = eval { getprotobyname('tcp') }; >> >> produces less opcodes than >> >> my $proto = getprotobyname('tcp') if $Config{d_getpbyname}; >> >> which also produces less opcodes than >> >> my $proto; >> if ( $Config{d_getpbyname} ) { >> $proto = getprotobyname('tcp'); >> } >> >> I'll benchmark these to check which is faster, but in terms of >> readability, I really prefer eval.

> > Yeah, the eval version is much nicer to read. However, consider that the > long version with the if can be optimized away by perl so that only the 'my > $proto;' is ever run.

Hmm, can it be optimized in such a case? I think only when there's a if 0 at compilation time, the expression is optimized away. Show quoted text

> The my $proto = ... if ...; version might give > deprecation warnings too, because it's unintentionally creating a state > variable.

Indeed, although this construct does not warn yet, it might one day. Show quoted text

> Also, the less opcodes of eval might not be as less as you imagine; off the > top of my head, you'd be dealing with entertry, dotry, possibly a > croak+longjmp+$SIG{__DIE__}, plus the risk that __DIE__ handler is not > checking for $^S, and then leavetry. > Perhaps this would be better? > my $proto = $Config{d_getpbyname} ? getprotobyname('tcp') : undef;

I'm benchmarking the different solutions with Dumbbench (supposedly better than Benchmark) to see how well they perform, and currently, eval-block seem to always win, but in some cases with such a difference that I'm doubting the results. -- Sébastien Aperghis-Tramoni Close the world, txEn eht nepO.

Thu Jan 23 19:15:57 2014 fraserbn [...] gmail.com - Correspondence added

From:

fraserbn [...] gmail.com

On Fri Nov 22 03:26:03 2013, SAPER wrote: Show quoted text

> Brian Fraser wrote via RT: >

> > Sébastien Aperghis-Tramoni wrote via RT: > >

> >> Brian Fraser wrote via RT: > >>

> >>> On Perls built without those functions (either because the system > >>> lacks > >>> them, ala Android, or because they were undefined during > >>> Configure), > >>> calling these results in an exception, so they should either be > >>> protected > >>> by an eval {} or only called as 'if ($Config{func}) { func() }'.

> >> > >> Urk! This is awful! > >> I know I'm micro-optimizing here, but I wonder if eval-ing isn't > >> both > >> faster and visually easier to swallow. > >> In terms of generated opcodes at least, > >> > >> my $proto = eval { getprotobyname('tcp') }; > >> > >> produces less opcodes than > >> > >> my $proto = getprotobyname('tcp') if $Config{d_getpbyname}; > >> > >> which also produces less opcodes than > >> > >> my $proto; > >> if ( $Config{d_getpbyname} ) { > >> $proto = getprotobyname('tcp'); > >> } > >> > >> I'll benchmark these to check which is faster, but in terms of > >> readability, I really prefer eval.

> > > > Yeah, the eval version is much nicer to read. However, consider that > > the > > long version with the if can be optimized away by perl so that only > > the 'my > > $proto;' is ever run.

> > Hmm, can it be optimized in such a case? I think only when there's a > if 0 at compilation time, the expression is optimized away.

Whoops, pardons, I missed this message. I think you're right -- it won't optimize it away at runtime, much less with %Config which is tried. You could do something like use Config; use constant { HAS_GETPROTOBYNAME => $Config::Config{d_getpbyname}, HAS_GETPROTOBYNUMBER => $Config::Config{d_getpbynumber} }; my $proto = HAS_GETPROTOBYNAME ? getprotobyname('tcp') : undef; Show quoted text

>

> > The my $proto = ... if ...; version might give > > deprecation warnings too, because it's unintentionally creating a > > state > > variable.

> > Indeed, although this construct does not warn yet, it might one day. >

> > Also, the less opcodes of eval might not be as less as you imagine; > > off the > > top of my head, you'd be dealing with entertry, dotry, possibly a > > croak+longjmp+$SIG{__DIE__}, plus the risk that __DIE__ handler is > > not > > checking for $^S, and then leavetry. > > Perhaps this would be better? > > my $proto = $Config{d_getpbyname} ? getprotobyname('tcp') : undef;

> > > I'm benchmarking the different solutions with Dumbbench (supposedly > better than Benchmark) to see how well they perform, and currently, > eval-block seem to always win, but in some cases with such a > difference that I'm doubting the results.

Benchmarks are hard, let's go shopping :P

Fri Jul 25 12:24:47 2014 fraserbn [...] gmail.com - Correspondence added

From:

fraserbn [...] gmail.com

On Thu Jan 23 19:15:57 2014, Hugmeir wrote: Show quoted text

> On Fri Nov 22 03:26:03 2013, SAPER wrote:

> > Brian Fraser wrote via RT: > >

> > > Sébastien Aperghis-Tramoni wrote via RT: > > >

> > >> Brian Fraser wrote via RT: > > >>

> > >>> On Perls built without those functions (either because the system > > >>> lacks > > >>> them, ala Android, or because they were undefined during > > >>> Configure), > > >>> calling these results in an exception, so they should either be > > >>> protected > > >>> by an eval {} or only called as 'if ($Config{func}) { func() }'.

> > >> > > >> Urk! This is awful! > > >> I know I'm micro-optimizing here, but I wonder if eval-ing isn't > > >> both > > >> faster and visually easier to swallow. > > >> In terms of generated opcodes at least, > > >> > > >> my $proto = eval { getprotobyname('tcp') }; > > >> > > >> produces less opcodes than > > >> > > >> my $proto = getprotobyname('tcp') if $Config{d_getpbyname}; > > >> > > >> which also produces less opcodes than > > >> > > >> my $proto; > > >> if ( $Config{d_getpbyname} ) { > > >> $proto = getprotobyname('tcp'); > > >> } > > >> > > >> I'll benchmark these to check which is faster, but in terms of > > >> readability, I really prefer eval.

> > > > > > Yeah, the eval version is much nicer to read. However, consider > > > that > > > the > > > long version with the if can be optimized away by perl so that only > > > the 'my > > > $proto;' is ever run.

> > > > Hmm, can it be optimized in such a case? I think only when there's a > > if 0 at compilation time, the expression is optimized away.

> > Whoops, pardons, I missed this message. I think you're right -- it > won't optimize it away at runtime, much less with %Config which is > tried. You could do something like > > use Config; > use constant { > HAS_GETPROTOBYNAME => $Config::Config{d_getpbyname}, > HAS_GETPROTOBYNUMBER => $Config::Config{d_getpbynumber} > }; > > my $proto = HAS_GETPROTOBYNAME ? getprotobyname('tcp') : undef; >

> >

> > > The my $proto = ... if ...; version might give > > > deprecation warnings too, because it's unintentionally creating a > > > state > > > variable.

> > > > Indeed, although this construct does not warn yet, it might one day. > >

> > > Also, the less opcodes of eval might not be as less as you imagine; > > > off the > > > top of my head, you'd be dealing with entertry, dotry, possibly a > > > croak+longjmp+$SIG{__DIE__}, plus the risk that __DIE__ handler is > > > not > > > checking for $^S, and then leavetry. > > > Perhaps this would be better? > > > my $proto = $Config{d_getpbyname} ? getprotobyname('tcp') : undef;

> > > > > > I'm benchmarking the different solutions with Dumbbench (supposedly > > better than Benchmark) to see how well they perform, and currently, > > eval-block seem to always win, but in some cases with such a > > difference that I'm doubting the results.

> > Benchmarks are hard, let's go shopping :P

Hey, sorry to nag, but would it be possible to get some variant of this applied? It's making smoking android a bit of a pain :)

Fri Jul 25 15:51:54 2014 sebastien [...] aperghis.net - Correspondence added

Subject:	Re: [rt.cpan.org #90218] Handle getprotobyn{ame,umber} not being available
Date:	Fri, 25 Jul 2014 21:51:44 +0200 (CEST)
To:	bug-Sys-Syslog [...] rt.cpan.org
From:	Sébastien Aperghis-Tramoni <sebastien [...] aperghis.net>

Brian Fraser wrote via RT: Show quoted text

> Hey, sorry to nag, but would it be possible to get some variant of > this applied? It's making smoking android a bit of a pain :)

Don't apologize, I'm a terrible maintainer :( I actually did quite some benchmarks in OSX and Linux and found the less costly code. This code is on the computer in repair. I'll try to fetch from the backup and commit. Not sure when I can though. -- Sébastien Aperghis-Tramoni Close the world, txEn eht nepO.

Fri Jul 25 17:47:31 2014 fraserbn [...] gmail.com - Correspondence added

From:

fraserbn [...] gmail.com

On Fri Jul 25 15:51:54 2014, sebastien@aperghis.net wrote: Show quoted text

> Brian Fraser wrote via RT: >

> > Hey, sorry to nag, but would it be possible to get some variant of > > this applied? It's making smoking android a bit of a pain :)

> > Don't apologize, I'm a terrible maintainer :( > I actually did quite some benchmarks in OSX and Linux and found the > less costly code. This code is on the computer in repair. I'll try > to fetch from the backup and commit. Not sure when I can though. >

Actually, I'm worse still: There was a discussion in another module about the most efficient approach, and I notice now that I somehow neglected to relay the results here. getprotobyname("tcp") in scalar context is exactly the same as Socket::IPPROTO_TCP; for 'udp', the constant is Socket::IPPROTO_UDP. If you were using the function in list context, the eval/%Config games become more attractive, but in scalar context, the constants are not just correct but faster. Score :D For https://rt.cpan.org/Public/Bug/Display.html?id=90224 I don't think there is a consensus; I imagine that use Config; use constant HAVE_SETLOCALE => $Config::Config{d_setlocale}; And then replace the $Config::Config{d_setlocale} in that patch on that ticket with HAVE_SETLOCALE would do away with any performance hit, since the if() would get optimized away at compile time, one way or another. ..so here's an updated patch with those changes. diff --git a/cpan/Sys-Syslog/Syslog.pm b/cpan/Sys-Syslog/Syslog.pm index 25164af..1d9c3f7 100644 --- a/cpan/Sys-Syslog/Syslog.pm +++ b/cpan/Sys-Syslog/Syslog.pm @@ -9,6 +9,9 @@ use POSIX qw< strftime setlocale LC_TIME >; use Socket qw< :all >; require 5.005; +use Config; +use constant HAVE_SETLOCALE => $Config::Config{d_setlocale}; + { no strict 'vars'; $VERSION = '0.33'; @@ -433,10 +436,13 @@ sub syslog { $whoami .= "[$$]" if $options{pid}; $sum = $numpri + $numfac; - my $oldlocale = setlocale(LC_TIME); - setlocale(LC_TIME, 'C'); + my $oldlocale; + if ( HAVE_SETLOCALE ) { + $oldlocale = setlocale(LC_TIME); + setlocale(LC_TIME, 'C'); + } my $timestamp = strftime "%b %d %H:%M:%S", localtime; - setlocale(LC_TIME, $oldlocale); + setlocale(LC_TIME, $oldlocale) if HAVE_SETLOCALE; # construct the stream that will be transmitted $buf = "<$sum>$timestamp $whoami: $message"; @@ -622,11 +628,7 @@ sub connect_log { sub connect_tcp { my ($errs) = @_; - my $proto = getprotobyname('tcp'); - if (!defined $proto) { - push @$errs, "getprotobyname failed for tcp"; - return 0; - } + my $proto = Socket::IPPROTO_TCP; my $port = $sock_port || getservbyname('syslog', 'tcp'); $port = getservbyname('syslogng', 'tcp') unless defined $port; @@ -670,11 +672,7 @@ sub connect_tcp { sub connect_udp { my ($errs) = @_; - my $proto = getprotobyname('udp'); - if (!defined $proto) { - push @$errs, "getprotobyname failed for udp"; - return 0; - } + my $proto = Socket::IPPROTO_UDP; my $port = $sock_port || getservbyname('syslog', 'udp'); if (!defined $port) {

Thu May 05 17:32:22 2016 SAPER [...] cpan.org - Correspondence added

Apologies, I had forgot about Sys::Syslog again. Regarding the part about setlocale(), I have mostly applied your patch (with a few adjustments because of other changes). For the part about getprotobyname() / IPPROTO_TCP, I am going to use local constants which get their values from getprotobyname(), or from Socket::IPPROTO_*. This will simply the code. -- Close the world, txEn eht nepO.

Fri May 06 05:27:00 2016 SAPER [...] cpan.org - Correspondence added

This issue should be fixed with version 0.34, just released. -- Close the world, txEn eht nepO.

Fri May 06 05:27:06 2016 SAPER [...] cpan.org - Taken

Fri May 06 05:27:07 2016 SAPER [...] cpan.org - Fixed in 0.34 added

Fri May 06 07:35:09 2016 SAPER [...] cpan.org - Status changed from 'open' to 'resolved'