Implement npfctl table replace subcommand. (#52)

[yazshel]

Here's an implementation of a frontend command for the table replacement functionality.

Command syntax is:
npfctl table <tid> replace [-n <newid>] [-t ipset|lpm|const] <path>

where path is the path to the file containing IPs/networks for the table. It all uses the same npfctl_build_table() function from the config parser behind the scenes.

Let me know of any changes you'd like me to make.

[yazshel]

BTW I've just realised that one change I made early in the implementation is no longer necessary - I changed the signature of npfctl_table_getid() to allow passing in the config; but I've refactored since then and its no longer needed for the new functionality.

I'll look to back out the signature change and unnecessary changes; unless you see some other benefit to having the function work in this way.

[rmind]

Thanks for working on this! Feedback provided.

[rmind]

Maybe: "table '%s' not found"

[rmind]

Actually: "table '%s' not found in the active configuration"

Also, can you please move this logic into a separate function, e.g. something like npfctl_active_table_byname()? I will use it elsewhere..

[yazshel]

I had a look at doing this but it's a bit tricky to manage without introducing a memory leak; if we npf_config_destroy() the config, it destroys the table we're wanting to return too...

Is there another way you can think of doing this? Two options I can think of are:

1. Add a global variable called e.g. active_ncf, with functions npfctl_active_config_load() (which wraps npf_config_retrieve()) and npfctl_active_config_destroy() to load and destroy it. Usage example:

if (!npfctl_active_config_load()) {
		err(EXIT_FAILURE, "npf_active_config_load()");
}
npfctl_table_getbyname(active_ncf, tablename);
npfctl_active_config_destroy();

2. add a npf_table_clone() utility function to libnpf which wraps nvlist_clone(); so that the table can be cloned from the retrieved config, allowing the config to be destroyed. This would allow a npfctl_active_table_byname() function which does something like:

nl_table_t *
npfctl_active_table_byname(const char *name)
{
	nl_config_t *ncf;
	nl_table_t *t;
	/* Get existing config to lookup ID of existing table */
	if ((ncf = npf_config_retrieve(fd)) == NULL) {
		err(EXIT_FAILURE, "npf_config_retrieve()");
	}
	if ((t = npfctl_table_getbyname(ncf, name)) != NULL) {
		t = npf_table_clone(t); /* new function which clones the nl_table_t nvlist */
	}
	npf_config_destroy(ncf);
	return t;
}

Neither seems an ideal solution; the first is clumsy and still requires a fair bit of inline logic; perhaps the second is a little better but it's not efficient if fetching more than one table is desired. Any preference, or other ideas?

[yazshel]

Hi Mindaugas,

I haven't forgotten about this, life has just hit a busy patch lately and it might be a few weeks before things settle down. I'll chip away at the changes in the meantime; I hope that's OK.

Cheers,

Timshel

[yazshel]

I've pushed fixes for most of the reviews; just the npfctl_active_table_byname() review is remains outstanding. Please see my reply to your review comment above :)