make device_printf use sbuf

[cem]

Summary

device_printf does multiple calls to printf allowing other console messages to
be inserted between the device name, and the rest of the message. This change
uses sbuf to compose to two into a single buffer, and prints it all at once...

I could use PRINTF_BUFR_SIZE + smug instead of hard coding 128, suggestions?

previously:

Trying to mount root from ufs:/dev/ufs/rootfs [rw]...
mmc0:  Instruction Set Attributes 0 = <AES+PMULL,SHA1,SHA2,CRC32>
ACMD42 failed, RESULT: 4
 Instruction Set Attributes 1 = <>
mmc0:          Processor Features 0 = <AdvSIMD,Float,EL3 32,EL2 32,EL1 32,EL0 32>
Card at relative address 43690 failed to set bus width

after change:

mmc0: ACMD42 failed, RESULT: 4
Trying to mount root from ufs:/dev/ufs/rootfs [rw]...
 Instruction Set Attributes 0 = <AES+PMULL,SHA1,SHA2,CRC32>
mmc0: Card at relative address 43690 failed to set bus width
 Instruction Set Attributes 1 = <>
         Processor Features 0 = <AdvSIMD,Float,EL3 32,EL2 32,EL1 32,EL0 32>

[cem]

I really like the general idea! Some nits and suggestions below.

sys/kern/subr_bus.c

2409–2410

We already have one of these in kern_sysctl.c — we should only have one.

2419

Now we're buffering twice — that's a bit unfortunate. To make this work nicely I think you'd want to just invoke prf_putbuf() directly from your drain function. (putchar also includes a nice check for kdb_active, which you could redirect to cnputs.)

Also, probably use PRINTF_BUFR_SIZE instead of the hardcoded 128.

2427

style(9) nit: sizeof(buf)

2442

This is sort of pointless for a totally static sbuf.

[jmg]

Thanks for the comments, you solved some of the issues I knew about and didn't know how to solve best.

sys/kern/subr_bus.c

2409–2410

That one is slightly different. It doesn't end up emulating printf by returned total printed, but I agree that we should consolidate them.

I'll consolidate them to subr_prf.c, w/ a NULL check if you don't need the printf count. There is already sbuf_putbuf there.

Looks like sbuf_putbuf isn't documented.

2419

Yeah, I thought about that. Didn't know what the correct solution was.

Putting the function alongside sbuf_putbuf will solve the problem of prf_putbuf being a private interface.

2427

Yeah, figured someone would catch that. This is one of the few nits I have w/ style(9). I like dropping the parens to make it more apparent what is going on (i.e. you don't gloss over thinking it's a function call), but I'll fix it.

2442

I know, but it's documented as required:
There must be a call to sbuf_delete() for every call to sbuf_new().

[cem]

sys/kern/subr_bus.c

2409–2410

Yeah, agree they are slightly different, but they are not so different they cannot be consolidated :-).

sbuf_putbuf only takes a finished non-draining sbuf, so it's sort of a different use case.

2419

Yeah, that's a good approach.

2442

That's fair.

[jmg]

Working on testing sbuf, and adding new drain function from printing, w/ both a userland and kernel versions of the function.

[jmg]

Update. Use a new created printf drain function that works in both
userland and kernel. Document the function, and add tests for it.

We don't have tests for the kernel part of the printf_drain function.

[jmg]

share/man/man9/sbuf.9

28

I'll update this before committing.

[jmg]

This does add some tests that were missing. I can commit those and the MLINK for _putbuf separately if people would like. I'm also fine breaking out the sys/kern/subr_bus.c changes as well.

[cem]

Looking very good! Some nits and doc suggestions below.

share/man/man9/sbuf.9

195

Drain functions should have return type int, not void.

594

.Vt size_t and .Dv NULL if you're feeling really pedantic :-)

594–595

I would rephrase with two simpler sentences for clarity:

"The argument arg must be either NULL or a pointer to a size_t byte counter.
If arg is not NULL, the number of bytes drained is added to *arg."

