socsvn commit: r255644 - in soc2013/mattbw/backend: . jobs

[mattbw at FreeBSD.org]

Author: mattbw
Date: Wed Aug  7 15:59:57 2013
New Revision: 255644
URL: http://svnweb.FreeBSD.org/socsvn/?view=rev&rev=255644

Log:
  Slightly cleaned up the checking code and factored out some more functions.
  
  (Pattern emerging here...)
  
  Also fixed a logical error in checking which would likely disable checking
  entirely.
  

Modified:
  soc2013/mattbw/backend/jobs.c
  soc2013/mattbw/backend/jobs/check.c

Modified: soc2013/mattbw/backend/jobs.c
==============================================================================
--- soc2013/mattbw/backend/jobs.c	Wed Aug  7 12:03:34 2013	(r255643)
+++ soc2013/mattbw/backend/jobs.c	Wed Aug  7 15:59:57 2013	(r255644)
@@ -287,7 +287,7 @@
 
 	success = true;
 
-	if (package_ids == NULL) {
+	if (package_ids != NULL) {
 		success = jobs_check_package_ids(jobs, package_ids, count,
 		    spec->reject_non_updates);
 	}

Modified: soc2013/mattbw/backend/jobs/check.c
==============================================================================
--- soc2013/mattbw/backend/jobs/check.c	Wed Aug  7 12:03:34 2013	(r255643)
+++ soc2013/mattbw/backend/jobs/check.c	Wed Aug  7 15:59:57 2013	(r255644)
@@ -33,6 +33,8 @@
 
 static bool jobs_check_id_on_package(struct pkg *pkg, gchar **split_id, const char *name, const char *version);
 static bool jobs_check_split_package_ids(struct pkg_jobs *jobs, gchar ***id_splits, guint count, bool reject_non_updates);
+static gchar ***make_package_id_splits(gchar **package_ids, guint count);
+static void free_package_id_splits(gchar ****id_splits_p, guint count);
 
 /* Checks a solved job against a string vector of PackageIDs to ensure any
  * packages that match the PackageIDs match them fully.
@@ -42,50 +44,26 @@
     guint count, bool reject_non_updates)
 {
 	bool		success;
-	bool		split_success;
-	unsigned int	i;
 	gchar	     ***id_splits;
 
 	assert(jobs != NULL);
 	assert(package_ids != NULL);
+	assert(0 < count);
 
 	success = false;
 	id_splits = NULL;
 
-	count = g_strv_length(package_ids);
-	if (count == 0)
-		goto cleanup;
-
 	/* Split all the IDs first so we don't have to do it multiple times */
-	id_splits = calloc(count, sizeof(gchar **));
-	if (id_splits == NULL)
-		goto cleanup;
-
-	split_success = true;
-	for (i = 0; i < count; i++) {
-		id_splits[i] = pk_package_id_split(package_ids[i]);
-		if (id_splits[i] == NULL) {
-			split_success = false;
-			break;
-		}
-	}
-	if (!split_success) {
-		goto cleanup;
-	}
+	id_splits = make_package_id_splits(package_ids, count);
+	if (id_splits != NULL) {
 
-	/* Now do the actual checking, per package. */
-	success = jobs_check_split_package_ids(jobs, id_splits, count,
-	    reject_non_updates);
+		/* Now do the actual checking, per package. */
+		success = jobs_check_split_package_ids(jobs, id_splits, count,
+		    reject_non_updates);
 
-cleanup:
-	if (id_splits != NULL) {
-		for (i = 0; i < count; i++)
-			g_strfreev(id_splits[i]);
-		free(id_splits);
+		free_package_id_splits(&id_splits, count);
 	}
 
-	/*pkg_free(pkg);*/
-
 	return success;
 }
 
@@ -135,11 +113,16 @@
 	assert(id_splits != NULL);
 	assert(0 < count);
 
+
+	pkg = NULL;
 	success = true;
  	while (success) {
  		err = pkg_jobs(jobs, &pkg);
  		if (err != EPKG_OK) {
- 			success = false;
+ 			/* Did we reach the end of the job iterator? */
+ 			if (err != EPKG_END) {
+ 				success = false;
+ 			}
  			break;	
  		}
 
@@ -150,6 +133,7 @@
 
  		for (i = 0; i < count; i++) {
  			if (!success) {
+ 				/* Leave the for loop, not the while loop. */
  				break;
  			}
 
@@ -163,7 +147,60 @@
  				success = false;
 	 		}
  		}
+
+ 		/* Do not free the struct pkg, we actually don't own it. */
  	}
 
  	return success;
- }
+}
+
+static gchar ***
+make_package_id_splits(gchar **package_ids, guint count)
+{
+	guint		i;
+	gchar	     ***id_splits;
+
+	id_splits = calloc(count, sizeof(gchar **));
+
+	if (id_splits != NULL) {
+		bool		split_success;
+
+		split_success = true;
+		for (i = 0; i < count; i++) {
+			id_splits[i] = pk_package_id_split(package_ids[i]);
+			if (id_splits[i] == NULL) {
+				split_success = false;
+				break;
+			}
+		}
+
+		if (!split_success) {
+			free_package_id_splits(&id_splits, count);
+		}
+
+		/* split_success if and only if id_splits != NULL */
+		assert(split_success || id_splits == NULL);
+		assert(!split_success || id_splits != NULL);
+	}
+
+	return id_splits;
+}
+
+static void
+free_package_id_splits(gchar ****id_splits_p, guint count)
+{
+	guint		i;
+
+	assert(id_splits_p != NULL);
+
+	if (*id_splits_p != NULL) {
+		for (i = 0; i < count; i++) {
+			g_strfreev((*id_splits_p)[i]);
+		}
+
+		free(*id_splits_p);
+		*id_splits_p = NULL;
+	}
+
+	assert(*id_splits_p == NULL);
+}