Release v3.2.5

This release includes a fix to replace system calls by local system_argv
functions, cleanup patches for error output and code style, adding delta
manifests to delta packs, consistency changes for tar and string allocation,
and removes creation of delta manifests out of create_update.
This is a re-roll of release 3.2.4 to fix a time skew issue that caused some
build failures.

Signed-off-by: Tudor Marcu <tudor.marcu@intel.com>

basic_creator.sh

@@ -45,10 +45,6 @@ for job in $(jobs -p); do
 	fi
 done
 
-# create delta packs for 2 versions back
-NUM_PACKS=2
-${SWUPDREPO}/pack_maker.sh ${VER} ${NUM_PACKS}
-
 # expose the new build to staging / testing
 echo ${VER} > ${UPDATEDIR}/image/latest.version
 

configure.ac

@@ -2,7 +2,7 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ([2.66])
-AC_INIT(swupd-server, 3.2.3, timothy.c.pepper@linux.intel.com)
+AC_INIT(swupd-server, 3.2.5, timothy.c.pepper@linux.intel.com)
 AM_INIT_AUTOMAKE([foreign -Wall -W subdir-objects])
 AM_SILENT_RULES([yes])
 AC_PROG_CC

include/swupd.h

@@ -19,17 +19,21 @@
 #if SWUPD_WITH_BSDTAR
 #define TAR_COMMAND "bsdtar"
 #define TAR_XATTR_ARGS ""
+#define TAR_XATTR_ARGS_STRLIST
 #define TAR_WARN_ARGS ""
 #else
 #define TAR_COMMAND "tar"
 #define TAR_XATTR_ARGS "--xattrs --xattrs-include='*'"
+#define TAR_XATTR_ARGS_STRLIST "--xattrs", "--xattrs-include='*'",
 #define TAR_WARN_ARGS "--warning=no-timestamp"
 #endif
 
 #if SWUPD_WITH_SELINUX
 #define TAR_PERM_ATTR_ARGS "--preserve-permissions --selinux " TAR_XATTR_ARGS
+#define TAR_PERM_ATTR_ARGS_STRLIST TAR_XATTR_ARGS_STRLIST "--preserve-permissions", "--selinux"
 #else
 #define TAR_PERM_ATTR_ARGS "--preserve-permissions " TAR_XATTR_ARGS
+#define TAR_PERM_ATTR_ARGS_STRLIST TAR_XATTR_ARGS_STRLIST "--preserve-permissions"
 #endif
 
 #if SWUPD_WITH_STATELESS
@@ -246,8 +250,11 @@ extern void account_delta_miss(void);
 extern FILE *fopen_exclusive(const char *filename); /* no mode, opens for write only */
 extern void dump_file_info(struct file *file);
 extern void string_or_die(char **strp, const char *fmt, ...);
-extern void print_elapsed_time(struct timeval *previous_time, struct timeval *current_time);
+extern void print_elapsed_time(const char *step, struct timeval *previous_time, struct timeval *current_time);
 extern int system_argv(char *const argv[]);
+extern int system_argv_fd(char *const argv[], int newstdin, int newstdout, int newstderr);
+extern int system_argv_pipe(char *const argvp1[], int stdinp1, int stderrp1,
+			    char *const argvp2[], int stdoutp2, int stderrp2);
 
 extern bool signature_initialize(void);
 extern void signature_terminate(void);

pack_maker.sh

