So this looks pretty good. I've got a couple of minor remarks.

The main blocker for this patch is that it changes the interface between kernel and userspace. I'm correctly working on converting ADDRULE/GETRULE to use nvlists, which will ensure we don't break that ABI. See https://lists.freebsd.org/pipermail/freebsd-pf/2021-March/009419.html

I hope to post patches for that in the coming week.

The nvlist ioctl commit series is:

• https://reviews.freebsd.org/D29556
• https://reviews.freebsd.org/D29557
• https://reviews.freebsd.org/D29558
• https://reviews.freebsd.org/D29559
• https://reviews.freebsd.org/D29560
• https://reviews.freebsd.org/D29561
• https://reviews.freebsd.org/D29562

That's not quite everything that's needed for this though. It occurred to me that we still don't have a good way of expressing the change in pf_pool_opts. I think I want to copy the definition into libpfctl so pfctl can use it to populate the nvlist. In main we can (once the relevant ioctls have changed over) remove pf_pool_opts.

I've posted the rest of the patches that should let us add this without ABI breakage. The last of them is D29644 (and it lists its required parent revisions).

With those patches it's safe to add fields to pfctl_rule, which is now an entirely userspace structure.

@kp thanks for notifying the changes.
I'd like to hear your idea for the following points to rebase the code to the nvlist-based implementation.

• I plan to keep the struct pf_pool.mape member and the struct pf_kpool.mape member unchanged from the current implementation, while introducing the nvlist-based representation as an array of 3 uint16_t's with the name "mape" under "rpool". Is this design consistent with your implementation?
• I plan to implement the code to allow the nvlist-based rules without the "mape" names, not handing this as an error. This is because there might be a timing that a newer kernel that knows "mape" with the older pfctl that does not know "mape", while I feel the care might be a little too much.

In D29468#665184, @takahiro.kurosawa_gmail.com wrote:

@kp thanks for notifying the changes.
I'd like to hear your idea for the following points to rebase the code to the nvlist-based implementation.

• I plan to keep the struct pf_pool.mape member and the struct pf_kpool.mape member unchanged from the current implementation, while introducing the nvlist-based representation as an array of 3 uint16_t's with the name "mape" under "rpool". Is this design consistent with your implementation?

I think you want to keep pf_pool unchanged. Other than that, yes: add mape to pf_kpool (that's kernel-internal, so it's safe to modify).
We can also safely add new fields to the nvlist, so adding a "mape" nvlist child to "rpool" would be the way to go as well.
It may or may not make more sense to have three separate number entries in the "mape" nvlist rather than one three entry array.

• I plan to implement the code to allow the nvlist-based rules without the "mape" names, not handing this as an error. This is because there might be a timing that a newer kernel that knows "mape" with the older pfctl that does not know "mape", while I feel the care might be a little too much.

Yes. The "mape" nvlist in "rpool" should be treated as optional. Well, maybe. Given that we've not released any version that has nvlist support but not MAP-E support it'd arguably be fine to just require it.
Some ABI/API breakage in current is to be expected.
I'd say that if it's harder than if (nvlist_exists_nvlist(nvl, "mape")) { ... } it might be fine to just make it mandatory.

D29669 may be a useful example.

In D29468#665329, @kp wrote:

In D29468#665184, @takahiro.kurosawa_gmail.com wrote:

@kp thanks for notifying the changes.
I'd like to hear your idea for the following points to rebase the code to the nvlist-based implementation.

• I plan to keep the struct pf_pool.mape member and the struct pf_kpool.mape member unchanged from the current implementation, while introducing the nvlist-based representation as an array of 3 uint16_t's with the name "mape" under "rpool". Is this design consistent with your implementation?

I think you want to keep pf_pool unchanged.

Thinking about this some more, I think that what we want to do here is to introduce a pfctl_pool, for use in libpfctl / pfctl. Similar to how we already have struct pfctl_rule and struct pfctl_anchor.

In D29468#665332, @kp wrote:

Thinking about this some more, I think that what we want to do here is to introduce a pfctl_pool, for use in libpfctl / pfctl. Similar to how we already have struct pfctl_rule and struct pfctl_anchor.

That makes sense!
I'll make a patch that introduce pfctl_pool and remake this MAP-E patch based on that, keeping pf_pool unchanged.