Similar to what was done for device_printfs in r347229.
Details
Details
This is a proof of concept. If it seems reasonable, I'll extend it to the rest
of the copy-pasted DEBUG() macros in GEOM.
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 25730 Build 24312: arc lint + arc unit
Event Timeline
Comment Actions
- Fix a missed case or two (didn't have "DEBUG" in the names)
- Inline remaining g_print_bio() callers and remove the API; document g_format_bio() in g_bio.9
Comment Actions
I like that this also moves the logging code text out of line.
| sys/geom/geom_int.h | ||
|---|---|---|
| 40–41 ↗ | (On Diff #60486) | 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. |
| 46–47 ↗ | (On Diff #60486) | (Have we tried to get FreeBSD to take #define __UNIQ __CONCAT(__uniq, __COUNTER__) ?) |
| sys/geom/geom_io.c | ||
| 1040 | Why __unused? | |
| sys/geom/label/g_label.h | ||
| 37 | 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(). | |
Comment Actions
Thanks for taking a look!
| sys/geom/geom_int.h | ||
|---|---|---|
| 40–41 ↗ | (On Diff #60486) | 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. |
| 46–47 ↗ | (On Diff #60486) | 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 :-). |
| sys/geom/geom_io.c | ||
| 1040 | (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. | |
| sys/geom/label/g_label.h | ||
| 37 | To avoid the include, I think I have two options:
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? | |
| sys/geom/geom_int.h | ||
|---|---|---|
| 46–47 ↗ | (On Diff #60486) | 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. |
| sys/geom/geom_io.c | ||
| 1040 | Ah, yes. Thanks. | |
| sys/geom/label/g_label.h | ||
| 37 | 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. | |
Comment Actions
My inline comments are just nitpicking.
Since we now have a few inline code fragments that create an sbuf and use it to print a BIO to the console with some optional prefix, doesn't that suggest that we should keep a version of g_bio_print() that does exactly that? For logging errors we could move the error value into the prefix. I think it would be inconvenient to have to write several lines of boilerplate any time one wants to just print a BIO to the console.
| share/man/man9/Makefile | ||
|---|---|---|
| 1008 | This is no longer sorted. | |
| share/man/man9/g_bio.9 | ||
| 288 | It doesn't really make sense to have these be separate printf() calls. | |
| sys/geom/geom_subr.c | ||
| 82 | gcls isn't used as a name anywhere else; class or classname would be more clear to me. | |
Comment Actions
Thanks!
Sure, that would be fine.
For logging errors we could move the error value into the prefix.
Hm, I don't follow.
I think it would be inconvenient to have to write several lines of boilerplate any time one wants to just print a BIO to the console.
Well, sure. It appears no one is actually using this function => I just figured no one wants to actually do that.
| share/man/man9/Makefile | ||
|---|---|---|
| 1008 | Will fix | |
| share/man/man9/g_bio.9 | ||
| 288 | Ah, sure. Will fix | |
| sys/geom/geom_subr.c | ||
| 82 | I wanted to avoid using the C++ keyword class. classname is fine. Will fix. | |
| sys/geom/label/g_label.h | ||
| 37 | 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. | |
Comment Actions
Since we now have a few inline code fragments that create an sbuf and use it to print a BIO to the console with some optional prefix, doesn't that suggest that we should keep a version of g_bio_print() that does exactly that?
Sure, that would be fine.
For logging errors we could move the error value into the prefix.
Hm, I don't follow.
I'm talking about g_vfs_done(), which prints "g_vfs_done: " <bio info> "error = %d".
I think it would be inconvenient to have to write several lines of boilerplate any time one wants to just print a BIO to the console.
Well, sure. It appears no one is actually using this function => I just figured no one wants to actually do that.
g_vfs_done(), the fdc driver and the example in the man page all do that. I can imagine it being useful for ad-hoc debugging outside of a GEOM class. (Within a class we have the debug macro.)
Comment Actions
Address review feedback:
- Push definition out to separate header, geom_dbg.h
- Push includes out from class headers into individual compilation units
- Restore a version of g_print_bio, which takes a prefix and formatted suffix (and use it in fdc, geom_vfs)
- Update g_bio.9
- Update / sort g_bio.9 MLINKS