env: Allow returning errors from hdelete_r()

At present this function returns 1 on success and 0 on failure. But in
the latter case it provides no indication of what went wrong.

If an attempt is made to delete a non-existent variable, the caller may
want to ignore this error. This happens when setting a non-existent
variable to "", for example.

Update the function to return 0 on success and a useful error code on
failure. Add a function comment too.

Make sure that env_set() does not return an error if it is deleting a
variable that doesn't exist. We could update env_set() to return useful
error numbers also, but that is beyond the scope of this change.

Signed-off-by: Simon Glass <sjg@chromium.org>

wip

cmd/nvedit.c

@@ -266,7 +266,9 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 	/* Delete only ? */
 	if (argc < 3 || argv[2] == NULL) {
 		int rc = hdelete_r(name, &env_htab, env_flag);
-		return !rc;
+
+		/* If the variable didn't exist, don't report an error */
+		return rc && rc != -ENOENT ? 1 : 0;
 	}
 
 	/*
@@ -895,7 +897,7 @@ static int do_env_delete(struct cmd_tbl *cmdtp, int flag,
 	while (--argc > 0) {
 		char *name = *++argv;
 
-		if (!hdelete_r(name, &env_htab, env_flag))
+		if (hdelete_r(name, &env_htab, env_flag))
 			ret = 1;
 	}
 

include/search.h

@@ -80,7 +80,16 @@ int hsearch_r(struct env_entry item, enum env_action action,
 int hmatch_r(const char *match, int last_idx, struct env_entry **retval,
 	     struct hsearch_data *htab);
 
-/* Search and delete entry matching "key" in internal hash table. */
+/**
+ * hdelete_r() - Search and delete entry in internal hash table
+ *
+ * @key: Name of entry to delete
+ * @htab: Hash table
+ * @flag: Flags to use (H_...)
+ * @return 0 on success, -ENOENT if not found, -EPERM if the hash table callback
+ *	rejected changing the variable, -EINVAL if the hash table refused to
+ *	delete the variable
+ */
 int hdelete_r(const char *key, struct hsearch_data *htab, int flag);
 
 ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,

lib/hashtable.c

@@ -472,7 +472,7 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 	idx = hsearch_r(e, ENV_FIND, &ep, htab, 0);
 	if (idx == 0) {
 		__set_errno(ESRCH);
-		return 0;	/* not found */
+		return -ENOENT;	/* not found */
 	}
 
 	/* Check for permission */
@@ -481,7 +481,7 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 		debug("change_ok() rejected deleting variable "
 			"%s, skipping it!\n", key);
 		__set_errno(EPERM);
-		return 0;
+		return -EPERM;
 	}
 
 	/* If there is a callback, call it */
@@ -490,12 +490,12 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 		debug("callback() rejected deleting variable "
 			"%s, skipping it!\n", key);
 		__set_errno(EINVAL);
-		return 0;
+		return -EINVAL;
 	}
 
 	_hdelete(key, htab, ep, idx);
 
-	return 1;
+	return 0;
 }
 
 #if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV))
@@ -917,7 +917,7 @@ int himport_r(struct hsearch_data *htab,
 			if (!drop_var_from_set(name, nvars, localvars))
 				continue;
 
-			if (hdelete_r(name, htab, flag) == 0)
+			if (hdelete_r(name, htab, flag))
 				debug("DELETE ERROR ##############################\n");
 
 			continue;
@@ -979,7 +979,7 @@ int himport_r(struct hsearch_data *htab,
 		 * b) if the variable was not present in current env, we notify
 		 *    it might be a typo
 		 */
-		if (hdelete_r(localvars[i], htab, flag) == 0)
+		if (hdelete_r(localvars[i], htab, flag))
 			printf("WARNING: '%s' neither in running nor in imported env!\n", localvars[i]);
 		else
 			printf("WARNING: '%s' not in imported env, deleting it!\n", localvars[i]);

test/env/hashtable.c

@@ -80,7 +80,7 @@ static int htab_create_delete(struct unit_test_state *uts,
 		ut_asserteq_str(key, ritem->key);
 		ut_asserteq_str(key, ritem->data);
 
-		ut_asserteq(1, hdelete_r(key, htab, 0));
+		ut_asserteq(0, hdelete_r(key, htab, 0));
 	}
 
 	return 0;