Might need ("partial?") MFC into stable/11 stable/12.

In D22099#482956, @brooks wrote:

I believe the correct way to handle this is to update the vendor tree and then merge. I'm happy to do that.
Is this only in the new version I imported on the 8th or do we need to MFC as well? If the latter, I suspect it makes sense to direct commit this patch to 12.1 rather than doing a full update this late in the release process.

Document usage scenario bit further. Fix some mandoc formattings nits. Suggested by @mckusick

I will keep it at reviews for 1 week and then it will land unless major objections are received. Thanks!

Hey @imp can you please review the latest revision and confirm that it looks OK now?

Rename MTREE -> DISTR_MTREE for clarity and in hope to make @rgrimes happier.

In D20520#443376, @imp wrote:

bootools is a terrible name, so bad I'm ticking 'request changes'.

src.bootools.mk -> src.tools.mk as suggested by @imp

Looks good, as long as it compiles.

Looks good to my VFS-naive eye. Thanks!

Overall looks like a good start, however there are some things that I personally dislike:

Not sure why this is still open. @mizhka_gmail.com please GC?

In D19707#422201, @kib wrote:

Your 'fix' is not enough even for mtime. Look at the tmpfs_sync(MNT_LAZY).

Check read-only flag in an unified manner.

In D19682#421511, @kib wrote:

As I understand, currently ro tmpfs mounts are useless and the code is incomplete. I did very quick look and the first VOP I looked at, tmpfs_mkdir(), seems to allow modifications of the ro mounts (this is from code reading, not from testing). Was code review done to ensure that read-only tmpfs mounts indeed operate as intended ?

In D18586#397007, @mckusick wrote:

Given that this plausibly works only with two different /etc/fstab files, I think your change should also include an update to the manual page to explain how the update keyword is intended to be used.

Document "update" option, as suggested by @mckusick

In D19266#413225, @kib wrote:

Rebase; fix conflicts.
Formatting and comment changes.

In D19266#412957, @kib wrote:

In D19266#412909, @cem wrote:

In D19266#412904, @sobomax wrote:

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

What you cited is clear example why I (and others) do not do this dependency tracking insanity in the conf/files syntax. Config(8) is only effective at flat dependencies where all required config options are explicitly provided in the kernel config file. Tracking dependencies like turn on B when A is on, but also consider side effects from C, D, E, architecture, and so on, is impossible with it.
I thought it is obvious that the only reason why I wrote this patch is because there are more planned consumers of xz in kernel, which alone is the reason why I want it to be a dedicated option, and why I want (need) to avoid the dependency drama.
Also see the recent discussion of iflib modularization.
That said, are there any other _technical_ notes about the diff ?

In D19266#412644, @kib wrote:

Remove 'device geom_uzip', add xz to notes.

Also "device xz" needs to be added to the conf/NOTES.

Looks good overall. I am little bit puzzled over usage of "option GEOM_UZIP" vs. "device geom_uzip + device xz". I suspect those are functionally equivalent. Half of the kernel configs in the tree use former (including NOTES) and half the latter. Some subset of the two use both. :( Since you are touching those files anyways, perhaps you can go and convert all cases of "device geom_uzip" into plain "option GEOM_UZIP" and not patch it up by adding device xz? It might be also a good idea to adjust geom_uzip(4) accordingly to provide "One-True-Way" to compile geom_uzip(4) statically into the kernel? Thanks!

In D18382#398850, @eugen_grosbein.net wrote:

In D18382#391348, @sobomax wrote:

o Due to popular request rename "erase" into "trim".

You need now fix clist[] array in the args.c as it must be sorted or else bsearch() fails to find "trim" and dd conv=trim bails out with error "unknown conversion trim".

Keep clist[] array in the args.c sorted by the name so that bsearch() DTRT.

In D18586#396514, @kib wrote:

So what happen when somebody run 'mount -a' second time, with non-idempotent mount option + update ?

In D18586#396505, @mckusick wrote:

This change does not seem wrong, though the only way I can see it being usable is to have the read-only mount's in /etc/fstab, then a special fstab that has the desired update mount lines in it.

In D18546#395707, @eugen_grosbein.net wrote:

In D18546#395642, @sobomax wrote:

2. At some point administrator logins and changes MTU from value X prescribed by the DHCP to some other value Y of his liking.

JFYI: the PR 229432 mentiones working way to ignore option 26 (MTU) supplied by DHCP server:
interface "em0" {

supersede interface-mtu 0;

}

@eugen_grosbein.net actually upon thinking a little bit more about the difference between this proposed change and r336195 I came to a conclusion that those two are not necessarily mutually exclusive, but rather complementing each other. The problem with just r336195 alone is that it invalidates implicit expectations that dhclient should not try to touch interface at all after initial configuration, unless there are some changes on the DHCP side. As an administrator, I'd not expect dhclient to be actively policing interface parameters and try to enforce them, which would be the case for MTU now. Consider the following hypothetical scenario:

In D18535#395442, @eugen_grosbein.net wrote:

In D18535#395370, @sobomax wrote:

In D18535#395273, @eugen_grosbein.net wrote:

Yes, it requires corruption of private node memory. As owner of multiple routers mass servicing thousands of customers using multiple NETGRAPH nodes I can assure you that panic is not appropriatie action. Appropriate action is some form of block for traffic flow trough the node in question (with logging) leaving other nodes working.

Well, that's where I respectively disagree. As an owner of hundreds of FreeBSD systems servicing many millions of customers I think that rebooting the system immediately after any slight kernel heap/stack memory corruption is detected is not just appropriate but the only sane action available. Shutting down particular netgraph node and hope for the best would just leave the service down indefinitely with no hope for any sorts of automatic recovery.

Node destruction and recreation is easily implemented automatic recovery. We obviously have different usage patterns. I use net/mpd5 that can use ng_nat connecting it to the session that can be destroyed and re-created by any of "client" or "server" recovering from corruption and not interfering with other sessions. Other usage patterns can react on such event to re-create nodes too.

In D18546#395445, @eugen_grosbein.net wrote:

Hmm, there was https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229432 and corresponding commit https://svnweb.freebsd.org/base?view=revision&revision=336195 fixing the problem.
Was it insufficient or your tree does not have that fix?

In D18535#395273, @eugen_grosbein.net wrote:

Looks good with one exception: additional plain panic(). Can it be replaced with KASSERT?

IDK, there is no way this option to be set to anything but DLT_RAW or DLT_EN10MB in the course of normal operation of the node. So it would require some form of memory corruption to actually happen. IDK, panic(9) seems an appropriate action in that case. There are other panic(9) call in the code in similar situations.

Yes, it requires corruption of private node memory. As owner of multiple routers mass servicing thousands of customers using multiple NETGRAPH nodes I can assure you that panic is not appropriatie action. Appropriate action is some form of block for traffic flow trough the node in question (with logging) leaving other nodes working.

In D18535#395220, @eugen_grosbein.net wrote:

Looks good with one exception: additional plain panic(). Can it be replaced with KASSERT?

Make sure we have enough data for ethernet header.

o Due to popular request rename "erase" into "trim".