Might also want to mention the return value (but I am ok eliding it, too).

599

Maybe: "... prints the finished sbuf to ..."

Although that is made clear by your subsequent addition, so maybe skip this.

683–684

This isn't quite true. It returns any error accumulated in s_error.

685–686

short lines can be combined

sys/kern/kern_sysctl.c

370

Need to switch this reference over to the new drain routine.

sys/kern/subr_bus.c

2423

The tests and the docs say this must be a size_t to be passed as the argument for sbuf_printf_drain.

2427

sizeof() still :-)

2444

Given our bytecount is size_t and device_printf() returns int, I'm not sure what the best way to squash that is.

Very few routines in subr_bus actually use the return value from device_printf but it seems calcified in the bus_if ::print_child interface :-(. Maybe you can just ASSERT the size_t value is sane or clamp to something in the range of int. I don't see how this information could be useful to BUS_PRINT_CHILD consumers.

sys/kern/subr_prf.c

1298

Seems like tmpint must be initialized in this case.

1323

style nit: return (r)

[jmg]

I'll update the patch shortly. Thanks for the review.

share/man/man9/sbuf.9

594–595

Yeah, I'll make that change, I wasn't happy with the original wording.

IMO, no need mentioning the return value, as that is an "internal" detail for _set_drain, and you can read that else where.

599

Only reason I touched this section is minor cleanup of _putbuf.

683–684

that's an additional case, the following clause limits the ENOMEM.

This change was minor cleanup of the ENOMEM lacking .Er.

sys/kern/kern_sysctl.c

370

I picked the same name so I didn't have to.

sys/kern/subr_bus.c

2444

Yeah, me either. There's also the fact that if you manage to have a device_printf that contains more than 2GB of data, I think we've done something wrong as well.

sys/kern/subr_prf.c

1298

ok, fixed. I just always assign, and then do the NULL check before the accumulation.

1323

another issue I have w/ style(9).. :p

[cem]

share/man/man9/sbuf.9

594–595

Sounds good to me.

599

That works for me.

683–684

Ah, you're right. Sorry, I don't know what I was thinking.

sys/kern/kern_sysctl.c

370

Doh! I didn't pick up on that, sorry.

sys/kern/subr_bus.c

2444

Very much so. Plus the printf would probably never complete anyway. Maybe KASSERT + blind int cast is fine.

sys/kern/subr_prf.c

1298

I'm not sure I understand the description of the fix but any solution that doesn't involve incrementing an uninitialized value is fine :-).

1323

Yeah, me too, but I really value the consistency of rigorously following a single style document. (And this file is more or less compliant today.)

[jmg]

Update and address various comments.

[jmg]

Fix most of the issues. I will address the cast to int w/ a KASSERT in another update.

[jmg]

All but the printf error is addressed, and the next patch will address this issue.

sys/kern/subr_bus.c

2444

This is not an issue. len is passed in as an int. size_t is unsigned, and so should not over flow (unless 4GB is printed), and the return is always len. There was an issue w/ the userland in that printf could return an error, and we need to return -errno to denote that error, and the next patch will have that fixed.

[jmg]

add fix for when printf returns an error, pass the error up
correctly.. prf_buf does not return an error..

[cem]

LGTM

share/man/man9/sbuf.9

590

Maybe .Xr from printf.9.

sys/kern/subr_bus.c

2427

Still there

[jmg]

share/man/man9/sbuf.9

590

Let me think about this.

sys/kern/subr_bus.c

2427

sorry, wasn't trying to sneak something by, was just too aggressive in marking things done. fixed in my tree. will include it when I decide about xr printf 9.

[cem]

sys/kern/subr_bus.c

2427

Yeah, I figured it was something like that. No worries.

[jmg]

ok, this should be ready to land.. Not going to update printf(9),
that'll be a different patch, and there are missing vprintf and vlog
from the body of the man page...

[cem]

Please commit this, or let me know if you'd like me to hold off from committing it on your behalf :-).

[imp]

Although I was tardy in reviewing, this looks good to me. glad it went in.