Implement table replacements

[yazshel]

Hi Mindaugas,

Here is some initial work on a new ioctl & library function to replace a table contents, which I'm submitting mainly for comment.

It's completely untested at this stage so please proceed with caution! I am hoping to add a test to npftest and also attempt to build a NetBSD kernel with the updated sources so I can test functionality from greyd.

Let me know if you have any thoughts, criticisms or suggestions!

Cheers,

Timshel

[yazshel]

FYI you may notice that I've taken quite a different approach than originally suggested. Instead of adding an NPF_CMD_TABLE_SWAP command to the existing IOC_NPF_TABLE ioctl, this implements a completely new IOC_NPF_TABLE_REPLACE ioctl call which takes an nvlist_t (rather than the struct used by IOC_NPF_TABLE - nvlist overhead isn't really an issue here as there will only be a single ioctl() to replace an entire table). So I thought I should add something about my reasoning and the details of this (still untested!) implementation :)

Essentially I realised that implementing the NPF_CMD_TABLE_SWAP command for the existing IOC_NPF_TABLE ioctl requires a significant amount of support infrastructure to allow use in any reasonable use-case. This would require several fairly significant changes to the existing codebase, including:

1. addition of NPF_CMD_TABLE_CREATE and NPF_CMD_TABLE_REMOVE commands to create the table to be swapped in and remove the swapped-out table
2. changes to the table allocation system to be able to add extra tables after allocation (since space for the number of tables is allocated exactly at config time)
3. helper code to change the name and IDs of a table when swapping in a new table - also not easy to do in an atomic way
4. some way to determine the next available table ID and allocate table IDs atomically

After all this, the application logic required to create, populate and replace a table will require a stack of ioctl(2) calls, each with a context switch. It will also need to come up with a temporary table name that doesn't conflict with any existing table (not difficult but it's still a little messy).

So when I realised this and also saw that the table population logic is all already there for IOC_NPF_LOAD, this seemed the better solution. Essentially this allows creating & populating a new table using existing libnpf(3) functions - npf_table_create() and npf_table_add_entry() - before submitting it using the new npf_table_replace() library function (which calls the new IOC_NPF_TABLE_REPLACE ioctl(2) in turn). The kernel side is also much more simple as it mostly utilises existing config functionality.

Hopefully that all makes sense; comments, critiques or suggestions welcome!

[rmind]

@yazshel: Just an update -- I have been extremely busy, but have not forgotten about this PR.

Also, FYI -- the latest Github version of NPF has been synced to NetBSD-current.

[rmind]

@yazshel: Feedback provided. Generally, the changes are very good, just few small things to fix.

[rmind]

Over to you.

[yazshel]

I'm also thinking that I should add a command to npfctl to replace an active table with one rebuilt from a file. Not only would this be handy for testing, it's also likely to have some real-world usefulness.
I wasn't 100% sure as to where to put this sub-command; keeping it with other commands under npfctl table makes the most sense from a usability perspective. But this does raise the issue of how to incorporate it within the current npfctl table subcommands: these currently all share the same ioctl(2), with a payload of npf_ioctl_table_t, and thus fit well together in the logic of the npfctl_table() command handler. However, adding a replace subcommand would involve a bit of messing up that neat subcommand logic, as the new IOC_NPF_TABLE_REPLACE ioctl is its own beast, more similar to load than anything else. So I'd need to restructure npfctl_table() a bit to accomodate this replace subcommand. Are you happy for me to give that a go, or would you prefer a different approach?

What I'm proposing would be to:

1. Add replace subcommand to npfctl_table() in src/npfctl/npfctl.c. A command test early in npfctl_table() would pass control to a new npfctl_table_replace() command handler function:
2. Add a new npfctl_table_replace(int fd, int argc, char **argv) function, which will:
   a. Parse table name, type and filename from command line arguments
   b. Check that table name exists in the active config and fetch its tid
   c. Build a new nl_table_t with the passed name, type & file using a modified version of npfctl_build_table() (ie. split into 2 to decouple it from the nl_config_t npf_conf global variable, and also allow manually setting the TID)
   d. Call npf_table_replace() with the new table structure.

Does that sound like an acceptable approach?

Cheers,

Timshel

[rmind]

On npfctl improvement: sounds about right, but I think this can be done as a separate pull request (PR).

I can merge this PR later today or tomorrow, if you will be done with the changes.

[yazshel]

Requested changes are all done, and I'm happy to make the discussed npfctl a separate PR as you suggest.

Let me know if you'd like me to rebase and squash related commits to clean up the git history, or if there is anything else that needs to be changed.

Thanks for your feedback and help 👍

[rmind]

Thanks. I squash-merge, so it doesn't matter how you commit.

I will include your name (and email?) in the NetBSD commit message, unless you prefer otherwise.