[PATCH uci 2/2] remove internal usage of redundant uci_ptr.last
In uci_lookup_ptr and uci_set the pointer uci_ptr ptr.last is set to the element corresponding to the first of: ptr.o, ptr.s, ptr.p. Thus, ptr.last is redundant and in case of uci_set is (and was) not always consistently set. In order to simplify the code this commit removes internal usage of ptr.last, and remove setting it from uci_set (and from uci_add_list that was never used anyway). As it is part of the public C api ptr.last cannot be completely removed though. A search on lxr.openwrt.org shows that it is used as the output of uci_lookup_ptr in several packages. So we leave setting ptr.last in uci_lookup_ptr intact. Signed-off-by: Jan Venekamp --- cli.c | 39 +++ delta.c | 10 ++ list.c| 6 -- lua/uci.c | 42 +++--- 4 files changed, 32 insertions(+), 65 deletions(-) diff --git a/cli.c b/cli.c index b4c4f95..f169e42 100644 --- a/cli.c +++ b/cli.c @@ -314,7 +314,6 @@ static void uci_show_changes(struct uci_package *p) static int package_cmd(int cmd, char *tuple) { - struct uci_element *e = NULL; struct uci_ptr ptr; int ret = 1; @@ -323,7 +322,6 @@ static int package_cmd(int cmd, char *tuple) return 1; } - e = ptr.last; switch(cmd) { case CMD_CHANGES: uci_show_changes(ptr.p); @@ -349,20 +347,14 @@ static int package_cmd(int cmd, char *tuple) cli_perror(); goto out; } - switch(e->type) { - case UCI_TYPE_PACKAGE: - uci_show_package(ptr.p); - break; - case UCI_TYPE_SECTION: - uci_show_section(ptr.s); - break; - case UCI_TYPE_OPTION: - uci_show_option(ptr.o, true); - break; - default: - /* should not happen */ - goto out; - } + if (ptr.o) + uci_show_option(ptr.o, true); + else if (ptr.s) + uci_show_section(ptr.s); + else if (ptr.p) + uci_show_package(ptr.p); + else + goto out; /* should not happen */ break; } @@ -475,7 +467,6 @@ done: static int uci_do_section_cmd(int cmd, int argc, char **argv) { - struct uci_element *e; struct uci_ptr ptr; int ret = UCI_OK; int dummy; @@ -493,7 +484,6 @@ static int uci_do_section_cmd(int cmd, int argc, char **argv) (cmd != CMD_RENAME) && (cmd != CMD_REORDER)) return 1; - e = ptr.last; switch(cmd) { case CMD_GET: if (!(ptr.flags & UCI_LOOKUP_COMPLETE)) { @@ -501,17 +491,10 @@ static int uci_do_section_cmd(int cmd, int argc, char **argv) cli_perror(); return 1; } - switch(e->type) { - case UCI_TYPE_SECTION: - printf("%s\n", ptr.s->type); - break; - case UCI_TYPE_OPTION: + if (ptr.o) uci_show_value(ptr.o, false); - break; - default: - break; - } - /* throw the value to stdout */ + else if (ptr.s) + printf("%s\n", ptr.s->type); break; case CMD_RENAME: ret = uci_rename(ctx, &ptr); diff --git a/delta.c b/delta.c index 85ec970..5437fc1 100644 --- a/delta.c +++ b/delta.c @@ -213,7 +213,6 @@ error: static void uci_parse_delta_line(struct uci_context *ctx, struct uci_package *p) { - struct uci_element *e = NULL; struct uci_ptr ptr; int cmd; @@ -244,11 +243,14 @@ static void uci_parse_delta_line(struct uci_context *ctx, struct uci_package *p) UCI_INTERNAL(uci_del_list, ctx, &ptr); break; case UCI_CMD_ADD: + UCI_INTERNAL(uci_set, ctx, &ptr); + if (!ptr.option && ptr.s) + ptr.s->anonymous = true; + break; case UCI_CMD_CHANGE: UCI_INTERNAL(uci_set, ctx, &ptr); - e = ptr.last; - if (!ptr.option && e && (cmd == UCI_CMD_ADD)) - uci_to_section(e)->anonymous = true; + break; + default: break; } return; diff --git a/list.c b/list.c index 1640213..304c9e1 100644 --- a/list.c +++ b/list.c @@ -616,7 +616,6 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr *ptr) UCI_TRAP_SAVE(ctx, error);
Re: [PATCH uci 2/2] remove internal usage of redundant uci_ptr.last
On 7/14/23 20:28, Jan Venekamp wrote: In uci_lookup_ptr and uci_set the pointer uci_ptr ptr.last is set to the element corresponding to the first of: ptr.o, ptr.s, ptr.p. Thus, ptr.last is redundant and in case of uci_set is (and was) not always consistently set. In order to simplify the code this commit removes internal usage of ptr.last, and remove setting it from uci_set (and from uci_add_list that was never used anyway). As it is part of the public C api ptr.last cannot be completely removed though. A search on lxr.openwrt.org shows that it is used as the output of uci_lookup_ptr in several packages. So we leave setting ptr.last in uci_lookup_ptr intact. Signed-off-by: Jan Venekamp --- cli.c | 39 +++ delta.c | 10 ++ list.c| 6 -- lua/uci.c | 42 +++--- 4 files changed, 32 insertions(+), 65 deletions(-) .. @@ -349,20 +347,14 @@ static int package_cmd(int cmd, char *tuple) cli_perror(); goto out; } - switch(e->type) { - case UCI_TYPE_PACKAGE: - uci_show_package(ptr.p); - break; - case UCI_TYPE_SECTION: - uci_show_section(ptr.s); - break; - case UCI_TYPE_OPTION: - uci_show_option(ptr.o, true); - break; - default: - /* should not happen */ - goto out; - } + if (ptr.o) + uci_show_option(ptr.o, true); + else if (ptr.s) + uci_show_section(ptr.s); + else if (ptr.p) + uci_show_package(ptr.p); + else + goto out; /* should not happen */ break; How do we make sure that only one of these is set at a time? Before the code would print the last element added. } @@ -475,7 +467,6 @@ done: .. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH uci 2/2] remove internal usage of redundant uci_ptr.last
On 01/08/2023 22:27, Hauke Mehrtens wrote: On 7/14/23 20:28, Jan Venekamp wrote: In uci_lookup_ptr and uci_set the pointer uci_ptr ptr.last is set to the element corresponding to the first of: ptr.o, ptr.s, ptr.p. Thus, ptr.last is redundant and in case of uci_set is (and was) not always consistently set. In order to simplify the code this commit removes internal usage of ptr.last, and remove setting it from uci_set (and from uci_add_list that was never used anyway). As it is part of the public C api ptr.last cannot be completely removed though. A search on lxr.openwrt.org shows that it is used as the output of uci_lookup_ptr in several packages. So we leave setting ptr.last in uci_lookup_ptr intact. Signed-off-by: Jan Venekamp --- cli.c | 39 +++ delta.c | 10 ++ list.c | 6 -- lua/uci.c | 42 +++--- 4 files changed, 32 insertions(+), 65 deletions(-) .. @@ -349,20 +347,14 @@ static int package_cmd(int cmd, char *tuple) cli_perror(); goto out; } - switch(e->type) { - case UCI_TYPE_PACKAGE: - uci_show_package(ptr.p); - break; - case UCI_TYPE_SECTION: - uci_show_section(ptr.s); - break; - case UCI_TYPE_OPTION: - uci_show_option(ptr.o, true); - break; - default: - /* should not happen */ - goto out; - } + if (ptr.o) + uci_show_option(ptr.o, true); + else if (ptr.s) + uci_show_section(ptr.s); + else if (ptr.p) + uci_show_package(ptr.p); + else + goto out; /* should not happen */ break; How do we make sure that only one of these is set at a time? Before the code would print the last element added. The code in uci_lookup_ptr makes sure that the element being looked up is the last of: ptr.p, ptr.s, ptr.o with a non null value (pointer). Thus, if an option is looked up ptr.o is set and printed. When a section is looked up ptr.o is null, but ptr.s is set and printed. Else a package is looked up, ptr.o and ptr.s are null so ptr.p is printed. (When a looked up element is not found UCI_LOOKUP_COMPLETE is not set which will result in an error.) So the code prints the same element as before. Hope this explains it. } @@ -475,7 +467,6 @@ done: .. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel