Prevent storage backends name collisions

Because of Transient storage being a special-case stevedore, the
STV_Foreach logic would only work after the child's startup once
all storage backends are defined. So during the setup, collisions
would only be detected for the Transient storage.

The underlying STV__iter function is now pretty much like a plain
VTAILQ_FOREACH loop with an additional CHECK_OBJ_ORNULL step on all
iterations.

As a result, the transient storage is appended to the list at the
end of the manager's setup and is now reported last in the CLI's
storage.list command.

Error messages related to the -s option are slightly more helpful.

Fixes #2321

bin/varnishd/storage/mgt_stevedore.c

@@ -57,17 +57,11 @@ STV__iter(struct stevedore ** const pp)
 
 	AN(pp);
 	CHECK_OBJ_ORNULL(*pp, STEVEDORE_MAGIC);
-	if (*pp == stv_transient) {
-		*pp = NULL;
-		return (0);
-	}
 	if (*pp != NULL)
 		*pp = VTAILQ_NEXT(*pp, list);
 	else
 		*pp = VTAILQ_FIRST(&stevedores);
-	if (*pp == NULL)
-		*pp = stv_transient;
-	return (1);
+	return (*pp != NULL);
 }
 
 /*--------------------------------------------------------------------*/
@@ -128,6 +122,26 @@ static const struct choice STV_choice[] = {
 	{ NULL,		NULL }
 };
 
+static void
+stv_check_ident(const char *spec, const char *ident)
+{
+	struct stevedore *stv;
+	unsigned found = 0;
+
+	if (!strcmp(ident, TRANSIENT_STORAGE))
+		found = (stv_transient != NULL);
+	else {
+		STV_Foreach(stv)
+			if (!strcmp(stv->ident, ident)) {
+				found = 1;
+				break;
+			}
+	}
+
+	if (found)
+		ARGV_ERR("(-s %s) '%s' is already defined\n", spec, ident);
+}
+
 void
 STV_Config(const char *spec)
 {
@@ -135,7 +149,6 @@ STV_Config(const char *spec)
 	const char *p, *q;
 	struct stevedore *stv;
 	const struct stevedore *stv2;
-	struct stevedore *stv3;
 	int ac, l;
 	static unsigned seq = 0;
 
@@ -183,15 +196,12 @@ STV_Config(const char *spec)
 		bprintf(stv->ident, "%.*s", l, spec);
 	}
 
-	STV_Foreach(stv3)
-		if (!strcmp(stv3->ident, stv->ident))
-			ARGV_ERR("(-s%s=%s) already defined once\n",
-			    stv->ident, stv->name);
+	stv_check_ident(spec, stv->ident);
 
 	if (stv->init != NULL)
 		stv->init(stv, ac, av);
 	else if (ac != 0)
-		ARGV_ERR("(-s%s) too many arguments\n", stv->name);
+		ARGV_ERR("(-s %s) too many arguments\n", stv->name);
 
 	AN(stv->allocobj);
 	AN(stv->methods);
@@ -199,9 +209,8 @@ STV_Config(const char *spec)
 	if (!strcmp(stv->ident, TRANSIENT_STORAGE)) {
 		AZ(stv_transient);
 		stv_transient = stv;
-	} else {
+	} else
 		VTAILQ_INSERT_TAIL(&stevedores, stv, list);
-	}
 	/* NB: Do not free av, stevedore gets to keep it */
 }
 
@@ -216,4 +225,5 @@ STV_Config_Transient(void)
 	VCLS_AddFunc(mgt_cls, MCF_AUTH, cli_stv);
 	if (stv_transient == NULL)
 		STV_Config(TRANSIENT_STORAGE "=malloc");
+	VTAILQ_INSERT_TAIL(&stevedores, stv_transient, list);
 }

bin/varnishtest/tests/r02321.vtc

+varnishtest "Storage name collisions"
+
+# intentional collision
+shell -err -expect "Error: (-s main=malloc,100m) 'main' is already defined" {
+	exec varnishd -a :0 -f '' -n ${tmpdir} \
+		-s main=malloc,10m -s main=malloc,100m
+}
+
+# pseudo-accidental collision
+shell -err -expect "Error: (-s s0=malloc,100m) 's0' is already defined" {
+	exec varnishd -a :0 -f '' -n ${tmpdir} \
+		-s malloc,10m -s s0=malloc,100m
+}
+
+# Transient collision
+shell -err -expect "Error: (-s Transient=malloc,100m) 'Transient' is already defined" {
+	exec varnishd -a :0 -f '' -n ${tmpdir} \
+		-s Transient=malloc,10m -s Transient=malloc,100m
+}