Bug#224506: [Pkg-awstats-devel] Bug#224506: cronjob: locking, conf/logfile checking

[Jonas Smedegaard]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Arne Rusek wrote:

> I wrote myself a awstats cron wrapper which should solve at least three
> bugreports on awstats :-)
> 
> It does:
>   * locking: no more than one instance -- one configuration -- will run
> 	* config checking: check required config file (and included confs)
> 	* check that required logfile is available
> 	* should work out of the box when run from /etc/cron.d/awstats
> 	* it also makes awstats more nice :-)
> 
> If it's of any use, please include it into the awstats package and run
> from cron. Questions, comments, suggestions and complains are welcomed
> as always.

Excellent work.

I do actually have some comments:

  * Don't ever use a static filename below /tmp.

Use mktemp instead!


  * Don't allow variables being overridded in ENV.

Instead first declare the variables as plain static values, and then
source some config file. Have a look at /etc/init.d/skeleton for an example.


  * Always quote strings containing variables!


  * Don't invoke script itself without safety belt against loops

It might not work within awstats either, but it seems to me that your
little script will loop forever if an include file includes itself.


  * Avoid bashisms if possiple

I don't use the "local" keyword myself, so don't know if it is a bashism.

You can drop the "function" keyword with no ill effect.

Change combined tests from "[ A -a B ]" to "[ A ] && [ B ]".

Test if the script (with bashisms removed) works with dash, and if it
does then change the hashbang from #!/bin/bash to #!/bin/sh



  * Are you sure about your quotes within quotes within quotes?

They seem risky to me.


  * Why invent the wheel?

Why use custom lock routine and not either lockfile (from the procmail
package) or preferrably the tools from thwe package liblockfile-progs?



  * Parsing perl code using shell?

What about ignoring stuff below __EOF__ (or whatever the magic perl
string is)?

Are you sure there's no other surprises when you try interpreting perl
code using shell code?

I mean - you are proposing this as a general tool, so it should work for
all, not just your own coding style.



Hope I am not too hard on you :-)



Charles Fry wrote:
> Also, could you suggest where the script should be installed? /usr/sbin?
> 
> And named? awstats-run?

(Please let's work in the open, Charles!)

I agree with /usr/sbin as location.

I suggest calling it update-awstats, to follow the trend of other
update-* scripts.



But I feel that it need more work to be usable. Seems too risky to me to
apply by default as is.


 - Jonas

- --
* Jonas Smedegaard - idealist og Internet-arkitekt
* Tlf.: +45 40843136  Website: http://dr.jones.dk/

 - Enden er nær: http://www.shibumi.org/eoti.htm
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFx1pDn7DbMsAkQLgRAtIxAJ99b3m8COdxSVbyvEQ3ySQF26PhfACeNFwG
M76x3a0J5kDSwyMX6dPsSuA=
=P4kT
-----END PGP SIGNATURE-----