diff mbox series

[uci,2/2] remove internal usage of redundant uci_ptr.last

Message ID 20230714182811.20444-3-jan@venekamp.net
State Accepted
Delegated to: Hauke Mehrtens
Headers show
Series remove internal usage of redundant uci_ptr.last | expand

Commit Message

Jan Venekamp July 14, 2023, 6:28 p.m. UTC
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 <jan@venekamp.net>
---
 cli.c     | 39 +++++++++++----------------------------
 delta.c   | 10 ++++++----
 list.c    |  6 ------
 lua/uci.c | 42 +++++++++++++++---------------------------
 4 files changed, 32 insertions(+), 65 deletions(-)

Comments

Hauke Mehrtens Aug. 1, 2023, 8:27 p.m. UTC | #1
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 <jan@venekamp.net>
> ---
>   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:
......
Jan Venekamp Aug. 2, 2023, 12:12 a.m. UTC | #2
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 <jan@venekamp.net>
>> ---
>>   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:
> ......
diff mbox series

Patch

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);
 		ptr->o = uci_alloc_list(ptr->s, ptr->option, NULL);
 		UCI_TRAP_RESTORE(ctx);
-		ptr->last = &ptr->o->e;
 	} else if (ptr->o->type == UCI_TYPE_STRING) {
 		/* create new list and add old string value as item to list */
 		struct uci_option *old = ptr->o;
@@ -630,7 +629,6 @@  int uci_add_list(struct uci_context *ctx, struct uci_ptr *ptr)
 		if (ptr->option == old->e.name)
 			ptr->option = ptr->o->e.name;
 		uci_free_option(old);
-		ptr->last = &ptr->o->e;
 	}
 
 	/* add new item to list */
@@ -708,10 +706,8 @@  int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
 		return uci_delete(ctx, ptr);
 	} else if (!ptr->o && ptr->option) { /* new option */
 		ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, NULL);
-		ptr->last = &ptr->o->e;
 	} else if (!ptr->s && ptr->section) { /* new section */
 		ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section, NULL);
-		ptr->last = &ptr->s->e;
 	} else if (ptr->o && ptr->option) { /* update option */
 		if (ptr->o->type == UCI_TYPE_STRING && !strcmp(ptr->o->v.string, ptr->value))
 			return 0;
@@ -724,7 +720,6 @@  int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
 			if (ptr->option == old->e.name)
 				ptr->option = ptr->o->e.name;
 			uci_free_option(old);
-			ptr->last = &ptr->o->e;
 		}
 	} else if (ptr->s && ptr->section) { /* update section */
 		if (!strcmp(ptr->s->type, ptr->value))
@@ -740,7 +735,6 @@  int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
 				ptr->section = ptr->s->e.name;
 			uci_free_section(old);
 			ptr->s->package->n_section--;
-			ptr->last = &ptr->s->e;
 		}
 	} else {
 		UCI_THROW(ctx, UCI_ERR_INVAL);
diff --git a/lua/uci.c b/lua/uci.c
index 196a25b..78b5e1f 100644
--- a/lua/uci.c
+++ b/lua/uci.c
@@ -374,19 +374,16 @@  static int
 uci_lua_get_any(lua_State *L, bool all)
 {
 	struct uci_context *ctx;
-	struct uci_element *e = NULL;
 	struct uci_ptr ptr;
 	int offset = 0;
 	int nret = 1;
 	char *s = NULL;
-	int err = UCI_ERR_NOTFOUND;
 
 	ctx = find_context(L, &offset);
 
 	if (lookup_args(L, ctx, offset, &ptr, &s))
 		goto error;
 
-	lookup_ptr(ctx, &ptr, NULL, true);
 	if (!all && !ptr.s) {
 		ctx->err = UCI_ERR_INVAL;
 		goto error;
@@ -396,33 +393,24 @@  uci_lua_get_any(lua_State *L, bool all)
 		goto error;
 	}
 
-	err = UCI_OK;
-	e = ptr.last;
-	switch(e->type) {
-		case UCI_TYPE_PACKAGE:
-			uci_push_package(L, ptr.p);
-			break;
-		case UCI_TYPE_SECTION:
-			if (all) {
-				uci_push_section(L, ptr.s, -1);
-			}
-			else {
-				lua_pushstring(L, ptr.s->type);
-				lua_pushstring(L, ptr.s->e.name);
-				nret++;
-			}
-			break;
-		case UCI_TYPE_OPTION:
-			uci_push_option(L, ptr.o);
-			break;
-		default:
-			ctx->err = UCI_ERR_INVAL;
-			goto error;
+	if (ptr.o) {
+		uci_push_option(L, ptr.o);
+	} else if (ptr.s) {
+		if (all) {
+			uci_push_section(L, ptr.s, -1);
+		}
+		else {
+			lua_pushstring(L, ptr.s->type);
+			lua_pushstring(L, ptr.s->e.name);
+			nret++;
+		}
+	} else {
+		uci_push_package(L, ptr.p);
 	}
+
 	if (s)
 		free(s);
-	if (!err)
-		return nret;
+	return nret;
 
 error:
 	if (s)