In D19902#437962, @jilles wrote:

Perhaps it is cleaner to pass the int age to be searched for by reference. However, the asymmetry in the compare function is intentional as this allows finding an object with more data given the key, while having only one storage array.

As already pointed out on the mailing list, this example has a lot of problems which are not only style bugs. Ones I think as critical except for the style issues are the following:

• The function to compare two elements should accept the same type, not using an "age" field value by using a pointer type. It is a function to compare, not look up a matched element in the definition of bsearch(3). Passing a raw value as (void *) type is not impossible, but it is not suitable for a "textbook" example.
• Using assert() looks strange to me. Instead, this program should accept a value of "key" from the command line argument and show the results of bsearch(3) depending on the specified key, and this manual page should describe what results are expected.

Adding jumbo frame support looks good to me. However, is it better to support this in ether_ioctl() instead of a driver-specific ioctl handler? Check of (ifr->ifr_mtu > ETHERMTU) in ether_ioctl() can be changed to check if the interface has IFCAP_JUMBO_MTU or not.

Fixed in a different way in r334094.

In D19488#417207, @bz wrote:

In D19488#417164, @hrs wrote:

For (2), I think the old version unconditionally runs rtsol (w/o "d") when rtsold_enable="NO", and does not when rtsold_enable="YES" because rtsold (w/ "d") will be invoked later by rc.d/rtsold should handle it. In short, the condition which determines if rtsol is invoked or not depends only on "accept_rtadv" flag. Isn't it enough?

Hmm, are you sure? Looking at the logic I changed above we invoked rtsol only if rtsold was disabled and otherwise rtsold was started later and done it (so far we agree); I think that again meant that dhcp was run before the first RS/RA cycle happened hence me needing a way to trigger that early. Hence adding the extra interface flag. I think my description above was a cache miss and is wrong (I did this change months ago). I am almost tempted to remove the extra logic and run rtsol (no d) unconditionally if "accept_rtadv" and then start rtsold later or not depending on whether the user wants long-term stability and DNS options from RAs or not.
What do you think?

In D19487#417206, @bz wrote:

In D19487#417158, @hrs wrote:

Looks good to me. We might want to put an INFO log line when ND6_IFF_IPV6_ONLY flag is changed by receiving RA or link-down event so that the sysadmin can know what is going on.

Ifconfig will show the flag in the ndoptions line, in my case currently the manual one.

nd6 options=423<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL,IPV6_ONLY_MANUAL>

Logging each time we receive an RA is not a good idea as the IPv4 exemption logic logging has shown me (need to fix that). Logging on every "state toggle" would be good, I agree. I'll prepare that.

One more comment: immediate change of the flag just after a link-down event may be problematic when the link is unstable. In such a situation, IPv4 packets can be allowed after a link-up and before another RA arrives even if the all of advertising routers keep sending ipv6only flag. We can assume that the interface should be configured to accept RA when the machine moves from a network to another, so sending a RS after a link-up then clearing ND6_IFF_IPV6_ONLY only when no RA arrives in a few seconds looks a more reasonable behavior. If at least one RA arrives after the RS, the normal handling of ipv6only flag works.

(1) looks good to me. For (2), I think the old version unconditionally runs rtsol (w/o "d") when rtsold_enable="NO", and does not when rtsold_enable="YES" because rtsold (w/ "d") will be invoked later by rc.d/rtsold should handle it. In short, the condition which determines if rtsol is invoked or not depends only on "accept_rtadv" flag. Isn't it enough?

Looks good to me. We might want to put an INFO log line when ND6_IFF_IPV6_ONLY flag is changed by receiving RA or link-down event so that the sysadmin can know what is going on.

Looks good to me.

In D19318#415233, @rigoletto wrote:

I've updated the revision summary to include the GitHub address I will be working with the OCaml ports[1].
I will keep updating OCaml itself in here from time-to-time since this is the main thing, and for better internal visibility.
Thank you.
[1] currently it was no changes over the FreeBSD official ports tree but I will move the present changes to there ASAP.

-nox11 should be removed and converted by using FLAVOR.

I would suggest the following changes using getaddrinfo() instead of inet_aton() to avoid address-family dependency. Is cap_getaddrinfo() allowed by capdns?

@@ -147,8 +163,9 @@
 const char *typelimit = "ADDR";
 int familylimit;
 const char *ipstr = "127.0.0.1";
-struct in_addr ip;
-struct hostent *hp;
+char hname[NI_MAXHOST];
+struct addrinfo hints, *res;
+int error;

Looks good to me.

This is my submission, so I will leave someone else to do further review of this change. Please do not count my "accept revision" as an immediate approval.

IMHO strvisx(3) in machine.c should have a VIS_OCTAL flag as in my original submission because meta char encoding of strvisx(3), which uses iscntrl(3), does not make sense for most of multibyte encodings. Displaying characters not for the current locale in octal (or hexadecimal) number looks more intuitive to me.

Reverting a change does not need any approval or review. Please document the reason in the commit log instead of just saying "rollback".

Please split this changes into two commits; one is to revert r335836 and another is to add setlocale() and remove printable() function. A single commit including the both is confusing.

Looks good to me. Thank you for taking care of it!