Re: [bug-mailutils] [PATCHES] various collected patches for comsatd

[Damon Harper]

hi sergey!

> > i'd also love to see a biff command included in the package.  right
> > now i'm using biff from biff+comsat-0.17, which does the trick.  biff
> > is still very useful for temporarily stopping output to a particular
> > tty.  i wouldn't mind writing a new one if it would be included in
> > mailutils.
> 
> Frankly, I don't know if it is worth including. The entire biff program
> is 6 lines of shell code:
> 
> case $1 in
> y)    chmod u+x `tty`;;
> n)    chmod u-x `tty`;;
> '')   ls -l `tty`|sed 's/...x.*/is y/;s/...-.*/is n/';;
> *)    echo "usage: $0 "'[y|n]' >&2;;
> esac
> 
> It can be added as an install target to comsat/Makefile.am, of course.

good point, it's hardly worth coding in c.

> 2. New macros $f and $o.
> 
> It is useful as well. However the code implementing it is rather
> ineffective. Notice, that using libmailutils you can format any
> integer number to a string using mu_umaxtostr function, like this:
> 
>    p = mu_umaxtostr(0, off);
> 
> So, implementing $o is just a matter of adding an extra argument
> to expand_escape and calling this function in the right place of
> the switch.

ah, nice - i wasn't aware of that function.  i didn't really like the
way i implemented it either but it was the quickest way i could think
of with my rather rusty C. ;)

> However, the need to pass two new parameters to expand_line and
> run_user_action just two implement two new macros shows that the
> whole argument passing scheme should be redesigned. Ideally,
> some kind of symbol table should be used.

true, i didn't like adding those new parameters either; however
they're all internal-only functions so it didn't seem like a huge
deal.  it could always be implemented as two extra parameters for now,
but changed to a symbol table sometime down the road if it expands
again - but i don't see there likely being any other useful
information to pass along.

or another simple way to do this might be just to make path and offset
global variables.  i mean, they are pretty fundamental to what comsatd
does, so there'd be an argument for doing this.

if i rewrite the more efficient version with mu_umaxtostr but still
just keep the two extra parameters or make them global variables, will
that be enough for you to include it?

> 3. Resetting access/mod times after comsatd accesses a mailbox
> 
> Well, I'd rather say that a MUA should not depend on the access time
> to tell whether there is some new mail in the mailbox. If it just has to
> use any st_.time, then st_mtime seems to be the proper candidate. Anyway,
> since mutt depends on atime, then what we face is a deeper problem: any
> access to the mailbox using any UNIX utilities will reset mailbox access
> time, which will make mutt believe there is no new mail in that mailbox.

my understanding is that mutt uses this as a matter of efficiency, to
be able to see new mail without having to store any state about the
mailboxes or actually read each mailbox.  so mtime is involved too,
actually.  if mtime is more recent than atime, then there must be new
mail in the folder.

i agree that it's not ideal - i've run into the problem that the
atimes are reset when indexing my mail, etc, and i've had to put
workarounds in place.  it's a pain, but it's a reality, and i agree
with the mutt devs that at least it's a nice, cheap way to tell.

the old biff+comsat in.comsat behaved well in this regards, though
looking at the code it doesn't seem to explicitly reset the atime - it
just fopens the mailbox, seeks and reads, then closes.  does that not
normally set the atime?

all that said, would you consider including the patch, to make
gnu-comsatd behave nicely in this regard?  or is there a better
way/place to implement this (maybe as an option to one of the library
calls?)

> 4. SIGCHLD handling
> 
> Have you tried replacing `signal (SIGCHLD, SIG_IGN)' with `signal
> (SIGCHLD, SIG_DFL)'? It could help with the perl's case. Besides,
> comsat_parent_sig_chld breaks `comsatd -d': the daemon will exit
> after having executed its first child.

yes, i actually figured out that that would work, after i started
implementing my change.  the only issue i can see here is that at
least my method works around the rather ungainly select() `pause' at
the end of comsat_main, since in inetd mode the sigchld will cause the
program to exit immediately if its child exits, rather than waiting
around, then trying to kill a pid that maybe no longer exists.

i didn't think about daemon mode since i don't use it, but sigchld
could simply use the daemon_param variable as a condition to the exit
call.  from my reading, shouldn't there be a wait or waidpid call
anyway, to reap all the child processes spawned by comsat_main, even
in daemon mode?

i found a couple of other problems after sending my last email, i'll
post the patches for those as well, shortly.

-damon