It seems like ESNs are just AAD in integrity modes and maybe sometimes plaintext or sometimes AAD in other ipsec modes. I’m not sure why they’d be considered an algorithm? I think this will be easier to express in the rework branch so I’m glad to hear John plans to circulate it soon.

In D22363#489261, @mw wrote:

Hi @jhb - so this patch will not apply onto your solution, right? When do you expect to push it?
Do you have some open repo to see and try it?

Can you provide more background of ESNs and motivation for including them as an ocf transformation? Thanks!

The majority of ocf drivers aren’t ready for this. Can you give me some context on what ESNs are and why they ought to be part of doing an OCF transform? Have you considered integrating these in @jhb’s OCF rework branch?

(The changes look good to me, aside from the question.)

So this fixes a single live && MROUTING && VIMAGE case, but leaves the !live && MROUTING && VIMAGE case broken, with the same misleading print, as well as any other !live && VIMAGE code. It seems like a better solution might be teaching it about VIMAGE.

Change looks functionally correct to me, my suggestions are all stylistic.

Any reason to avoid generating to a temporary file first (possibly in tmpfs) and installing with install -C (i.e., don't re-write or bump mtime if the contents did not change)? Slightly more friendly to low-endurance media /var devices.

With the caveat that I am unfamiliar with this part of the build, looks reasonable to me. (And I am supportive of the objective.)

In D22252#486649, @emaste wrote:

Maybe not, here's where the GNU one was removed: https://github.com/llvm/llvm-project/commit/e8bd4e50e4a30e1a046b4f6a8768dce8569a982f There's a comment in CrossWindows.cpp that "LLVM currently does not know how to emit" non-x86 unwind tables still.

In D22252#486604, @emaste wrote:

Also how does this interact with (32-bit) arm which uses its own unwind format?

In D22252#486603, @emaste wrote:

I am not sure if we should instead default to off everywhere though, and enable it via cmdline flags.

Carrying around an openssh patch is far from ideal. Is there any chance it can be upstreamed?

FWIW, tinderbox is clean.

Some initial tentative feedback and CCing kib@. Caveat, I haven't yet consulted any official documentation nor have I looked at the adjacent code to understand what it is doing, so my comments may just be uninformed. But, provisionally, I don't understand the motivation for some of this changeset.

Is "not terrible" the bar now?

This is a weird and not obviously better change for FreeBSD. I assume it makes sense for some particular reason on CHERI, but CHERI isn't even a Tier 4 FreeBSD arch. Most of the changes are just obfuscating formatted prints rather than any actual change to casts.

Looks good to me, thanks!

In D18028#484871, @scottph wrote:

ah sorry, here's the same patch, but with context restored.

In D22173#484781, @lwhsu wrote:

Perhaps we can add STRIP= to this this case first and then discuss the global setting.

I’m not sure it’s a good idea to default no-strip all tests. It likely fixes my test in particular, but my test could add STRIP= in isolation.

The new patch doesn't have context, so phabricator doesn't know how to show inter-diff and us casual readers cannot expand adjacent code in the web interface. Would you mind uploading a diff with full context (i.e., diff -U99999 ... or using the arc utility)? Thanks!

I didn't look too closely but this look a lot better, thanks. One question below.

Thanks, this is kinda weird but I like it better than before.

In D22122#483447, @emaste wrote:

I think you could go ahead and commit the END() additions independently in any case?

Fix a number of ENTRY()s without matched END()s.

Looks good to me, thanks.

Scott, thanks for picking this up! I'm really glad to see this being driven towards commit. I also really appreciate the cf_dev -> dev revert; keeping name changes divorced from functional changes reduces the size of the patch and increases clarity of what has changed.

(Committed, link/revision above.)

In D21421#482078, @emaste wrote:

For some reason Phabricator is not auto-closing some reviews.

Committed in r353685, I'm not sure why Phabricator has not closed this yet.