FS#29769 - [backuppc] Init script fails silently

Attached to Project: Community Packages
Opened by Jacob Joseph (jacobjjoseph.org) - Saturday, 05 May 2012, 20:13 GMT
Last edited by Sébastien Luttringer (seblu) - Thursday, 10 May 2012, 21:49 GMT
Task Type Bug Report
Category Packages
Status Closed
Assigned To Sébastien Luttringer (seblu)
Architecture All
Severity Medium
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

The backuppc init script fails to start when the configuration file contains errors, and does not print the errors. They are not logged, either. The issue is that the init script does not print the errors, due to an over-aggressive "&> /dev/null" after the start-stop-daemon. Only after removing this was I able to debug the actual issue. (It turns out that I had configured a mistaken $Conf{SendmailPath} = '').

I believe the redirection to /dev/null should be removed. The start-stop-daemon --quiet should be sufficient. If there are messages that one would prefer not be printed as a matter of course, pipe through a "grep -v" for each of them. I find it frustrating when the init script gets in the way of a program that has otherwise fine output.

(More generally, and too much so for this bug, is there a policy in Arch about handling output from programs in init scripts?)

Thanks much.
~Jacob
This task depends upon

Closed by  Sébastien Luttringer (seblu)
Thursday, 10 May 2012, 21:49 GMT
Reason for closing:  Fixed
Additional comments about closing:  fix in backuppc-3.2.1-6. Thanks to Dave Reisner.
Comment by Sébastien Luttringer (seblu) - Sunday, 06 May 2012, 16:48 GMT
I think you understood that it is to delete the following error messages.

Use of qw(...) as parentheses is deprecated at /usr/share/backuppc/lib/BackupPC/Storage/Text.pm line 301.
Use of qw(...) as parentheses is deprecated at /usr/share/backuppc/lib/BackupPC/Lib.pm line 1412.

About grep, it's not serious. The best way to avoid those messages would to provide a patch upstream.

I agree we should remove redirection when those lines are gone.
Comment by Jacob Joseph (jacobjjoseph.org) - Monday, 07 May 2012, 19:12 GMT
Sebastien,
You're right that the two deprecation messages could be a slight nuisance. However, I believe removing *ALL* useful output on account of a two verbose warnings is misguided. To me, it is a bug when the init script does not behave as intended, which is the case here, and the reason for my filing.

I propose that this script be fixed by purposely ignoring the messages:
2>&1 | grep -v "parentheses is deprecated"

In any case, the redirection should be removed. It masks meaningful errors.

An upstream fix is likely warranted, too.

~Jacob
Comment by Sébastien Luttringer (seblu) - Tuesday, 08 May 2012, 16:14 GMT
Would you contact upstream about fixing those warnings?

