On Wed, Mar 28, 2007 at 08:26:27AM +0200, José Luis Tallón wrote: > Jeroen van Wolffelaar wrote: > > Uploaded as NMU, because I actually made some changes. > > > Reviewed them. Gotta ACK, anyway. > (You are right in the changes you made, I would have done it differently)
That's of course fine, your decision as maintainer. > >> Changes: > >> > >> - function "Daemonize ( const char* pidfile );" > >> > > > > Fine. I did put the function back where it was though, instead of moving > > it way below, because the stuff in between can hang (for example, when > > failing to connect, it'll indefinitely retry every 15 seconds, causing > > 'start' to hang). > > > Yup. I noticed that. > From my POV, it is related to a "misconfiguration" of the server --- > being configured to connect to a nonexistent/malfunctioning IMAP server. > However, I do realize it could hang the booting process, so it should be > fixed. > > ACK to your solution It's a misconfiguration, sure, but one that happens on default install (out of the box imapproxy is misconfigured? Well, it was in my case, but probably related to the fact that I don't actually have any imap server running on my test machine :) In any case, yeah, the problem is that start should simply never hang. > > Moving Daemonize to later on was not related to this RC bug. Putting it > > in its own function isn't either, but is pretty harmless, so ok. > > > > It does allow to move where Daemonize is called easily ;) > >> - added support for the "-p pidfile" option > >> > > > > Cool. There was a buglet here -- you didn't initialize the var and then > > only overwrote it with the default if it started with a nulbyte. I > > changed that to simply always initialize it to the default. > > > Indeed. I know better than to have a variable unitialized, BUT I > followed upstream's style here (the same pattern as with the configfile) > -- I am aiming for inclusion of this by now HUGE patch, anyway... Yes, but in that case, you failed to properly copy&paste the full thing -- upstream *does* set the first byte of ConfigFile to \0, you didn't copy that bit. You also didn't completely copy the logging to be analogous. > >> I have tested starting / restarting / stopping / re-stopping > >> > > > > Added code to not actually fail on second start / fail on second stop > > that I had already prepared. It now should really work fine in all > > cases. > > > > I also removed dead code from checking the exit state of a 'true'. I > > removed the "|| true" so that the script just fails right there when it > > eh, fails. > > > ACK. I will re-check the Developer's Reference for mandated/recommended > messages and initscript behaviour in those cases not covered by the Policy. I'm not so sure either dev-ref or policy are very verbose on this, it'd be good for someone to look into this at some point... But world peace would also be nice :). > >> Comments? I'll be up until late... > >> > > > > This late :)? > > > Weeeell... not today :-\ Tsk tsk :-P. Anyway, thanks a lot for your work on the pidfile handling, it was convenient to only need to verify, and not really write it all. --Jeroen -- Jeroen van Wolffelaar [EMAIL PROTECTED] (also for Jabber & MSN; ICQ: 33944357) http://Jeroen.A-Eskwadraat.nl -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
