Page MenuHomePhabricator

Add new EINTEGRITY errno
AbandonedPublic

Authored by dim on Jan 7 2019, 10:45 AM.

Details

Summary

https://reviews.freebsd.org/D18765 will introduce a new errno value
EINTEGRITY to FreeBSD's system headers. In that review, the author
has also modified two libc++ headers to match, which I would like to
incorporate in stock libc++.

Event Timeline

dim created this revision.Jan 7 2019, 10:45 AM
mclow.lists added inline comments.Jan 10 2019, 11:05 AM
include/errno.h
34–35

This chunk of ifdefs more than tripled in size when we went from "two possible additions" to "three possible additions". What happens when someone adds a fourth?

"Not sustainable" is the kindest thing I can say about this.

emaste added inline comments.Sep 7 2019, 6:05 AM
include/errno.h
34–35

Any suggestion on a sensible approach?

dim added a comment.Oct 21 2019, 11:29 AM

Now that I'm reading this header again, why do we even bother to define ELAST at all? On Linux, there is no such thing, while on BSDs and macOS, it is already provided by the regular errno.h.

I also don't see why we would define EOWNERDEAD, ENOTRECOVERABLE and so on in this difficult way, if they are not defined by the system's errno.h. Could we just not use the same system as further down below, where we assign hopefully unique integer values over 9000?

dim updated this revision to Diff 225929.Oct 21 2019, 11:42 AM

Get rid of the ELAST trickery, which is hard to maintain, and does not
appear to serve any purpose. There is no mention of ELAST in the C or
C++ standards, as far as I know.

Then, simply add the new EINTEGRITY define at the end with a unique
value, if it does not yet exist.

dim added a comment.Oct 21 2019, 12:35 PM

N.B., EOWNERDEAD and ENOTRECOVERABLE are already defined on lines 158 and line 170, respectively.

dim added a comment.Nov 8 2019, 10:31 AM

Another attempt to grab your attention :)

cem accepted this revision.Nov 11 2019, 10:37 PM
This revision is now accepted and ready to land.Nov 11 2019, 10:37 PM
dim added a comment.Nov 12 2019, 1:58 PM

cem == dim?

No, cem is Conrad Meyer <cem@FreeBSD.org>, while I'm Dimitry Andric (dimitry@andric.com or alternatively, dim@FreeBSD.org).

xbolva00 added a subscriber: jfb.Nov 12 2019, 2:06 PM

Aha, okay, but it would be better to get more “LGTM”s.

Maybe @jfb could take a look?

ldionne requested changes to this revision.Nov 13 2019, 9:29 AM

Sorry if this is a basic question, but why are we trying to provide EINTEGRITY in libc++'s errno.h header at all? I don't see this being part of the C or C++ Standard.

If systems want to provide this macro, they can in their own errno.h header -- that's why we have a #include_next. I'd like to understand that before this proceeds. I do love removing the ELAST logic if we don't need it, though.

This revision now requires changes to proceed.Nov 13 2019, 9:29 AM

Sorry if this is a basic question, but why are we trying to provide EINTEGRITY in libc++'s errno.h header at all? I don't see this being part of the C or C++ Standard.

That is a very good question, this review has been sitting for so long that the reason also slipped my mind. :) @emaste any idea? As far as I remember, this "new" errno value was added by Kirk McKusick after he added integrity checks to the BSD FFS file system, and I suspect he just went through all headers that referred to file system specific errno values to extend them.

That said, doesn't libc++'s errno.h also provide lots of errno values that are not standardized anywhere? (I'm not even sure if there *is* a standardized list of "must support" errno values...)

If systems want to provide this macro, they can in their own errno.h header -- that's why we have a #include_next. I'd like to understand that before this proceeds. I do love removing the ELAST logic if we don't need it, though.

Yes, I could not really figure out the rationale for the ELAST logic was there, since it was there from the start. Maybe @howard.hinnant remembers.

I don't recall exactly what ELAST was for either. Though something like this is typically used to see if an enum/integral value is within the range of the declared enums.

I recommend looking at https://github.com/llvm-mirror/libcxx/blob/master/src/include/config_elast.h (which I don't recall writing) for clues about which platform needs ELAST and which don't. And for those that do need it, why.

In D56398#1744453, @dim wrote:

Sorry if this is a basic question, but why are we trying to provide EINTEGRITY in libc++'s errno.h header at all? I don't see this being part of the C or C++ Standard.

That is a very good question, this review has been sitting for so long that the reason also slipped my mind. :) @emaste any idea? As far as I remember, this "new" errno value was added by Kirk McKusick after he added integrity checks to the BSD FFS file system, and I suspect he just went through all headers that referred to file system specific errno values to extend them.

That said, doesn't libc++'s errno.h also provide lots of errno values that are not standardized anywhere? (I'm not even sure if there *is* a standardized list of "must support" errno values...)

I'd ask the same question. Actually, more generally, why does libc++ even provide errno.h? Shouldn't we simply assume the C Standard library implementation to provide one? I have the impression that this sort of header cleverness just provides us with a blessed location to insert platform-specific hacks, instead of actually making those platforms conforming. But I don't really understand the context, so I might be wrong and maybe <errno.h> is important in libc++.

Since we should pick up FreeBSD's EINTEGRITY via include_next, I don't see why we need this patch. Can we abandon?

dim added a comment.May 15 2020, 7:50 AM

Since we should pick up FreeBSD's EINTEGRITY via include_next, I don't see why we need this patch. Can we abandon?

I guess so. But maybe the whole ELAST logic could be ripped out, as it does not seem to serve any purpose anymore? The whole difficult #ifdef stuff is only to define some "really last" error number, and it's only used to define the EOWNERDEAD and ENOTRECOVERABLE values, which for some reason are 'special'. But just a few lines below, a bunch of EXXX values just get defined to self-chosen values...

Let's indeed abandon this, and I'll come up with a diff to get rid of this whole first block.

dim abandoned this revision.May 15 2020, 7:50 AM
In D56398#2038671, @dim wrote:

I guess so. But maybe the whole ELAST logic could be ripped out, as it does not seem to serve any purpose anymore? The whole difficult #ifdef stuff is only to define some "really last" error number, and it's only used to define the EOWNERDEAD and ENOTRECOVERABLE values, which for some reason are 'special'. But just a few lines below, a bunch of EXXX values just get defined to self-chosen values...

Let's indeed abandon this, and I'll come up with a diff to get rid of this whole first block.

Thank you, cleaning that up would be great!