gmane.network.inn

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 everything),
doing the rotation with scanlogs, and 'ctlinnd postrotatelogs' (reopening
the log files and flushing exploder and process channels).
Hmmm...  Shouldn't then flushing these channels also be done before rotating
the log files, so as to log in the proper daily log file what belongs to it?

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 -Wnested-externs -Winline -Wvla -Werror

with other packages.  (The order of flags is intended to match the order
in which they're presented in the GCC manual, which appears to be
semi-random.)

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 probably wouldn't be hard.  IIRC, this is
only a float due to possible calculation size problems with a long.


This should really be checking against an epsilon.


Here's what I currently use, FYI:

WARNINGS = -g -O -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wendif-labels           \
        -Wformat=2 -Winit-self -Wswitch-enum -Wdeclaration-after-statement  \
        -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-align           \
        -Wwrite-strings -Wjump-misses-init -Wlogical-op                     \
        -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls          \
        -Wnested-externs -Werror

I do think it's worth fixing code that triggers -Wlogical-op because it's
often unclear.

-Wtraditional-conversion warns about problems when upgrading from K&R to
ANSI C and isn't something we really need to worry about.




I left the PACKED on there because I was afraid that removing it might
change the layout of the data structure on disk on some platform, thus
invalidating old history files.  I think it's fairly unlikely that this is
a problem, though.





Yes, absolutely.

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}
-           EXT=`expr ${EXT} - 1`
-           test -f ${BASE}.${EXT}${Z} \
-               && rm -f ${BASE}.${NEXT}${Z} \
-               && mv ${BASE}.${EXT}${Z} ${BASE}.${NEXT}${Z}
-       done
+       ##  Move the file once instead of daily rotation.
+       mv -f ${BASE}.0${Z} ${BASE}.`date +%d`${Z}
     done

     ##  Remove lock.

I felt properly supporting that in a configurable manner in scanlogs
along with the old behaviour is too cumbersome; allowing to switch log
rotation components e.g. to logrotate or something built into the file
system is likely a better way to spend one's time.

Florian

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
 moves errlog file
 does livefile rotation
 calls flushlogs

innd
 ICDwrite (flush history, articles in the storage manager, and active)
   does NOT rename the news file
 reopen the news file
   does NOT rename the errlog file
 reopen the errlog file
 flush exploder and process channels

scanlogs
 goes on to summarize log files, call innreport, compress and rotate OLD
 logs


Florian

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 erec;

Can the PACKED attribute be removed?  I do not understand its usefulness for
a struct of only one variable.




