- User Since
- Jul 9 2015, 9:56 PM (218 w, 3 d)
Some drive by comments — I haven't had time to read the 3-4 thousand lines of code in this review fully.
Fri, Sep 13
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?
Thu, Sep 12
Can you post the coccinelle script used somewhere? Thanks.
Wed, Sep 11
Drop bfinval, add B_INVALONERR flag with appropriate behavior.
Tue, Sep 10
@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.
Mon, Sep 9
Any objection in principle? Planning to review or ENOTIME? Suggestions for reviewers with free time and interest in this layer of the network stack?
Sun, Sep 8
Sat, Sep 7
Fri, Sep 6
- 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.)
Thu, Sep 5
- Fix race in the msdosfs ro->rw upgrade case by permitting a variant of markvoldirty to bypass the msdosfs RONLY flag otherwise preventing writes
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...
Wed, Sep 4
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.
Tue, Sep 3
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?
Mon, Sep 2
Sun, Sep 1
Fri, Aug 30
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.
- check supported before UP
- 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.
Thu, Aug 29
Wed, Aug 28
Rename the routines.
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.
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.
Tue, Aug 27
Address review feedback:
One missed boring name change. No functional change.
Just rename to debugnet, etc. No other changes included in this revision.
Mon, Aug 26
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.
Sat, Aug 24
Bit sad to be fixing DES in 2019 but unfortunately, this looks correct to me...
Thu, Aug 22
I'd like the commit message to provide more context but the change is good.