gmane.network.inn

Re: [PATCH]Resolve file name conflicts in inn-2.5.3.

Russ Allbery — 2013-05-22T19:28:14




That's probably also a good idea in an ideal world, although I'm not sure
it's as important since I don't think anyone has reported a conflict
there.  It's also a bit harder to handle the upgrades, since that requires
messing about in the readers.conf file instead of just renaming a
configuration file.

Really, all of the auth programs should have had an inn-* prefix
originally.  I'm not sure whether it's worth renaming them unless someone
reports a specific problem, though.


inews is the standard name for doing the thing that inews does, so we
definitely shouldn't rename that one.

history and expire should, in retrospect, have had less generic names, but
they've been called that in netnews systems for forever, and I think it's
too late to rename them now.  On those, INN actually has precedence over
anyone else who wants to use the name (unlike with the radius.conf, where
we're a newcomer and clased with an existing file name).

ident and domain are less likely to clash, but in an ideal world would
als

Re: [PATCH]Resolve file name conflicts in inn-2.5.3.

Julien ÉLIE — 2013-05-22T19:07:30

Hi Russ,


Shouldn't we then also rename radius(8) to inn-radius(8)?

I also wonder if names like history(5), inews(1), ident(8), domain(8) or 
expire(8) would not suffer from the same problem...  They probably are 
not specific to INN...

Re: [PATCH]Resolve file name conflicts in inn-2.5.3.

Russ Allbery — 2013-05-22T17:58:58





It would probably be better to change the configuration file as well,
since otherwise it's hard for the user to specify which man page they want
to see.  That avoids the file conflict, but there's still a conceptual
conflict; it pushes the burden to the user.

I suspect there aren't that many people using the radius authenticator and
renaming the configuration file wouldn't have that much impact.  But yes,
we would want to rename it during innupgrade as well.

Re: [PATCH]Resolve file name conflicts in inn-2.5.3.

Julien ÉLIE — 2013-05-22T17:56:29

Hi Jochen,


[...]


Hmm...  It means that the configuration name changed, and we then need
taking care of that during "innupgrade".

I wonder whether this change is necessary.  Couldn't only the name
of the man page be changed?

According to:
    https://fedoraproject.org/wiki/Packaging:Conflicts#Man_Page_Name_Conflicts

we could just rename man5/radius.conf.5.gz to man5/radius.conf.5inn.gz;
wouldn't it be enough?

Re: Odd issue with pod2man when building on Fedora build server

Russ Allbery — 2013-05-22T17:56:01







Sorry, yeah, I committed a change yesterday that should fix this problem.
I forced all of the =item tags to descriptions.  pod2man wouldn't warn
about the first one since it would take that as the sign that this was a
numeric list, but some POD translators might mangle it into "1." because
they do automatic renumbering.


It shouldn't matter where the Z<> is put as long as it's there somewhere.
Putting C<> around the number would also work, although that also changes
the formatting.

Re: Odd issue with pod2man when building on Fedora build server

Julien ÉLIE — 2013-05-22T17:48:21

Hi Jochen,


Thanks for having had a look at it.




I see that the man page of pgpverify currently uses
=item 0Z<>

Isn't it enough to fix the problem?  Should all the items be changed 
similarly?




Does it also work with:
=item Z<>1
(and same thing for all the items)

Russ suggested this change yesterday for actsync.pod:
     http://inn.eyrie.org/trac/changeset/9474/trunk

[PATCH]Resolve file name conflicts in inn-2.5.3.

Jochen Schmitt — 2013-05-22T17:43:21

Hello,

several weeks ago, I have reported an issue that inn-2.5.3, that this
package contains files with names conlicting with the libradius package.

Therefore, I have created the following patch. It may be nice, if you
can take a review of the patch and integrating the patch into the
upstream sources.

Best Regards:

Jochen Schmitt

diff -up inn-2.5.3/doc/man/Makefile.radius inn-2.5.3/doc/man/Makefile
--- inn-2.5.3/doc/man/Makefile.radius2013-02-24 16:27:53.340492703 +0100
+++ inn-2.5.3/doc/man/Makefile2013-02-24 16:27:53.353491839 +0100
< at >< at > -18,7 +18,7 < at >< at > SEC5= active.5 active.times.5 buffindex
 cycbuff.conf.5 distrib.pats.5 distributions.5 expire.ctl.5 history.5 incoming.conf.5 \
 inn.conf.5 innfeed.conf.5 innwatch.ctl.5 moderators.5 motd.news.5 \
 newsfeeds.5 newsgroups.5 newslog.5 nnrpd.track.5 nntpsend.ctl.5 ovdb.5 \
-passwd.nntp.5 radius.conf.5 readers.conf.5 \
+passwd.nntp.5 inn-radius.conf.5 readers.conf.5 \
 storage.conf.5 subscriptions.5
 
 SEC8= actsync.8 archive.8 batcher.8 buffchan.8

Re: Odd issue with pod2man when building on Fedora build server

Jochen Schmitt — 2013-05-22T17:35:48

On Tue, May 21, 2013 at 09:04:49PM +0200, Julien ÉLIE wrote:

Hallo Julien,

thank you for your quick response. I have created an patch
which solved the issue. I have attached this patch to this
mail for review and upstream integration.

Best Regards:

Jochen Schmitt

diff -up inn-2.5.3/control/pgpverify.in.pdx inn-2.5.3/control/pgpverify.in
--- inn-2.5.3/control/pgpverify.in.pdx2013-05-22 18:45:57.132464138 +0200
+++ inn-2.5.3/control/pgpverify.in2013-05-22 18:47:39.056806362 +0200
< at >< at > -738,19 +738,19 < at >< at > B<pgpverify> may exit with the following
 
 The control message had a good PGP signature.
 
-=item 1
+=item Z<>1
 
 The control message had no PGP signature.
 
-=item 2
+=item Z<>2
 
 The control message had an unknown PGP signature.
 
-=item 3
+=item Z<>3
 
 The control message had a bad PGP signature.
 
-=item 255
+=item Z<>255
 
 A problem occurred not directly related to PGP analysis of signature.
 
diff -up inn-2.5.3/doc/pod/actsync.pod.pdx inn-2.5.3/doc/pod/actsync.pod
--- inn-2.5.3/doc/pod/actsync

Re: Odd issue with pod2man when building on Fedora build server

Julien ÉLIE — 2013-05-21T19:04:49

Hi Jochen,


It looks like recent POD versions do not cope with "=item 0" followed
by "=item 1":
    http://www.nntp.perl.org/group/perl.perl5.porters/2013/02/msg199266.html

A fix would be to replace:

=item 0

with:

=item 0Z<>


Does it solve the issue you see with inn-2.5.3?

Odd issue with pod2man when building on Fedora build server

Jochen Schmitt — 2013-05-21T18:43:52

Hello,

when I try to build inn-2.5.4 on the Fedora build server for rawhide
I will get the following error messages:

pod2man -c 'InterNetNews Documentation' -r 'INN 2.5.3' -s 5 subscriptions.pod > ../man/subscriptions.5
pod2man -c 'InterNetNews Documentation' -r 'INN 2.5.3' -s 8 actsync.pod > ../man/actsync.8
actsync.pod around line 532: Expected text after =item, not a number
actsync.pod around line 536: Expected text after =item, not a number
actsync.pod around line 541: Expected text after =item, not a number
actsync.pod around line 545: Expected text after =item, not a number
POD document had syntax errors at /usr/bin/pod2man line 69.

It may be nice, if anyone have a hint to solve this issues.

Best Regards:

Jochen Schmitt

Re: Adding new gcc warnings

Julien ÉLIE — 2013-05-20T19:43:38

Responding to myself:


I read too fast.  You clearly said "it's worth fixing code that triggers 
-Wlogical-op because it's often unclear", so this warning should be used.

Re: Adding new gcc warnings

Julien ÉLIE — 2013-05-20T19:33:49

Hi Russ,


Just a few remarks:

-Werror can be placed before -Wall because it appears before -Wall in 
the GCC manual.

-Wendif-labels is not necessary because it is the default behaviour.

-Wuninitialized is not necessary because it is implied by -Wall.

-Wdeclaration-after-statement can sometimes be useful for clarity (for 
instance declaring a variable only in the loop it is used).  I do not 
know whether this warning should really be enforced.

-Wlogical-op led to unclear code when fixed (according to the other mail 
you wrote about INN; unless I misunderstood your meaning?)

-Wredundant-decls is not used in INN because of noise generated by 
system headers.

-Wmissing-declarations and -Wsync-nand could be added.

Couldn't also the combination "-Wconversion -Wno-sign-conversion" be added?

Couldn't -Wstack-protector along with -fstack-protector be useful?

Re: Adding new gcc warnings

Julien ÉLIE — 2013-05-20T19:33:13

Hi Russ,


Noted.  The reason why I did not suggest -Wformat=2 is because of
a few "format-nonliteral" errors that are currently generated.

    "warn if the format string is not a string literal and so cannot be
    checked, unless the format function takes its format arguments as a
    va_list."

Fixing these errors would be needed before enforcing -Wformat=2.
I will try to have a look at that.




-Wold-style-declaration is enabled by -Wextra.
However, -Wold-style-definition is not in the default set.




According to:
    http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50134

"it's a style warning: a global function should be 
declared in a header, so warn for

int
f (void)
{
  return 0; 
}

if there was no previous declaration for f.  (For C++, the definition and 
any previous declaration will always provide a prototype.)

As Ian said in <http://gcc.gnu.org/ml/gcc/2011-08/msg00366.html>, for C++ 
the two options reduce to the same thing because no non-prototype 
declarations or definitions exist.  For C,

i

Re: [PATCH] innfeed does not reopen (rotated) log file

Julien ÉLIE — 2013-04-29T19:50:06

Hi Florian,


Thanks for your proposal of patch.

I wonder whether sending the signal twice is necessary (once before the 
rotation, and once after the rotation).  Isn't it enough to send it 
after the rotation?

That is to say we keep a "ctlinnd flushlogs" before the rotation, and we 
add the innfeed signal after the rotation (instead of the second 
"ctlinnd flushlogs" introduced in a recent commit, that should be reverted)?

It would then be fine for INN 2.5.  And we could consider a complete fix 
for INN 2.6 with the addition of "ctlinnd prerotatelogs"/"ctlinnd 
postrotatelogs"?

Re: double 'ctlinnd flushlogs' deletes news, errlog

Julien ÉLIE — 2013-04-24T20:34:42

Hi Florian,


I believe it is for the same reason why the "copytruncate" option exists
for logrotate.

    http://blog.adityapatawari.com/2012/05/logrotate-most-basic-log-management.html

"Copytruncate comes handy in the situation where process writes to the inode
of the log and rotating the log might cause process to go defunct or stop
logging or a bunch of other issues.  Copytruncate copies the log and the further
processing is done on the copy.  It also truncates the original file to zero bytes.
Therefore the inode of the file is unchanged and process keeps on writing
to the log file as if nothing has happened."




Maybe this behaviour is not true on all platforms, and the "copytruncate"
feature is more portable for handling log rotation.




Yes, we could do that for INN 2.5.4.




It seems strange that the rotation of the news and errlog files is done
before flushing these log files.  ICDwrite() also flushes these two (buffered)
files.
Whence the suggestion of 'ctlinnd prerotatelogs' (flushing everythi

Re: Adding new gcc warnings

Russ Allbery — 2013-04-22T21:59:43



And, as it turns out, I started using it for my other projects somewhere
along the line.  :)

Based on this discussion, I read through the other options, and am now
trying:

# A set of flags for warnings.  Add -O because gcc won't find some warnings
# without optimization turned on.  Desirable warnings that can't be turned
# on due to other problems:
#
#     -Wconversionhttp://bugs.debian.org/488884 (htons warnings)
#
# Last checked against gcc 4.7.2 (2013-04-22).  -D_FORTIFY_SOURCE=2 enables
# warn_unused_result attribute markings on glibc functions on Linux, which
# catches a few more issues.
WARNINGS = -g -O -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wendif-labels   \
-Wformat=2 -Winit-self -Wswitch-enum -Wuninitialized -Wfloat-equal \
-Wdeclaration-after-statement -Wshadow -Wpointer-arith   \
-Wbad-function-cast -Wcast-align -Wwrite-strings   \
-Wjump-misses-init -Wlogical-op -Wstrict-prototypes   \
-Wold-style-definition -Wmissing-prototypes -Wnormalized=nfc   \
-Wpacked -Wredundant-decls -Wne

Re: Adding new gcc warnings

Russ Allbery — 2013-04-22T21:42:56


The system will get upgraded to wheezy as soon as I have some time,
although based on current trends that might still be a few more months.


I recommend -Wformat=2 instead.  It includes that and a bunch of other
useful stuff.


I always left this off because if I were to ever do that, it would be
intentional; it's a hard mistake to make by accident.  But I don't recall
ever using it, so I have no objections.


Oh, huh, that's not part of the default set?


I use -Wmissing-prototypes.  I'm not sure what the difference is.  The
documentation is not particularly illuminating.


These all seem fine, although probably won't trigger with our code (except
for the -Wpacked point that you note below).


