Discussion:
cleanup on dmesg.
(too old to reply)
Rob Landley
2017-06-09 19:59:21 UTC
Permalink
Raw Message
I lost a couple days to a bad cold. I've got the release notes written
up but I don't want to leave dmesg demoted for another release.

Did I make it clear how deeply horrible this new API is? We're talking
_craptacular_:

http://landley.net/notes-2017.html#06-02-2017

Ok, command: first we xopen("/dev/kmsg") which means dmesg fails if
devtmpfs isn't mounted, instead of falling back to the original API that
doesn't depend on that. And I really prefer grouping the variable
declarations at the start of blocks (yes c99 allows them to be anywhere
but they're hard to find when you do that; code style thing).

Is returning avoid void function from another void function like that
reliable? My c99-tc3 draft says (6.8.6.4):

Constraints

A return statement with an expression shall not appear in a function
whose return type is void. A return statement without an expression
shall only appear in a function whose return type is void.


Is this a gcc extension? Or did C11 add it? If not, it strikes me as a
thing the "everything is undefined behavior!!!1!!one!" C++ guys who
hijacked compiler development will intentionally break as soon as they
think of it?

I had to look up what the lseek(SEEK_DATA) was for, redid the comment a
bit: SYSLOG_ACTION_CLEAR makes /dev/kmesg pretend that the data is a
hole, except it reads as nonzero. Because violating the heck out of VFS
conventions is yet more crap this terrible API pulls.

Then we do this:

char msg[8192]; // CONSOLE_EXT_LOG_MAX. No really.

do len = read(fd, msg, sizeof(msg));
if (len<1) break;
msg[len] = 0;

Meaning if we read exactly 8192 bytes, we write a zero right _after_ the
array and overflow the allocated space.

print_all() seems to be a separate function (rather than inlined in
main) just so it can return if it goes down the klogctl() path? Hmmm...
I should inline both those code paths in main() as an if/else case.

Hmmm...

$ diff -u <(./dmesg) <(./dmesg -S)
...
@@ -3321,7 +3321,7 @@
[330349.285200] Enabling non-boot CPUs ...
[330349.304335] x86: Booting SMP configuration:
[330349.304339] smpboot: Booting Node 0 Processor 1 APIC 0x1
-[330349.307528] cache: parent cpu1 should not be sleeping
+[330349.307528] cache: parent cpu1 should not be sleeping
[330349.308023] CPU1 is up
[330349.310236] ACPI: Waking up from system sleep state S3
[330349.311488] ohci-pci 0000:00:12.0: System wakeup disabled by ACPI

The new API is sometimes allowing extra whitespace at the start of dmesg
lines... Ah, I can add a space to the sscanf() line and fix that.

If you ctrl-c out of the middle of colored output it can leave the
command line colored, we should have a signal handler to clean that up.
(Which also means we don't need a color(0) at the end of each line. But
we would then need to add it it before loglevel, but that's still less
data output in the default case...)

Why are we calling time(0) in format_time()? We're doing that but _not_
calling sysinfo() at the same place so if we pipe the output to less the
times change during the delay. Except all we ever use out of sysinfo is
uptime so why are we saving the whole of sysinfo in TT rather than just
a time_t?

Ok, it's possible I missed something (still sick) but it seems to work.
I should try to do a test for it under the mkroot stuff, but not holding
the release up for that.

Rob

Loading...