In D19273#412556, @markj wrote:

In D19273#412540, @cem wrote:

In D19273#412458, @markj wrote:

Solve the race using a per-process generation counter which gets updated
before a tracepoint is removed from the hash table, and after it is
removed from the target process' vmspace.

We do the latter, but I think we're missing the former.

Why? The code immediately after the increment updates the hash table.

In D19273#412458, @markj wrote:

I wasn't careful about preserving #ifdefs here: fasttrap has been virtually untouched upstream for the past 5 years, so once this commit is in I'm going to get rid of illumos ifdefs there in an attempt to make the code a bit easier to read.

Prior to this change, ctfdump shows [2] FUNC (kern_chflagsat) returns: 1 args: (1173, 1, 5, 142, 91, 1); ... <142> ENUM uio_seg (i.e., no idea it's been const-ed out of the ABI).

In D19264#412340, @markj wrote:

Can you provide an example DIE where this occurs? Which compiler are you using?

Thanks, the locking around the global ioat list seems more robust. How much testing have you been able to perform with this patch? Do you have a test that can induce an error condition?

Why is the MK_PIE PIE_SUFFIX “_pic”? That seems inconsistent. Otherwise, LGTM.

+CC jimharris@, who might have some feedback or ability to help facilitate access to NDA'd register documents

I realize this is old, but I figure the same group is still likely to be interested. I think we'll need more like this to support GCC6+ ctfconvert. E.g.,

Thanks for doing this, Ben! I'm sorry it took me so long to take a look and appreciate your patience.

r344183

r344184

r344185

r344186

r344187

LGTM, thanks

All of the changes since the initial patch LGTM; I count 4 issues from my initial pass that I'd consider unresolved for now (all commented on in the previous update, should be pretty clear). Like before, mostly LGTM, minus the few relatively minor issues.

In D3715#409480, @trasz wrote:

Okay, so apart from implementation problems, there's actually a major architectural issue here: the fact that we wait for all the scripts from the previous generation. Do you see any way to actually fix it, or should we perhaps just scrap it and go back to the drawing board?

Looks good except for one remaining item.

In D3715#409280, @imp wrote:

I wonder: why not leverage tsort?

In D3715#409266, @cem wrote:

I suspect the rcorder -p mode could be implemented much less invasively than this change. It's fundamentally the same graph algorithm. I'll give it a shot.

I suspect the rcorder -p mode could be implemented much less invasively than this change. It's fundamentally the same graph algorithm. I'll give it a shot.

In D19109#408718, @mav wrote:

I think I would remove this code completely, together with above NULL assignments. One way or another, this check is at least incomplete, since just in if block before execution can go to out bypassing this check.

My guess is that the author intended for defconf input to parse_file() to be a *reference* rather than a bare pointer. If you look at parse_file(), you can see it initializes its copy of defconf_p when the special entry "<default>" is encountered in a file. However, that pointer is never stored anywhere and is leaked when parse_file() returns.

Two ideas (just spitballing) re: forward compatibility.

Thanks, mostly looks good! The feedback is a bit verbose but I try to go over crypto code with a fine-toothed comb. I'll try to get some time to look at the latest version of the previous change.

I think this one can be closed, a version of this was committed in r340425

In D19082#408030, @mjg wrote:

Lock acquisition can be moved few lines later, after branching on data. Similarly, unlock can be done earlier.

In D18592#407487, @sef wrote:

In D18592#407486, @cem wrote:

In D18592#400076, @sef wrote:

NB: This is mostly tested simply by compilation, as I've still got some more feedback to incorporate.

Is this still the case or do you figure you've finished that? I was waiting for that to wrap up before I took another look.

It's not plumbed into the rest of OCF, since that was requested to be a different review.

In D18592#400076, @sef wrote:

NB: This is mostly tested simply by compilation, as I've still got some more feedback to incorporate.

In D18592#407465, @sef wrote:

Switch to using a single type macro.

To elaborate a bit more, the stated commit log:

I really like this, even if it isn't ready to land yet.

In D18765#401260, @mckusick wrote:

It is useful to the application or user to distinguish EINTEGRITY from EIO. With EIO they know that the underlying media has failed and there is really nothing they can do. With EINTEGRITY the individual file has become corrupted, but if they still have its contents, they can remove it and rewrite it as the filesystem media itself is still working. Alternatively they can request the system administrator to run a check of the filesystem to attempt to clean up the file.

In D18765#401259, @mckusick wrote:

Mount is just one of the places that can return EINTEGRITY. It can also come back when accessing a file or directory whose meta information fails a check-hash.

I didn't really look at this driver thoroughly and don't plan to, but just skimmed through and pointed out some possible improvements.

(Lua lerrno.c change looks fine.)

In D18765#400410, @mckusick wrote:

Mirror might be able to read both sides but find that they have different values. So it would not know which one to return.

Thanks for the quick review and test!

This proposal is to create a new EINTEGRITY error number. EINTEGRITY's purpose is to respond to errors where an integrity check such as a check-hash or a cross-correlation has failed. For example, an uncorrectable error has been found in a filesystem or a disk subsystem such as gmirror(8) or graid3(8).