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
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
[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, );
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, );
break;
case UCI_CMD_ADD:
+ UCI_INTERNAL(uci_set, ctx, );
+ if (!ptr.option && ptr.s)
+ ptr.s->anonymous = true;
+ break;
case UCI_CMD_CHANGE:
UCI_INTERNAL(uci_set, ctx, );
- 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);
ptr->o =
