Date: Thu, 30 Jan 2025 15:49:10 GMT From: John Baldwin <jhb@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 703b03a8e325 - main - ctld: Use nvlist instead of home-rolled name-value lists Message-ID: <202501301549.50UFnAHJ006418@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by jhb: URL: https://cgit.FreeBSD.org/src/commit/?id=703b03a8e325101f85610bf09f098442697f2f23 commit 703b03a8e325101f85610bf09f098442697f2f23 Author: John Baldwin <jhb@FreeBSD.org> AuthorDate: 2025-01-30 15:19:51 +0000 Commit: John Baldwin <jhb@FreeBSD.org> CommitDate: 2025-01-30 15:19:51 +0000 ctld: Use nvlist instead of home-rolled name-value lists Reviewed by: asomers (older version) Sponsored by: Chelsio Communications Differential Revision: https://reviews.freebsd.org/D48595 --- usr.sbin/ctld/ctld.c | 87 ++++++++------------- usr.sbin/ctld/ctld.h | 18 +---- usr.sbin/ctld/kernel.c | 200 ++++++++++++++--------------------------------- usr.sbin/ctld/parse.y | 15 ++-- usr.sbin/ctld/uclparse.c | 13 +-- 5 files changed, 111 insertions(+), 222 deletions(-) diff --git a/usr.sbin/ctld/ctld.c b/usr.sbin/ctld/ctld.c index a67ff51a451d..d5e4547179d9 100644 --- a/usr.sbin/ctld/ctld.c +++ b/usr.sbin/ctld/ctld.c @@ -30,6 +30,7 @@ */ #include <sys/types.h> +#include <sys/nv.h> #include <sys/time.h> #include <sys/socket.h> #include <sys/stat.h> @@ -618,7 +619,7 @@ portal_group_new(struct conf *conf, const char *name) if (pg == NULL) log_err(1, "calloc"); pg->pg_name = checked_strdup(name); - TAILQ_INIT(&pg->pg_options); + pg->pg_options = nvlist_create(0); TAILQ_INIT(&pg->pg_portals); TAILQ_INIT(&pg->pg_ports); pg->pg_conf = conf; @@ -635,7 +636,6 @@ portal_group_delete(struct portal_group *pg) { struct portal *portal, *tmp; struct port *port, *tport; - struct option *o, *otmp; TAILQ_FOREACH_SAFE(port, &pg->pg_ports, p_pgs, tport) port_delete(port); @@ -643,8 +643,7 @@ portal_group_delete(struct portal_group *pg) TAILQ_FOREACH_SAFE(portal, &pg->pg_portals, p_next, tmp) portal_delete(portal); - TAILQ_FOREACH_SAFE(o, &pg->pg_options, o_next, otmp) - option_delete(&pg->pg_options, o); + nvlist_destroy(pg->pg_options); free(pg->pg_name); free(pg->pg_offload); free(pg->pg_redirection); @@ -1371,7 +1370,7 @@ lun_new(struct conf *conf, const char *name) log_err(1, "calloc"); lun->l_conf = conf; lun->l_name = checked_strdup(name); - TAILQ_INIT(&lun->l_options); + lun->l_options = nvlist_create(0); TAILQ_INSERT_TAIL(&conf->conf_luns, lun, l_next); lun->l_ctl_lun = -1; @@ -1382,7 +1381,6 @@ void lun_delete(struct lun *lun) { struct target *targ; - struct option *o, *tmp; int i; TAILQ_FOREACH(targ, &lun->l_conf->conf_targets, t_next) { @@ -1393,8 +1391,7 @@ lun_delete(struct lun *lun) } TAILQ_REMOVE(&lun->l_conf->conf_luns, lun, l_next); - TAILQ_FOREACH_SAFE(o, &lun->l_options, o_next, tmp) - option_delete(&lun->l_options, o); + nvlist_destroy(lun->l_options); free(lun->l_name); free(lun->l_backend); free(lun->l_device_id); @@ -1480,56 +1477,23 @@ lun_set_ctl_lun(struct lun *lun, uint32_t value) lun->l_ctl_lun = value; } -struct option * -option_new(struct options *options, const char *name, const char *value) +bool +option_new(nvlist_t *nvl, const char *name, const char *value) { - struct option *o; + int error; - o = option_find(options, name); - if (o != NULL) { + if (nvlist_exists_string(nvl, name)) { log_warnx("duplicated option \"%s\"", name); - return (NULL); + return (false); } - o = calloc(1, sizeof(*o)); - if (o == NULL) - log_err(1, "calloc"); - o->o_name = checked_strdup(name); - o->o_value = checked_strdup(value); - TAILQ_INSERT_TAIL(options, o, o_next); - - return (o); -} - -void -option_delete(struct options *options, struct option *o) -{ - - TAILQ_REMOVE(options, o, o_next); - free(o->o_name); - free(o->o_value); - free(o); -} - -struct option * -option_find(const struct options *options, const char *name) -{ - struct option *o; - - TAILQ_FOREACH(o, options, o_next) { - if (strcmp(o->o_name, name) == 0) - return (o); + nvlist_add_string(nvl, name, value); + error = nvlist_error(nvl); + if (error != 0) { + log_warnc(error, "failed to add option \"%s\"", name); + return (false); } - - return (NULL); -} - -void -option_set(struct option *o, const char *value) -{ - - free(o->o_value); - o->o_value = checked_strdup(value); + return (true); } #ifdef ICL_KERNEL_PROXY @@ -1590,6 +1554,19 @@ connection_new(struct portal *portal, int fd, const char *host, } #if 0 +static void +options_print(const char *prefix, nvlist_t *nvl) +{ + const char *name; + void *cookie; + + cookie = NULL; + while ((name = nvlist_next(nvl, NULL, &cookie)) != NULL) { + fprintf(stderr, "%soption %s %s\n", prefix, name, + nvlist_get_string(nvl, name)); + } +} + static void conf_print(struct conf *conf) { @@ -1601,7 +1578,6 @@ conf_print(struct conf *conf) struct portal *portal; struct target *targ; struct lun *lun; - struct option *o; TAILQ_FOREACH(ag, &conf->conf_auth_groups, ag_next) { fprintf(stderr, "auth-group %s {\n", ag->ag_name); @@ -1621,14 +1597,13 @@ conf_print(struct conf *conf) fprintf(stderr, "portal-group %s {\n", pg->pg_name); TAILQ_FOREACH(portal, &pg->pg_portals, p_next) fprintf(stderr, "\t listen %s\n", portal->p_listen); + options_print("\t", pg->pg_options); fprintf(stderr, "}\n"); } TAILQ_FOREACH(lun, &conf->conf_luns, l_next) { fprintf(stderr, "\tlun %s {\n", lun->l_name); fprintf(stderr, "\t\tpath %s\n", lun->l_path); - TAILQ_FOREACH(o, &lun->l_options, o_next) - fprintf(stderr, "\t\toption %s %s\n", - o->o_name, o->o_value); + options_print("\t\t", lun->l_options); fprintf(stderr, "\t}\n"); } TAILQ_FOREACH(targ, &conf->conf_targets, t_next) { diff --git a/usr.sbin/ctld/ctld.h b/usr.sbin/ctld/ctld.h index 88490a94464e..c57bd32c77a2 100644 --- a/usr.sbin/ctld/ctld.h +++ b/usr.sbin/ctld/ctld.h @@ -31,6 +31,7 @@ #ifndef CTLD_H #define CTLD_H +#include <sys/_nv.h> #include <sys/queue.h> #ifdef ICL_KERNEL_PROXY #include <sys/types.h> @@ -103,8 +104,6 @@ struct portal { int p_socket; }; -TAILQ_HEAD(options, option); - #define PG_FILTER_UNKNOWN 0 #define PG_FILTER_NONE 1 #define PG_FILTER_PORTAL 2 @@ -114,7 +113,7 @@ TAILQ_HEAD(options, option); struct portal_group { TAILQ_ENTRY(portal_group) pg_next; struct conf *pg_conf; - struct options pg_options; + nvlist_t *pg_options; char *pg_name; struct auth_group *pg_discovery_auth_group; int pg_discovery_filter; @@ -158,16 +157,10 @@ struct port { uint32_t p_ctl_port; }; -struct option { - TAILQ_ENTRY(option) o_next; - char *o_name; - char *o_value; -}; - struct lun { TAILQ_ENTRY(lun) l_next; struct conf *l_conf; - struct options l_options; + nvlist_t *l_options; char *l_name; char *l_backend; uint8_t l_device_type; @@ -351,11 +344,8 @@ void lun_set_serial(struct lun *lun, const char *value); void lun_set_size(struct lun *lun, int64_t value); void lun_set_ctl_lun(struct lun *lun, uint32_t value); -struct option *option_new(struct options *os, +bool option_new(nvlist_t *nvl, const char *name, const char *value); -void option_delete(struct options *os, struct option *co); -struct option *option_find(const struct options *os, const char *name); -void option_set(struct option *o, const char *value); void kernel_init(void); int kernel_lun_add(struct lun *lun); diff --git a/usr.sbin/ctld/kernel.c b/usr.sbin/ctld/kernel.c index eed9f13d42fa..1fa05cb159c3 100644 --- a/usr.sbin/ctld/kernel.c +++ b/usr.sbin/ctld/kernel.c @@ -105,15 +105,6 @@ kernel_init(void) #endif } -/* - * Name/value pair used for per-LUN attributes. - */ -struct cctl_lun_nv { - char *name; - char *value; - STAILQ_ENTRY(cctl_lun_nv) links; -}; - /* * Backend LUN information. */ @@ -126,7 +117,7 @@ struct cctl_lun { char *serial_number; char *device_id; char *ctld_name; - STAILQ_HEAD(,cctl_lun_nv) attr_list; + nvlist_t *attr_list; STAILQ_ENTRY(cctl_lun) links; }; @@ -140,7 +131,7 @@ struct cctl_port { char *cfiscsi_target; uint16_t cfiscsi_portal_group_tag; char *ctld_portal_group_name; - STAILQ_HEAD(,cctl_lun_nv) attr_list; + nvlist_t *attr_list; STAILQ_ENTRY(cctl_port) links; }; @@ -187,7 +178,7 @@ cctl_start_element(void *user_data, const char *name, const char **attr) devlist->num_luns++; devlist->cur_lun = cur_lun; - STAILQ_INIT(&cur_lun->attr_list); + cur_lun->attr_list = nvlist_create(0); STAILQ_INSERT_TAIL(&devlist->lun_list, cur_lun, links); for (i = 0; attr[i] != NULL; i += 2) { @@ -207,6 +198,7 @@ cctl_end_element(void *user_data, const char *name) struct cctl_devlist_data *devlist; struct cctl_lun *cur_lun; char *str; + int error; devlist = (struct cctl_devlist_data *)user_data; cur_lun = devlist->cur_lun; @@ -260,18 +252,12 @@ cctl_end_element(void *user_data, const char *name) } else if (strcmp(name, "ctllunlist") == 0) { /* Nothing. */ } else { - struct cctl_lun_nv *nv; - - nv = calloc(1, sizeof(*nv)); - if (nv == NULL) - log_err(1, "%s: can't allocate %zd bytes for nv pair", - __func__, sizeof(*nv)); - - nv->name = checked_strdup(name); - - nv->value = str; + nvlist_move_string(cur_lun->attr_list, name, str); + error = nvlist_error(cur_lun->attr_list); + if (error != 0) + log_errc(1, error, "%s: failed to add nv pair for %s", + __func__, name); str = NULL; - STAILQ_INSERT_TAIL(&cur_lun->attr_list, nv, links); } free(str); @@ -309,7 +295,7 @@ cctl_start_pelement(void *user_data, const char *name, const char **attr) devlist->num_ports++; devlist->cur_port = cur_port; - STAILQ_INIT(&cur_port->attr_list); + cur_port->attr_list = nvlist_create(0); STAILQ_INSERT_TAIL(&devlist->port_list, cur_port, links); for (i = 0; attr[i] != NULL; i += 2) { @@ -329,6 +315,7 @@ cctl_end_pelement(void *user_data, const char *name) struct cctl_devlist_data *devlist; struct cctl_port *cur_port; char *str; + int error; devlist = (struct cctl_devlist_data *)user_data; cur_port = devlist->cur_port; @@ -386,18 +373,12 @@ cctl_end_pelement(void *user_data, const char *name) } else if (strcmp(name, "ctlportlist") == 0) { /* Nothing. */ } else { - struct cctl_lun_nv *nv; - - nv = calloc(1, sizeof(*nv)); - if (nv == NULL) - log_err(1, "%s: can't allocate %zd bytes for nv pair", - __func__, sizeof(*nv)); - - nv->name = checked_strdup(name); - - nv->value = str; + nvlist_move_string(cur_port->attr_list, name, str); + error = nvlist_error(cur_port->attr_list); + if (error != 0) + log_errc(1, error, "%s: failed to add nv pair for %s", + __func__, name); str = NULL; - STAILQ_INSERT_TAIL(&cur_port->attr_list, nv, links); } free(str); @@ -422,14 +403,15 @@ conf_new_from_kernel(struct kports *kports) struct pport *pp; struct port *cp; struct lun *cl; - struct option *o; struct ctl_lun_list list; struct cctl_devlist_data devlist; struct cctl_lun *lun; struct cctl_port *port; XML_Parser parser; + const char *key; char *str, *name; - int len, retval; + void *cookie; + int error, len, retval; bzero(&devlist, sizeof(devlist)); STAILQ_INIT(&devlist.lun_list); @@ -615,26 +597,17 @@ retry_port: cp->p_ctl_port = port->port_id; } while ((port = STAILQ_FIRST(&devlist.port_list))) { - struct cctl_lun_nv *nv; - STAILQ_REMOVE_HEAD(&devlist.port_list, links); free(port->port_frontend); free(port->port_name); free(port->cfiscsi_target); free(port->ctld_portal_group_name); - while ((nv = STAILQ_FIRST(&port->attr_list))) { - STAILQ_REMOVE_HEAD(&port->attr_list, links); - free(nv->value); - free(nv->name); - free(nv); - } + nvlist_destroy(port->attr_list); free(port); } free(name); STAILQ_FOREACH(lun, &devlist.lun_list, links) { - struct cctl_lun_nv *nv; - if (lun->ctld_name == NULL) { log_debugx("CTL lun %ju wasn't managed by ctld; " "ignoring", (uintmax_t)lun->lun_id); @@ -666,40 +639,45 @@ retry_port: lun_set_size(cl, lun->size_blocks * cl->l_blocksize); lun_set_ctl_lun(cl, lun->lun_id); - STAILQ_FOREACH(nv, &lun->attr_list, links) { - if (strcmp(nv->name, "file") == 0 || - strcmp(nv->name, "dev") == 0) { - lun_set_path(cl, nv->value); + cookie = NULL; + while ((key = nvlist_next(lun->attr_list, NULL, &cookie)) != + NULL) { + if (strcmp(key, "file") == 0 || + strcmp(key, "dev") == 0) { + lun_set_path(cl, + nvlist_get_string(lun->attr_list, key)); continue; } - o = option_new(&cl->l_options, nv->name, nv->value); - if (o == NULL) - log_warnx("unable to add CTL lun option %s " - "for CTL lun %ju \"%s\"", - nv->name, (uintmax_t) lun->lun_id, + nvlist_add_string(cl->l_options, key, + nvlist_get_string(lun->attr_list, key)); + error = nvlist_error(cl->l_options); + if (error != 0) + log_warnc(error, "unable to add CTL lun option " + "%s for CTL lun %ju \"%s\"", + key, (uintmax_t)lun->lun_id, cl->l_name); } } while ((lun = STAILQ_FIRST(&devlist.lun_list))) { - struct cctl_lun_nv *nv; - STAILQ_REMOVE_HEAD(&devlist.lun_list, links); - while ((nv = STAILQ_FIRST(&lun->attr_list))) { - STAILQ_REMOVE_HEAD(&lun->attr_list, links); - free(nv->value); - free(nv->name); - free(nv); - } + nvlist_destroy(lun->attr_list); free(lun); } return (conf); } +static void +nvlist_replace_string(nvlist_t *nvl, const char *name, const char *value) +{ + if (nvlist_exists_string(nvl, name)) + nvlist_free_string(nvl, name); + nvlist_add_string(nvl, name, value); +} + int kernel_lun_add(struct lun *lun) { - struct option *o; struct ctl_lun_req req; int error; @@ -733,43 +711,18 @@ kernel_lun_add(struct lun *lun) req.reqdata.create.flags |= CTL_LUN_FLAG_DEVID; } - if (lun->l_path != NULL) { - o = option_find(&lun->l_options, "file"); - if (o != NULL) { - option_set(o, lun->l_path); - } else { - o = option_new(&lun->l_options, "file", lun->l_path); - assert(o != NULL); - } - } + if (lun->l_path != NULL) + nvlist_replace_string(lun->l_options, "file", lun->l_path); - o = option_find(&lun->l_options, "ctld_name"); - if (o != NULL) { - option_set(o, lun->l_name); - } else { - o = option_new(&lun->l_options, "ctld_name", lun->l_name); - assert(o != NULL); - } + nvlist_replace_string(lun->l_options, "ctld_name", lun->l_name); - o = option_find(&lun->l_options, "scsiname"); - if (o == NULL && lun->l_scsiname != NULL) { - o = option_new(&lun->l_options, "scsiname", lun->l_scsiname); - assert(o != NULL); - } + if (!nvlist_exists_string(lun->l_options, "scsiname") && + lun->l_scsiname != NULL) + nvlist_add_string(lun->l_options, "scsiname", lun->l_scsiname); - if (!TAILQ_EMPTY(&lun->l_options)) { - req.args_nvl = nvlist_create(0); - if (req.args_nvl == NULL) { - log_warn("error allocating nvlist"); - return (1); - } - - TAILQ_FOREACH(o, &lun->l_options, o_next) - nvlist_add_string(req.args_nvl, o->o_name, o->o_value); - - req.args = nvlist_pack(req.args_nvl, &req.args_len); + if (!nvlist_empty(lun->l_options)) { + req.args = nvlist_pack(lun->l_options, &req.args_len); if (req.args == NULL) { - nvlist_destroy(req.args_nvl); log_warn("error packing nvlist"); return (1); } @@ -777,7 +730,6 @@ kernel_lun_add(struct lun *lun) error = ioctl(ctl_fd, CTL_LUN_REQ, &req); free(req.args); - nvlist_destroy(req.args_nvl); if (error != 0) { log_warn("error issuing CTL_LUN_REQ ioctl"); @@ -806,7 +758,6 @@ kernel_lun_add(struct lun *lun) int kernel_lun_modify(struct lun *lun) { - struct option *o; struct ctl_lun_req req; int error; @@ -818,43 +769,18 @@ kernel_lun_modify(struct lun *lun) req.reqdata.modify.lun_id = lun->l_ctl_lun; req.reqdata.modify.lun_size_bytes = lun->l_size; - if (lun->l_path != NULL) { - o = option_find(&lun->l_options, "file"); - if (o != NULL) { - option_set(o, lun->l_path); - } else { - o = option_new(&lun->l_options, "file", lun->l_path); - assert(o != NULL); - } - } - - o = option_find(&lun->l_options, "ctld_name"); - if (o != NULL) { - option_set(o, lun->l_name); - } else { - o = option_new(&lun->l_options, "ctld_name", lun->l_name); - assert(o != NULL); - } + if (lun->l_path != NULL) + nvlist_replace_string(lun->l_options, "file", lun->l_path); - o = option_find(&lun->l_options, "scsiname"); - if (o == NULL && lun->l_scsiname != NULL) { - o = option_new(&lun->l_options, "scsiname", lun->l_scsiname); - assert(o != NULL); - } + nvlist_replace_string(lun->l_options, "ctld_name", lun->l_name); - if (!TAILQ_EMPTY(&lun->l_options)) { - req.args_nvl = nvlist_create(0); - if (req.args_nvl == NULL) { - log_warn("error allocating nvlist"); - return (1); - } - - TAILQ_FOREACH(o, &lun->l_options, o_next) - nvlist_add_string(req.args_nvl, o->o_name, o->o_value); + if (!nvlist_exists_string(lun->l_options, "scsiname") && + lun->l_scsiname != NULL) + nvlist_add_string(lun->l_options, "scsiname", lun->l_scsiname); - req.args = nvlist_pack(req.args_nvl, &req.args_len); + if (!nvlist_empty(lun->l_options)) { + req.args = nvlist_pack(lun->l_options, &req.args_len); if (req.args == NULL) { - nvlist_destroy(req.args_nvl); log_warn("error packing nvlist"); return (1); } @@ -862,7 +788,6 @@ kernel_lun_modify(struct lun *lun) error = ioctl(ctl_fd, CTL_LUN_REQ, &req); free(req.args); - nvlist_destroy(req.args_nvl); if (error != 0) { log_warn("error issuing CTL_LUN_REQ ioctl"); @@ -1039,7 +964,6 @@ kernel_limits(const char *offload, int s, int *max_recv_dsl, int *max_send_dsl, int kernel_port_add(struct port *port) { - struct option *o; struct ctl_port_entry entry; struct ctl_req req; struct ctl_lun_map lm; @@ -1055,7 +979,7 @@ kernel_port_add(struct port *port) if (port->p_portal_group) { strlcpy(req.driver, "iscsi", sizeof(req.driver)); - req.args_nvl = nvlist_create(0); + req.args_nvl = nvlist_clone(pg->pg_options); nvlist_add_string(req.args_nvl, "cfiscsi_target", targ->t_name); nvlist_add_string(req.args_nvl, @@ -1067,10 +991,6 @@ kernel_port_add(struct port *port) nvlist_add_string(req.args_nvl, "cfiscsi_target_alias", targ->t_alias); } - - TAILQ_FOREACH(o, &pg->pg_options, o_next) - nvlist_add_string(req.args_nvl, o->o_name, - o->o_value); } if (port->p_ioctl_port) { diff --git a/usr.sbin/ctld/parse.y b/usr.sbin/ctld/parse.y index 128a5b4ea042..9f4759303e22 100644 --- a/usr.sbin/ctld/parse.y +++ b/usr.sbin/ctld/parse.y @@ -29,8 +29,9 @@ * SUCH DAMAGE. */ -#include <sys/queue.h> #include <sys/types.h> +#include <sys/nv.h> +#include <sys/queue.h> #include <sys/stat.h> #include <assert.h> #include <stdio.h> @@ -432,12 +433,12 @@ portal_group_offload: OFFLOAD STR portal_group_option: OPTION STR STR { - struct option *o; + bool ok; - o = option_new(&portal_group->pg_options, $2, $3); + ok = option_new(portal_group->pg_options, $2, $3); free($2); free($3); - if (o == NULL) + if (!ok) return (1); } ; @@ -1033,12 +1034,12 @@ lun_ctl_lun: CTL_LUN STR lun_option: OPTION STR STR { - struct option *o; + bool ok; - o = option_new(&lun->l_options, $2, $3); + ok = option_new(lun->l_options, $2, $3); free($2); free($3); - if (o == NULL) + if (!ok) return (1); } ; diff --git a/usr.sbin/ctld/uclparse.c b/usr.sbin/ctld/uclparse.c index b48a96cb0263..69efba823cc5 100644 --- a/usr.sbin/ctld/uclparse.c +++ b/usr.sbin/ctld/uclparse.c @@ -29,8 +29,9 @@ * SUCH DAMAGE. */ -#include <sys/queue.h> #include <sys/types.h> +#include <sys/nv.h> +#include <sys/queue.h> #include <assert.h> #include <stdio.h> #include <stdint.h> @@ -604,9 +605,10 @@ uclparse_portal_group(const char *name, const ucl_object_t *top) while ((tmp = ucl_iterate_object(obj, &it2, true))) { - option_new(&portal_group->pg_options, + if (!option_new(portal_group->pg_options, ucl_object_key(tmp), - ucl_object_tostring_forced(tmp)); + ucl_object_tostring_forced(tmp))) + return (false); } } @@ -937,9 +939,10 @@ uclparse_lun(const char *name, const ucl_object_t *top) while ((child = ucl_iterate_object(obj, &child_it, true))) { - option_new(&lun->l_options, + if (!option_new(lun->l_options, ucl_object_key(child), - ucl_object_tostring_forced(child)); + ucl_object_tostring_forced(child))) + return (false); } }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202501301549.50UFnAHJ006418>