PERFORCE change 178188 for review

Garrett Cooper gcooper at FreeBSD.org
Thu May 13 03:38:23 UTC 2010


http://p4web.freebsd.org/@@178188?ac=10

Change 178188 by gcooper at starr-bastion on 2010/05/13 03:38:14

	- Take the useful bits out of the ADD_FILE macro and move it into a real function called add_file.
	- Clean up create_from_installed_recursive so that it's style-compliant and the flow is easier to
	follow.
	- Fix some delightful bugs with wild pointers because it's been sufficiently long since I've used
	*stat and I made the time old mistake of not allocating sb beforehand, etc, etc. Thanks to the
	folks who replied on hackers@ for noting my stupid mistake.
	- Need to determine what to do about adding +CONTENTS, etc 
	- Disable mmap(2) for now because it doesn't work; need to determine why not.
	
	Need to determine what to do about adding +CONTENTS, etc in make_dist since it's added twice today
	if Prefix is NULL and we're bootstrapping a package which isn't installed on the disk :(.
	
	The metadata files should be at the top of the archive because it would speed up pkg_info, and
	regular metadata reads when reading from a package archive opposed to reading data directly off
	the disk in $PKG_PREFIX.

Affected files ...

.. //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/usr.sbin/pkg_install/create/perform.c#20 edit

Differences ...

==== //depot/projects/soc2007/gcooper-pkg_install-enhancements-simplified/usr.sbin/pkg_install/create/perform.c#20 (text+ko) ====

@@ -43,6 +43,8 @@
 #include "create.h"
 
 static void sanity_check(void);
+static const char* add_file(struct archive *, const char *, const char *,
+		const int);
 static void make_dist(const char *, const char *, const char *, Package *);
 static int create_from_installed_recursive(const char *, const char *);
 static int create_from_installed(const char *, const char *, const char *);
@@ -262,7 +264,7 @@
     /* mark_plist(&plist); */
 
     /* Now put the release specific items in */
-    if (!Prefix)
+    if (Prefix == NULL)
 	add_plist(&plist, PLIST_CWD, ".");
 
     write_file(COMMENT_FNAME, Comment);
@@ -274,38 +276,38 @@
     add_plist(&plist, PLIST_FILE, DESC_FNAME);
     add_cksum(&plist, plist.tail, DESC_FNAME);
 
-    if (Install) {
+    if (Install != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, INSTALL_FNAME);
 	add_cksum(&plist, plist.tail, INSTALL_FNAME);
     }
-    if (PostInstall) {
+    if (PostInstall != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, POST_INSTALL_FNAME);
 	add_cksum(&plist, plist.tail, POST_INSTALL_FNAME);
     }
-    if (DeInstall) {
+    if (DeInstall != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, DEINSTALL_FNAME);
 	add_cksum(&plist, plist.tail, DEINSTALL_FNAME);
     }
-    if (PostDeInstall) {
+    if (PostDeInstall != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, POST_DEINSTALL_FNAME);
 	add_cksum(&plist, plist.tail, POST_DEINSTALL_FNAME);
     }
-    if (Require) {
+    if (Require != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, REQUIRE_FNAME);
 	add_cksum(&plist, plist.tail, REQUIRE_FNAME);
     }
-    if (Display) {
+    if (Display != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, DISPLAY_FNAME);
 	add_cksum(&plist, plist.tail, DISPLAY_FNAME);
 	add_plist(&plist, PLIST_DISPLAY, DISPLAY_FNAME);
     }
-    if (Mtree) {
+    if (Mtree != NULL) {
 	add_plist(&plist, PLIST_IGNORE, NULL);
 	add_plist(&plist, PLIST_FILE, MTREE_FNAME);
 	add_cksum(&plist, plist.tail, MTREE_FNAME);
@@ -324,7 +326,7 @@
 	err(2, "%s: error occurred when closing %s", __func__, CONTENTS_FNAME);
     }
 
-    /* And stick it into a tar ball */
+    /* And stick it into a tarball */
     make_dist(home, pkg, suf, &plist);
 
     /* Cleanup */
@@ -335,47 +337,110 @@
     return TRUE;	/* Success */
 }
 
+static const char* 
+add_file(struct archive *archive, const char *srcfile, const char *destfile,
+	const int archive_entry_open_flags)
+{
+
+/*
+ * XXX (gcooper):
+ * The mmap code below doesn't function with archive(3) today; need to
+ * determine why as no diags are fed back as to why it doesn't work, because
+ * it would probably be a performance boost to map the pages for the backing
+ * files into memory and just do one read as opposed to many small reads.
+ *
+ * #define BROKEN_MMAP
+ */
+
+	struct archive_entry *entry = NULL;
+	struct stat sb;
+	const char *error = NULL;
+	int archive_entry_fd;
+#ifdef BROKEN_MMAP
+	void *archive_entry_map_addr = NULL;
+#else
+	char archive_entry_buf[BUFSIZ];
+	size_t len;
+#endif
+
+	if (Verbose)
+		printf("C - %s\n", srcfile);
+
+	if ((archive_entry_fd = open(srcfile, archive_entry_open_flags)) == -1)
+		error = strerror(errno);
+	else if (fstat(archive_entry_fd, &sb) == -1) 
+		error = strerror(errno);
+#ifdef BROKEN_MMAP
+	else if ((archive_entry_map_addr = mmap(NULL, sb.st_size, PROT_READ,
+	    MAP_SHARED, archive_entry_fd, 0)) == NULL)
+		error = strerror(errno);
+#endif
+	else {
+
+		if ((entry = archive_entry_new()) == NULL)
+			error = archive_error_string(archive);
+		else {
+
+			archive_entry_copy_stat(entry, &sb);
+			archive_entry_copy_pathname(entry, destfile);
+			if (archive_write_header(archive, entry) != ARCHIVE_OK)
+				error = archive_error_string(archive);
+#ifdef BROKEN_MMAP
+			/* 
+			 * XXX (gcooper): fails to write data here with mmap(2)
+			 * enabled.
+			 */
+			else if (archive_write_data(archive,
+			    archive_entry_map_addr, sb.st_size) != ARCHIVE_OK)
+				error = archive_error_string(archive);
+			(void) munmap(archive_entry_map_addr, sb.st_size);
+#else
+			else if (error != NULL) {
+
+				do {
+
+					len = read(archive_entry_fd,
+					    archive_entry_buf,
+					    sizeof(archive_entry_buf));
+
+					if (archive_write_data(archive,
+					    archive_entry_buf,
+					    sizeof(archive_entry_buf)) !=
+					    ARCHIVE_OK)
+						error = archive_error_string(archive);
+
+				} while (error == NULL && len > 0);
+
+			}
+
+#endif
+			archive_entry_free(entry);
+
+		}
+
+	}
+
+	if (0 <= archive_entry_fd)
+		close(archive_entry_fd);
+
+	return error;
+
+}
+
 static void
 make_dist(const char *homedir, const char *pkg, const char *suff, Package *plist)
 {
 
-	/* XXX (gcooper): add acl and xattr support? */
-#define ADD_FILE(srcfile, destfile, archive_entry_open_flags)		      \
-	if (error == NULL) {						      \
-		if ((archive_entry_fd = open(srcfile,			      \
-		    archive_entry_open_flags)) == -1 ||			      \
-		    fstat(archive_entry_fd, sb) == -1) {		      \
-			error = strerror(errno);			      \
-		} else if ((archive_entry_map_addr = mmap(NULL,		      \
-		    PROT_READ, sb->st_size, MAP_SHARED,			      \
-		    archive_entry_fd, 0)) == NULL) {			      \
-			error = strerror(errno);			      \
-		} else {						      \
-			if ((entry = archive_entry_new()) == NULL)	      \
-				error = archive_error_string(archive);	      \
-			else {						      \
-				archive_entry_copy_stat(entry, sb);	      \
-				archive_entry_copy_pathname(entry, destfile); \
-				if (archive_write_header(archive,	      \
-				    entry) != ARCHIVE_OK)		      \
-					error = archive_error_string(archive);\
-				else if (archive_write_data(archive,	      \
-				    archive_entry_map_addr,		      \
-				    sb->st_size) != ARCHIVE_OK)		      \
-					error = archive_error_string(archive);\
-				(void) munmap(archive_entry_map_addr,	      \
-				    sb->st_size);			      \
-				archive_entry_free(entry);		      \
-			}						      \
-		}							      \
-		if (0 <= archive_entry_fd)				      \
-			close(archive_entry_fd);			      \
-	}
+#define ADD_FILE(SRCFILE, DESTFILE, OPEN_FLAGS)				\
+	do {								\
+		if (error == NULL) {					\
+			error = add_file(archive, SRCFILE, DESTFILE,	\
+			    OPEN_FLAGS);				\
+		}							\
+	} while (0)
 
 	PackingList p;
-	struct stat *sb;
 	struct archive *archive = NULL;
-	struct archive_entry *entry = NULL;
 	struct lafe_matching *match_patterns = NULL;
 	char *destbase = NULL;
 	char *destfile = NULL;
@@ -388,11 +453,9 @@
 	const char *error = NULL;
 	int archive_fd = -1;
 	int archive_open_flags;
-	int archive_entry_fd = -1;
 	int archive_entry_open_flags;
 	int archive_metadata_open_flags;
 	int destbase_len, srcbase_len;
-	void *archive_entry_map_addr;
 
 	if (*pkg == '/')
 		snprintf(tball, sizeof(tball), "%s.%s", pkg, suff);
@@ -740,41 +803,40 @@
 static int
 create_from_installed_recursive(const char *pkg, const char *suf)
 {
-    FILE *fp;
-    Package plist;
-    PackingList p;
-    char tmp[PATH_MAX];
-    int rval;
+	FILE *fp;
+	Package plist;
+	PackingList p;
+	char tmp[PATH_MAX];
+	int rval;
 
-    if (!create_from_installed(InstalledPkg, pkg, suf))
-	return FALSE;
-    snprintf(tmp, sizeof(tmp), "%s/%s/%s", LOG_DIR, InstalledPkg, CONTENTS_FNAME);
-    if (!fexists(tmp)) {
-	warnx("can't find package '%s' installed!", InstalledPkg);
-	return FALSE;
-    }
-    /* Suck in the contents list */
-    plist.head = plist.tail = NULL;
-    fp = fopen(tmp, "r");
-    if (!fp) {
-	warnx("unable to open %s file", tmp);
-	return FALSE;
-    }
-    read_plist(&plist, fp);
-    fclose(fp);
-    rval = TRUE;
-    for (p = plist.head; p ; p = p->next) {
-	if (p->type != PLIST_PKGDEP)
-	    continue;
-	if (Verbose)
-	    printf("Creating package %s\n", p->name);
-	if (!create_from_installed(p->name, p->name, suf)) {
-	    rval = FALSE;
-	    break;
+	if (!create_from_installed(InstalledPkg, pkg, suf))
+		return FALSE;
+	snprintf(tmp, sizeof(tmp), "%s/%s/%s",
+	    LOG_DIR, InstalledPkg, CONTENTS_FNAME);
+	if (!fexists(tmp)) {
+		warnx("can't find package '%s' installed!", InstalledPkg);
+		return FALSE;
+	}
+	/* Suck in the contents list */
+	plist.head = plist.tail = NULL;
+	fp = fopen(tmp, "r");
+	if (fp == NULL) {
+		warnx("unable to open %s file", tmp);
+		return FALSE;
+	}
+	read_plist(&plist, fp);
+	fclose(fp);
+	rval = TRUE;
+	for (p = plist.head; p != NULL && rval == TRUE; p = p->next) {
+		if (p->type == PLIST_PKGDEP) {
+			if (Verbose)
+				printf("Creating package %s\n", p->name);
+			if (!create_from_installed(p->name, p->name, suf))
+				rval = FALSE;
+		}
 	}
-    }
-    free_plist(&plist);
-    return rval;
+	free_plist(&plist);
+	return rval;
 }
 
 static int


More information about the p4-projects mailing list