Re: incorrect use of pidfile(3)

On Thu, Oct 13, 2011 at 02:54:16PM +0200, Dag-Erling Smørgrav wrote:
> After discussing this with pjd_at_ on IRC, I arrived at the attached patch,
> which increases the length of time pidfile_open() itself waits (I hadn't
> noticed that it already looped) and sets *pidptr to -1 if it fails to read
> a pid.

I'm still in opinion that EWOULDBLOCK and EAGAIN (which is the same
value on FreeBSD) should be converted to EEXIST on pidfile_open()
return.

Also if we now have for loop, why not to put count in there?

I'm not very happy about touching pidptr in case of error other than
EEXIST. This is not documented, but a bit unexpected anyway.

I'm happy with increasing the timeout.

> Index: lib/libutil/pidfile.c
> ===================================================================
> --- lib/libutil/pidfile.c	(revision 226271)
> +++ lib/libutil/pidfile.c	(working copy)
> _at__at_ -118,22 +118,19 _at__at_
>  	 */
>  	fd = flopen(pfh->pf_path,
>  	    O_WRONLY | O_CREAT | O_TRUNC | O_NONBLOCK, mode);
> -	if (fd == -1) {
> -		count = 0;
> +	if (fd == -1 && errno == EWOULDBLOCK && pidptr != NULL) {
> +		*pidptr = -1;
> +		count = 20;
>  		rqtp.tv_sec = 0;
>  		rqtp.tv_nsec = 5000000;
> -		if (errno == EWOULDBLOCK && pidptr != NULL) {
> -		again:
> +		for (;;) {
>  			errno = pidfile_read(pfh->pf_path, pidptr);
> -			if (errno == 0)
> -				errno = EEXIST;
> -			else if (errno == EAGAIN) {
> -				if (++count <= 3) {
> -					nanosleep(&rqtp, 0);
> -					goto again;
> -				}
> -			}
> +			if (errno != EAGAIN || --count == 0)
> +				break;
> +			nanosleep(&rqtp, 0);
>  		}
> +		if (errno == 0)
> +			errno = EEXIST;
>  		free(pfh);
>  		return (NULL);
>  	}

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://yomoli.com