-Wold-style-definition returns an issue in innd/icd.c:

 char *
 ICDreadactive(endp)
    char                **endp;
 {


Can it be replaced with the following definition?

 char *
 ICDreadactive(char **endp)
 {

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.

Re: double 'ctlinnd flushlogs' deletes news, errlog

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

Hi Florian,


Oooops!  You're totally right.  Thanks for having caught the issue in 
log archives.




That's a possibility.  But shouldn't "news" and "errlog" then treated 
like what is done for live files just below?  (using a symbolic link 
rather than a copy)

A point to take into account:  changing the "flushlogs" behaviour would 
not be possible in the STABLE 2.5 branch, but only in 2.6... so the bug 
you report will still be present.


Another possibility, suggested by the TODO file:

* ctlinnd flushlogs currently renames all of the log files.  It would be
nice to support the method of log rotation that most other daemons
support, namely to move the logs aside and then tell innd to reopen its
log files.  Ideally, that behavior would be triggered with a SIGHUP.
scanlogs would have to be modified to handle this.

The best way to support this seems to be to leave scanlogs as is by
default, but also add two additional modes.  One would flush all the
logs and prepare for the syslog logs to be rotated, and the other would
do all the work needed after the logs have been rotated.  That way, if
someone wanted to plug in a separate log rotation handler, they could do
so and just call scanlogs on either side of it.  The reporting portions
of scanlogs should be in a separate program.




The current "flushlogs" does:

- ICDwrite (flush history, articles in the storage manager, and active)
- rename the news file
- reopen the news file
- rename the errlog file
- reopen the errlog file
- flush exploder and process channels


We could add "prerotatelogs" and "postrotatelogs" modes (another 
suggestion of naming?) which would do:


prerotatelogs
-------------

- ICDwrite (flush history, articles in the storage manager, and active)


postrotatelogs
--------------

- reopen the news file
- reopen the errlog file
- flush exploder and process channels



Then, scanlogs will call "prerotatelogs" instead of the first 
"flushlogs", rename the news and errlog files like live files, and call 
"postrotatelogs" instead of the second "flushlogs".

Would this suggestion suit you?

Re: double 'ctlinnd flushlogs' deletes news, errlog

John F. Morse — 2013-04-10T15:35:06

Hi Florian,

On 04/10/2013 05:14 AM, Florian Schlichting wrote:


I would prefer the addition to the filename be the ISO 8601 date and time.

This certainly provides more information than ".old" plus making sorts easy,
and would eliminate overwrites.

double 'ctlinnd flushlogs' deletes news, errlog

Florian Schlichting — 2013-04-10T10:14:41

Hi Julien,

I just noticed a critical flaw in your recent commit to flush process /
exploder channels. You introduced a second call to 'ctlinnd flushlogs'
in scanlogs. Unfortunately, when 'ctlinnd flushlogs' is called, innd
doesn't just flush its logs, it also rotates (moves) its log and errlog
to news.old and errlog.old, overwriting what's already there. So the
second call effectively destroys both logs. This happens in
innd/innd.c:ReopenLog().

From ctlinnd(8):

   flushlogs
       Close the news and error log files and rename them to add ".old" to
       the file name, then open fresh news and error logs.  

Perhaps innd should stop do the .old rotation and restrict itself to
flushing, and scanlogs should mv to .old rather than cp, and not just
"in case innd is down", and the then the first invocation of 'ctlinnd
flushlogs' can be deleted?

Florian

Re: innfeed issue

Julien ÉLIE — 2013-03-28T21:23:44

Hi The Doctor,


Same thing.  Empty on Google Groups:
     https://groups.google.com/forum/?fromgroups#!forum/edm.news.stats

Not present on news.individual.net.

Empty on nntp.aioe.org:

GROUP edm.news.stats
211 0 779 778 edm.news.stats
ARTICLE
420 Current article number 0 is invalid

Re: innfeed issue

The Doctor — 2013-03-28T19:47:58


Empty?

Try edm.news.stats then



Yes.

Re: innfeed issue

Julien ÉLIE — 2013-03-28T19:00:55

Dear The Doctor,


This newsgroup has been empty since August 2012 on news.individual.net.
Also empty since June 2012 on Google Groups:
   https://groups.google.com/forum/?fromgroups#!forum/can.canet.stats

Do you feed it to some peers?

Re: Using patterns in incoming.conf for exclusion?

Jesse Rehmer — 2013-03-28T14:20:41

I do have wanttrash set to true.  I wasn't sure if that would affect 
whether or not articles matching the pattern would be accepted or if it 
trumps all other avenues of filtering.
I realize that I will still be accepting the articles, but would prefer 
not to propagate them to downstream peers.  I can control that in the 
newsfeeds file, but in the situation I had yesterday it would have been 
easier to just add an exclude pattern to the one inbound peer who was 
feeding me those unwanted articles.

Cheers,
Jesse

incompréhension totale,

Karl Oman — 2013-03-28T14:01:23

je ne comprends toujours pas comment INN server peut poser autant de
problèmes d'installation, de paramétrage
et de gestion ?!

sous Ubuntu et processeur 64 bits, il est inutile de vouloir installer
l'INN2 proposé dans la logithèque.

pour moi INN devrait être encore plus facile à installer qu'Emacs !!!

alors les programmeurs d'ISC, on se remue les méninges..

fff.

Re: Using patterns in incoming.conf for exclusion?

Florian Schlichting — 2013-03-28T10:46:41

Hi Jesse,

do you have wanttrash set to true in inn.conf?


I don't use incoming patters, but the way newsfeeds work, the
newsgroups line of an article can only be checked *after* that article
has been received. A feeding server either sends all the articles it has
for a peer, or it asks if the peer wants a certain message-id, and then
sends that or not. There is a filter_messageid() function in
filter_innd.pl, but as said, the content of the Newsgroups header are
not known at that time.

Florian

innfeed issue

The Doctor — 2013-03-28T10:42:35

You might want to look again at can.canet.stats to see
if the problem is solved.

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

Florian Schlichting — 2013-03-28T09:55:54

Hi Julien,

thanks for your patch, and sorry for taking so long to really look at
it:


well it does indeed solve the new issue well, but it doesn't touch the
original issue as innfeed in funnel-file mode is a standalone daemon
that doesn't get restartet when innd flushes the funnel-file. So I
revisited your earlier suggestion to add a call to openLogFile() to the
sighup handler (sigusr1 is already used for increasing debug level), and
this is what I'm successfully running with now:

==========
Subject: [PATCH] innfeed, scanlogs: reopen log files on sighup

The recent patch to flush exploder and process channels in innd, and to
send 'ctlinnd flushlogs' again after log rotation solved the problem of
controlchan or ninpaths holding open an already deleted errlog file.
However this does not yet solve the original issue with innfeed.log when
innfeed is run as a standalone daemon via a funnel-file.

This patch implements the changes suggested by Julien Elie in January
2013, to add an openLogFile() call to the sighup handler. The sigusr1
handler is not available, as it is already used to increase / decrease
the debugging level along with sigusr2. Also, reopening the log after
re-reading the configuration might be a good idea anyway, as the log
file location may have been changed.

Additionally, call sighup on the innfeed pid in scanlogs, and document
the changes briefly in innfeed(8) / innfeed.conf(5).
---
 doc/pod/innfeed.conf.pod |  1 +
 doc/pod/innfeed.pod      |  2 +-
 innfeed/main.c           |  3 +++
 scripts/scanlogs.in      | 24 ++++++++++++++++++++++++
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/doc/pod/innfeed.conf.pod b/doc/pod/innfeed.conf.pod
index 3702c12..660c1c3 100644
--- a/doc/pod/innfeed.conf.pod
+++ b/doc/pod/innfeed.conf.pod
< at >< at > -692,6 +692,7 < at >< at > changed (although note that I<bindaddress> and I<bindaddress6> changes
 will only affect new connections).
 
 Any new peers are added and any missing peers have their connections closed.
+The log file is reopened.
 
 =head1 EXAMPLE
 
diff --git a/doc/pod/innfeed.pod b/doc/pod/innfeed.pod
index 3604f2f..ab9bd86 100644
--- a/doc/pod/innfeed.pod
+++ b/doc/pod/innfeed.pod
< at >< at > -141,7 +141,7 < at >< at > on the command line, and will reopen it (see funnel file description above).
 B<innfeed> with catch SIGINT and will write a large debugging snapshot of
 the state of the running system.
 
-B<innfeed> will catch SIGHUP and will reload the config file.
+B<innfeed> will catch SIGHUP and will reload the config and log file.
 See innfeed.conf(5) for more details.
 
 B<innfeed> will catch SIGCHLD and will close and reopen all backlog files.
diff --git a/innfeed/main.c b/innfeed/main.c
index 3b27a65..9851edc 100644
--- a/innfeed/main.c
+++ b/innfeed/main.c
< at >< at > -572,6 +572,9 < at >< at > static void sighup (int sig UNUSED)
     }
 
   configHosts (talkToSelf) ;
+
+  notice ("ME reloading log file %s", logFile) ;
+  openLogFile() ;
 }
 
 static void sigemt (int sig UNUSED)
