Page MenuHomePhabricator

Add new EINTEGRITY errno
Needs RevisionPublic

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.Mon, Oct 21, 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.Mon, Oct 21, 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.Mon, Oct 21, 12:35 PM

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

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

Another attempt to grab your attention :)

cem accepted this revision.Mon, Nov 11, 10:37 PM
This revision is now accepted and ready to land.Mon, Nov 11, 10:37 PM
dim added a comment.Tue, Nov 12, 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.Tue, Nov 12, 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.Wed, Nov 13, 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.Wed, Nov 13, 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++.