Still accepted from earlier

Outside of ossl_chacha20.c, everything looks fine.

Hm, it is little endian, but I'm not confident about the two sentences prior.

The analysis makes sense and the fix looks fine to me. You could allocate two fds and check socketpair against both (instead of just low) in the final test, but I don't think there's much marginal value in that. (Frankly, I'm not a fan of unix' must-assign-lowest-fd behavior in general, but we have to abide by it.)

In the commit summary, is this a typo?

No objection in principle, but the diff is basically unreviewable as-is for me; it's like 90% autoconf generated shell.

LGTM, thanks.

Looks great to me!

In D28395#636443, @arichardson wrote:

Turns out the bug was that I messed up the size being passed to the SSE4.2 version. It does work correctly.

In D28375#634909, @cem wrote:

incore has always been able to produce false negatives, FWIW. It's fundamentally racy. I think it's mostly used as a heuristic for avoiding unnecessary prefetch.

That's true, but it's possible for callers to depend on the instantaneous result being correct because some external synchronization guarantees that the result is stable. For instance, some lock might serialize calls to getblk() for a particular vnode, so holding that lock ensures that incore() returns a stable result. I'm worried about false negatives caused by reassignbuf() in cases like this.

In D28395#635155, @arichardson wrote:

I think QEMU provides a basic amd64 CPU by default which does not include any newer features (wikipedia suggests SSE4.2 was introduced ~2008).

I think the idea here is that CRC32 is part of SSE4.2, which is part of the base feature set of amd64. If you're hitting SIGILL in QEMU in amd64 mode, that suggests QEMU's amd64 emulation is somewhat invalid. However, it's certainly optional on at least some early models of i386.

In D28375#634856, @markj wrote:

Hmm. It's not really obvious to me that all callers handle false negatives correctly. Some callers simply call getblk() in that case, but others like ufs_bmaparray() are more complicated.

I think incore was already always racy? The race scenarios may have changed slightly.

Excellent. Thanks!

I think maybe we should revert the change, if it is truly unfounded and was only observed on 12. If this wasn't ever a problem on CURRENT, the commit message rationale is wrong and misleading. Reverting is the canonical way to correct that kind of thing.

In D28260#632588, @markj wrote:

Hmm, the kldxref change doesn't fully fix the problem as I understand it (not that my commit does). The real problem is that the pnp info structure doesn't contain the module name, so devmatch(8) doesn't know which PNP info goes with which module. The kldxref change at least ensures that devmatch won't try to process a PNP info definition without having seen an MDT_MODULE record.

Wait, kldxref should be ordering these correctly since rS348309. Why isn't it?

👍

Sure, seems fine to me.

I don’t know anything about cscope but it looks fine. Ideally files that would only be found in the root (eg tinderbox crap) are “/“ prefixed to reduce the search cost in subdirectories. Orthogonal to this change.

In D27943#624066, @greg_unrelenting.technology wrote:

Well, wouldn't mixing an extra source be better?

See d7b665dfd7a6 / rS368542 / 7cda7375e629 / rS366943.

Ok, so the bug was that we never took any lock at all? But we're also moving a block of code surrounding the new vn_lock(); operation, which isn't related to our not-locking-the-vp-bug. It looks like we want to reject EINVAL flags before we start locking, which is fine. I would just like to see a better commit message than "fix a [non-specific] bug." Something like "add missing lock in fuse advlock. also, reject invalid parameters sooner (style)."

The other nice thing about this routine (ok, conditional on debug.vfs_badlock_vnode=1, but it is the default) — or VNASSERT(), VPASS() — is that they'll print out details of the vp. MPASS can't do that.

Thanks!

The make-bits and other integration look fine. I didn't have time to verify the meat of the implementation (and probably won't).

Mostly LGTM, thanks!

Awesome, thanks!

Thanks!

Re: hyphenated metadata attribute names — I'd just keep our annotations the same as they are now. If we were starting from scratch as a git project, sure, we should adopt the common git scheme. But the annotations are not special in git (just part of the commit message), and any FreeBSD tooling has to understand our previous annotations anyway.

(Otherwise, lgtm)

Can we lift the condition out and just have two switches? Not sure if that’s any better but it seems sort of ugly as is.

Thanks!

I like the approach, thanks!

Thanks for adding the new hardware. Seems like a reasonable approach to me.

Is kernel.KERNCONF actually a convention? I tend to use INSTKERNNAME and 'test.KERNCONF'.

I'm not sure I understand the solution, but totally believe there's a bug in this area.

In D27376#614415, @kib wrote:

Goal is to give/show it to users earlier, while not degrading existing old FAT support while continuing the development. E.g. the issue with the (virtually) contiguous bitmap alloc makes it unfeasible to allow any form of automatic mounting of exFAT with the current patch IMO. But on the other hand I do not see why don't you want to get reports of compat issues with real-world filesystems.

At the moment where you feel that the code is mature enough, I expect that the exFAT module will be removed together with MSDOS_EXFAT option.

Thanks for the quick look, folks!

In D27387#614192, @bdragon wrote:

I'm fine with it on readthrough, I just hadn't gotten around to the actual tests. If it broke anything in powerpc land, it should be easy to fix. Thanks for tidying up the procedure annotations!