diff --git a/scripts/scanlogs.in b/scripts/scanlogs.in
index cba8311..225232d 100644
--- a/scripts/scanlogs.in
+++ b/scripts/scanlogs.in
< at >< at > -25,12 +25,18 < at >< at > CONTROLBATCH=${MOST_LOGS}/controlbatch.log
 INNFEEDCONF=${PATHETC}/innfeed.conf
 if [ -f "${INNFEEDCONF}" ]; then
     INNFEEDLOG=`${AWK} '{gsub(/:|#/, " & ")} {if ($1 == "log-file" && $2 == ":") print $3}' ${INNFEEDCONF}`
+    INNFEEDPIDFILE=`${AWK} '{gsub(/:|#/, " & ")} {if ($1 == "pid-file" && $2 == ":") print $3}' ${INNFEEDCONF}`
 fi
 INNFEED=
 for F in "${INNFEEDLOG}" ; do
     test -f "${MOST_LOGS}/${F}" && INNFEED="${INNFEED} ${MOST_LOGS}/${F}"
 done
 test -z "${INNFEED}" && test -f "${MOST_LOGS}/innfeed.log" && INNFEED="${MOST_LOGS}/innfeed.log"
+INNFEEDPID=
+for F in "${INNFEEDPIDFILE}" ; do
+    test -f "${PATHRUN}/${F}" && INNFEEDPID=`cat "${PATHRUN}/${F}"`
+done
+test -z "${INNFEEDPID}" && test -f "${PATHRUN}/innfeed.pid" && INNFEEDPID=`cat "${PATHRUN}/innfeed.pid"`
 NNTPSEND=${MOST_LOGS}/nntpsend.log
 PERLNOCEM=${MOST_LOGS}/perl-nocem.log
 SENDIHAVE=${MOST_LOGS}/send-ihave.log
