Jeroen van Wolffelaar wrote:
> Can you please bounce this mail to the bug log? I didn't want to do so
> because this is private mail, but this really really should be part of
> the bug log.
>
> On Wed, Mar 28, 2007 at 12:13:39AM +0200, José Luis Tallón wrote:
>
>> Jeroen van Wolffelaar wrote:
>>
>>> On Tue, Mar 27, 2007 at 10:54:58PM +0200, José Luis Tallón wrote:
>>>
>>>
>>>> Jeroen van Wolffelaar wrote:
>>>>
>>>>
>>>>> Package: imapproxy
>>>>> Version: 1.2.4-10
>>>>> Severity: important
>>>>>
>>>>> The pid-handling of imapproxy is pretty fragile, as documented in
>>>>> #369020 amongst others. The current workaround of writing a new pidfile
>>>>> after start based on 'ps ax' output is, eh, fragile at best, and
>>>>> actually pretty bad.
>>>>>
>>>>> The proper solution would be to patch imapproxy so that it writes out a
>>>>> pidfile itself, like proper daemons should.
>>>>>
>>>>>
>>>> Fixed. Will submit the patch ASAP
>>>>
>>>>
>>> Why didn't you attach it? I'm pretty busy tomorrow, and want to get this
>>> fixed ASAP. I mean, the plan is to release this weekend...
>>>
>>>
>> I was finishing testing. Everything seems to be fine now :-)
>>
>> Please check and re-check as you wish.
>> (Vorlon, if you possibly can, do that also)
>>
> 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)
>> 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
> 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...
> I also made pidfile creation predictable -- always create one when
> running in background mode, instead of only in some cases.
>
OK
>> - updated Usage() to reflect new option
>>
>
> I updated the manpage too.
>
Thanks. I missed that.
>> - modified initscript. Vastly simplified logic.
>> -- I also added a couple more "cosmetic" changes: DEFAULT ->
>> DEFAULTS; test -f -> test -x;
>>
>
> Reverted, not related to the RC bug.
>
OK
>> 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.
>> The behaviour upon some, certain, connection failures is a bit annoying
>> (upstream), but I can't change those.
>>
>
> If you mean that start simply hangs indefinitely there, sorry, that's
> not just 'a bit annoying', but a showstopper, and actually introduced by
> your change to where to Daemonize().
>
ACK. I didn't notice the looping before daemonizing, or else I wouldn't
have moved that.
Attempting to reconnect once in the background does make sense, though
>> Comments? I'll be up until late...
>>
>
> This late :)?
>
Weeeell... not today :-\
Thanks again,
J.L.
