svn commit: r191904 - head/lib/libarchive

[Tim Kientzle]

Author: kientzle
Date: Thu May  7 23:01:03 2009
New Revision: 191904
URL: http://svn.freebsd.org/changeset/base/191904

Log:
  Partially revert r191171, which went too far in trying
  to eliminate some duplicated code.  In particular,
  archive_read_open_filename() has different close
  handling than archive_read_open_fd(), so delegating
  the former to the latter in the degenerate case
  (a NULL filename is treated as stdin) broke reading
  from pipelines.  In particular, this fixes occasional
  port failures that were seen when using "gunzip | tar"
  pipelines under /bin/csh.
  
  Thanks to Alexey Shuvaev for reporting this failure and
  patiently helping me to track down the cause.

Modified:
  head/lib/libarchive/archive_read_open_filename.c

Modified: head/lib/libarchive/archive_read_open_filename.c
==============================================================================
--- head/lib/libarchive/archive_read_open_filename.c	Thu May  7 21:51:13 2009	(r191903)
+++ head/lib/libarchive/archive_read_open_filename.c	Thu May  7 23:01:03 2009	(r191904)
@@ -85,20 +85,32 @@ archive_read_open_filename(struct archiv
 	int fd;
 
 	archive_clear_error(a);
-	if (filename == NULL || filename[0] == '\0')
-		return (archive_read_open_fd(a, 0, block_size));
-
-	fd = open(filename, O_RDONLY | O_BINARY);
-	if (fd < 0) {
-		archive_set_error(a, errno, "Failed to open '%s'", filename);
-		return (ARCHIVE_FATAL);
+	if (filename == NULL || filename[0] == '\0') {
+		/* We used to invoke archive_read_open_fd(a,0,block_size)
+		 * here, but that doesn't (and shouldn't) handle the
+		 * end-of-file flush when reading stdout from a pipe.
+		 * Basically, read_open_fd() is intended for folks who
+		 * are willing to handle such details themselves.  This
+		 * API is intended to be a little smarter for folks who
+		 * want easy handling of the common case.
+		 */
+		filename = ""; /* Normalize NULL to "" */
+		fd = 0;
+	} else {
+		fd = open(filename, O_RDONLY | O_BINARY);
+		if (fd < 0) {
+			archive_set_error(a, errno,
+			    "Failed to open '%s'", filename);
+			return (ARCHIVE_FATAL);
+		}
 	}
 	if (fstat(fd, &st) != 0) {
 		archive_set_error(a, errno, "Can't stat '%s'", filename);
 		return (ARCHIVE_FATAL);
 	}
 
-	mine = (struct read_file_data *)malloc(sizeof(*mine) + strlen(filename));
+	mine = (struct read_file_data *)calloc(1,
+	    sizeof(*mine) + strlen(filename));
 	b = malloc(block_size);
 	if (mine == NULL || b == NULL) {
 		archive_set_error(a, ENOMEM, "No memory");
@@ -117,15 +129,20 @@ archive_read_open_filename(struct archiv
 	if (S_ISREG(st.st_mode)) {
 		archive_read_extract_set_skip_file(a, st.st_dev, st.st_ino);
 		/*
-		 * Skip is a performance optimization for anything
-		 * that supports lseek().  Generally, that only
-		 * includes regular files and possibly raw disk
-		 * devices, but there's no good portable way to detect
-		 * raw disks.
+		 * Enabling skip here is a performance optimization
+		 * for anything that supports lseek().  On FreeBSD
+		 * (and probably many other systems), only regular
+		 * files and raw disk devices support lseek() (on
+		 * other input types, lseek() returns success but
+		 * doesn't actually change the file pointer, which
+		 * just completely screws up the position-tracking
+		 * logic).  In addition, I've yet to find a portable
+		 * way to determine if a device is a raw disk device.
+		 * So I don't see a way to do much better than to only
+		 * enable this optimization for regular files.
 		 */
 		mine->can_skip = 1;
-	} else
-		mine->can_skip = 0;
+	}
 	return (archive_read_open2(a, mine,
 		NULL, file_read, file_skip, file_close));
 }
@@ -139,8 +156,11 @@ file_read(struct archive *a, void *clien
 	*buff = mine->buffer;
 	bytes_read = read(mine->fd, mine->buffer, mine->block_size);
 	if (bytes_read < 0) {
-		archive_set_error(a, errno, "Error reading '%s'",
-		    mine->filename);
+		if (mine->filename[0] == '\0')
+			archive_set_error(a, errno, "Error reading stdin");
+		else
+			archive_set_error(a, errno, "Error reading '%s'",
+			    mine->filename);
 	}
 	return (bytes_read);
 }
@@ -190,8 +210,15 @@ file_skip(struct archive *a, void *clien
 		 * likely caused by a programmer error (too large request)
 		 * or a corrupted archive file.
 		 */
-		archive_set_error(a, errno, "Error seeking in '%s'",
-		    mine->filename);
+		if (mine->filename[0] == '\0')
+			/*
+			 * Should never get here, since lseek() on stdin ought
+			 * to return an ESPIPE error.
+			 */
+			archive_set_error(a, errno, "Error seeking in stdin");
+		else
+			archive_set_error(a, errno, "Error seeking in '%s'",
+			    mine->filename);
 		return (-1);
 	}
 	return (new_offset - old_offset);
@@ -225,7 +252,9 @@ file_close(struct archive *a, void *clie
 				    mine->block_size);
 			} while (bytesRead > 0);
 		}
-		close(mine->fd);
+		/* If a named file was opened, then it needs to be closed. */
+		if (mine->filename[0] != '\0')
+			close(mine->fd);
 	}
 	free(mine->buffer);
 	free(mine);