4 messages in net.sourceforge.lists.courier-sqwebmailRe: [sqwebmail] some notes about upgr...
FromSent OnAttachments
Kurt BiglerApr 11, 2005 6:43 pm 
Brian CandlerApr 12, 2005 12:34 am 
Paul L. AllenApr 12, 2005 5:02 am 
Kurt BiglerApr 12, 2005 5:58 pm 
Actions with this message:
Paste this link in email or IM:
Paste this link in email or IM:
Atom feed for this thread
Paste this URL into your reader:
Subject:Re: [sqwebmail] some notes about upgrade from sqwebmail 3.5.0Actions...
From:Kurt Bigler (kk@breathsense.com)
Date:Apr 12, 2005 5:58:20 pm
List:net.sourceforge.lists.courier-sqwebmail

on 4/12/05 12:35 AM, Brian Candler <B.Ca@pobox.com> wrote:

On Mon, Apr 11, 2005 at 06:43:37PM -0700, Kurt Bigler wrote:

If there were any problem with the executable not being there, I'd off-hand have expected this error instead:

ispell_run: fork_ispell failed

The fork was successful; it was the exec that failed.

Yes, fork_ispell also takes care of the exec. My meaning was that the function fork_ispell was not really successful, although I suppose that's a matter of definition, and I suppose there currently is none.

But if it's achievable I think fork_ispell should return non-zero if any of the fork or exec functions fail, so that the "fork_ispell failed" error could be given in any of these cases.

The code does say:

execv(ISPELL, (char **)args); perror("fork_ispell: execv " ISPELL);

But perhaps the stderr descriptor isn't available by then. I can't see why through, from a quick look at the code.

Yes, this is not the message that appeared in the log. The one that did (ispell_run: incomplete result from ispell) is output by ispell_run.

Or if you *did* get this message, perhaps it should be changed to

perror("fork_ispell: failed to execv " ISPELL);

Well I didn't get that message but it is certainly an improvement. But it looks like something's not quite right and stderr is not behaving according to what *appears* to be the design.

I can't see the stderr problem yet.

But I do see one thing that might be a potential improvement. Right now fork_ispell is not trying to catch the status values returned by any of the forked processes. My suggestion would be to start by changing wait4pid, currently:

static void wait4pid() { int waitstat;

if (prevsighandler == SIG_DFL) while ( wait(&waitstat) != ispell_pid) ; }

to this:

static int wait4pid() { int waitstat;

if (prevsighandler == SIG_DFL) { while ( wait(&waitstat) != ispell_pid) ; } else waitstat = ???; /*not sure what to do here*/ return waitstat; }

and then make all the relevant status values passed to exit unique within fork_ispell, and then have fork_ispell utilize the value returned by wait4pid, perhaps as the function return value at the bottom of the function, instead of 0.

There is still something missing from this picture, because there are two levels of fork, but only one wait on the single ispell_pid. But that's enough said from my side for starters.

-Kurt