Curently, go-uci is a "best effort" parser and does not strictly follow the rules imposed on UCI files.

This is mainly due to the fact, that the parser function in the original UCI C source does not declare the syntax in an obvious way (i.e. I could not find such a declaration).

In PR #7, I've accepted a change, which expands the accepted characters for identifiers from _a-zA-Z to -_a-zA-Z0-9. A valid UCI config file is /etc/config/wireless, which contains entries like

config wifi-device 'radio0'
	option type 'mac80211'
	...

Contrast this with this statement from the OpenWRT wiki on the file syntax (last paragraph in that section):

It is important to know that UCI identifiers and config file names may contain only the characters a-z, 0-9 and _. E.g. no hyphens (-) are allowed. Option values may contain any character (as long they are properly quoted).

The identifier wifi-device contains a hyphen, but does actually work and the uci binary accepts it without error.

It would be nice for the parser to be closer to uci/libuci and reject invalid identifiers (also the setter/getter methods should only accept valid identifiers).

A first step would be documenting the actually accepted file syntax, based on the C sources.

Parser seems to be unable to handle empty lists
E.g., list example ''
Try it out

Hi.

I noticed that the parser is a bit more relaxed than the uci parser in OpenWRT. Would you be interested in a patch that would fail is we see something other than a-z, A-Z, 0-9 or "_"?

@relet made me aware of this.

This is another problem I've noticed, while debugging #4.

On many OpenWRT device /etc/config is a read-only filesystem overlaid with a writable filesystem, while/tmp/ is usually an actual tmpfs/ramdisk.

Consider saveConfig() being called for network.
The call to ioutil.TempFile in saveConfig creates a temporary file in\tmp\.
A subsequent call to os.Rename() tries to hardlink the file to /etc/network.
While this a considered best practice to ensure atomic file changes it will fail for this case as the hardlink crosses filesystem boundaries.
A typical error looks like this:
rename /tmp/network012772557 /etc/config/network: invalid cross link

As the user can't choose a different location for the temporary files, this bug renders this library useless on many OpenWRT installations.

Seems like I found another bug.
I've just noticed that in case of an error writing to the config file (e.g. permission problem, out of disk space ...) Commit() still returns nil.

It ignores any errors returned by t.saveConfig(config).

I'll have a pull request ready in a few moments.

I just found an off-by-one-error in Del.

Consider the following Config-snippet from /etc/config/system:

config timeserver 'ntp'
	option enabled '1'
	option enable_server '0'
	list server '0.lede.pool.ntp.org'
	list server '1.lede.pool.ntp.org'
	list server '2.lede.pool.ntp.org'
	list server '3.lede.pool.ntp.org'

When I execute

	r := NewTree("/etc/config")
	r.Del("system", "ntp", "server")
        r.Commit()

I expect the all the list server options to be be gone.
However the file /etc/config/system remains unchanged.

After some poking, I found an of -by-one-error in types.go:240.
Slicing a list with [:l] where l is the length of the list returns the same list without changes.
[:l-1] is correct.

Moreover I would suggest simplifying the switch statement to s.Options = append(s.Options[:i], s.Options[i+1:]...) as it is cleaner, more idiomatic and covers all the cases.

Arguably it has some performance overhead if the first or last option is deleted (append with empty lists), but I don't think anyone would want to change their configuration frequently enough that it becomes a real problem. (Also I would expect append have its own optimizations for those two corner cases.)

The following code is used quite a bit. The problem is that it will mask all sort of errors behind "section not found". It probably not possible to change this without breaking the API, though. Not sure if you want a suggested fix.

func (t *tree) ensureConfigLoaded(config string) (*config, bool) {
	cfg, loaded := t.configs[config]
	if !loaded {
		if err := t.loadConfig(config); err != nil {
			return nil, false
		}
		cfg = t.configs[config]
	}
	return cfg, true
}

Cheers,

Per.

Just nitpicking but in theory OpenWrt is written with uppercase O and W only :)