We prefer POSIX memset() over bzero() in new code.

What did bring in this requirement?

It looks like we're (going to be) changing the size of struct pfsyncreq here. Have you thought about how to cope with the ABI breakage that entails?

Same question here. If we change the size of pfsync_pkt we're going to be changing the on-wire protocol, and that's going to break sync between versions with and without IPv6 support.

I did not. Not on purpose, but because I made the change without being aware of the implications.

Should we resort to having one pfsyncreq struct per AF? We would then need to find another way to figure out the AF being used, as the current code relies on the sockaddr in pfsync_sockaddr.

What did bring in this requirement?

It doesn't belong on this diff, ended up here by accident.

With the IPv6 code in place, it's needed in order to have RT_DEFAULT_FIB for a in6_selectsrc_addr() call.

Same as the previous case, I did the change without being aware of the implications.

I honestly don't have an idea on how to deal with it. I welcome any suggestions.

Not blocking, but it might be good to think about how to move this into libpfctl.

There's not much point to checking for allocation failures in user space. The kernel will (by default, at least) overcommit memory. So the first thing a user space applications knows about a memory shortage is not when malloc fails, but when it touches previously unused memory and gets killed by the out-of-memory killer.

The ifconfig code is pretty inconsistent about checking for malloc() failure already. My personal view is that it's not worth writing error handling code that will never be exercised (and thus is almost certain to be buggy), but this isn't a blocking remark.

I'm not strictly happy with using ifr_cap_nv here, because that field is for "nv-based cap interface", and this isn't about interface capabilities.

There's no obvious alternative, so perhaps we should look at adding a new name in sys/net/if.h. Let's put that aside until we've got everything else done and tested.

We fail to free ifr.ifr_cap_nv.buffer here.

Is this worth considering? Do we every specify something that's not IPv4 or IPv6?

This might be something that can be usefully translated into a libpfctl library function.
Something like pfsyncctl_set_syncdev(int fd, const char *ifname);

This seems like pointless work. Can't we just nvlist_add_string(nvl, "synced", val);?

Is it worth doing this? All we want is to set an empty string for syncdev, so why not just do that unconditionally?

Also a good libpfctl candidate.

And we'd do case AF_INET6 here, right?
Which will come in a subsequent commit?

Yes. That's a status flag, and it'll get reset by the kernel on every read.
We can set or clear it on write, and it won't affect what the kernel does.

What's the purpose of pfsync_kstatus?

The 'k' prefix usual means this is a kernel-only structure (at least in the context of pf), so I'm a little confused at seeing that in user space.

This might be a structure definition that belongs in libpfctl.h

We should zero out the status struct so we don't end up with random garbage if the kernel doesn't supply a syncdev.

Should we be returning an error here?

It's arguable, because we're checking the result of pfsync_syncpeer_nvlist_to_sockaddr(). On the other hand, the kernel really shouldn't be sending us invalid data. Perhaps this needs an assertion.

We leak the nvlist if the ioctl fails.

We're leaking packed and nvl here.

We should probably separate the input parsing (struct vs nvlist) from the actual pfsync configuration so we can re-use the latter for both cases.

We'd leak data here. Unless we failed to allocate data, in which case I'm not sure what copyin() will do.

How so? It should be safe as soon as you've copied the strings out. (numbers are always copied, as are bools. Byte arrays and strings need to be copied, but always should be, because we can't reasonably rely on the lifetime of the nvlist structure).

Leaks data.

I don't know if it's worth putting this in a separate file. Maybe just in pf_nv?

(No strong views either way.)

The way pfsync was handling the case where no peer address was set (or was unset) was by leaving the address empty (that would later be filled by the multicast address in if_pfsync.c), which would result in a AF that is neither IPv4 nor IPv6.
I did not change that behavior, so the code here accommodated for it.

It was a complicated way of ensuring val was not bigger than IFNAMSIZ.

I added a check for the size, and adjusted the code per your suggestion.

	if (strlen(val) > IFNAMSIZ)
		errx(1, "interface name %s is too long", val);

I will simplify it, but we still need the nvlist_exists_string() check, otherwise I get a EEXIST error, as the syncdev is returned by the SIOCGETPFSYNCNV ioctl.

That is my current plan.

I tried to mimic what was done with pf_kstate in pf_ioctl.c but failed to understand it was a kernel-only structure.

I made this function considering that it would be useful for the other functions, but in the end it only got used once in pfsync_status. I'm considering getting rid of it and moving this code into pfsync_status directly.

Since we now zero out the status struct here, I removed the zeroing out that was done in line 367, before calling pfsync_nvstatus_to_kstatus

I looked around the ifconfig code and it seems they don't use assertions. I'm inclined towards returning an error.

I am freeing data before returning now.

According to copy(9), "All but copystr() return EFAULT if a bad address is encountered". So if the allocation fails, copyin() will see the address NULL and return EFAULT, right?

I wrote that when I still had a very poor understanding of nvlists. I removed the comment.

I went with freeing data right after the nvlist_destroy(), which covers this case and prevents leaking on normal execution as well.

I wasn't sure on how separate pf and pfsync are kept, so I made a new file.
I'm OK with moving this to pf_nv as well.

We should try to be consistent about using {} for single line statements. At least within the same function.

Is this comment still relevant?

I'd just ENOMEM here, that's by far the most likely cause anyway, and nvlist_error() isn't going to tell us that.

Needs an empty line before this.

I usually try to invert this. That is:

if (<something wrong)
   return (error);

do_good_thing();

It tends to reduce levels of indentation and make things slightly cleared. It's a very slight difference here, but this is the one I noticed.

return (error);

return (0);