diff --git a/libnvme/src/nvme/registry.c b/libnvme/src/nvme/registry.c index d719827165..29c30a1f06 100644 --- a/libnvme/src/nvme/registry.c +++ b/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, diff --git a/libnvme/src/nvme/registry.h b/libnvme/src/nvme/registry.h index 8c46cd231c..0e5960bc36 100644 --- a/libnvme/src/nvme/registry.h +++ b/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. */ diff --git a/libnvme/src/nvme/tree.c b/libnvme/src/nvme/tree.c index 0c93aa71d2..b9741bab6f 100644 --- a/libnvme/src/nvme/tree.c +++ b/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, ¶ms, &c); if (ret) { diff --git a/libnvme/tests/registry.c b/libnvme/tests/registry.c index d00103db6a..8a87f9f6a3 100644 --- a/libnvme/tests/registry.c +++ b/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); diff --git a/libnvme/tests/sysfs/data/tree-loop-duplicate-hostnqn.out b/libnvme/tests/sysfs/data/tree-loop-duplicate-hostnqn.out new file mode 100644 index 0000000000..897d966b99 --- /dev/null +++ b/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 diff --git a/libnvme/tests/sysfs/data/tree-loop-duplicate.out b/libnvme/tests/sysfs/data/tree-loop-duplicate.out new file mode 100644 index 0000000000..0ca5e4db77 --- /dev/null +++ b/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 diff --git a/libnvme/tests/sysfs/data/tree-loop-duplicate.tar.xz b/libnvme/tests/sysfs/data/tree-loop-duplicate.tar.xz new file mode 100644 index 0000000000..c0e65d455c Binary files /dev/null and b/libnvme/tests/sysfs/data/tree-loop-duplicate.tar.xz differ diff --git a/libnvme/tests/sysfs/meson.build b/libnvme/tests/sysfs/meson.build index 5a3099a02a..3ac154eda7 100644 --- a/libnvme/tests/sysfs/meson.build +++ b/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') diff --git a/src/nvme-print-json.c b/src/nvme-print-json.c index a6c54aaa21..dd35491241 100644 --- a/src/nvme-print-json.c +++ b/src/nvme-print-json.c @@ -113,6 +113,14 @@ static void obj_add_prix64(struct json_object *o, const char *k, uint64_t v) obj_add_str(o, k, str); } +static void obj_add_ctrl_address(struct json_object *o, struct libnvme_ctrl *c) +{ + const char *address = libnvme_ctrl_get_address(c); + + /* controllers without an address, e.g. loop, don't report one */ + obj_add_str(o, "Address", address ? address : ""); +} + static void obj_add_ctrl_address_details(struct json_object *o, const char *k, struct libnvme_ctrl *c) { @@ -2859,7 +2867,7 @@ static unsigned int json_print_nvme_subsystem_multipath(struct libnvme_subsystem path_attrs = json_create_object(); obj_add_str(path_attrs, "Name", libnvme_ctrl_get_name(c)); obj_add_str(path_attrs, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(path_attrs, "Address", libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(path_attrs, c); obj_add_ctrl_address_details(path_attrs, "AddressDetails", c); obj_add_str(path_attrs, "State", libnvme_ctrl_get_state(c)); obj_add_str(path_attrs, "ANAState", ana_state); @@ -2881,7 +2889,7 @@ static void json_print_nvme_subsystem_ctrls(struct libnvme_subsystem *s, path_attrs = json_create_object(); obj_add_str(path_attrs, "Name", libnvme_ctrl_get_name(c)); obj_add_str(path_attrs, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(path_attrs, "Address", libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(path_attrs, c); obj_add_ctrl_address_details(path_attrs, "AddressDetails", c); obj_add_str(path_attrs, "State", libnvme_ctrl_get_state(c)); array_add_obj(paths, path_attrs); @@ -4890,7 +4898,7 @@ static void json_print_detail_list_multipath(struct libnvme_subsystem *s, obj_add_str(jpath, "ModelNumber", model); obj_add_str(jpath, "Firmware", firmware); obj_add_str(jpath, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(jpath, "Address", libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(jpath, c); obj_add_ctrl_address_details(jpath, "AddressDetails", c); obj_add_str(jpath, "Slot", slot ? slot : ""); @@ -4930,7 +4938,7 @@ static void json_print_detail_list(struct libnvme_subsystem *s, struct json_obje obj_add_str(jctrl, "ModelNumber", model); obj_add_str(jctrl, "Firmware", firmware); obj_add_str(jctrl, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(jctrl, "Address", libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(jctrl, c); obj_add_ctrl_address_details(jctrl, "AddressDetails", c); obj_add_str(jctrl, "Slot", slot ? slot : ""); @@ -5053,7 +5061,7 @@ static void json_detail_list(struct libnvme_global_ctx *ctx) obj_add_str(jctrl, "ModelNumber", model); obj_add_str(jctrl, "Firmware", firmware); obj_add_str(jctrl, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(jctrl, "Address", libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(jctrl, c); obj_add_ctrl_address_details(jctrl, "AddressDetails", c); obj_add_str(jctrl, "Slot", slot); @@ -5262,7 +5270,7 @@ static unsigned int json_subsystem_topology_multipath(struct libnvme_subsystem * ctrl_attrs = json_create_object(); obj_add_str(ctrl_attrs, "Name", libnvme_ctrl_get_name(c)); obj_add_str(ctrl_attrs, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(ctrl_attrs, "Address", libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(ctrl_attrs, c); obj_add_ctrl_address_details(ctrl_attrs, "AddressDetails", c); obj_add_str(ctrl_attrs, "State", libnvme_ctrl_get_state(c)); array_add_obj(ctrls, ctrl_attrs); @@ -5300,8 +5308,7 @@ static void json_print_nvme_subsystem_topology(struct libnvme_subsystem *s, libnvme_ctrl_get_name(c)); obj_add_str(ctrl_attrs, "Transport", libnvme_ctrl_get_transport(c)); - obj_add_str(ctrl_attrs, "Address", - libnvme_ctrl_get_address(c)); + obj_add_ctrl_address(ctrl_attrs, c); obj_add_ctrl_address_details(ctrl_attrs, "AddressDetails", c); obj_add_str(ctrl_attrs, "State", libnvme_ctrl_get_state(c));