Re: POP3d locking

[Alain Magloire]

Bonjour

> Hello,
> 
> Sorry for being late with the answer.

Not a problem: Summer time ... and the living is easy.

....

> > So I left out this portion of the patch.
> Sure. You're right. I didn't think about it. But then we need to call
> pop3d_touchlock() right after reading a next line, like that:
> 
> Index: pop3d/pop3d.c
> ===================================================================
> RCS file: /cvs/mailutils/pop3d/pop3d.c,v
> retrieving revision 1.27
> diff -c -b -w -r1.27 pop3d.c
> *** pop3d/pop3d.c     2001/05/02 04:45:12     1.27
> --- pop3d/pop3d.c     2001/05/03 10:29:58
> ***************
> *** 303,308 ****
> --- 304,311 ----
>           pop3d_abquit (ERR_MBOX_SYNC); /* Out of sync, Bail out.  */
>       }
>   
> +       pop3d_touchlock ();
> +       
>         if (strlen (arg) > POP_MAXCMDLEN || strlen (cmd) > POP_MAXCMDLEN)
>       status = ERR_TOO_LONG;
>         else if (strlen (cmd) > 4)
> 
> 
> Thus we could keep the lock 'warm' all the time. What do you think?

Applied.

> > > 3. Current implementation of pop3d_signal would cause main process
> > >    to attempt to perform fprintf on NULL file pointer (ofile). Sure,
> > >    the main process should not crash, but one never knows... So I
> > >    have modified pop3d_signal to handle this appropriately.
> > 
> > Do not see why:
...
> > 
> > This pretty much guaranty that ofile will not be NULL and if it is
> > the server will immediately exit.
> Not quite so, I believe. This guarranty that ofile will not be NULL in
> the *child* process:
..
> 
> whereas in the parent one, ofile will always be NULL, since it lives
> in the BSS (FILE *ofile;) and gets initialized only in the child
> process. Thus, when the parent receives any signal, SIGTERM for
> example, it does pop3d_abquit(ERR_SIGNAL) and dies doing
> fprintf(ofile, ...

Absolutely!
Patch applied.

> > Questons:
> > - Is PIPELINING widely use in pop3d clients?
> I don't suppose so. Anyway, I believe it is a good idea to implement
> it. But I would rather propose to use streams instead of trying
> to do that manually, i.e.:
> 
> in pop3d_mainloop():
> 
>       FILE *fp;
> 
>       fp = fdopen(ifile, "r");
> 
> 
> then, in pop3d_readline, we could just do fgets(buf, sizeof(buf), fp)
> without worrying about buffering stuff. We will not be able, however,
> to use select(), but a signal handler for SIGALRM will do the work
> perfectly. The pop3d_readline() could be like that:
> 
...
> 
> What do you think?

Ok.
I have no strong feeling about this.  The original code was provided
by Jakob from GNU pop3d.  I do not know about his rationnale of not
using fgets() but I do not see any reasons against.
Code accepted with some modifications.

> Besides, there is a problem in the currently proposed implementation
> of pop3d_readline():
> 
> at the first invocation of the function memchr(buffer, '\n', total)
> is called upon NULL pointer (buffer = NULL at the start of the
> fuction).

Annoying, there is no reason for memchr() to do anything since
total == 0.  However the man pages nor the std back me up on this 8-)

> This makes program coredump on Solaris (and possibly
> on other SYSV-like systems).
> 
> Maybe this would be better:

We could fall back to this if Jakob(Kaivo) is complainning about
the way we rearrange is pop3d_readline() 8-).

... 
> 
> Another question: the removal of closelog(), openlog() from the pam part
> of user.c seems to cause problems. Unfortunately most of the pam modules
> do openlog() on their own, thus after authentication all logging
> output flows to where the last pam module has directed it, which
> is usually `auth' facility. May be we would better keep the closelog()
> openlog() pair? or at least make it somehow configuration-dependent?

Ok, it's back

-- 
au revoir, alain
----
Aussi haut que l'on soit assis, on est toujours assis que sur son cul !!!