< at >< at > -121,6 +127,15 < at >< at > if ${ROTATE} ; then
 rm -f ${LOCK}
 exit 1
     fi
+    if [ ! -z $INNFEEDPID ]; then
+        OUTPUT=`kill -HUP $INNFEEDPID 2>&1`
+        if [ "x$OUTPUT" != "x" ]; then
+            echo "$OUTPUT"
+            echo 'Cannot flush innfeed logs.'
+            rm -f ${LOCK}
+            exit 1
+        fi
+    fi
 
     ##  Make sure these .old files exist, in case innd is down.
     for F in ${LOG} ${ERRLOG} ; do
< at >< at > -169,6 +184,15 < at >< at > if ${ROTATE} ; then
         rm -f ${LOCK}
         exit 1
     fi
+    if [ ! -z $INNFEEDPID ]; then
+        OUTPUT=`kill -HUP $INNFEEDPID 2>&1`
+        if [ "x$OUTPUT" != "x" ]; then
+            echo "$OUTPUT"
+            echo 'Cannot flush innfeed logs.'
+            rm -f ${LOCK}
+            exit 1
+        fi
+    fi
 
     ${PAUSED} && ctlinnd -s go "Flushing log and syslog files" 2>&1
 

Florian

Using patterns in incoming.conf for exclusion?

Jesse Rehmer — 2013-03-27T22:14:05

I was wondering if anyone was using the "patterns" directive in 
incoming.conf to limit newsgroups accepted by remote peers?  I've got a 
need to exclude *.sex.* and *.sex newsgroups.  While I'm working with my 
peers to exclude these on their end I found that the patterns directive 
in incoming.conf should be able to do this as well.

I've got the following configured in incoming.conf:

peer neva.ru {
         hostname:            newsfeed.neva.ru
         patterns: "*,!*.sex.*,!*.sex"
}

However, I'm still seeing articles coming in from that peer that match 
the exclude pattern.  Can you not use exclusions with the patterns key 
in incoming.conf?


$ grephistory '51536dbd$0$9626$6c495a8a< at >news.tweak-news.eu'| sm -H
X-Received: by 10.180.98.103 with SMTP id eh7mr2425428wib.3.1364422078652;
         Wed, 27 Mar 2013 15:07:58 -0700 (PDT)
MIME-Version: 1.0
Path: 
usenet.blueworldhosting.com!goblin1!goblin.stu.neva.ru!yt3no10497887wib.0!news-out.google.com!p18ni21607wiv.0!nntp.google.com!feeder1.cambriumusenet.nl!feed.tweaknews.nl!82.197.221.133.MISMATCH!posting.tweaknews.nl!not-for-mail
From: gellebn< at >hotmail.com
Newsgroups: alt.sex.fetish.feet


Cheers,
Jesse

Re: Fwd: Multiple innfeed processes

Florian Schlichting — 2013-03-26T13:46:19

On Mon, Mar 18, 2013 at 08:12:17PM +0100, Julien ÉLIE wrote:

we use innfeed as a standalone daemon in funnel-file mode. While we
don't seem to have any such problem nowadays, shutting down innfeed with
its several hundred feeds has always taken quite some time. The
start/stop script we use for that first sends SIGTERM, then waits for up
to 120 seconds, and finally sends a SIGKILL. Our suspicion was that
innfeed develops a kind of "undead" connection with peers that fail to
respond properly, but never fully debugged it. So the fact that innfeed
now manages to always shut down within 10s here may be related to buggy
peers going out of service as much as bugs fixed between 2.3 and 2.5, or
perhaps with the way the kernel deals with stale network connections?

tl;dr - I'm not surprised, and kill -9 may be regularly needed to
terminate a process that's stuck in i/o

Florian