@@ -1,112 +0,0 @@
-#!/bin/bash -e
-# usage:
-#	pack_maker.sh <to version> <back_count>
-#	pack_maker.sh 2820 2
-#	pack_maker.sh 2730 3
-#
-# For each bundle changed as of the Manifest.MoM at the <to version>,
-# create version-pair packs going back <back_count>
-# versions of that bundle.  For example if bundle "editors" changed in 2650
-# and the script is run as "pack_maker.sh 2650 2", it will find that
-# "editors" last two change points were 2550 and 2260 and thus create a
-# 2550-to-2650 and a 2260-to-2650 version pair pack for the "editors"
-# bundle.
-
-#NOTE: fullfiles must be created before calling this script
-#NOTE: zero packs need similarly created
-
-#NOTE: build numbers are sparse, search back for the next oldest existing build
-#NOTE: in the future isn't simply -10, and when that change happens we must also
-#      consider which version-pair packs are meaningful for intermediate builds
-#      and revert builds.
-
-SWUPDREPO=${SWUPDREPO:-"/usr/src/clear-projects/swupd-server"}
-BUNDLEREPO=${BUNDLEREPO:-"/usr/src/clear-projects/clr-bundles"}
-UPDATEDIR=${UPDATEDIR:-"/var/lib/update"}
-SWUPDWEBDIR="${UPDATEDIR}/www"
-SWUPD_CERTS_DIR=${SWUPD_CERTS_DIR=:-"/root/swupd-certs"}
-
-export XZ_DEFAULTS="--threads 0"
-
-export SWUPD_CERTS_DIR
-export LEAF_KEY="leaf.key.pem"
-export LEAF_CERT="leaf.cert.pem"
-export CA_CHAIN_CERT="ca-chain.cert.pem"
-export PASSPHRASE="${SWUPD_CERTS_DIR}/passphrase"
-
-VER=$1
-BACK_COUNT=$2
-
-if [ "${BACK_COUNT}" == "" ]; then
-	echo "missing to pack count"
-	exit 1
-fi
-
-if [ "${VER}" == "" ]; then
-	echo "missing to version"
-	exit 1
-fi
-
-MOM=${SWUPDWEBDIR}/${VER}/Manifest.MoM
-if [ ! -e ${MOM} ]; then
-	echo "invalid version (no ${MOM})"
-	exit 1
-fi
-
-#time ${SWUPDREPO}/swupd_make_fullfiles --statedir ${UPDATEDIR} ${VER}
-
-BUNDLE_LIST=$(cat ${MOM} | awk -v V=${VER} '$1 ~ /^M\./ && $3 == V { print $4 }')
-
-# build packs for all bundles changed in $VER
-for BUNDLE in $BUNDLE_LIST; do
-	#${SWUPDREPO}/swupd_make_pack --statedir ${UPDATEDIR} 0 ${VER} ${BUNDLE} &
-
-	BUNDLE_VER_LIST=""
-	MANIFEST_VER=$(cat ${SWUPDWEBDIR}/$VER/Manifest.MoM | grep "^previous:" | cut -f 2)
-	BUNDLE_VER=""
-	COUNT=0
-	while [ $COUNT -lt $BACK_COUNT ] && [ $MANIFEST_VER -gt 0 ]; do
-		MOM=${SWUPDWEBDIR}/$MANIFEST_VER/Manifest.MoM
-		if [ -e ${MOM} ]; then
-			BUNDLE_VER=$(cat ${MOM} | grep "	${BUNDLE}$" | cut -f 3)
-			if [ "$BUNDLE_VER" != "" ]; then
-				BUNDLE_VER_LIST="$BUNDLE_VER $BUNDLE_VER_LIST"
-				MANIFEST_VER=$(cat ${SWUPDWEBDIR}/$BUNDLE_VER/Manifest.MoM | grep "^previous:" | cut -f 2)
-				let COUNT=$((COUNT+1))
-			else
-				# back in history to where bundle didn't exist yet
-				MANIFEST_VER=0
-			fi
-		else
-			# seek backwards until a manifest if found
-			MANIFEST_VER=$((MANIFEST_VER-10))
-		fi
-	done
-
-	for v in $BUNDLE_VER_LIST; do
-		if [ -e ${SWUPDWEBDIR}/${VER}/pack-${BUNDLE}-from-$v.tar ]; then
-			echo "${VER}/pack-${BUNDLE}-from-$v.tar already exists, skipping."
-		else
-			${SWUPDREPO}/swupd_make_pack --statedir ${UPDATEDIR} $v ${VER} ${BUNDLE} &
-		fi
-	done
-done
-
-for job in $(jobs -p)
-do
-	wait ${job}
-	RET=$?
-	if [ "$RET" != "0" ]; then
-		echo "pack creation failure!"
-		exit $RET
-	fi
-done
-
-# NOTE: your devops will want to expose the completed swupd server build to
-# trial usage at some point.  This would be done via code like:
-#
-#	echo ${VER} > ${SWUPDWEBDIR}/../image/latest.version
-#	STAGING=$(cat ${SWUPDWEBDIR}/version/formatstaging/latest)
-#	if [ "${STAGING}" -lt "${VER}" ]; then
-#		echo ${VER} > ${SWUPDWEBDIR}/version/formatstaging/latest
-#	fi

src/chroot.c

@@ -34,9 +34,8 @@
 
 void chroot_create_full(int newversion)
 {
-	int ret;
 	char *group;
-	char *command;
+	char *param;
 	char *full_dir;
 
 	string_or_die(&full_dir, "%s/%i/full/", image_dir, newversion);
@@ -45,11 +44,12 @@ void chroot_create_full(int newversion)
 
 	/* start with base */
 	LOG(NULL, "Copying chroot os-core to full", "");
-	string_or_die(&command, "rsync -aAX %s/%i/os-core/ %s",
-		      image_dir, newversion, full_dir);
-	ret = system(command);
-	assert(ret == 0);
-	free(command);
+	string_or_die(&param, "%s/%i/os-core/", image_dir, newversion);
+	char *const rsynccmd[] = { "rsync", "-aAX", param, full_dir, NULL };
+	if (system_argv(rsynccmd) != 0) {
+		assert(0);
+	}
+	free(param);
 
 	/* overlay any new files from each group */
 	while (1) {
@@ -59,11 +59,12 @@ void chroot_create_full(int newversion)
 		}
 
 		LOG(NULL, "Overlaying bundle chroot onto full", "%s", group);
-		string_or_die(&command, "rsync -aAX --ignore-existing %s/%i/%s/ %s",
-			      image_dir, newversion, group, full_dir);
-		ret = system(command);
-		assert(ret == 0);
-		free(command);
+		string_or_die(&param, "%s/%i/%s/", image_dir, newversion, group);
+		char *const rsynccmd[] = { "rsync", "-aAX", "--ignore-existing", param, full_dir, NULL };
+		if (system_argv(rsynccmd) != 0) {
+			assert(0);
+		}
+		free(param);
 	}
 	free(full_dir);
 }

src/create_update.c

@@ -288,7 +288,7 @@ int main(int argc, char **argv)
 	printf("Syncing bundle chroots with full chroot\n");
 	chroot_create_full(newversion);
 
-	print_elapsed_time(&previous_time, &current_time);
+	print_elapsed_time("full chroot creation", &previous_time, &current_time);
 
 	printf("Calculating full manifest (this is expensive/slow)\n");
 	old_full = manifest_from_file(current_version, "full");
@@ -307,7 +307,7 @@ int main(int argc, char **argv)
 #warning disabled rename detection for some simplicity
 	// rename_detection(new_full);
 
-	print_elapsed_time(&previous_time, &current_time);
+	print_elapsed_time("full manifest creation", &previous_time, &current_time);
 
 	printf("Entering phase 2: Doing the os-core bundle\n");
 	/* Phase 2 : the os-core set */
