libubox: add convenience functions for moving the cursor

[pprindeville]

You might not always want to populate JSON serially, so a way to save/restore the cursor through a convenience function is handy.

Also, adding anonymous (unnamed) strings to an array isn't obvious, so added a convenience function for that.

Lastly, some minor cleanup to show what the parameters are to functions, etc.

Requires PR #16.

[pprindeville]

@nbd168 Can I get a review please? I need some of these changes to help with a migration from ISC-DHCP to Kea.

[pprindeville]

cc: @Ansuel @dangowrt @neheb @stintel @jow-

[pprindeville]

@jow- Can you please look this over?

[jow-]

Most (all?) existing functions use reference style returns (caller passes a destination variable name, function populates it and uses it's numeric return value to signify success/error) while your new functions introduce echo style returns requiring calling code to use ugly subshell invocations.

Personally I would also change the naming like that:

• json_get_pos -> json_get_position
• json_set_pos -> json_move_to
• json_get_parent -> json_move_up
• json_add_anon_string -> json_push_string

[pprindeville]

Most (all?) existing functions use reference style returns (caller passes a destination variable name, function populates it and uses it's numeric return value to signify success/error) while your new functions introduce echo style returns requiring calling code to use ugly subshell invocations.

Personally I would also change the naming like that:

• json_get_pos -> json_get_position
• json_set_pos -> json_move_to
• json_get_parent -> json_move_up
• json_add_anon_string -> json_push_string

Thanks, making those changes now.

By json_get_pos you meant json_get_cur, and that should become json_get_position?

And json_set_cur should become json_move_to? Or json_set_position (for symmetry)?

Where do the functions _get_var and _set_var get used? I can only find references to _json_get_var and _json_set_var...

json_get_parent isn't meant to move the cursor explicitly. It's a way to get the parent of a node if you need to do something like query the parent's type (array, table, etc).

For instance, you could implement json_select ".." as:

    local parent
    json_get_parent parent
    json_set_position "$parent"

if json_select ".." didn't exist.

[pprindeville]

Casting a wider net

cc: @blogic

[pprindeville]

Creating 2nd PR (#12) without the positioning getter/setting commit which seems to be stalled.

[pprindeville]

Most (all?) existing functions use reference style returns (caller passes a destination variable name, function populates it and uses it's numeric return value to signify success/error) while your new functions introduce echo style returns requiring calling code to use ugly subshell invocations.

Personally I would also change the naming like that:

• json_get_pos -> json_get_position
• json_set_pos -> json_move_to
• json_get_parent -> json_move_up
• json_add_anon_string -> json_push_string

@jow- I believe I've made these changes as requested. Can you please re-review?

[pprindeville]

@jow- I believe I've addressed the comments you made. Can I get a merge please or a re-review?

[pprindeville]

@nbd168 Can I get a review please? I need some of these changes to help with a migration from ISC-DHCP to Kea.

Still blocked on improvements to libubox before I can merge these this migration tool to Kea.

[Neustradamus]

This migration is really important from ISC DHCP to Kea.

[systemcrash]

I figure you have commit rights to fix this @pprindeville

[pprindeville]

I figure you have commit rights to fix this @pprindeville

No, my rights are only for openwrt/packages.

[systemcrash]

Get the test formalities fixed and then we can ping a maintainer.

[pprindeville]

Get the test formalities fixed and then we can ping a maintainer.

Do you know how to write test scripts? I've not dug into it.

[systemcrash]

?

: Commit subject line MUST start with '<package name>: ' (jshn.sh: Add getters for the cursor and its parent)

[pprindeville]

?

: Commit subject line MUST start with '<package name>: ' (jshn.sh: Add getters for the cursor and its parent)

Is that a new requirement?

ca3f6d0 udebug: fix file descriptor initialization for __udebug_buf_map
6339204 CMakeLists.txt: bump minimum cmake version
c1be505 udebug: fix crash in udebug_entry_vprintf with longer strings
a2fce00 CI: add build test run
12bda4b CI: add CodeQL workflow tests
eb9bcb6 ustream: prevent recursive calls to the read callback
3868f47 blob: constify attr argument to blob_memdup
b7acc8e remove compatibility code for older json-c versions
a1acd99 udebug: remove obsolete debug message
49056d1 udebug: fix issue with snapshot of remote ring
eeb6fcf examples: CMakeLists: update cmake minimum required version to 3.10
c163d7a lua: build: require CMake >= 3.10 due to dropped legacy support
7a3863a lua: CMakeLists: drop redundant cmake_minimum_required
61cc1e6 examples: CMakeLists: drop redundant cmake_minimum_required
7d6b9d9 add debian/ directory
9caf555 (origin/master) github: improve CI build

[pprindeville]

Get the test formalities fixed and then we can ping a maintainer.

Do you know how to write test scripts? I've not dug into it.

Sorry, misread. Thought you were suggesting we add test coverage as well. Which doesn’t seem like a bad idea.

[systemcrash]

ping @robimarko for merge :)

[systemcrash]

Get the test formalities fixed and then we can ping a maintainer.

Do you know how to write test scripts? I've not dug into it.

Sorry, misread. Thought you were suggesting we add test coverage as well. Which doesn’t seem like a bad idea.

Nah - the commit subject requirement has been like that for a while. It makes the overview when all the commit subjects get collated for a release and for info on openwrt.org.

[l-jonas]

I see that this makes JSON creation more powerful/flexible. A demonstration for this (how it could be used) would be nice. @pprindeville I tried to find the commit for that you need this new helper functions but I could not find it. I fear that this new functionality does not belong here and your dhcp server wrapper could be written differently.

[pprindeville]

I see that this makes JSON creation more powerful/flexible. A demonstration for this (how it could be used) would be nice. @pprindeville I tried to find the commit for that you need this new helper functions but I could not find it. I fear that this new functionality does not belong here and your dhcp server wrapper could be written differently.

Have a look at https://github.com/openwrt/packages/pull/27439/files#diff-bb791b0d9dcf1149197b51b8d13ae098141248335e144d3dc9d9810758509ead in the file dhcpv4.sh.

I've had to play games with json_select in completely counter-intuitive ways with a lot of trial and error, when it would be much simpler to get the cursor, save it for later, and then move back to it when you need to.

Indeed, I've had to write it differently but any rewrite has highlighted deficiencies in the API. At a minimum json_get_index is needed to note where you are in an array or object (i.e. table) that you're building out.

The reason for this is as you add members inside an array, because names aren't required to be unique, you must reference them by their index... you can't use json_select on the member name to select it.

[l-jonas]

Have a look at https://github.com/openwrt/packages/pull/27439/files#diff-bb791b0d9dcf1149197b51b8d13ae098141248335e144d3dc9d9810758509ead in the file dhcpv4.sh.

I only see json_get_index used there but not the other added functions.

Edit: and the whole (new) script is about converting configuration to JSON. That's what ucode is good at and shell scripts are not good at. So in the end you try to extend one tool for your use case while another one is actually made for it. It's not some old script that would be worth keeping for convenience.

[pprindeville]

Have a look at https://github.com/openwrt/packages/pull/27439/files#diff-bb791b0d9dcf1149197b51b8d13ae098141248335e144d3dc9d9810758509ead in the file dhcpv4.sh.

I only see json_get_index used there but not the other added functions.

I have another version which uses json_get_cursor and json_move_to. Six of one, half dozen of the other. Trying to use whichever is more palatable to the people who can mash the Merge button.

Edit: and the whole (new) script is about converting configuration to JSON. That's what ucode is good at and shell scripts are not good at. So in the end you try to extend one tool for your use case while another one is actually made for it. It's not some old script that would be worth keeping for convenience.

And I'm sure if I wait five minutes someone will tell me to write it in Lua.

[l-jonas]

And I'm sure if I wait five minutes someone will tell me to write it in Lua.

There are many possible paths. I still have the opinion that you are solving the wrong problem here by using a tool that is not made for that purpose and trying to "fix" this particular tool - shell scripts are not made for complex data transformations.

[l-jonas]

I cannot unresolve the discussion about "eval export" in the added code and still consider it highly relevant and not resolved.

[pprindeville]

I cannot unresolve the discussion about "eval export" in the added code and still consider it highly relevant and not resolved.

I’m following the paradigms of the existing code. I’m trying to make a low-touch change.

To scrub away all of the instances, I’d file a bug against that and hope that a maintainer takes it up.

[pprindeville]

And I'm sure if I wait five minutes someone will tell me to write it in Lua.

There are many possible paths. I still have the opinion that you are solving the wrong problem here by using a tool that is not made for that purpose and trying to "fix" this particular tool - shell scripts are not made for complex data transformations.

But shell libraries are for generating complex tree-structured data? That seems antithetical.

A lot of effort goes into jshn.sh to emulate arrays (both integer-indexed and associative) using just Busybox shell capabilities. The library would be 40% shorter in Bash which provides these, for what it's worth. So I disagree that shell scripts aren't made for complex data transformations. It might be a lift, but it's clearly not impossible and we're doing it now.

[pprindeville]

Have a look at https://github.com/openwrt/packages/pull/27439/files#diff-bb791b0d9dcf1149197b51b8d13ae098141248335e144d3dc9d9810758509ead in the file dhcpv4.sh.

I only see json_get_index used there but not the other added functions.

Write, as I explained previously I have two different versions of that script depending on whether I'm using json_get_index or json_get_position and json_move_to. Two years later I'm still waiting for guidance from the maintainers about which approach is preferred.

Edit: and the whole (new) script is about converting configuration to JSON. That's what ucode is good at and shell scripts are not good at. So in the end you try to extend one tool for your use case while another one is actually made for it. It's not some old script that would be worth keeping for convenience.

If shell scripts weren't good at it, we wouldn't even have jshn.sh to begin with. Clearly someone thought it was worth the effort or they wouldn't have written the shell interface to begin with.

[systemcrash]

Clearly someone thought it was worth the effort or they wouldn't have written the shell interface to begin with.

I think it was the only option we had. It still works, but it's tricky to debug.

[pprindeville]

Clearly someone thought it was worth the effort or they wouldn't have written the shell interface to begin with.

I think it was the only option we had. It still works, but it's tricky to debug.

No argument there. So that’s why I went with it two years ago.

[l-jonas]

I’m following the paradigms of the existing code. I’m trying to make a low-touch change.

This line uses export without eval. Now we could look at the git blame about newer and older lines.

But shell libraries are for generating complex tree-structured data? That seems antithetical.

For simple cases (serial/linear writing), they are good enough. But as soon as you start reading them or write them non linear, it becomes bad.

I think it was the only option we had. It still works, but it's tricky to debug.
No argument there. So that’s why I went with it two years ago.

https://github.com/openwrt/packages/pull/27439/commits is two years old? The world can change after proposing a change and it can happen that it is better to redo it due to that. If you are interested, I could try rewriting that PR.

[pprindeville]

I’m following the paradigms of the existing code. I’m trying to make a low-touch change.

This line uses export without eval. Now we could look at the git blame about newer and older lines.

But shell libraries are for generating complex tree-structured data? That seems antithetical.

For simple cases (serial/linear writing), they are good enough. But as soon as you start reading them or write them non linear, it becomes bad.

Then what's the point of json_select? Not every source form is going to map directly (and serially) to the target structure. That's not a reasonable expectation to have.

I think it was the only option we had. It still works, but it's tricky to debug.
No argument there. So that’s why I went with it two years ago.

https://github.com/openwrt/packages/pull/27439/commits is two years old? The world can change after proposing a change and it can happen that it is better to redo it due to that. If you are interested, I could try rewriting that PR.

I started writing it two years ago, yeah. I finally pushed it as a draft to concretize the fact that there actually was tangible work that was blocking on the libubox PR's.

What's your point? That if this drags on any longer, then a third option might materialize and I'll be asked yet again to rewrite to the flavor-of-the-month?

This is letting the perfect be the enemy of the good meanwhile people have no clean migration strategy away from and end-of-life product.

[l-jonas]

Then what's the point of json_select?

Reading JSON?

What's your point? That if this drags on any longer, then a third option might materialize and I'll be asked yet again to rewrite to the flavor-of-the-month?

No one asks to use the most recent option. But taking an old option that is so unsuitable that you want to modify it is an approach I do not support.

This is letting the perfect be the enemy of the good meanwhile people have no clean migration strategy away from and end-of-life product.

Use the "end of life product" (jshn.sh) as it is or use the new one.

[pprindeville]

Then what's the point of json_select?

Reading JSON?

Then it wouldn't need setters or dump.

[l-jonas]

Then it wouldn't need setters or dump.

Setters and dump for writing, json_select and getters for reading.

[pprindeville]

Then it wouldn't need setters or dump.

Setters and dump for writing, json_select and getters for reading.

json_select works for both reading and writing.

[l-jonas]

The code search provides many examples for reading using this function. Writing can only be found in a procd helper script but this is for editing an existing JSON and not creating a brand new one.

[pprindeville]

Added test coverage.

cc: @nbd168 @jow- @systemcrash @Ansuel @robimarko

[pprindeville]

Most (all?) existing functions use reference style returns (caller passes a destination variable name, function populates it and uses it's numeric return value to signify success/error) while your new functions introduce echo style returns requiring calling code to use ugly subshell invocations.

Personally I would also change the naming like that:

• json_get_pos -> json_get_position
• json_set_pos -> json_move_to
• json_get_parent -> json_move_up
• json_add_anon_string -> json_push_string

Two years on and I've made those changes...