libnvmf: various fixes

libnvme/src/nvme/registry.c

@@ -74,25 +74,6 @@ static char *registry_path(struct libnvme_global_ctx *ctx, const char *device,
 	return n < 0 ? NULL : path;
 }
 
-static int ensure_device_dir(struct libnvme_global_ctx *ctx, const char *device)
-{
-	__cleanup_free char *path = NULL;
-	int ret;
-
-	ret = shr_mkdir_p(registry_dir(ctx), 0755);
-	if (ret)
-		return ret;
-
-	path = registry_path(ctx, device, NULL);
-	if (!path)
-		return -ENOMEM;
-
-	/* EEXIST is success: two concurrent creates race to the same result. */
-	if (mkdir(path, 0755) < 0 && errno != EEXIST)
-		return -errno;
-	return 0;
-}
-
 static bool valid_device(const char *device)
 {
 	const char *p;
@@ -253,33 +234,6 @@ static int write_attr_atomic(int dir_fd, const char *dir_path,
 	return ret;
 }
 
-/*
- * Open the directory for @device (creating it if needed) and write @attr=@value
- * atomically.  Shared by the create and update paths.
- */
-static int write_device_attr(struct libnvme_global_ctx *ctx, const char *device,
-			     const char *attr, const char *value)
-{
-	__cleanup_free char *dir_path = NULL;
-	int dir_fd, ret;
-
-	ret = ensure_device_dir(ctx, device);
-	if (ret)
-		return ret;
-
-	dir_path = registry_path(ctx, device, NULL);
-	if (!dir_path)
-		return -ENOMEM;
-
-	dir_fd = open(dir_path, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
-	if (dir_fd < 0)
-		return -errno;
-
-	ret = write_attr_atomic(dir_fd, dir_path, attr, value);
-	close(dir_fd);
-	return ret;
-}
-
 /*
  * Read the kernel's global uevent sequence number from
  * /sys/kernel/uevent_seqnum.  This monotonic counter is used to stamp a
@@ -386,29 +340,26 @@ static int delete_dir_atomic(struct libnvme_global_ctx *ctx, const char *path)
 }
 
 /*
- * libnvmf_registry_create_instance - Write a registry entry for a freshly
- * connected controller.  Internal; called from the connect path in fabrics.c
- * once the kernel returns instance=N.  Always overwrites any pre-existing
- * entry: instance recycling means an old nvmeN/ directory is stale by
- * definition.
+ * Create the registry entry for @device holding the seqnum stamp and
+ * @attr=@value.
  *
  * The entry is built in a temporary directory and rename()'d into place, so it
  * is only ever observed complete or absent — never as a directory that exists
  * but is missing its seqnum stamp.  Without this, the udev REMOVE rule could
  * read a just-created entry between the mkdir and the seqnum write, see no
  * stamp, and delete the live entry the library is still populating.
+ *
+ * With @replace, any pre-existing entry is removed first.  Otherwise -EEXIST
+ * is returned if an entry already exists.
  */
-int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx,
-				     int instance, const char *owner)
+static int create_entry(struct libnvme_global_ctx *ctx, const char *device,
+			const char *attr, const char *value, bool replace)
 {
 	__cleanup_free char *tmp_dir = NULL;
 	__cleanup_free char *final_path = NULL;
 	__cleanup_free char *seqnum = NULL;
-	char device[32];
 	int dir_fd, ret;
 
-	snprintf(device, sizeof(device), "nvme%d", instance);
-
 	final_path = registry_path(ctx, device, NULL);
 	if (!final_path)
 		return -ENOMEM;
@@ -446,19 +397,21 @@ int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx,
 	if (seqnum)
 		write_attr_atomic(dir_fd, tmp_dir, "seqnum", seqnum);
 
-	ret = write_attr_atomic(dir_fd, tmp_dir, "owner", owner);
+	ret = write_attr_atomic(dir_fd, tmp_dir, attr, value);
 	close(dir_fd);
 	if (ret)
 		goto err;
 
 	/*
-	 * Reveal the fully-populated entry atomically.  Instance recycling
-	 * means any pre-existing nvmeN/ is stale, so remove it first: rename()
-	 * onto a non-empty directory fails.
+	 * Reveal the fully-populated entry atomically.  rename() onto a
+	 * non-empty directory fails, so a stale entry has to be removed first.
 	 */
-	delete_dir_atomic(ctx, final_path);
+	if (replace)
+		delete_dir_atomic(ctx, final_path);
 	if (rename(tmp_dir, final_path) < 0) {
 		ret = -errno;
+		if (ret == -ENOTEMPTY)
+			ret = -EEXIST;
 		goto err;
 	}
 	return 0;
@@ -468,6 +421,23 @@ int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx,
 	return ret;
 }
 
+/*
+ * libnvmf_registry_create_instance - Write a registry entry for a freshly
+ * connected controller.  Internal; called from the connect path in fabrics.c
+ * once the kernel returns instance=N.  Always overwrites any pre-existing
+ * entry: instance recycling means an old nvmeN/ directory is stale by
+ * definition.
+ */
+int libnvmf_registry_create_instance(struct libnvme_global_ctx *ctx,
+				     int instance, const char *owner)
+{
+	char device[32];
+
+	snprintf(device, sizeof(device), "nvme%d", instance);
+
+	return create_entry(ctx, device, "owner", owner, true);
+}
+
 int libnvmf_registry_delete_instance(struct libnvme_global_ctx *ctx,
 				     int instance)
 {
@@ -532,12 +502,43 @@ __shr_public int libnvmf_registry_update(struct libnvme_global_ctx *ctx,
 					     const char *device,
 					     const char *attr, const char *value)
 {
+	__cleanup_free char *dir_path = NULL;
+	int dir_fd, ret;
+
 	if (!ctx)
 		return -EINVAL;
 	if (!device || !valid_device(device))
 		return -EINVAL;
+	if (!valid_attr(attr))
+		return -EINVAL;
+
+	dir_path = registry_path(ctx, device, NULL);
+	if (!dir_path)
+		return -ENOMEM;
+
+	dir_fd = open(dir_path, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	if (dir_fd < 0 && errno == ENOENT) {
+		/* Nothing to remove from an entry that does not exist. */
+		if (!value)
+			return 0;
+
+		/*
+		 * A new entry needs its seqnum stamp for the udev REMOVE rule,
+		 * so create it the same way as on connect.
+		 */
+		ret = create_entry(ctx, device, attr, value, false);
+		if (ret != -EEXIST)
+			return ret;
 
-	return write_device_attr(ctx, device, attr, value);
+		/* A concurrent writer created the entry first; update it. */
+		dir_fd = open(dir_path, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+	}
+	if (dir_fd < 0)
+		return -errno;
+
+	ret = write_attr_atomic(dir_fd, dir_path, attr, value);
+	close(dir_fd);
+	return ret;
 }
 
 __shr_public int libnvmf_registry_delete(struct libnvme_global_ctx *ctx,

libnvme/src/nvme/registry.h

@@ -64,8 +64,10 @@ int libnvmf_registry_attr_equal(struct libnvme_global_ctx *ctx,
  * @value:	New attribute value, or NULL to remove the attribute file
  *
  * Writes the attribute value to the controller's registry entry using an
- * atomic tmp-file rename.  Creates the entry directory if it does not exist.
- * If @value is NULL, the attribute file is removed instead.
+ * atomic tmp-file rename.  If the entry does not exist, it is created
+ * atomically the same way as on connect, including the best-effort seqnum
+ * stamp for the udev cleanup rule.  If @value is NULL, the attribute file is
+ * removed instead; a missing entry is left alone in this case.
  *
  * Return: 0 on success, negative errno from underlying system calls otherwise.
  */

libnvme/src/nvme/tree.c

@@ -1457,9 +1457,6 @@ int libnvme_ctrl_alloc(struct libnvme_global_ctx *ctx, struct libnvme_subsystem
 		}
 	} while (c);
 
-	if (!c)
-		c = p;
-
 	if (!c) {
 		ret = libnvme_subsystem_create_ctrl(s, &params, &c);
 		if (ret) {

libnvme/tests/registry.c

@@ -191,6 +191,90 @@ static bool test_update_and_retrieve(struct libnvme_global_ctx *ctx)
 	return pass;
 }
 
+/*
+ * An entry created by libnvmf_registry_update() must look like one created on
+ * connect: it carries the seqnum stamp for the udev cleanup rule and appears
+ * atomically.  Removing an attribute of a missing entry must not create one.
+ */
+static bool test_update_creates_entry(struct libnvme_global_ctx *ctx)
+{
+	char *value = NULL;
+	char path[512];
+	struct stat st;
+	bool pass = true;
+	int ret;
+
+	printf("test_update_creates_entry:\n");
+
+	ret = libnvmf_registry_update(ctx, "nvme6", "owner", NULL);
+	snprintf(path, sizeof(path), "%s/nvme6", regdir);
+	if (ret || stat(path, &st) == 0) {
+		printf(" - removing from a missing entry returned %d or created it [FAIL]\n",
+		       ret);
+		pass = false;
+	} else {
+		printf(" - removing from a missing entry is a no-op [PASS]\n");
+	}
+
+	ret = libnvmf_registry_update(ctx, "nvme6", "owner", "udisks");
+	if (ret) {
+		printf(" - update returned %d [FAIL]\n", ret);
+		return false;
+	}
+
+	/* best-effort, see test_create() */
+	if (access("/sys/kernel/uevent_seqnum", R_OK) == 0) {
+		ret = libnvmf_registry_retrieve(ctx, "nvme6", "seqnum", &value);
+		if (ret || !value || !*value) {
+			printf(" - no seqnum stamp, ret=%d [FAIL]\n", ret);
+			pass = false;
+		} else {
+			printf(" - seqnum='%s' [PASS]\n", value);
+		}
+		free(value);
+		value = NULL;
+	}
+
+	/* updating an existing entry keeps the stamp */
+	ret = libnvmf_registry_update(ctx, "nvme6", "note", "hello");
+	if (ret) {
+		printf(" - second update returned %d [FAIL]\n", ret);
+		pass = false;
+	}
+	ret = libnvmf_registry_retrieve(ctx, "nvme6", "owner", &value);
+	if (ret || !value || strcmp(value, "udisks") != 0) {
+		printf(" - owner lost after second update [FAIL]\n");
+		pass = false;
+	} else {
+		printf(" - owner='%s' kept [PASS]\n", value);
+	}
+	free(value);
+
+	{
+		struct dirent *de;
+		bool leftover = false;
+		DIR *d = opendir(regdir);
+
+		if (d) {
+			while ((de = readdir(d)) != NULL) {
+				if (strstr(de->d_name, ".tmp.")) {
+					printf(" - leftover '%s' [FAIL]\n",
+					       de->d_name);
+					leftover = true;
+				}
+			}
+			closedir(d);
+		}
+		if (leftover)
+			pass = false;
+		else
+			printf(" - no leftover temp dir [PASS]\n");
+	}
+
+	libnvmf_registry_delete(ctx, "nvme6");
+	return pass;
+}
+
 static bool test_special_chars(struct libnvme_global_ctx *ctx)
 {
 	/*
@@ -344,6 +428,10 @@ static void collect_attr(const char *attr, const char *value, void *user_data)
 {
 	struct attr_result *r = user_data;
 
+	/* the best-effort seqnum stamp, see test_create() */
+	if (!strcmp(attr, "seqnum"))
+		return;
+
 	if (r->count < 8) {
 		snprintf(r->keys[r->count], sizeof(r->keys[0]), "%s", attr);
 		snprintf(r->values[r->count], sizeof(r->values[0]), "%s", value);
@@ -539,6 +627,7 @@ int main(int argc, char *argv[])
 
 	pass &= test_create(ctx);
 	pass &= test_update_and_retrieve(ctx);
+	pass &= test_update_creates_entry(ctx);
 	pass &= test_special_chars(ctx);
 	pass &= test_delete(ctx);
 	pass &= test_retrieve_missing(ctx);

libnvme/tests/sysfs/data/tree-loop-duplicate-hostnqn.out

@@ -0,0 +1 @@
+resolved host: hostnqn=nqn.2014-08.org.nvmexpress:uuid:066d3885-3e3f-498c-b6e8-695c71636a13 hostid=84a30b8e-670d-4d8f-a006-f85d146ca9fa

libnvme/tests/sysfs/data/tree-loop-duplicate.out

@@ -0,0 +1,7 @@
+host nqn.2014-08.org.nvmexpress:uuid:066d3885-3e3f-498c-b6e8-695c71636a13 hostid=84a30b8e-670d-4d8f-a006-f85d146ca9fa
+  subsystem nvme-subsys0 nqn.2014-08.org.example:dup
+    ns 1 nvme0n1
+      path nvme0c0n1 ana=optimized numa=- qdepth=0
+        ctrl nvme0 transport=loop
+      path nvme0c1n1 ana=optimized numa=- qdepth=0
+        ctrl nvme1 transport=loop

libnvme/tests/sysfs/meson.build

@@ -17,6 +17,7 @@ if diff.found() and bash_prog.found()
     tree_data = [
         'tree-pcie',
         'tree-apple-nvme',
+        'tree-loop-duplicate',
     ]
 
     tree_diff = files('tree-diff.sh')