A lot of initscripts redirect stdout and stderr to dave as daemons have to drop those fd to be properly daemonized.
Daemons must remove their links to its launching console. Here you suggest to link it to a shell pipe.
Besides that, using your grep suggestion, initscripts always return a failure (i don't look why).

In you case, you make mistake with configuration and initscripts show you a failure. This is the expected behaviour.
As many daemon debugging you should look into his log file (/var/log/backuppc/LOG) to fix you issue, or start it manually disabling daemon.
You can also look suggestion from systemd about this.
Comment by Jacob Joseph (jacobjjoseph.org) - Tuesday, 08 May 2012, 16:58 GMT
Rather than redirecting to /dev/null, perhaps the correct behavior would be to redirect to a file, or write to the syslog. In the case I experienced, /var/log/backuppc/LOG was *not* written.
Comment by Sébastien Luttringer (seblu) - Tuesday, 08 May 2012, 17:09 GMT
Exact, if there is issue in config file, nothing is written into /var/log/backuppc. Launching daemon (with foreground) manually lets the best way to debug this.

Jacob, the best way to cleanly remove those redirection (when daemon is not started with @) is to report this issue upstream and have a fix.
Comment by Jacob Joseph (jacobjjoseph.org) - Tuesday, 08 May 2012, 17:21 GMT
Upstream appears already aware of the deprecation messages:
http://sourceforge.net/mailarchive/forum.php?thread_name=4EEE5465.5050603%40unige.ch&forum_name=backuppc-users

Also, I don't quite follow your comment about daemons disconnecting from the console stdout/stderr. BackupPC, of course, does disconnect from STDIN et. al, but only after potentially printing some other very important messages. Consider:

-------------------------
sub Main_Initialize
{
umask($Conf{UmaskMode});

#
# Check for another running process, verify executables are configured
# correctly and make sure $TopDir is on a file system that supports
# hardlinks.
#
if ( $Info{pid} ne "" && kill(0, $Info{pid}) ) {
print(STDERR $bpc->timeStamp,
"Another BackupPC is running (pid $Info{pid}); quitting...\n");
exit(1);
}

foreach my $progName ( qw(SmbClientPath NmbLookupPath PingPath DfPath
SendmailPath SshPath) ) {
next if ( $Conf{$progName} eq "" || -x $Conf{$progName} );
print(STDERR $bpc->timeStamp,
"\$Conf{$progName} = '$Conf{$progName}' is not a"
. " valid executable program\n");
exit(1);
}

if ( !$bpc->HardlinkTest("$TopDir/pc", "$TopDir/cpool") ) {
print(STDERR $bpc->timeStamp, "Can't create a test hardlink between a file"
. " in $TopDir/pc and $TopDir/cpool. Either these are different"
. " file systems, or this file system doesn't support hardlinks,"
. " or these directories don't exist, or there is a permissions"
. " problem, or the file system is out of inodes or full. Use"
. " df, df -i, and ls -ld to check each of these possibilities."
. " Quitting...\n");
exit(1);
}

if ( $opts{d} ) {
#
# daemonize by forking; more robust method per:
# http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=301057
#
my $pid;
defined($pid = fork) or die("Can't fork: $!");
exit if ( $pid ); # parent exits

POSIX::setsid();
defined($pid = fork) or die("Can't fork: $!");
exit if $pid; # parent exits

chdir ("/") or die("Cannot chdir to /: $!\n");
close(STDIN);
open(STDIN , ">/dev/null") or die("Cannot open /dev/null as stdin\n");
# STDOUT and STDERR are handled in LogFileOpen() right below,
# otherwise we would have to reopen them too.
}

#
# Open the LOG file and redirect STDOUT, STDERR etc
#
LogFileOpen();
....
--------------------------------------------

I argue that dropping all output to /dev/null can never be an appropriate solution for an init script.

~Jacob
Comment by Sébastien Luttringer (seblu) - Tuesday, 08 May 2012, 20:54 GMT
> http://sourceforge.net/mailarchive/forum.php?thread_name=4EEE5465.5050603%40unige.ch&forum_name=backuppc-users
I doesn't see any patch in backuppc-devel which i can include prior to next release.

> but only after potentially printing some other very important messages.
Warnings are not important message.

> I argue that dropping all output to /dev/null can never be an appropriate solution for an init script.
Never? Cons-example: Look the code of parallel daemons (@) in our initscripts project.

Jacob, we agree together, this redirection should not be here. Let's be consistent, either all outputs are removed or nothing is removed.
We don't want those useless warnings, so all is removed. Please provide a patch to fix the warning display, and i will drop the redirection.

Cheers,
Comment by Jacob Joseph (jacobjjoseph.org) - Wednesday, 09 May 2012, 03:24 GMT
>> but only after potentially printing some other very important messages.
>Warnings are not important message.

All of the messages described in the opening stanza of the BackupPC code I pasted are definitely not warnings. These are all followed by exit(1). They are absolutely important. Further, the error that prompted me to open this bug was not reported, as it should have been.

We agree to a point. However, I believe the position you are taking is in direct contrast to the "Code Correctness over Convenience" clause of the Arch Way document: https://wiki.archlinux.org/index.php/The_Arch_Way. Sure, redirecting to /dev/null may be a quick way to get rid of a few harmless warnings, but it masks important behavior--behavior that prevented me from understanding the function of the script. No, I don't agree that this should be debugged by skipping the init script, save as a very last resort. I encourage you to remove the redirection now, and resolve the warnings if need-be in a more robust manner.

~Jacob
Comment by Sébastien Luttringer (seblu) - Wednesday, 09 May 2012, 12:52 GMT
I doesn't share you point with "Code correctess vs Convenience". Using grep to remove those warning if for convenience. Code correctness is at least to be consistent. Remove all output (like on others daemons) or not (like on others daemons).

Here we agree on:
- remove redirection
- remove warnings prints

So please, would you help archlinux to fix this by providing a patch for warnings or deal with upstream to do it.
Comment by Sébastien Luttringer (seblu) - Wednesday, 09 May 2012, 14:13 GMT

Loading...