That makes sense, thanks.

In D18546#395642, @sobomax wrote:

In D18546#395596, @sobomax wrote:

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?

Yes, you are right. We were looking at the FreeBSD 11.2 code here, I have not noticed there is another change in a trunk to fix the same issue. :(

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.

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.

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.

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

In D18380#391441, @imp wrote:

I'm pissed this was committed. It wasn't ready and in total breach of protocol.
This matter is *NOT* settled and you're lucky I don't just remove it from the tree.

Closed prematurely.

In D18380#391343, @cem wrote:

In D18380#391248, @eugen_grosbein.net wrote:

First, DIOCGDELETE is not GEOM-specific ioctl()

I don't know why you say that. It was created specifically for GEOM (r169284). That's what the 'G' in the name represents, along with other similar ioctls (r92698).

I strongly disagree against GEOM'ifying or CAM'ifyng such simple code.

I'm fine with rest of changes.

The check for CHR and SBLK is to issue nice and correct error message in case of pilot error instead of generic and obscure error after ioctl() so I'd prefer to keep it.

The operation in question is dangerous and can easily result in loss of data if used by unexperienced root. This is why I want it to NOT defaults to -f but instead defaults to verbose dry-run mode. Hence the need for -f and -q.

Use .Cm instead of .Nm for keyword. Begin sentences with new line.

Looks good. While you are here, please also replace last "/etc" literal in the pw.c's main() function with _PATH_PWD used with all other places of pw(1) sources.

In D17566#374928, @yuripv wrote:

In D17566#374923, @eugen_grosbein.net wrote:

While the change seems to be semantically right, I don't really like we have more and more places with hard-coded "pw.conf" in the pw(8) sources.

Could you please to add #define _PW_CONF "pw.conf" to pw.h and use "%s/" _PW_CONF (concatenation of C string literals) instead of "%s/pw.conf" ? And make same change to pw_utils.c' get_userconfig() function too, while you are here.

Sure, done.

While the change seems to be semantically right, I don't really like we have more and more places with hard-coded "pw.conf" in the pw(8) sources.

Just as a comment: boolean_str() problem seems to be a left-over after my change r326848 that changed magic values -2 .. 1 to symbols P_NONE etc. but missed a change for boolean_str().

Why is it acceptable to break KBI here adding new member to struct inpcblbgroup not to its end?

This also needs an update for share/man/man9/EVENTHANDLER.9

Same code, more context.

In D16789#357757, @mat wrote:

In D16789#357745, @dg_syrec.org wrote:

In D16789#357744, @mat wrote:

The flavor does not make much sense.

First because it is only needed for i386 processors before pentium 4, so it only concerns on i386 architecture, so having a flavor on all other archs is bogus.

I was also wondering if it would be possible to somehow declare this flavor arch-specific (i386-only).

I think you could get away with doing something like this:

FLAVORS= ${FLAVORS_${ARCH}}
FLAVORS_i386= blah
FLAVOR?= ${FLAVORS:[1]}

But I am still not sure this is a good idea to begin with.

Second, do you have real example of people actually running Go on machines from last century?

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=230731

That is completely insane.

This was fixed long time ago, please close.

I'm going to commit this soon unless an objection is raised.

I'm going to commit latest revision soon unless an objection is raised.

The only change comparing with previous revision is added truncation of interface description obtained with sysctl(3) if it appears longer than 64 octets.

In D16459#349483, @avg wrote:

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.

In D16459#349416, @bz wrote:

In D16459#349396, @bz wrote:

Hmm RFC 2863 says:

ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

Style changes.

While the intention is good, I'm curious why someone would want to use "netstat -rn" these days for BGPv4 full view having about 700 thousands prefixes?

It seems that rc.shutdown.8 is just an alias for rc.8, do we add a cross-references in such a case?

NAME

rc - command scripts for auto-reboot and daemon startup

Looks just fine.

Add reference to rcorder(8) manual page.

sizeof counts in octets (bytes), not bits
and you do not need complexity of "if (sizeof...)" but simply cast hostid to uint64_t unconditionally first.

In D15247#323710, @julian wrote:

Do you have any idea how to mention rc.resume in rc(8) correctly?

as mentioned before, add some cross referencing in the related man pages. Use the "See also" sections.

In D15329#323385, @wollman wrote:

It's probably worth a little bit of thought as to what is the more common case, a zillion epair interfaces on one host (read: half a zillion vnet jails) or a much smaller number of epairs on a larger number of hosts. It sounds like you are well placed to assign your own addresses, whatever the default may be.

In D15247#323501, @avg wrote:

It would be nice to add a mention of rc.resume and the new resume keyword to rc(8) as Julian suggested.

We have only one manual page acpiconf(8) that mentions /etc/rc.resume with exception of ancient apmd(8) page that is installed for i386 systems only. I've added needed description to acpiconf(8).

Well, I have many hosts having over housand of ngXXX interfaces, so yes, two bytes are needed at least.

In D15112#323137, @harti wrote:

How does it do that? The problem is whether a 64-bit increment or add is atomic with regard to a read from another CPU.

Using hostid should be fine as it adds protection against inter-jail MAC collisions. Just reuse it when possible instead of calling second time for it.

Why do you bother calling getcredhostid (which involves mutex lock/inlock) and arc4random second time while creating second interface?
Can't you just use already computed values from scb->oifp->if_hw_addr ?

The change passes "make validation" tests.

In D14547#306289, @pi wrote:

In D14547#306226, @eugen_grosbein.net wrote:

Also, why don't you just use "ifconfig em0 x.y.z.A/24 -alias; ifconfig em1 x.y.z.B/24 alias" to perform the switch? It should just work.

This has a short timewindow, during which packets will be rejected. On high-traffic ports this will cause connection drops or losses.

In D14547#306227, @rstone wrote:

I don't think that this conversation is going to go anywhere productive.

In D14547#306225, @rstone wrote:

In D14547#306224, @eugen_grosbein.net wrote:

Care to elaborate what are you trying to achieve by moving subnet route between interfaces?

My customer is using it as a lame form of failover. If the interface with address A goes down, we can fail over the subnet route to address B (on a different interface) and address B remains functional (address A remains down, of course. I did same it was a lame form of failover).

I have no ability to push back on the customer on this point. Their position is that this configuration was supported on previous versions and therefore must remain supported.

In D14547#306213, @rstone wrote:

In D14547#305425, @rstone wrote:

And you do you insist in doing things manually while moving IP (first route change, then reassign IP) instead of just allowing system do all housekeeping for you by reassigning IP first?

What FreeBSD version (including revision) do you use for TEST PLAN script, e.g. what is your "stock FreeBSD"?

I vote to block it for now. We can re-think it in case some useful example of real case came up.

Indeed, people sometimes tend to do strange things with their network and that's nothing wrong with it (I do sometimes, too). But we should have a distinction for supported configuration and unsupported ones (leading to panic or anothe misbehavour). For example, we do not support assigning IP address to lagg or bridge member - one can do that but that won't work generally.

Why don't we just reject creation of vlan interface on lagg member (as opposed to lagg itself)?

Do you really need to mix whitespace change with the fix (last chunk)?

Initial submitter of the PR 224556 is a bit too rash.

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

In D13426#280611, @kib wrote:

In D13426#280595, @eugen_grosbein.net wrote:

nop and others not mentioned in the diff use "off_t" internaly already.

Look at the argument types for g_nop_create() as an example.

More changes: geom_nop, geom_stripe and geom_redboot.

In D13426#280612, @imp wrote:

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

In D13426#280534, @kib wrote:

Does this change break ABI or KBI ? Or both ?

Some providers take stripesize/offset in create, e.g. nop. Do they need a fix included into the change ?