Re: pop3d errno assignments and pthreads.

[Alain Magloire]

Bonjour

> 
> Many functions of the package assign a value to errno variable. This
> is no problem when the package is compiled with --disable-pthread
> option. But when compiled with pthreads this causes grief, since
> in threaded environment errno location is shared between threads.
> For example, in GNU/Linux:

You got this backward, in a multi-thread environment errno is global
to the thread, but local between threads .i.e  errno location is uniq
per thread.

> #if defined(_POSIX_THREAD_SAFE_FUNCTIONS) || defined(_REENTRANT)
> extern int*     __errno_location  __P((void));
> #define errno   (*__errno_location ())
> 
> in Solaris/i86:
> 
> #if (defined(_REENTRANT) || defined(_TS_ERRNO) || \
>         _POSIX_C_SOURCE - 0 >= 199506L) && !(defined(lint) || defined(__lint))
> extern int *___errno();
> #define errno (*(___errno()))

Yes, Phtread provides a per-thread errno.

http://pauillac.inria.fr/~xleroy/linuxthreads/faq.html

> 
> , etc. Thus, any assignment to errno made by child process clobbers
> its value in the parent also. Consequently, the following fragment

I'm not sure I understand your point.  In a process model, a child
can not clobber the value of the parent.  The child has a different
memory space.

> in pop3d.c does not work:
> 
>       connfd = accept (listenfd, (struct sockaddr *)&client, &size);
>       if (connfd == -1)
>         {
>           if (errno == EINTR)
>           continue;
>           syslog (LOG_ERR, "accept: %s", strerror (errno));
>           exit (-1);
>         }
> 
> When accept() gets interrupted by, say SIGCHLD, the value of errno
> is not EINTR, so the master daemon just complains and dies after
> the first child is finished. Possible solution would be

That can not be, if accept was interrupted by a signal errno == EINTR
unless ... it was clobber somewhere like in the signal handler(I'm
not sure about this, I will have to double check).  The other problem
that I can see that you patch address is that signal() is not
restartable, and I should re-arm the signal(SIGCHLD ..) or
use sigaction().


> Index: pop3d/signal.c
> ===================================================================
> RCS file: /cvs/mailutils/pop3d/signal.c,v
> retrieving revision 1.2
> diff -c -r1.2 signal.c
> *** signal.c  2001/01/11 04:46:34     1.2
> --- signal.c  2001/04/22 11:39:33
> ***************
> *** 8,14 ****
>     pid_t pid;
>     int status;
>   
> -   (void)signo;
>     while ( (pid = waitpid(-1, &status, WNOHANG)) > 0)
>         --children;
>   }
> --- 8,15 ----
>     pid_t pid;
>     int status;
>   
>     while ( (pid = waitpid(-1, &status, WNOHANG)) > 0)
>         --children;
> +   signal(signo, pop3_sigchld);
> +   errno = EINTR; 
>   }

In any case your patch is correct :
- make sure we restore errno if it was clobber by waitpid(again I'm not
  sure this is necessary but it will not hurt and the right thing)
- reinstate the signal handler(on some platform, signal() is not
  restartable)

I would prefer to use sigaction() instead of signal() wich will make sure
that the signal is re-arm(POSIX semantics).

Many thanks for catching this.

You also brought to light another point, that is when configuring
with pthread, I should define -D_REENTRANT or similar since some
libraries may have a different behaviour if this is not set.

> 
> But I fear the problem will manifest itself in other places that rely
> on the value of errno. It may be better to use some internal variable
> instead of the global errno, how do you think?

There is no problem, with errno, I think you actually found 2 bugs and
a good fix but for the wrong reasons.

The library takes the same approach as POSIX.1c(Pthreads) meaning instead
of return -1 and setting errno, all functions return a value(int) that
represent an error code when not == 0.  This actually save us from
clobbering the global errno (per thread).


> Cheers,

Many, Thanks

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