lib: cleanup transaction_store and handle errors

Author: Duncaen

include/xbps_api_impl.h

@@ -90,7 +90,8 @@ bool HIDDEN xbps_transaction_check_revdeps(struct xbps_handle *, xbps_array_t);
 bool HIDDEN xbps_transaction_check_shlibs(struct xbps_handle *, xbps_array_t);
 bool HIDDEN xbps_transaction_check_replaces(struct xbps_handle *, xbps_array_t);
 int HIDDEN xbps_transaction_check_conflicts(struct xbps_handle *, xbps_array_t);
-int HIDDEN xbps_transaction_store(struct xbps_handle *, xbps_array_t, xbps_dictionary_t, bool);
+int HIDDEN transaction_store(struct xbps_handle *xhp, xbps_dictionary_t pkgrd,
+                             bool autoinstall);
 int HIDDEN xbps_transaction_init(struct xbps_handle *);
 int HIDDEN xbps_transaction_files(struct xbps_handle *,
 		xbps_object_iterator_t);

lib/transaction_ops.c

@@ -217,24 +217,16 @@ trans_find_pkg(struct xbps_handle *xhp, const char *pkg, bool force)
 	if (!force && xbps_dictionary_get(pkg_repod, "hold"))
 		ttype = XBPS_TRANS_HOLD;
 
-	/*
-	 * Store pkgd from repo into the transaction.
-	 */
-	if (!xbps_transaction_pkg_type_set(pkg_repod, ttype)) {
-		return EINVAL;
-	}
 
 	/*
 	 * Set automatic-install to true if it was requested and this is a new install.
 	 */
 	if (ttype == XBPS_TRANS_INSTALL)
 		autoinst = xhp->flags & XBPS_FLAG_INSTALL_AUTO;
 
-	if (!xbps_transaction_store(xhp, pkgs, pkg_repod, autoinst)) {
+	if (!xbps_transaction_pkg_type_set(pkg_repod, ttype))
 		return EINVAL;
-	}
-
-	return 0;
+	return transaction_store(xhp, pkg_repod, autoinst);
 }
 
 /*
@@ -464,96 +456,100 @@ xbps_transaction_remove_pkg(struct xbps_handle *xhp,
 			    bool recursive)
 {
 	xbps_dictionary_t pkgd;
-	xbps_array_t pkgs, orphans, orphans_pkg;
-	xbps_object_t obj;
-	int rv = 0;
+	int r;
 
 	assert(xhp);
 	assert(pkgname);
 
-	if ((pkgd = xbps_pkgdb_get_pkg(xhp, pkgname)) == NULL) {
-		/* pkg not installed */
-		return ENOENT;
-	}
-	/*
-	 * Prepare transaction dictionary and missing deps array.
-	 */
-	if ((rv = xbps_transaction_init(xhp)) != 0)
-		return rv;
+	r = xbps_transaction_init(xhp);
+	if (r < 0)
+		return r;
 
-	pkgs = xbps_dictionary_get(xhp->transd, "packages");
+	pkgd = xbps_pkgdb_get_pkg(xhp, pkgname);
+	if (!pkgd)
+		return -errno;
 
-	if (!recursive)
-		goto rmpkg;
 	/*
 	 * If recursive is set, find out which packages would be orphans
 	 * if the supplied package were already removed.
 	 */
