From owner-svn-src-all@FreeBSD.ORG Thu May 7 23:01:04 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E77331065709; Thu, 7 May 2009 23:01:03 +0000 (UTC) (envelope-from kientzle@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id D4E978FC20; Thu, 7 May 2009 23:01:03 +0000 (UTC) (envelope-from kientzle@FreeBSD.org) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id n47N134H070890; Thu, 7 May 2009 23:01:03 GMT (envelope-from kientzle@svn.freebsd.org) Received: (from kientzle@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id n47N13Bv070889; Thu, 7 May 2009 23:01:03 GMT (envelope-from kientzle@svn.freebsd.org) Message-Id: <200905072301.n47N13Bv070889@svn.freebsd.org> From: Tim Kientzle Date: Thu, 7 May 2009 23:01:03 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r191904 - head/lib/libarchive X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 May 2009 23:01:04 -0000 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);