I do tend to use this, but it can be quite annoying if you have large
enums and you're not currently handling every case.  Basically, the more
you intentionally use default, the more annoying this is.


Since this is avoiding division by zero, checking against epsilon probably
isn't really needed, but it also proba

Re: double 'ctlinnd flushlogs' deletes news, errlog

Florian Schlichting — 2013-04-16T14:22:21

John,


we use the following local patch to add the number of the day, but of
course you can easily change the format parameter of the date call
(there may be some fuzz, but you'll get the idea):

--- a/scripts/scanlogs.in
+++ b/scripts/scanlogs.in
< at >< at > -303,30 +275,10 < at >< at > if [ -s ${OLD_SYSLOG} -o -s ${OLD_LOG} ] ; then
 ##  Compress and rotate the logs.
 if ${ROTATE} ; then
     cd ${OLD}
-    if [ X${LOGCYCLES} = X ]; then
-        LOGCYCLES=3
-    fi
     for F in ${LOGS} ; do
-       ##  Skip if it's unwanted.log, since it's already rotated.
-       if [ ${F} = ${UNWANTED_LOG} ]; then
-           continue
-       fi
        ##  Skip if file doesn't exist.
        BASE=`basename ${F}`
        test -f ${BASE} || continue
< at >< at > -325,16 +277,8 < at >< at > if ${ROTATE} ; then
        ${LOG_COMPRESS} <${BASE} >${BASE}.0${Z} && rm -f ${BASE}
        chmod 0440 ${BASE}.0${Z}

-       ##  Do rotation.
-       EXT=${LOGCYCLES}
-       rm -f ${BASE}.${LOGCYCLES}${Z}
-       while [ ${EXT} -gt 0 ] ; do
-           NEXT=${EXT}
-

Re: double 'ctlinnd flushlogs' deletes news, errlog

Florian Schlichting — 2013-04-16T14:12:37

Hi Julien,


I'm not sure why the livefiles are rotated using hard links, that is,
why it is necessary to make absolutely sure that at no time does the
regular log file not exist. To my limited knowledge, if a file is opened
for writing that does not exist, it is created, period. That's the way
e.g. logrotate works: it moves the current log file out of the way,
while the daemon is still writing to it. Then it sends sighup, the
daemon finishes writing and reopens (creates) the new log file. The day
after, the old logfile is being compressed.


it may be better to revert the commit that introduced the second call to
flushlogs, then - after all, the issue it corrects is fairly minor.


this is what I suggest


While I support splitting reporting off scanlogs, let scanlogs do log
rotation only and support plugging in other log rotation handlers, I'd
think this is a separate issue.

But I don't understand why two "modes" should be needed. This seems
overly complex, for no benefit?

scanlogs:
 moves news file
 mov

Adding new gcc warnings

Julien ÉLIE — 2013-04-14T20:41:58

Hi all,

As gcc 4.4.x is currently used for the daily generation of snapshots,
maybe we could add new warnings for "make warnings" builds.

I suggest the following flags:
-Wformat-y2k
-Winit-self
-Wold-style-definition
-Wmissing-declarations
-Wnormalized=nfc
-Wpacked
-Winline
-Wsync-nand
-Wvla


Should -Wswitch-default be added?  It implies that all switch statements
are required to have a default case, even though all possible cases
are correctly handled.


Should -Wfloat-equal be added?  Errors like these ones appear, and
I wonder whether a fix is needed.
In innd/status.c, size is a float:
  if (!size) size = 1; /* avoid divide by zero here too */

In innfeed/host.c:
  if (h->params->dynBacklogFilter != oldBacklogFilter)



-Wunreachable-code, -Wstrict-overflow=2, -Wtraditional-conversion and
-Wlogical-op give false positives, so I believe adding them is not wise.



Incidentally, a warning is triggered by -Wpacked in include/inn/dbz.h:

 typedef struct {
     charhash[DBZ_INTERNAL_HASH_SIZE];
 } PACKED

Re: double 'ctlinnd flushlogs' deletes news, errlog

Julien ÉLIE — 2013-04-13T16:57:59

Hi John,


The ".old" extension is used temporarily, between the moment the current 
log is moved (to ".old") and the moment it is rotated (to for instance 
".1.gz").
In your <pathlog> directory, you will only see a "news" log file; the 
"news.old" file does not exist.  And in <pathlog>/OLD, you will see 
"news.1.gz", which allows to sort the log files.