[pacman-dev] [PATCH] Cleanups: libalpm/utils

24 Jan 2007

Hi,

I tried to clean up lib/libalpm/util.[ch]. It's not yet complete, I
would like to have some comments on it first.

Some notes:

	There has to be a proper way handling out-of-memory. The lib
	can't do anything anymore without memory - am I right?

	_alpm_runscriptlet is really long and I'm going to split it.

So, here goes the patch:

	Signed-off-by: Johannes Weiner <hannes@saeurebad.de>

diff -Naur pacman-lib.old/lib/libalpm/util.c pacman-lib/lib/libalpm/util.c

--- pacman-lib.old/lib/libalpm/util.c	2007-01-24 16:09:16.000000000 +0100
+++ pacman-lib/lib/libalpm/util.c	2007-01-24 16:10:13.000000000 +0100
@@ -64,9 +64,49 @@
 #include "package.h"
 #include "alpm.h"
 
+/* Memory allocation frontends with return value check.
+ * TODO: Think about an elegant way to die on failure */
+static void *safe_mem(void *r)
+{
+	if (r) {
+		return (r);
+	}
+
+	_alpm_log(PM_LOG_ERROR, "Memory exhausted.\n");
+
+	/* TODO: This is probably the wrong way */
+	alpm_release();
+	exit(2);
+}
+
+void *pmalloc(size_t s)
+{
+	return (safe_mem(malloc(s)));
+}
+
+void *pcalloc(size_t e, size_t s)
+{
+	return (safe_mem(calloc(e, s)));
+}
+
+void *prealloc(void *p, size_t s)
+{
+	return (safe_mem(realloc(p, s)));
+}
+
+char *pstrdup(const char *s)
+{
+	return (safe_mem((char *) strdup(s)));
+}
+
+void *palloca(size_t s)
+{
+	return (safe_mem(alloca(s)));
+}
+
 #ifdef __sun__
 /* This is a replacement for strsep which is not portable (missing on Solaris).
- * Copyright (c) 2001 by François Gouget <fgouget_at_codeweavers.com> */
+ * Copyright (c) 2001 by Fran�ois Gouget <fgouget_at_codeweavers.com> */
 char* strsep(char** str, const char* delims)
 {
 	char* token;
@@ -92,13 +132,12 @@
 
 /* Backported from Solaris Express 4/06
  * Copyright (c) 2006 Sun Microsystems, Inc. */
-char * mkdtemp(char *template)
+char * mkdtemp(const char *template)
 {
-	char *t = alloca(strlen(template) + 1);
-	char *r;
+	char *r, *t = palloca(strlen(template) + 1);
 
 	/* Save template */
-	(void) strcpy(t, template);
+	strcpy(t, template);
 	for (; ; ) {
 		r = mktemp(template);
 
@@ -113,7 +152,7 @@
 			return (NULL);
 
 		/* Reset template */
-		(void) strcpy(template, t);
+		strcpy(template, t);
 	}
 }
 #endif
@@ -121,14 +160,16 @@
 /* does the same thing as 'mkdir -p' */
 int _alpm_makepath(char *path)
 {
-	char *orig, *str, *ptr;
-	char full[PATH_MAX] = "";
 	mode_t oldmask;
+	char *orig, *str, *ptr, full[PATH_MAX];
 
 	oldmask = umask(0000);
 
-	orig = strdup(path);
+	memset(full, 0, sizeof(full));
+
+	orig = pstrdup(path);
 	str = orig;
+
 	while((ptr = strsep(&str, "/"))) {
 		if(strlen(ptr)) {
 			struct stat buf;
@@ -149,32 +190,46 @@
 	return(0);
 }
 
-int _alpm_copyfile(char *src, char *dest)
+int _alpm_copyfile(const char *src, const char *dest)
 {
-	FILE *in, *out;
-	size_t len;
-	char buf[4097];
+	FILE *fp;
+	char *buf;
+	struct stat sbuf;
 
-	in = fopen(src, "r");
-	if(in == NULL) {
+	if (stat(src, &sbuf)) {
 		return(1);
 	}
-	out = fopen(dest, "w");
-	if(out == NULL) {
-		fclose(in);
+
+	if (!(fp = fopen(src, "r"))) {
 		return(1);
 	}
 
-	while((len = fread(buf, 1, 4096, in))) {
-		fwrite(buf, 1, len, out);
+	buf = pmalloc(sbuf.st_size);
+
+	if (fread(buf, sbuf.st_size, 1, fp) != 1) {
+		FREE(buf);
+		return(1);
+	}
+
+	fclose(fp);
+
+	if (!(fp = fopen(dest, "w"))) {
+		FREE(buf);
+		return(1);
+	}
+
+	if (fwrite(buf, sbuf.st_size, 1, fp) != 1) {
+		FREE(buf);
+		fclose(fp);
+		return(1);
 	}
 
-	fclose(in);
-	fclose(out);
+	FREE(buf);
+	fclose(fp);
 	return(0);
 }
 
-/* Convert a string to uppercase
+/* Convert a string to uppercase destructively
  */
 char *_alpm_strtoupper(char *str)
 {
@@ -187,7 +242,7 @@
 	return str;
 }
 
-/* Trim whitespace and newlines from a string
+/* Trim whitespace and newlines from a string destructively
  */
 char *_alpm_strtrim(char *str)
 {
@@ -210,8 +265,8 @@
 		return(str);
 	}
 
-	pch = (char *)(str + (strlen(str) - 1));
-	while(isspace((int)*pch)) {
+	pch = (str + (strlen(str) - 1));
+	while(isspace(*pch)) {
 		pch--;
 	}
 	*++pch = '\0';
@@ -221,35 +276,36 @@
 
 /* Create a lock file
  */
-int _alpm_lckmk(char *file)
+int _alpm_lckmk(const char *file)
 {
 	int fd, count = 0;
 	char *dir, *ptr;
 
 	/* create the dir of the lockfile first */
-	dir = strdup(file);
+	dir = pstrdup(file);
 	ptr = strrchr(dir, '/');
 	if(ptr) {
 		*ptr = '\0';
 	}
 	_alpm_makepath(dir);
 
-	while((fd = open(file, O_WRONLY | O_CREAT | O_EXCL, 0000)) == -1 && errno == EACCES) { 
+	while((fd = open(file, O_WRONLY | O_CREAT | O_EXCL, 0000) == -1) &&
+	      (errno == EACCES)) { 
 		if(++count < 1) {
 			sleep(1);
-		}	else {
+		} else {
 			return(-1);
 		}
 	}
 
-	free(dir);
+	FREE(dir);
 
 	return(fd > 0 ? fd : -1);
 }
 
 /* Remove a lock file
  */
-int _alpm_lckrm(char *file)
+int _alpm_lckrm(const char *file)
 {
 	if(unlink(file) == -1 && errno != ENOENT) {
 		return(-1);
@@ -272,8 +328,12 @@
 	archive_read_support_compression_all(_archive);
 	archive_read_support_format_all(_archive);
 
-	if(archive_read_open_file(_archive, archive, ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) {
-		_alpm_log(PM_LOG_ERROR, _("could not open %s: %s\n"), archive, archive_error_string(_archive));
+	if(archive_read_open_file(_archive,
+				  archive,
+				  ARCHIVE_DEFAULT_BYTES_PER_BLOCK)
+	   != ARCHIVE_OK) {
+		_alpm_log(PM_LOG_ERROR, _("could not open %s: %s\n"), archive,
+			  archive_error_string(_archive));
 		RET_ERR(PM_ERR_PKG_OPEN, -1);
 	}
 
@@ -283,10 +343,18 @@
 				return(1);
 			continue;
 		}
-		snprintf(expath, PATH_MAX, "%s/%s", prefix, archive_entry_pathname(entry));
+
+		snprintf(expath, PATH_MAX, "%s/%s", prefix,
+			 archive_entry_pathname(entry));
 		archive_entry_set_pathname(entry, expath);
-		if(archive_read_extract(_archive, entry, ARCHIVE_EXTRACT_FLAGS) != ARCHIVE_OK) {
-			_alpm_log(PM_LOG_ERROR, _("could not extract %s: %s\n"), archive_entry_pathname(entry), archive_error_string(_archive));
+
+		if(archive_read_extract(_archive,
+					entry,
+					ARCHIVE_EXTRACT_FLAGS)
+		   != ARCHIVE_OK) {
+			_alpm_log(PM_LOG_ERROR, _("could not extract %s: %s\n"),
+				  archive_entry_pathname(entry),
+				  archive_error_string(_archive));
 			 return(1);
 		}
 
@@ -300,49 +368,53 @@
 }
 
 /* does the same thing as 'rm -rf' */
-int _alpm_rmrf(char *path)
+int _alpm_rmrf(const char *path)
 {
 	int errflag = 0;
 	struct dirent *dp;
 	DIR *dirp;
 	char name[PATH_MAX];
-  struct stat st;
+	struct stat st;
 
-	if(stat(path, &st) == 0) {
-		if(S_ISREG(st.st_mode)) {
-			if(!unlink(path)) {
-				return(0);
-			} else {
-				if(errno == ENOENT) {
-					return(0);
-				} else {
-					/* not a directory */
-					return(1);
-				}
-			}
-		} else if(S_ISDIR(st.st_mode)) {
-			if((dirp = opendir(path)) == (DIR *)-1) {
-				return(1);
-			}
-			for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
-				if(dp->d_ino) {
-					sprintf(name, "%s/%s", path, dp->d_name);
-					if(strcmp(dp->d_name, "..") && strcmp(dp->d_name, ".")) {
-						errflag += _alpm_rmrf(name);
-					}
+	if(stat(path, &st)) {
+		return(0);
+	}
+
+	if(S_ISREG(st.st_mode)) {
+		if(!unlink(path)) {
+			return(0);
+		}
+		
+		if(errno == ENOENT) {
+			return(0);
+		} else {
+			/* not a directory */
+			return(1);
+		}
+	} else if(S_ISDIR(st.st_mode)) {
+		if(!(dirp = opendir(path))) {
+			return(1);
+		}
+		for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
+			if(dp->d_ino) {
+				sprintf(name, "%s/%s", path, dp->d_name);
+				if(strcmp(dp->d_name, "..") &&
+				   strcmp(dp->d_name, ".")) {
+					errflag += _alpm_rmrf(name);
 				}
 			}
-			closedir(dirp);
-			if(rmdir(path)) {
-				errflag++;
-			}
 		}
-		return(errflag);
+		closedir(dirp);
+
+		if(rmdir(path)) {
+			errflag++;
+		}
 	}
-	return(0);
+
+	return(errflag);
 }
 
-int _alpm_logaction(unsigned short usesyslog, FILE *f, const char *str)
+int _alpm_logaction(unsigned char usesyslog, FILE *f, const char *str)
 {
 	_alpm_log(PM_LOG_DEBUG, _("logaction called: %s"), str);
 
@@ -387,37 +459,39 @@
 
 /* A cheap grep for text files, returns 1 if a substring
  * was found in the text file fn, 0 if it wasn't
+ *
+ * TODO: Remove hardcoded buffersize
  */
 static int grep(const char *fn, const char *needle)
 {
 	FILE *fp;
+	char line[1024];
 
 	if((fp = fopen(fn, "r")) == NULL) {
 		return(0);
 	}
-	while(!feof(fp)) {
-		char line[1024];
-		fgets(line, 1024, fp);
-		if(feof(fp)) {
-			continue;
-		}
+
+	while(fgets(line, 1024, fp)) {
 		if(strstr(line, needle)) {
 			fclose(fp);
 			return(1);
 		}
 	}
+
 	fclose(fp);
 	return(0);
 }
 
-int _alpm_runscriptlet(char *root, char *installfn, char *script, char *ver, char *oldver, pmtrans_t *trans)
+/*
+ * TODO: This function is WAY too complex and long!!!
+ */
+int _alpm_runscriptlet(const char *root, const char *installfn,
+		       const char *script, const char *ver,
+		       const char *oldver, pmtrans_t *trans)
 {
-	char scriptfn[PATH_MAX];
-	char cmdline[PATH_MAX];
-	char tmpdir[PATH_MAX] = "";
-	char *scriptpath;
+	char scriptfn[PATH_MAX], cmdline[PATH_MAX], tmpdir[PATH_MAX];
+	char *scriptpath, cwd[PATH_MAX] = "";
 	struct stat buf;
-	char cwd[PATH_MAX] = "";
 	pid_t pid;
 	int retval = 0;
 
@@ -433,7 +507,8 @@
 		}
 		snprintf(tmpdir, PATH_MAX, "%stmp/alpm_XXXXXX", root);
 		if(mkdtemp(tmpdir) == NULL) {
-			_alpm_log(PM_LOG_ERROR, _("could not create temp directory"));
+			_alpm_log(PM_LOG_ERROR,
+				  _("could not create temp directory"));
 			return(1);
 		}
 		_alpm_unpack(installfn, tmpdir, ".INSTALL");
@@ -453,14 +528,18 @@
 
 	/* save the cwd so we can restore it later */
 	if(getcwd(cwd, PATH_MAX) == NULL) {
-		_alpm_log(PM_LOG_ERROR, _("could not get current working directory"));
-		/* in case of error, cwd content is undefined: so we set it to something */
+		_alpm_log(PM_LOG_ERROR,
+			  _("could not get current working directory"));
+		/* in case of error, cwd content is undefined:
+		 * so we set it to something */
 		cwd[0] = 0;
 	}
 
 	/* just in case our cwd was removed in the upgrade operation */
 	if(chdir(root) != 0) {
-		_alpm_log(PM_LOG_ERROR, _("could not change directory to %s (%s)"), root, strerror(errno));
+		_alpm_log(PM_LOG_ERROR,
+			  _("could not change directory to %s (%s)"),
+			  root, strerror(errno));
 	}
 
 	_alpm_log(PM_LOG_FLOW2, _("executing %s script..."), script);
@@ -476,52 +555,70 @@
 
 	pid = fork();
 	if(pid == -1) {
-		_alpm_log(PM_LOG_ERROR, _("could not fork a new process (%s)"), strerror(errno));
+		_alpm_log(PM_LOG_ERROR,
+			  _("could not fork a new process (%s)"),
+			  strerror(errno));
 		retval = 1;
 		goto cleanup;
 	}
 
 	if(pid == 0) {
 		FILE *pp;
+		char line[1024];
 		_alpm_log(PM_LOG_DEBUG, _("chrooting in %s"), root);
 		if(chroot(root) != 0) {
-			_alpm_log(PM_LOG_ERROR, _("could not change the root directory (%s)"), strerror(errno));
+			_alpm_log(PM_LOG_ERROR,
+				  _("could not change the root directory (%s)"),
+				  strerror(errno));
 			return(1);
 		}
 		if(chdir("/") != 0) {
-			_alpm_log(PM_LOG_ERROR, _("could not change directory to / (%s)"), strerror(errno));
+			_alpm_log(PM_LOG_ERROR,
+				  _("could not change directory to / (%s)"),
+				  strerror(errno));
 			return(1);
 		}
 		umask(0022);
 		_alpm_log(PM_LOG_DEBUG, _("executing \"%s\""), cmdline);
 		pp = popen(cmdline, "r");
 		if(!pp) {
-			_alpm_log(PM_LOG_ERROR, _("call to popen failed (%s)"), strerror(errno));
+			_alpm_log(PM_LOG_ERROR,
+				  _("call to popen failed (%s)"),
+				  strerror(errno));
 			retval = 1;
 			goto cleanup;
 		}
-		while(!feof(pp)) {
-			char line[1024];
-			if(fgets(line, 1024, pp) == NULL)
-				break;
+		while(fgets(line, 1024, pp)) {
+			size_t slen, dlen;
+			slen = strlen(SCRIPTLET_START);
+			dlen = strlen(SCRIPTLET_DONE);
+
 			/* "START <event desc>" */
-			if((strlen(line) > strlen(SCRIPTLET_START)) && !strncmp(line, SCRIPTLET_START, strlen(SCRIPTLET_START))) {
-				EVENT(trans, PM_TRANS_EVT_SCRIPTLET_START, _alpm_strtrim(line + strlen(SCRIPTLET_START)), NULL);
+			if((strlen(line) > slen) &&
+			   !strncmp(line, SCRIPTLET_START, slen)) {
+				EVENT(trans, PM_TRANS_EVT_SCRIPTLET_START,
+				      _alpm_strtrim(line + slen), NULL);
 			/* "DONE <ret code>" */
-			} else if((strlen(line) > strlen(SCRIPTLET_DONE)) && !strncmp(line, SCRIPTLET_DONE, strlen(SCRIPTLET_DONE))) {
-				EVENT(trans, PM_TRANS_EVT_SCRIPTLET_DONE, (void*)atol(_alpm_strtrim(line + strlen(SCRIPTLET_DONE))), NULL);
+			} else if((strlen(line) > dlen) &&
+				  !strncmp(line, SCRIPTLET_DONE, dlen)) {
+				EVENT(trans, PM_TRANS_EVT_SCRIPTLET_DONE,
+				      (void *)atol(_alpm_strtrim(line + dlen)),
+				      NULL);
 			} else {
 				_alpm_strtrim(line);
 				/* log our script output */
 				alpm_logaction(line);
-				EVENT(trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL);
+				EVENT(trans, PM_TRANS_EVT_SCRIPTLET_INFO,
+				      line, NULL);
 			}
 		}
 		pclose(pp);
 		exit(0);
 	} else {
 		if(waitpid(pid, 0, 0) == -1) {
-			_alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)"), strerror(errno));
+			_alpm_log(PM_LOG_ERROR,
+				  _("call to waitpid failed (%s)"),
+				  strerror(errno));
 			retval = 1;
 			goto cleanup;
 		}
@@ -529,14 +626,16 @@
 
 cleanup:
 	if(strlen(tmpdir) && _alpm_rmrf(tmpdir)) {
-		_alpm_log(PM_LOG_WARNING, _("could not remove tmpdir %s"), tmpdir);
+		_alpm_log(PM_LOG_WARNING,
+			  _("could not remove tmpdir %s"),
+			  tmpdir);
 	}
 	if(strlen(cwd)) {
 		chdir(cwd);
 	}
 
 	return(retval);
-}
+} /* Thank goddess, I'm through. */
 
 #ifndef __sun__
 static long long get_freespace()
@@ -584,23 +683,16 @@
 		}
 	}
 	freespace = get_freespace();
-	_alpm_log(PM_LOG_DEBUG, _("check_freespace: total pkg size: %lld, disk space: %lld"), pkgsize, freespace);
+	_alpm_log(PM_LOG_DEBUG,
+		  _("check_freespace: total pkg size: %lld, disk space: %lld"),
+		  pkgsize, freespace);
 	if(pkgsize > freespace) {
 		if(data) {
-			long long *ptr;
-			if((ptr = (long long*)malloc(sizeof(long long)))==NULL) {
-				_alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %d bytes"), sizeof(long long));
-				pm_errno = PM_ERR_MEMORY;
-				return(-1);
-			}
+			long long *ptr = pmalloc(sizeof(long long));
 			*ptr = pkgsize;
 			*data = alpm_list_add(*data, ptr);
-			if((ptr = (long long*)malloc(sizeof(long long)))==NULL) {
-				_alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %d bytes"), sizeof(long long));
-				FREELIST(*data);
-				pm_errno = PM_ERR_MEMORY;
-				return(-1);
-			}
+
+			ptr = pmalloc(sizeof(long long));
 			*ptr = freespace;
 			*data = alpm_list_add(*data, ptr);
 		}
@@ -621,8 +713,8 @@
 		struct tm *lt;
 		lt = localtime(&t);
 		sprintf(buffer, "%4d%02d%02d%02d%02d%02d",
-						lt->tm_year+1900, lt->tm_mon+1, lt->tm_mday,
-						lt->tm_hour, lt->tm_min, lt->tm_sec);
+			lt->tm_year+1900, lt->tm_mon+1, lt->tm_mday,
+			lt->tm_hour, lt->tm_min, lt->tm_sec);
 		buffer[14] = '\0';
 	}
 }
diff -Naur pacman-lib.old/lib/libalpm/util.h pacman-lib/lib/libalpm/util.h
--- pacman-lib.old/lib/libalpm/util.h	2007-01-24 16:09:16.000000000 +0100
+++ pacman-lib/lib/libalpm/util.h	2007-01-24 16:10:13.000000000 +0100
@@ -43,7 +43,9 @@
 	s1[(len)-1] = 0; \
 } while(0)
 
-#define ARCHIVE_EXTRACT_FLAGS ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | ARCHIVE_EXTRACT_TIME
+#define ARCHIVE_EXTRACT_FLAGS (ARCHIVE_EXTRACT_OWNER	\
+			       | ARCHIVE_EXTRACT_PERM 	\
+			       | ARCHIVE_EXTRACT_TIME)
 
 #ifdef ENABLE_NLS
 #define _(str) dgettext ("libalpm", str)
@@ -54,18 +56,25 @@
 #define SCRIPTLET_START "START "
 #define SCRIPTLET_DONE "DONE "
 
+void *pmalloc(size_t s);
+void *pcalloc(size_t e, size_t s);
+void *prealloc(void *p, size_t s);
+char *pstrdup(const char *s);
+void *palloca(size_t s);
 int _alpm_makepath(char *path);
-int _alpm_copyfile(char *src, char *dest);
+int _alpm_copyfile(const char *src, const char *dest);
 char *_alpm_strtoupper(char *str);
 char *_alpm_strtrim(char *str);
-int _alpm_lckmk(char *file);
-int _alpm_lckrm(char *file);
+int _alpm_lckmk(const char *file);
+int _alpm_lckrm(const char *file);
 int _alpm_unpack(const char *archive, const char *prefix, const char *fn);
-int _alpm_rmrf(char *path);
-int _alpm_logaction(unsigned short usesyslog, FILE *f, const char *str);
+int _alpm_rmrf(const char *path);
+int _alpm_logaction(unsigned char usesyslog, FILE *f, const char *str);
 int _alpm_ldconfig(char *root);
 #ifdef _ALPM_TRANS_H
-int _alpm_runscriptlet(char *util, char *installfn, char *script, char *ver, char *oldver, pmtrans_t *trans);
+int _alpm_runscriptlet(const char *util, const char *installfn,
+		       const char *script, const char *ver,
+		       const char *oldver, pmtrans_t *trans);
 #ifndef __sun__
 int _alpm_check_freespace(pmtrans_t *trans, alpm_list_t **data);
 #endif

    