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;
> }
>