@@ -357,7 +357,6 @@ int main(int argc, char **argv)
 			printf("Core component manifest write failed\n");
 			goto exit;
 		}
-		create_manifest_deltas(new_core, manifests_last_versions_list);
 	}
 
 	nest_manifest(new_MoM, new_core);
@@ -458,7 +457,6 @@ int main(int argc, char **argv)
 					printf("%s component manifest write failed\n", group);
 					goto exit;
 				}
-				create_manifest_deltas(newm, manifests_last_versions_list);
 			} else {
 				LOG(NULL, "", "%s component has not changed (after pruning), no new manifest", group);
 				printf("%s component has not changed (after pruning), no new manifest\n", group);
@@ -469,7 +467,7 @@ int main(int argc, char **argv)
 		nest_manifest(new_MoM, newm);
 	}
 
-	print_elapsed_time(&previous_time, &current_time);
+	print_elapsed_time("bundle manifest creation", &previous_time, &current_time);
 
 	printf("Entering phase 4: completion manifests \n");
 	/* Phase 4 : manifest completion */
@@ -480,9 +478,8 @@ int main(int argc, char **argv)
 		printf("Failed to write new MoM\n");
 		goto exit;
 	}
-	create_manifest_deltas(new_MoM, manifests_last_versions_list);
 
-	print_elapsed_time(&previous_time, &current_time);
+	print_elapsed_time("MoM creation", &previous_time, &current_time);
 
 	printf("Entering phase 5: creating download content\n");
 	/* Phase 5: wrapping up */
@@ -499,7 +496,7 @@ int main(int argc, char **argv)
 	/* Step 8: Prepare delta directory */
 	prepare_delta_dir(new_full);
 
-	print_elapsed_time(&previous_time, &current_time);
+	print_elapsed_time("finalize update", &previous_time, &current_time);
 
 	exit_status = EXIT_SUCCESS;
 exit:

src/delta.c

