Some drive by comments — I haven't had time to read the 3-4 thousand lines of code in this review fully.

I'm still skeptical of the need to memcpy out a tree structure (are we doing queries on both sides? one side necessarily being stale?). This has never really been addressed?

Can you post the coccinelle script used somewhere? Thanks.

Drop bfinval, add B_INVALONERR flag with appropriate behavior.

@markj I've moved over discussion of netdump_configure() / CURVNET / etc from D21482 to here. I'll admit I don't understand VNETs at all, so I'm pretty naive about what is reasonable where. Given you have some idea of what the right thing to do is, I'm totally happy to do just do that.

In D21510#470182, @markj wrote:

I have no objection. I plan to review the pending diffs in the next day or so, but am swamped this week and not particularly qualified to review this particular one, so please don't wait on me if you're in a hurry.

Any objection in principle? Planning to review or ENOTIME? Suggestions for reviewers with free time and interest in this layer of the network stack?

• Take kib's suggestion of using incore() in place of getblk(). Not sure if it is obviously an improvement, but it seems to work (same tests pass).
• (Style concern about C99 declaration is obviated by removal of variable.)

• Fix race in the msdosfs ro->rw upgrade case by permitting a variant of markvoldirty to bypass the msdosfs RONLY flag otherwise preventing writes

In D21535#469071, @kevans wrote:

I had thought about this and I think I had a reason for not going that route. I don't recall now why, though, so I'll revisit.

Meta-question: Does /dev/null need special treatment or should we reject on the basis of "zero matching lines of context" anyway? Like, if the added hunk starts at @@ 0,0 @@ or whatever and the prior contents are empty...

Rebase on lexer changes.

Switch to per-lex flags for the hexadecimal numbers mode (instead of a global).

Rebase on lexer API change.

Rebase on lexer API change.

Switch from globals to per-lex flags.

In D21459#468593, @markj wrote:

Is there some reason it isn't easy to add a variant of db_read_token() that will optionally return tWSPACE and can take a caller-supplied radix? That would seem cleaner than adding more global state.

I'd like to go ahead and commit this one independent of netdump/debugnet/netgdb, actually, because I have other lexer extensions and ddb commands orthogonal to netdump/debugnet that stack on this. Can I get some eyeballs on this review, please?

In D21448#467334, @markj wrote:

I'm not sure the "_static" suffix really adds anything, FWIW.

I think I got all the oustanding issues, let me know if you see any others or need more time. (I still have more WIP that will go to phab before any of this goes into SVN, so, no big hurry). Thanks for the review.

• wording
• check supported before UP
• newline

• document in netdump.4 as well

• Drop unneeded ref, but add comment about why we don't need it
• Fako -> Fake
• Document in ddb.4; mild organizational changes while there to make sense of sometimes non-alphabetical command sorting.

Thanks!

In D21421#467347, @markj wrote:

I'm not crazy about the fact that debugnet subsumes most the netdump protocol itself:

• netdump defines some message types that are not in the main debugnet header, so if you define a new debugnet message type, you have to be careful not to collide with netdump.

Rename the routines.

In D21421#466989, @marius wrote:

I generally agree with the style(9) argument, however, if_t explicitly was introduced in r266974 as part of the - back then - new API:
"This commit introduces the 'if_t' type to replace 'struct ifnet *'as the type of a network interface."

Thanks — I'll plan to make the name change. Re: the flag, the idea is that this type of dumper is only added to the list during db/panic time, when no external event would cause a dumper_remove (unless I'm forgetting something). And then removed before normal context resumes (if !panic). So maybe the flag is redundant? It wouldn't hurt to add in any case.

In D21448#466951, @markj wrote:

Looks like some code is missing?

In D21421#466900, @marius wrote:

Could you please consistently use if_t instead of struct ifnet * in debugnet(4) components and its support code in e. g. drivers?

This approach looks technically correct to me, and it simplifies the logic, but there is definitely some microbenchmark cost to exiting to userspace every retry instead of doing so "a few times" in the kernel (with some algorithm). But this is maybe just premature optimization on my part.

Address review feedback:

One missed boring name change. No functional change.

Just rename to debugnet, etc. No other changes included in this revision.

Sorry about the long delay — life has been busy.

I'll do the big name change as a single step first, then work on addressing the other good feedback (to make it easier to use Differential's revision comparison feature). Please continue to submit feedback.

In D21421#466143, @emaste wrote:

I think net/ is justifiable from the perspective that netinet/netinet6 are files for the production IPv4/IPv6 stacks, while netdump/panicnet/debugnet is really a stripped-down special-purpose stack.

In D21421#466125, @jhb wrote:

Is the intention to support remote GDB over this?

Bit sad to be fixing DES in 2019 but unfortunately, this looks correct to me...

I'd like the commit message to provide more context but the change is good.

In D21362#464855, @jhb wrote:

In D21362#464773, @mjg wrote:

I disagree with mixing up LA_ and MA_ namespaces.

This is to avoid having lookup tables in every single lc_assert method.

In D21351#464707, @asomers wrote:

Personally, I could go either way for the default. What's most important IMHO is that ping and ping6 have the same default. Until now, they didn't.