Subject: Re: [need review] pax(1) change to fix PR #8868
To: None <tech-userlevel@netbsd.org>
From: David Brownlee <abs@netbsd.org>
List: tech-userlevel
Date: 02/10/2000 02:19:08
	Just curious - can the SIGINFO hack cause problems if pax is
	being used to read/write to an actual tape device?

	Also - you might want to cc gnats-bugs and start the subject
	with bin/8868: to have this thread automatically added to the
	PR :)


		David/absolute

On Thu, 10 Feb 2000, ITOH Yasufumi wrote:

> I'd like to make this modification to fix PR #8868 (pax(1) fails on SIGINFO).
> Please review.
> 
> The cause of the problem is that if the SIGINFO is caught
> in the middle of write(2) system call, write() does not
> continue the operation after the signal and returns with
> partial operation.
> 
> The signal(3) manpage says that the read(2) may also does
> partial operation on signals.
> 
> To fix the problem, we should either
>  1. disable SIGINFO hack, or
>  2. retry every partial read(2)/write(2).
> 
> The following changes implements 2.
> 
> On 4.2BSD and later BSD systems, read(2)/write(2) is
> restarted if no operation performed yet (not even partial),
> and the "#ifdef SYS_NO_RESTART" parts are not required.
> They are to help porting to other systems.
> 
> OK to commit this?
> 
> Regards,
> -- 
> ITOH, Yasufumi <itohy@netbsd.org>
> 
> diff -uF^[a-zA-Z_][a-z 	A-Z0-9_]*(.*[^;]$ ar_io.c.orig ar_io.c
> --- ar_io.c.orig	Mon Oct 25 08:16:15 1999
> +++ ar_io.c	Thu Feb 10 07:59:13 2000
> @@ -438,7 +438,7 @@ ar_drain()
>  	/*
>  	 * keep reading until pipe is drained
>  	 */
> -	while ((res = read(arfd, drbuf, sizeof(drbuf))) > 0)
> +	while ((res = read_with_restart(arfd, drbuf, sizeof(drbuf))) > 0)
>  		;
>  	lstrval = res;
>  }
> @@ -518,6 +518,132 @@ ar_app_ok()
>  	return(-1);
>  }
>  
> +#ifdef SYS_NO_RESTART
> +/*
> + * read_with_restart()
> + *	Equivalent to read() but does retry on signals.
> + *	This function is not needed on 4.2BSD and later.
> + * Return:
> + *	Number of bytes written.  -1 indicates an error.
> + */
> +
> +#if __STDC__
> +int
> +read_with_restart(int fd, void *buf, int bsz)
> +#else
> +int
> +read_with_restart(fd, buf, bsz)
> +	int fd;
> +	void *buf;
> +	int bsz;
> +#endif
> +{
> +	int r;
> +
> +	while (((r = read(fd, buf, bsz)) < 0) && errno == EINTR)
> +		;
> +
> +	return(r);
> +}
> +#endif
> +
> +/*
> + * xread()
> + *	Equivalent to read() but does retry on partial read, which may occur
> + *	on signals.
> + * Return:
> + *	Number of bytes read.  0 for end of file, -1 for an error.
> + */
> +
> +#if __STDC__
> +int
> +xread(int fd, void *buf, int bsz)
> +#else
> +int
> +xread(fd, buf, bsz)
> +	int fd;
> +	void *buf;
> +	int bsz;
> +#endif
> +{
> +	char *b = buf;
> +	int nread = 0;
> +	int r;
> +
> +	do {
> +		if ((r = read_with_restart(fd, b, bsz)) <= 0)
> +			break;
> +		b += r;
> +		bsz -= r;
> +		nread += r;
> +	} while (bsz > 0);
> +
> +	return(nread ? nread : r);
> +}
> +
> +#ifdef SYS_NO_RESTART
> +/*
> + * write_with_restart()
> + *	Equivalent to write() but does retry on signals.
> + *	This function is not needed on 4.2BSD and later.
> + * Return:
> + *	Number of bytes written.  -1 indicates an error.
> + */
> +
> +#if __STDC__
> +int
> +write_with_restart(int fd, void *buf, int bsz)
> +#else
> +int
> +write_with_restart(fd, buf, bsz)
> +	int fd;
> +	void *buf;
> +	int bsz;
> +#endif
> +{
> +	int r;
> +
> +	while (((r = write(fd, buf, bsz)) < 0) && errno == EINTR)
> +		;
> +
> +	return(r);
> +}
> +#endif
> +
> +/*
> + * xwrite()
> + *	Equivalent to write() but does retry on partial write, which may occur
> + *	on signals.
> + * Return:
> + *	Number of bytes written.  -1 indicates an error.
> + */
> +
> +#if __STDC__
> +int
> +xwrite(int fd, void *buf, int bsz)
> +#else
> +int
> +xwrite(fd, buf, bsz)
> +	int fd;
> +	void *buf;
> +	int bsz;
> +#endif
> +{
> +	char *b = buf;
> +	int written = 0;
> +	int r;
> +
> +	do {
> +		if ((r = write_with_restart(fd, b, bsz)) <= 0)
> +			break;
> +		b += r;
> +		bsz -= r;
> +		written += r;
> +	} while (bsz > 0);
> +
> +	return(written ? written : r);
> +}
> +
>  /*
>   * ar_read()
>   *	read up to a specified number of bytes from the archive into the
> @@ -550,7 +676,7 @@ ar_read(buf, cnt)
>  	 */
>  	switch (artyp) {
>  	case ISTAPE:
> -		if ((res = read(arfd, buf, cnt)) > 0) {
> +		if ((res = read_with_restart(arfd, buf, cnt)) > 0) {
>  			/*
>  			 * CAUTION: tape systems may not always return the same
>  			 * sized records so we leave blksz == MAXBLK. The
> @@ -588,7 +714,7 @@ ar_read(buf, cnt)
>  		 * and return. Trying to do anything else with them runs the
>  		 * risk of failure.
>  		 */
> -		if ((res = read(arfd, buf, cnt)) > 0) {
> +		if ((res = read_with_restart(arfd, buf, cnt)) > 0) {
>  			io_ok = 1;
>  			return(res);
>  		}
> @@ -637,7 +763,7 @@ ar_write(buf, bsz)
>  	if (lstrval <= 0)
>  		return(lstrval);
>  
> -	if ((res = write(arfd, buf, bsz)) == bsz) {
> +	if ((res = xwrite(arfd, buf, bsz)) == bsz) {
>  		wr_trail = 1;
>  		io_ok = 1;
>  		return(bsz);
> @@ -1064,7 +1190,7 @@ get_phys()
>  		 * we know we are at file mark when we get back a 0 from
>  		 * read()
>  		 */
> -		while ((res = read(arfd, scbuf, sizeof(scbuf))) > 0)
> +		while ((res = read_with_restart(arfd, scbuf, sizeof(scbuf))) > 0)
>  			padsz += res;
>  		if (res < 0) {
>  			syswarn(1, errno, "Unable to locate tape filemark.");
> @@ -1093,16 +1219,16 @@ get_phys()
>  		syswarn(1, errno, "Unable to backspace over last tape block.");
>  		return(-1);
>  	}
> -	if ((phyblk = read(arfd, scbuf, sizeof(scbuf))) <= 0) {
> +	if ((phyblk = read_with_restart(arfd, scbuf, sizeof(scbuf))) <= 0) {
>  		syswarn(1, errno, "Cannot determine archive tape blocksize.");
>  		return(-1);
>  	}
>  
>  	/*
> -	 * read foward to the file mark, then back up in front of the filemark
> +	 * read forward to the file mark, then back up in front of the filemark
>  	 * (this is a bit paranoid, but should be safe to do).
>  	 */
> -	while ((res = read(arfd, scbuf, sizeof(scbuf))) > 0)
> +	while ((res = read_with_restart(arfd, scbuf, sizeof(scbuf))) > 0)
>  		;
>  	if (res < 0) {
>  		syswarn(1, errno, "Unable to locate tape filemark.");
> diff -uF^[a-zA-Z_][a-z 	A-Z0-9_]*(.*[^;]$ buf_subs.c.orig buf_subs.c
> --- buf_subs.c.orig	Mon Oct 25 08:16:15 1999
> +++ buf_subs.c	Wed Feb  9 22:40:45 2000
> @@ -699,7 +699,7 @@ wr_rdfile(arcn, ifd, left)
>  			return(-1);
>  		}
>  		cnt = MIN(cnt, size);
> -		if ((res = read(ifd, bufpt, cnt)) <= 0)
> +		if ((res = read_with_restart(ifd, bufpt, cnt)) <= 0)
>  			break;
>  		size -= res;
>  		bufpt += res;
> @@ -884,10 +884,10 @@ cp_file(arcn, fd1, fd2)
>  	 * read the source file and copy to destination file until EOF
>  	 */
>  	for(;;) {
> -		if ((cnt = read(fd1, buf, blksz)) <= 0)
> +		if ((cnt = read_with_restart(fd1, buf, blksz)) <= 0)
>  			break;
>  		if (no_hole)
> -			res = write(fd2, buf, cnt);
> +			res = xwrite(fd2, buf, cnt);
>  		else
>  			res = file_write(fd2, buf, cnt, &rem, &isem, sz, fnm);
>  		if (res != cnt)
> diff -u extern.h.orig extern.h
> --- extern.h.orig	Tue Nov  2 16:13:03 1999
> +++ extern.h	Wed Feb  9 22:25:46 2000
> @@ -57,6 +57,15 @@
>  void ar_drain __P((void));
>  int ar_set_wr __P((void));
>  int ar_app_ok __P((void));
> +#ifdef SYS_NO_RESTART
> +int read_with_restart __P((int, void *, int));
> +int write_with_restart __P((int, void *, int));
> +#else
> +#define read_with_restart	read
> +#define write_with_restart	write
> +#endif
> +int xread __P((int, void *, int));
> +int xwrite __P((int, void *, int));
>  int ar_read __P((char *, int));
>  int ar_write __P((char *, int));
>  int ar_rdsync __P((void));
> diff -uF^[a-zA-Z_][a-z 	A-Z0-9_]*(.*[^;]$ tables.c.orig tables.c
> --- tables.c.orig	Tue Nov  2 16:13:04 1999
> +++ tables.c	Wed Feb  9 12:14:26 2000
> @@ -446,7 +446,7 @@ chk_ftime(arcn)
>  					    "Failed ftime table seek");
>  					return(-1);
>  				}
> -				if (read(ffd, ckname, namelen) != namelen) {
> +				if (xread(ffd, ckname, namelen) != namelen) {
>  					syswarn(1, errno,
>  					    "Failed ftime table read");
>  					return(-1);
> @@ -492,7 +492,7 @@ chk_ftime(arcn)
>  		 * offset. add the file to the head of the hash chain
>  		 */
>  		if ((pt->seek = lseek(ffd, (off_t)0, SEEK_END)) >= 0) {
> -			if (write(ffd, arcn->name, namelen) == namelen) {
> +			if (xwrite(ffd, arcn->name, namelen) == namelen) {
>  				pt->mtime = arcn->sb.st_mtime;
>  				pt->namelen = namelen;
>  				pt->fow = ftab[indx];
> @@ -1288,8 +1288,8 @@ add_dir(name, nlen, psb, frc_mode)
>  	dblk.atime = psb->st_atime;
>  	dblk.fflags = psb->st_flags;
>  	dblk.frc_mode = frc_mode;
> -	if ((write(dirfd, name, dblk.nlen) == dblk.nlen) &&
> -	    (write(dirfd, (char *)&dblk, sizeof(dblk)) == sizeof(dblk))) {
> +	if ((xwrite(dirfd, name, dblk.nlen) == dblk.nlen) &&
> +	    (xwrite(dirfd, (char *)&dblk, sizeof(dblk)) == sizeof(dblk))) {
>  		++dircnt;
>  		return;
>  	}
> @@ -1329,11 +1329,11 @@ proc_dir()
>  		 */
>  		if (lseek(dirfd, -((off_t)sizeof(dblk)), SEEK_CUR) < 0) 
>  			break;
> -		if (read(dirfd,(char *)&dblk, sizeof(dblk)) != sizeof(dblk))
> +		if (xread(dirfd,(char *)&dblk, sizeof(dblk)) != sizeof(dblk))
>  			break;
>  		if (lseek(dirfd, dblk.npos, SEEK_SET) < 0) 
>  			break;
> -		if (read(dirfd, name, dblk.nlen) != dblk.nlen)
> +		if (xread(dirfd, name, dblk.nlen) != dblk.nlen)
>  			break;
>  		if (lseek(dirfd, dblk.npos, SEEK_SET) < 0) 
>  			break;
> diff -uF^[a-zA-Z_][a-z 	A-Z0-9_]*(.*[^;]$ file_subs.c.orig file_subs.c
> --- file_subs.c.orig	Mon Nov  8 09:46:29 1999
> +++ file_subs.c	Wed Feb  9 22:40:56 2000
> @@ -948,7 +948,7 @@ file_write(fd, str, cnt, rem, isempt, sz
>  			}
>  			strncpy(gnu_hack_string, st, wcnt);
>  			gnu_hack_string[wcnt] = 0;
> -		} else if (write(fd, st, wcnt) != wcnt) {
> +		} else if (xwrite(fd, st, wcnt) != wcnt) {
>  			syswarn(1, errno, "Failed write to file %s", name);
>  			return(-1);
>  		}
> @@ -992,7 +992,7 @@ file_flush(fd, fname, isempt)
>  		return;
>  	}
>  
> -	if (write(fd, blnk, 1) < 0)
> +	if (write_with_restart(fd, blnk, 1) < 0)
>  		syswarn(1, errno, "Failed write to file %s", fname);
>  	return;
>  }
>