@@ -37,8 +37,8 @@
 
 void __create_delta(struct file *file, int from_version)
 {
-	char *original, *newfile, *outfile, *dotfile, *testnewfile, *sanitycheck;
-	char *conf;
+	char *original, *newfile, *outfile, *dotfile, *testnewfile;
+	char *conf, *param1, *param2;
 	int ret;
 
 	if (file->is_link) {
@@ -65,7 +65,6 @@ void __create_delta(struct file *file, int from_version)
 	string_or_die(&outfile, "%s/%i/delta/%i-%i-%s", conf, file->last_change, from_version, file->last_change, file->hash);
 	string_or_die(&dotfile, "%s/%i/delta/.%i-%i-%s", conf, file->last_change, from_version, file->last_change, file->hash);
 	string_or_die(&testnewfile, "%s/%i/delta/.%i-%i-%s.testnewfile", conf, file->last_change, from_version, file->last_change, file->hash);
-	string_or_die(&sanitycheck, "cmp -s \"%s\" \"%s\"", newfile, testnewfile);
 
 	LOG(file, "Making delta", "%s->%s", original, newfile);
 
@@ -110,13 +109,18 @@ void __create_delta(struct file *file, int from_version)
 		goto out;
 	}
 
-	ret = system(sanitycheck);
-	if (ret == -1 || !WIFEXITED(ret) || WEXITSTATUS(ret) == 2) {
+	string_or_die(&param1, "%s", newfile);
+	string_or_die(&param2, "%s", testnewfile);
+	char *const sanitycheck[] = { "cmp", "-s", param1, param2, NULL };
+	ret = system_argv(sanitycheck);
+	free(param1);
+	free(param2);
+	if (ret == -1 || ret == 2) {
 		printf("Sanity check system command failed %i. \n", ret);
 		printf("%s->%s via diff %s yielded %s\n", original, newfile, dotfile, testnewfile);
 		assert(0);
 		goto out;
-	} else if (WEXITSTATUS(ret) == 1) {
+	} else if (ret == 1) {
 		printf("Delta application resulted in file mismatch %i. \n", ret);
 		printf("%s->%s via diff %s yielded %s\n", original, newfile, dotfile, testnewfile);
 		LOG(file, "Delta mismatch:", "%s->%s via diff %s yielded %s", original, newfile, dotfile, testnewfile);
@@ -137,7 +141,6 @@ void __create_delta(struct file *file, int from_version)
 		LOG(NULL, "Failed to rename", "");
 	}
 out:
-	free(sanitycheck);
 	free(testnewfile);
 	free(conf);
 	free(newfile);

src/fullfiles.c

@@ -24,6 +24,7 @@
 #define _GNU_SOURCE
 #include <assert.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <glib.h>
 #include <libgen.h>
 #include <stdint.h>
@@ -41,7 +42,6 @@
 static void create_fullfile(struct file *file)
 {
 	char *origin;
-	char *tarcommand = NULL;
 	char *tarname = NULL;
 	char *rename_source = NULL;
 	char *rename_target = NULL;
@@ -49,6 +49,8 @@ static void create_fullfile(struct file *file)
 	int ret;
 	struct stat sbuf;
 	char *empty, *indir, *outdir;
+	char *param1, *param2;
+	int stderrfd;
 
 	if (file->is_deleted) {
 		return; /* file got deleted -> by definition we cannot tar it up */
@@ -91,13 +93,22 @@ static void create_fullfile(struct file *file)
 			assert(0);
 		}
 
-		string_or_die(&tarcommand, TAR_COMMAND " -C %s " TAR_PERM_ATTR_ARGS " -cf - --exclude='%s/?*' './%s' 2> /dev/null | " TAR_COMMAND " -C %s " TAR_PERM_ATTR_ARGS " -xf - 2> /dev/null",
-			      dir, base, base, rename_tmpdir);
-		if (system(tarcommand) != 0) {
-			LOG(NULL, "Failed to run command:", "%s", tarcommand);
+		string_or_die(&param1, "--exclude=%s/?*", base);
+		string_or_die(&param2, "./%s", base);
+		char *const tarcfcmd[] = { TAR_COMMAND, "-C", dir, TAR_PERM_ATTR_ARGS_STRLIST, "-cf", "-", param1, param2, NULL };
+		char *const tarxfcmd[] = { TAR_COMMAND, "-C", rename_tmpdir, TAR_PERM_ATTR_ARGS_STRLIST, "-xf", "-", NULL };
+
+		stderrfd = open("/dev/null", O_WRONLY);
+		if (stderrfd == -1) {
+			LOG(NULL, "Failed to open /dev/null", "");
 			assert(0);
 		}
-		free(tarcommand);
+		if (system_argv_pipe(tarcfcmd, -1, stderrfd, tarxfcmd, -1, stderrfd) != 0) {
+			assert(0);
+		}
+		free(param1);
+		free(param2);
+		close(stderrfd);
 
 		string_or_die(&rename_source, "%s/%s", rename_tmpdir, base);
 		string_or_die(&rename_target, "%s/%s", rename_tmpdir, file->hash);
@@ -108,13 +119,13 @@ static void create_fullfile(struct file *file)
 		free(rename_source);
 
 		/* for a directory file, tar up simply with gzip */
-		string_or_die(&tarcommand, TAR_COMMAND " -C %s " TAR_PERM_ATTR_ARGS " -zcf %s/%i/files/%s.tar %s",
-			      rename_tmpdir, outdir, file->last_change, file->hash, file->hash);
-		if (system(tarcommand) != 0) {
-			LOG(NULL, "Failed to run command:", "%s", tarcommand);
+		string_or_die(&param1, "%s/%i/files/%s.tar", outdir, file->last_change, file->hash);
+		char *const tarcmd[] = { TAR_COMMAND, "-C", rename_tmpdir, TAR_PERM_ATTR_ARGS_STRLIST, "-zcf", param1, file->hash, NULL };
+
+		if (system_argv(tarcmd) != 0) {
 			assert(0);
 		}
-		free(tarcommand);
+		free(param1);
 
 		if (rmdir(rename_target)) {
 			LOG(NULL, "rmdir failed for %s", rename_target);
@@ -144,29 +155,38 @@ static void create_fullfile(struct file *file)
 
 		/* step 2a: tar it with each compression type  */
 		// lzma
-		string_or_die(&tarcommand, TAR_COMMAND " --directory=%s " TAR_PERM_ATTR_ARGS " -Jcf %s/%i/files/%s.tar.xz %s",
-			      empty, outdir, file->last_change, file->hash, file->hash);
-		if (system(tarcommand) != 0) {
-			LOG(NULL, "Failed to run command:", "%s", tarcommand);
+		string_or_die(&param1, "--directory=%s", empty);
+		string_or_die(&param2, "%s/%i/files/%s.tar.xz", outdir, file->last_change, file->hash);
+		char *const tarlzmacmd[] = { TAR_COMMAND, param1, TAR_PERM_ATTR_ARGS_STRLIST, "-Jcf", param2, file->hash, NULL };
+
+		if (system_argv(tarlzmacmd) != 0) {
 			assert(0);
 		}
-		free(tarcommand);
+		free(param1);
+		free(param2);
+
 		// gzip
-		string_or_die(&tarcommand, TAR_COMMAND " --directory=%s " TAR_PERM_ATTR_ARGS " -zcf %s/%i/files/%s.tar.gz %s",
-			      empty, outdir, file->last_change, file->hash, file->hash);
-		if (system(tarcommand) != 0) {
-			LOG(NULL, "Failed to run command:", "%s", tarcommand);
+		string_or_die(&param1, "--directory=%s", empty);
+		string_or_die(&param2, "%s/%i/files/%s.tar.gz", outdir, file->last_change, file->hash);
+		char *const targzipcmd[] = { TAR_COMMAND, param1, TAR_PERM_ATTR_ARGS_STRLIST, "-zcf", param2, file->hash, NULL };
+
+		if (system_argv(targzipcmd) != 0) {
 			assert(0);
 		}
-		free(tarcommand);
+		free(param1);
+		free(param2);
+
 #ifdef SWUPD_WITH_BZIP2
-		string_or_die(&tarcommand, TAR_COMMAND " --directory=%s " TAR_PERM_ATTR_ARGS " -jcf %s/%i/files/%s.tar.bz2 %s",
-			      empty, outdir, file->last_change, file->hash, file->hash);
-		if (system(tarcommand) != 0) {
-			LOG(NULL, "Failed to run command:", "%s", tarcommand);
+		string_or_die(&param1, "--directory=%s", empty);
+		string_or_die(&param2, "%s/%i/files/%s.tar.bz2", outdir, file->last_change, file->hash);
+		char *const tarbzip2cmd[] = { TAR_COMMAND, param1, TAR_PERM_ATTR_ARGS_STRLIST, "-jcf", param2, file->hash, NULL };
+
+		if (system_argv(tarbzip2cmd) != 0) {
 			assert(0);
 		}
-		free(tarcommand);
+		free(param1);
+		free(param2);
+
 #endif
 
 		/* step 2b: pick the smallest of the three compression formats */
@@ -302,17 +322,14 @@ void create_fullfiles(struct manifest *manifest)
 {
 	GList *deduped_file_list;
 	char *path;
-	char *cmd;
 	char *conf, *empty;
 
 	empty = config_empty_dir();
 
-	string_or_die(&cmd, "rm -rf %s", empty);
-	if (system(cmd) != 0) {
-		LOG(NULL, "Failed to run command:", "%s", cmd);
+	char *const rmcmd[] = { "rm", "-rf", empty, NULL };
+	if (system_argv(rmcmd) != 0) {
 		assert(0);
 	}
-	free(cmd);
 	g_mkdir_with_parents(empty, S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
 	free(empty);
 

src/helpers.c

@@ -101,22 +101,27 @@ void string_or_die(char **strp, const char *fmt, ...)
 	va_list ap;
 
 	va_start(ap, fmt);
-	if (vasprintf(strp, fmt, ap) <= 0) {
+	if (vasprintf(strp, fmt, ap) < 0) {
 		assert(0);
 	}
 	va_end(ap);
 }
 
-void print_elapsed_time(struct timeval *previous_time, struct timeval *current_time)
+/* "step" should be a descriptive string for the process that completed before
+ * this function is called */
+void print_elapsed_time(const char *step, struct timeval *previous_time,
+			struct timeval *current_time)
 {
 	char *elapsed;
 
+	assert(strlen(step) > 0);
+
 	gettimeofday(current_time, NULL);
 	elapsed = get_elapsed_time(previous_time, current_time);
 	if (strcmp(elapsed, " ") == 0) {
-		printf("\t~0.0 elapsed for Manifest.full creation\n");
+		printf("\t~0.0 elapsed for '%s' step\n", step);
 	} else {
-		printf("\t%s elapsed for full chroot creation\n", elapsed);
+		printf("\t%s elapsed for '%s' step\n", elapsed, step);
 	}
 	previous_time->tv_sec = current_time->tv_sec;
 	previous_time->tv_usec = current_time->tv_usec;
@@ -148,7 +153,7 @@ int system_argv(char *const argv[])
 {
 	int child_exit_status;
 	pid_t pid;
-	int status;
+	int status = -1;
 
 	pid = fork();
 
@@ -181,9 +186,93 @@ int system_argv(char *const argv[])
 			LOG(NULL, "Failed to run command:", "%s", cmdline);
 			free(cmdline);
 		}
-
-		return status;
 	}
+
+	return status;
+}
+
+int system_argv_fd(char *const argv[], int newstdin, int newstdout, int newstderr)
+{
+	int child_exit_status;
+	pid_t pid;
+	int status = -1;
+
+	pid = fork();
+
+	if (pid == 0) { /* child */
+		if (newstdin >= 0) {
+			if (dup2(newstdin, STDIN_FILENO) == -1) {
+				LOG(NULL, "Could not redirect stdin", "");
+				assert(0);
+			}
+			close(newstdin);
+		}
+		if (newstdout >= 0) {
+			if (dup2(newstdout, STDOUT_FILENO) == -1) {
+				LOG(NULL, "Could not redirect stdout", "");
+				assert(0);
+			}
+			close(newstdout);
+		}
+		if (newstderr >= 0) {
+			if (dup2(newstderr, STDERR_FILENO) == -1) {
+				LOG(NULL, "Could not redirect stderr", "");
+				assert(0);
+			}
+			close(newstderr);
+		}
+
+		execvp(*argv, argv);
+		LOG(NULL, "This line must not be reached", "");
+		assert(0);
+	} else if (pid < 0) {
+		LOG(NULL, "Failed to fork a child process", "");
+		assert(0);
+	} else {
+		pid_t ws = waitpid(pid, &child_exit_status, 0);
+
+		if (ws == -1) {
+			LOG(NULL, "Failed to wait for child process", "");
+			assert(0);
+		}
+
+		if (WIFEXITED(child_exit_status)) {
+			status = WEXITSTATUS(child_exit_status);
+		} else {
+			LOG(NULL, "Child process didn't exit", "");
+			assert(0);
+		}
+
+		if (status != 0) {
+			char *cmdline = NULL;
+
+			concat_str_array(&cmdline, argv);
+			LOG(NULL, "Failed to run command:", "%s", cmdline);
+			free(cmdline);
+		}
+	}
+
+	return status;
+}
+
+int system_argv_pipe(char *const argvp1[], int stdinp1, int stderrp1,
+		     char *const argvp2[], int stdoutp2, int stderrp2)
+{
+	int statusp2;
+	int pipefd[2];
+
+	if (pipe(pipefd)) {
+		LOG(NULL, "Failed to create a pipe", "");
+		return -1;
+	}
+	system_argv_fd(argvp1, stdinp1, pipefd[1], stderrp1);
+	close(pipefd[1]);
+	statusp2 = system_argv_fd(argvp2, pipefd[0], stdoutp2, stderrp2);
+	close(pipefd[0]);
+
+	/* Returns the status of the failed process if any
+       If both processes failed returns the status of first one */
+	return statusp2;
 }
 
 void check_root(void)

src/manifest.c

@@ -730,7 +730,6 @@ static int write_manifest_plain(struct manifest *manifest)
 	char *filename = NULL;
 	char *submanifest_filename = NULL;
 	char *manifest_tempdir = NULL;
-	char *tarcommand = NULL;
 	char *tempmanifest = NULL;
 	int ret = -1;
 
@@ -796,16 +795,22 @@ static int write_manifest_plain(struct manifest *manifest)
 			goto write_entry;
 		}
 
+		char *param1, *param2;
+
 		/* Untar the Manifest.BUNDLE.tar and calculate the hash on that,
 		 * otherwise we may get incorrect hashes due to owner permissions on
 		 * the files. */
-		string_or_die(&tarcommand, TAR_COMMAND " -C %s " TAR_PERM_ATTR_ARGS " -xf %s.tar 2> /dev/null",
-				manifest_tempdir, submanifest_filename);
-		if (system(tarcommand) != 0) {
-			LOG(NULL, "Failed to run command:", "%s", tarcommand);
+		string_or_die(&param1, "%s", manifest_tempdir);
+		string_or_die(&param2, "%s.tar", submanifest_filename);
+		char *const tarcmd[] = { TAR_COMMAND, "-C", param1, TAR_PERM_ATTR_ARGS_STRLIST, "-xf", param2, NULL };
+		ret = system_argv(tarcmd);
+		free(param1);
+		free(param2);
+
+		if (ret != 0) {
 			assert(0);
 		}
-		free(tarcommand);
+
 		string_or_die(&tempmanifest, "%s/Manifest.%s", manifest_tempdir, file->filename);
 		populate_file_struct(file, tempmanifest);
 		ret = compute_hash(file, tempmanifest);
@@ -815,18 +820,19 @@ static int write_manifest_plain(struct manifest *manifest)
 		}
 		unlink(tempmanifest);
 		free(tempmanifest);
-write_entry:
+	write_entry:
 		fprintf(out, "%s\t%s\t%i\t%s\n", file_type_to_string(file), file->hash, file->last_change, file->filename);
 		free(submanifest_filename);
 	}
 
 	ret = 0;
 exit:
-	if (rmdir(manifest_tempdir) != 0) {
-		LOG(NULL, "rmdir failed for %s: %s", manifest_tempdir, strerror(errno));
+	if (manifest_tempdir) {
+		if (rmdir(manifest_tempdir) != 0) {
+			LOG(NULL, "rmdir failed for %s: %s", manifest_tempdir, strerror(errno));
+		}
+		free(manifest_tempdir);
 	}
-	free(manifest_tempdir);
-
 	if (out) {
 		fclose(out);
 	}
@@ -840,34 +846,37 @@ exit:
 static int write_manifest_tar(struct manifest *manifest)
 {
 	char *conf = config_output_dir();
-	char *tarcmd = NULL;
-	int ret = -1;
+	char *directory, *manifesttar, *manifestcomp, *manifestsigned;
+	int ret = 0;
 
 	if (conf == NULL) {
 		assert(0);
 	}
 
+	string_or_die(&directory, "--directory=%s/%i", conf, manifest->version);
+	string_or_die(&manifesttar, "%s/%i/Manifest.%s.tar", conf, manifest->version, manifest->component);
+	string_or_die(&manifestcomp, "Manifest.%s", manifest->component);
+	string_or_die(&manifestsigned, "Manifest.%s.signed", manifest->component);
+
 	/* now, tar the thing up for efficient full file download */
 	/* and put the signature of the plain manifest into the archive, too */
 	if (enable_signing) {
-		string_or_die(&tarcmd, TAR_COMMAND " --directory=%s/%i " TAR_PERM_ATTR_ARGS " -Jcf "
-						   "%s/%i/Manifest.%s.tar Manifest.%s Manifest.%s.signed",
-			      conf, manifest->version, conf, manifest->version, manifest->component,
-			      manifest->component, manifest->component);
+		char *const tarcmd[] = { TAR_COMMAND, directory, TAR_PERM_ATTR_ARGS_STRLIST, "-Jcf",
+					 manifesttar, manifestcomp, manifestsigned, NULL };
+		ret = system_argv(tarcmd);
 	} else {
-		string_or_die(&tarcmd, TAR_COMMAND " --directory=%s/%i " TAR_PERM_ATTR_ARGS " -Jcf "
-						   "%s/%i/Manifest.%s.tar Manifest.%s",
-			      conf, manifest->version, conf, manifest->version, manifest->component,
-			      manifest->component);
+		char *const tarcmd[] = { TAR_COMMAND, directory, TAR_PERM_ATTR_ARGS_STRLIST, "-Jcf",
+					 manifesttar, manifestcomp, NULL };
+		ret = system_argv(tarcmd);
+	}
+	if (ret) {
+		fprintf(stderr, "Creation of Manifest.tar failed\n");
 	}
 
-	if (system(tarcmd) != 0) {
-		fprintf(stderr, "Creation of Manifest.tar failed\n");
-		goto exit;
-	}
-	ret = 0;
-exit:
-	free(tarcmd);
+	free(directory);
+	free(manifesttar);
+	free(manifestcomp);
+	free(manifestsigned);
 	free(conf);
 	return ret;
 }

src/pack.c

@@ -40,21 +40,19 @@
 
 static void empty_pack_stage(int full, int from_version, int to_version, char *module)
 {
-	char *cmd;
+	char *param;
 	char *path;
-	int ret;
 
 	// clean any stale data (eg: re-run after a failure)
-	string_or_die(&cmd, "rm -rf %s/%s/%i_to_%i/", packstage_dir, module,
-		      from_version, to_version);
-	ret = system(cmd);
-	if (ret) {
+	string_or_die(&param, "%s/%s/%i_to_%i/", packstage_dir, module, from_version, to_version);
+	char *const rmcmd[] = { "rm", "-fr", param, NULL };
+	if (system_argv(rmcmd) != 0) {
 		fprintf(stderr, "Failed to clean %s/%s/%i_to_%i\n",
 			packstage_dir, module, from_version, to_version);
-		free(cmd);
+		free(param);
 		exit(EXIT_FAILURE);
 	}
-	free(cmd);
+	free(param);
 
 	if (!full) {
 		// (re)create module/version/{delta,staged}
@@ -89,7 +87,7 @@ static void explode_pack_stage(int from_version, int to_version, char *module)
 	}
 	free(path);
 	while (1) {
-		char *path, *tar;
+		char *path, *param;
 		int ret;
 
 		entry = readdir(dir);
@@ -117,13 +115,12 @@ static void explode_pack_stage(int from_version, int to_version, char *module)
 		 * the resulting pack is slightly smaller, and in addition, we're saving CPU
 		 * time on the client...
 		 */
-		string_or_die(&tar, TAR_COMMAND " --directory=%s/%s/%i_to_%i/staged " TAR_WARN_ARGS " " TAR_PERM_ATTR_ARGS " -xf %s",
-			      packstage_dir, module, from_version, to_version, path);
-		ret = system(tar);
-		if (!ret) {
+		string_or_die(&param, "%s/%s/%i_to_%i/staged", packstage_dir, module, from_version, to_version);
+		char *const tarcmd[] = { TAR_COMMAND, "-C", param, TAR_WARN_ARGS, TAR_PERM_ATTR_ARGS_STRLIST, "-xf", path, NULL };
+		if (system_argv(tarcmd) == 0) {
 			unlink(path);
 		}
-		free(tar);
+		free(param);
 		free(path);
 	}
 	closedir(dir);
@@ -334,7 +331,7 @@ static int make_final_pack(struct packdata *pack)
 	GList *item;
 	struct file *file;
 	int ret;
-	char *tar;
+	char *param1, *param2;
 	double penalty;
 
 	LOG(NULL, "make_final_pack", "%s: %i to %i", pack->module, pack->from, pack->to);
@@ -438,6 +435,8 @@ static int make_final_pack(struct packdata *pack)
 		explode_pack_stage(pack->from, pack->to, pack->module);
 	}
 
+	char *bundle_delta = NULL;
+
 	/* now... link in the Manifest pack */
 	if (pack->from != 0) {
 		char *from, *to;
@@ -457,14 +456,18 @@ static int make_final_pack(struct packdata *pack)
 
 		ret = link(from, to);
 		if (ret) {
-			LOG(NULL, "Failed to link", "Manifest-delta-from-%i", pack->from);
+			LOG(NULL, "Failed to link", "Manifest-%s-delta-from-%i (%s)", pack->module, pack->from, strerror(errno));
+		} else {
+			string_or_die(&bundle_delta, "Manifest-%s-delta-from-%i", pack->module, pack->from);
 		}
 
 		free(from);
 		free(to);
 	}
 
-	/* now... link in the MoM Manifest into the base pack */
+	char *mom_delta = NULL;
+
+	/* now... link in the MoM Manifest into the os-core pack */
 	if ((pack->from != 0) && (strcmp(pack->module, "os-core") == 0)) {
 		char *from, *to;
 		struct stat st;
@@ -482,7 +485,9 @@ static int make_final_pack(struct packdata *pack)
 
 		ret = link(from, to);
 		if (ret) {
-			LOG(NULL, "Failed to link", "Manifest-delta-from-%i", pack->from);
+			LOG(NULL, "Failed to link", "Manifest-MoM-delta-from-%i (%s)", pack->from, strerror(errno));
+		} else {
+			string_or_die(&mom_delta, "Manifest-MoM-delta-from-%i", pack->from);
 		}
 
 		free(from);
@@ -491,15 +496,15 @@ static int make_final_pack(struct packdata *pack)
 
 	/* tar the staging directory up */
 	LOG(NULL, "starting tar for pack", "%s: %i to %i", pack->module, pack->from, pack->to);
-	string_or_die(&tar, TAR_COMMAND " " TAR_PERM_ATTR_ARGS " --directory=%s/%s/%i_to_%i/ "
-					"--numeric-owner -Jcf %s/%i/pack-%s-from-%i.tar delta staged",
-		      packstage_dir, pack->module, pack->from, pack->to, staging_dir, pack->to,
-		      pack->module, pack->from);
-	ret = system(tar);
-	free(tar);
+	string_or_die(&param1, "%s/%s/%i_to_%i/", packstage_dir, pack->module, pack->from, pack->to);
+	string_or_die(&param2, "%s/%i/pack-%s-from-%i.tar", staging_dir, pack->to, pack->module, pack->from);
+	char *const tarcmd[] = { TAR_COMMAND, "-C", param1, TAR_PERM_ATTR_ARGS_STRLIST, "--numeric-owner", "-Jcf", param2, "delta", "staged", bundle_delta, mom_delta, NULL };
+	ret = system_argv(tarcmd);
+	free(param1);
+	free(param2);
 	LOG(NULL, "finished tar for pack", "%s: %i to %i", pack->module, pack->from, pack->to);
 	/* FIXME: this is a hack workaround, needs diagnosed and removed */
-	if ((ret != 0) && (ret != 256)) {
+	if ((ret != 0) && (ret != 1)) {
 		fprintf(stderr, "Unexpected return value (%d) creating tar of pack %s from %i to %i\n",
 			ret, pack->module, pack->from, pack->to);
 	} else {

src/signature.c

@@ -35,10 +35,6 @@
 
 static char *make_filename(const char *, const char *, const char *);
 
-static const char *CMD_FMT = "openssl smime -sign -in %s -binary "
-			     "-out %s.signed -outform PEM -md sha256 -inkey %s -signer %s "
-			     "-certfile %s -passin file:%s";
-
 static char *leaf_key = NULL;
 static char *leaf_cert = NULL;
 static char *ca_chain_cert = NULL;
@@ -150,7 +146,7 @@ void signature_terminate(void)
  */
 bool signature_sign(const char *filename)
 {
-	char *cmd = NULL;
+	char *param1, *param2;
 	int status;
 
 	if (!enable_signing) {
@@ -160,12 +156,18 @@ bool signature_sign(const char *filename)
 	if (!initialized) {
 		return false;
 	}
-	string_or_die(&cmd, CMD_FMT, filename, filename, leaf_key, leaf_cert,
-		      ca_chain_cert, passphrase);
-	status = system(cmd);
-	if (status) {
-		printf("Bad status %d from signing command:%s\n", status, cmd);
+	string_or_die(&param1, "%s.signed", filename);
+	string_or_die(&param2, "file:%s", passphrase);
+	char *const opensslcmd[] = { "openssl", "smime", "-sign", "-in", (char *)filename, "-binary",
+				     "-out", param1, "-outform", " PEM", "-md", "sha256", "-inkey",
+				     leaf_key, "-signer", leaf_cert, "-certfile", ca_chain_cert,
+				     "-passin", param2, NULL };
+	status = system_argv(opensslcmd);
+	if (status != 0) {
+		printf("Bad status %d from signing command\n", status);
 	}
-	free(cmd);
+	free(param1);
+	free(param2);
+
 	return status == 0;
 }

src/versions.c

@@ -27,6 +27,7 @@
 #include <ctype.h>
 #include <dirent.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <glib.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -126,35 +127,63 @@ void ensure_version_image_exists(int version)
 void write_cookiecrumbs_to_download_area(int version)
 {
 	char *conf;
-	char *cmd;
+	char *filename, *strformat;
+	FILE *versionfd, *formatfd;
 
 	conf = config_output_dir();
 	if (conf == NULL) {
 		assert(0);
 	}
 
-	string_or_die(&cmd, "echo %s > %s/%i/swupd-server-src-version", VERSION, conf, version);
-	if (system(cmd) != 0) {
+	string_or_die(&filename, "%s/%i/swupd-server-src-version", conf, version);
+	versionfd = fopen(filename, "w");
+	if (versionfd == NULL) {
 		assert(0);
 	}
-	free(cmd);
-
-	string_or_die(&cmd, "echo %llu > %s/%i/format", format, conf, version);
-	if (system(cmd) != 0) {
+	if (fwrite(VERSION, 1, strlen(VERSION), versionfd) != strlen(VERSION)) {
 		assert(0);
 	}
-	free(cmd);
+	fclose(versionfd);
+	free(filename);
 
-/* written from pack_maker.sh */
+	string_or_die(&filename, "%s/%i/format", conf, version);
+	string_or_die(&strformat, "%llu", format);
+	formatfd = fopen(filename, "w");
+	if (formatfd == NULL) {
+		assert(0);
+	}
+	if (fwrite(strformat, 1, strlen(strformat), formatfd) != strlen(strformat)) {
+		assert(0);
+	}
+	fclose(formatfd);
+	free(filename);
+	free(strformat);
+
+/* Updating the WEBDIR/version/formatN/latest file is an operation very closely tied to a DevOps
+ * workflow and not something swupd should make a decision about.
+ *
+ * The supported format values (replacing the "N" in the path above) are:
+ *
+ * - "staging": intended for testing only
+ * - [1-+inf] (positive integer): the number that a corresponding version of swupd-client
+ *   understands to run software updates. For example, if a given swupd-client release understands
+ *   format 1, then WEBDIR/version/format1/latest contains the value of most recent release that
+ *   that swupd-client can update to.
+ *
+ * Adapt this step appropriately for your DevOps flow.
+ */
 #if 0
 	FILE *file;
 	char *fullfile = NULL;
+	char *param;
 
-	string_or_die(&cmd, "mkdir -p %s/version/formatstaging/", conf);
-	if (system(cmd) != 0) {
+	string_or_die($param, "%s/version/formatstaging/", conf);
+	char *const mkdircmd[] = { "mkdir", "-p", param, NULL };
+
+	if (system_argv(mkdircmd) != 0) {
 		assert(0);
 	}
-	free(cmd);
+	free(param);
 
 	string_or_die(&fullfile, "%s/version/formatstaging/latest", conf);
 	file = fopen(fullfile, "w");

test/functional/full-run-delta/test.bats

@@ -79,6 +79,11 @@ setup() {
   [[ 1 -eq $(grep '/foobarbaz$' $DIR/www/20/Manifest.included-nested | wc -l) ]]
   [[ 6 -eq $(tar -tf $DIR/www/10/pack-test-bundle-from-0.tar | wc -l) ]]
   [[ 4 -eq $(tar -tf $DIR/www/20/pack-test-bundle-from-0.tar | wc -l) ]]
+
+  # delta packs should contain appropriate delta manifests
+  [[ $(tar -tf $DIR/www/20/pack-os-core-from-10.tar | grep '^Manifest-os-core-delta-from-10') ]]
+  [[ $(tar -tf $DIR/www/20/pack-os-core-from-10.tar | grep '^Manifest-MoM-delta-from-10') ]]
+  [[ $(tar -tf $DIR/www/20/pack-test-bundle-from-10.tar | grep '^Manifest-test-bundle-delta-from-10') ]]
 }
 
 # vi: ft=sh ts=8 sw=2 sts=2 et tw=80