Jari Aalto
2009-Aug-16 09:40 UTC
[Logcheck-devel] Bug#322054: [PATCH] Add GNU --long options support (for scripts)
Hi, Could you brief me if these patches in BTS could be integrated to next logcheck release. http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=27;filename=0001-Change-code-remove-getopt-to-support-long-option.patch;att=1;bug=322054 http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=27;filename=0001-docs-logcheck.sgml-Document-new-long-options.patch;att=2;bug=322054 After testing, they apply without problems to the latest logcheck GIT repository. Thanks, Jari
Frédéric Brière
2009-Aug-24 17:52 UTC
[Logcheck-devel] Bug#322054: [PATCH] Add GNU --long options support (for scripts)
On Sun, Aug 16, 2009 at 12:40:21PM +0300, Jari Aalto wrote:> Could you brief me if these patches in BTS could be integrated to next > logcheck release.You're a very patient man, Jari. :) I've got some bugs and comments on your patch. First, the bugs: - Your second while loop will run endlessly and never exit. - Most of your options are missing a shift. Incidentally, try to keep the shifts at the end of each case, for consistency. - logcheck currently does two passes of the arguments list, so that some options are handled before the others. Your patch only does one pass, and requires that those options appear first on the command line. And now the comments: - Suddenly changing the -v option might not be seen as very nice. (Although I doubt there are many scripts out there asking for logcheck's version, it's best to play it safe.) I would suggest adding the -V|--version option, but keeping -v for compatibility, perhaps deprecated. - The --server option is inconsistent with the other two similar options. Given the rather long names, I would suggest a --reportlevel option that takes 'paranoid', 'server' or 'workstation' as argument. - I'd prefer --hostname to --host; it makes it clearer that we're not connecting to that host or anything. - --log sounds like an action instead of an option. How about --logfile and --logfiles-list? (Although --logfile typically means logging *to* that file, so I welcome any suggestion.) - I'm ambivalent on --email-subject-reboot; what about simply --reboot? (Maybe logcheck will eventually do more than change the subject on reboots, who knows.) - Good catch on $CONFFILE not existing. All it needs is a little quoting, actually; [ -r "" ] will return false. - There are many unrelated whitespace changes, which should be kept separate. (I'll take your suggestion and try to apply them beforehand.) - No need to submit two patches; go ahead and fix both the script and documentation in one swoop. Thank you for your work! -- /* * Please skip to the bottom of this file if you ate lunch recently * -- Alan */ -- from Linux kernel pre-2.1.91-1