I feel like this may cause consternation (non-standard, not really used in the project, and other geom headers use ifndef include guards). However, I have no love for the bad old way either.

(Have we tried to get FreeBSD to take #define __UNIQ __CONCAT(__uniq, __COUNTER__) ?)

Why __unused?

I think it would be preferable not to include geom_int.h in headers. I don't think it's necessary just to define macros in terms of _GEOM_DEBUG().

I sneak it in wherever I can, until someone complains. I'll replace it if you insist, but otherwise, it's supported by every compiler we could conceivably care about.

I'm not sure. Do you know how __COUNTER__ interacts with reproducible builds? If it's per-CU, that seems more readily reproducible than any other scheme.

Would it provide any benefit here? Worst case, this is a new scope and these variables shadow the external ones, right? Triggering -Wshadow, which is annoying with -Werror.

Unless ctrlvar or biop is literally named __{control,level} I think we're ok without __UNIQ in this instance. I might be missing something :-).

(Context for readers: This is about the struct sbuf *sbp __unused, which is no longer here but can be found in geom_subr.c.)

I only use the pointer to error-check the sbuf_new() return. And the error checking is only a KASSERT(). KASSERT is less like Isilon's ASSERT() and more like our ASSERT_DEBUG(), i.e., it is compiled out in !INVARIANTS builds.

Without __unused, the compiler would produce -Wunused-but-set-variable (included in -Wall) for !INVARIANTS configs, breaking the -Werror build.

To avoid the include, I think I have two options:

• I would have to move the geom_int.h include into a slightly greater number of C files instead, or
• I would have to move the _GEOM_DEBUG define into geom.h, which is already included by everything.

Of these two, I'd prefer the latter. My reservation with it is that geom.h is kind of the public API of GEOM, and consumed more broadly than just GEOM object classes.

Did you have something else in mind? Or a preference among these two?

Yeah, it was just a question, I definitely wouldn't insist on doing that for this review. Actually I had thought it was in the standard, but apparently it isn't. Perhaps that's why we haven't tried to upstream it. Yes, I believe it's set to 0 at the start of each CU.

There's probably no practical benefit here, but a theoretical one. It's as you noted about ctlvar and biop, but it also affects the format varargs.

I ran into a similar issue recently with fail points, which had me thinking about it.

Ah, yes. Thanks.

One of those two seems fine. I would have a slight preference for the former, for the API reasons. Another alternative might be a new geom_log.h or geom_dbg.h, which you'd then have to go include appropriately.

This isn't something I would insist on in any case, it just seemed non-optimal to be including an internals header in other headers like that.

This is no longer sorted.

It doesn't really make sense to have these be separate printf() calls.

gcls isn't used as a name anywhere else; class or classname would be more clear to me.

Will fix

Ah, sure. Will fix

I wanted to avoid using the C++ keyword class. classname is fine. Will fix.

New file is probably best, since geom_int.h is documented as "not included from classes," for whatever reason. I'll plan to do that.