-	if ((orphans_pkg = xbps_array_create()) == NULL)
-		return ENOMEM;
+	// XXX: this is stupid and incomplete if multiple packages
+	// are removed at once, in that case orphans would not be correctly
+	// be identified...
+	if (recursive) {
+		xbps_array_t orphans, orphans_pkg;
+
+		orphans_pkg = xbps_array_create();
+		if (!orphans_pkg)
+			return xbps_error_oom();
+
+		if (!xbps_array_set_cstring_nocopy(orphans_pkg, 0, pkgname)) {
+			xbps_object_release(orphans_pkg);
+			return xbps_error_oom();
+		}
 
-	xbps_array_set_cstring_nocopy(orphans_pkg, 0, pkgname);
-	orphans = xbps_find_pkg_orphans(xhp, orphans_pkg);
-	xbps_object_release(orphans_pkg);
-	if (xbps_object_type(orphans) != XBPS_TYPE_ARRAY)
-		return EINVAL;
 
-	for (unsigned int i = 0; i < xbps_array_count(orphans); i++) {
-		obj = xbps_array_get(orphans, i);
-		xbps_transaction_pkg_type_set(obj, XBPS_TRANS_REMOVE);
-		if (!xbps_transaction_store(xhp, pkgs, obj, false)) {
-			return EINVAL;
+		orphans = xbps_find_pkg_orphans(xhp, orphans_pkg);
+		if (!orphans) {
+			xbps_object_release(orphans_pkg);
+			return -errno;
+		}
+		xbps_object_release(orphans_pkg);
+		for (unsigned int i = 0; i < xbps_array_count(orphans); i++) {
+			xbps_dictionary_t opkgd;
+
+			opkgd = xbps_array_get(orphans, i);
+			if (!opkgd)
+				xbps_unreachable();
+
+			xbps_transaction_pkg_type_set(opkgd, XBPS_TRANS_REMOVE);
+			r = transaction_store(xhp, opkgd, false);
+			if (r < 0) {
+				xbps_object_release(orphans);
+				return r;
+			}
 		}
 	}
-	xbps_object_release(orphans);
-	return rv;
 
-rmpkg:
-	/*
-	 * Add pkg dictionary into the transaction pkgs queue.
-	 */
+	// XXX: why do we modify supposedly read only pkgd's
 	xbps_transaction_pkg_type_set(pkgd, XBPS_TRANS_REMOVE);
-	if (!xbps_transaction_store(xhp, pkgs, pkgd, false)) {
-		return EINVAL;
-	}
-	return rv;
+	return transaction_store(xhp, pkgd, false);
 }
 
 int
 xbps_transaction_autoremove_pkgs(struct xbps_handle *xhp)
 {
-	xbps_array_t orphans, pkgs;
-	xbps_object_t obj;
-	int rv = 0;
+	xbps_array_t orphans;
+	int r;
+
+	r = xbps_transaction_init(xhp);
+	if (r < 0)
+		return r;
 
 	orphans = xbps_find_pkg_orphans(xhp, NULL);
+	if (!orphans)
+		return -errno;
 	if (xbps_array_count(orphans) == 0) {
-		/* no orphans? we are done */
-		goto out;
+		xbps_object_release(orphans);
+		return 0;
 	}
-	/*
-	 * Prepare transaction dictionary and missing deps array.
-	 */
-	if ((rv = xbps_transaction_init(xhp)) != 0)
-		goto out;
 
-	pkgs = xbps_dictionary_get(xhp->transd, "packages");
-	/*
-	 * Add pkg orphan dictionary into the transaction pkgs queue.
-	 */
 	for (unsigned int i = 0; i < xbps_array_count(orphans); i++) {
-		obj = xbps_array_get(orphans, i);
-		xbps_transaction_pkg_type_set(obj, XBPS_TRANS_REMOVE);
-		if (!xbps_transaction_store(xhp, pkgs, obj, false)) {
-			rv = EINVAL;
-			goto out;
+		xbps_object_t pkgd;
+
+		pkgd = xbps_array_get(orphans, i);
+		if (!pkgd)
+			xbps_unreachable();
+
+		xbps_transaction_pkg_type_set(pkgd, XBPS_TRANS_REMOVE);
+		r = transaction_store(xhp, pkgd, false);
+		if (r < 0) {
+			return r;
 		}
 	}
-out:
-	if (orphans)
-		xbps_object_release(orphans);
 
-	return rv;
+	xbps_object_release(orphans);
+	return 0;
 }
 
 xbps_trans_type_t

lib/transaction_pkg_deps.c

@@ -148,6 +148,7 @@ repo_deps(struct xbps_handle *xhp,
 	while ((obj = xbps_object_iterator_next(iter))) {
 		bool error = false, foundvpkg = false;
 		bool autoinst = true;
+		int r;
 
 		ttype = XBPS_TRANS_UNKNOWN;
 		reqpkg = xbps_string_cstring_nocopy(obj);
@@ -342,12 +343,13 @@ repo_deps(struct xbps_handle *xhp,
 		if (ttype == XBPS_TRANS_CONFIGURE) {
 			if (!xbps_transaction_pkg_type_set(curpkgd, ttype)) {
 				rv = EINVAL;
-				xbps_dbg_printf("xbps_transaction_pkg_type_set failed for `%s': %s\n", reqpkg, strerror(rv));
+				xbps_error_printf("xbps_transaction_pkg_type_set failed for `%s': %s\n", reqpkg, strerror(rv));
 				break;
 			}
-			if (!xbps_transaction_store(xhp, pkgs, curpkgd, autoinst)) {
+			r = transaction_store(xhp, curpkgd, autoinst);
+			if (r < 0) {
+				xbps_error_printf("transaction_store failed for `%s': %s\n", reqpkg, strerror(rv));
 				rv = EINVAL;
-				xbps_dbg_printf("xbps_transaction_store failed for `%s': %s\n", reqpkg, strerror(rv));
 				break;
 			}
 			continue;
@@ -482,9 +484,10 @@ repo_deps(struct xbps_handle *xhp,
 			xbps_dbg_printf("xbps_transaction_pkg_type_set failed for `%s': %s\n", reqpkg, strerror(rv));
 			break;
 		}
-		if (!xbps_transaction_store(xhp, pkgs, repopkgd, autoinst)) {
+		r = transaction_store(xhp, repopkgd, autoinst);
+		if (r < 0) {
 			rv = EINVAL;
-			xbps_dbg_printf("xbps_transaction_store failed for `%s': %s\n", reqpkg, strerror(rv));
+			xbps_error_printf("transaction_store failed for `%s': %s\n", reqpkg, strerror(rv));
 			break;
 		}
 	}

lib/transaction_store.c

@@ -90,22 +90,26 @@ package_self_replace(xbps_dictionary_t pkgd, const char *pkgname)
 }
 
 int HIDDEN
-xbps_transaction_store(struct xbps_handle *xhp, xbps_array_t pkgs,
-                       xbps_dictionary_t pkgrd, bool autoinst)
+transaction_store(struct xbps_handle *xhp, xbps_dictionary_t pkgrd,
+                  bool autoinst)
 {
+	xbps_array_t pkgs;
 	xbps_dictionary_t pkgd;
 	const char *pkgver = NULL, *pkgname = NULL, *repo = NULL;
 	int r;
 
 	assert(xhp);
-	assert(pkgs);
 	assert(pkgrd);
 
 	if (!xbps_dictionary_get_cstring_nocopy(pkgrd, "pkgver", &pkgver))
 		xbps_unreachable();
 	if (!xbps_dictionary_get_cstring_nocopy(pkgrd, "pkgname", &pkgname))
 		xbps_unreachable();
 
+	pkgs = xbps_dictionary_get(xhp->transd, "packages");
+	if (!pkgs)
+		xbps_unreachable();
+
 	r = transaction_replace_package(pkgs, pkgname, pkgver);
 	if (r < 0)
 		return r;