Page MenuHomeFreeBSD

make device_printf use sbuf
ClosedPublic

Authored by cem on Aug 12 2018, 3:07 AM.

Details

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>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jmg created this revision.Aug 12 2018, 3:07 AM
cem added a subscriber: cem.Aug 16 2018, 6:49 PM

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

sys/kern/subr_bus.c
2409–2410 ↗(On Diff #46586)

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

2431 ↗(On Diff #46586)

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.

2439 ↗(On Diff #46586)

style(9) nit: sizeof(buf)

2454 ↗(On Diff #46586)

This is sort of pointless for a totally static sbuf.

cem added a reviewer: cem.Aug 16 2018, 6:50 PM
jmg added a comment.Aug 17 2018, 3:41 PM

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 ↗(On Diff #46586)

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.

2431 ↗(On Diff #46586)

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.

2439 ↗(On Diff #46586)

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.

2454 ↗(On Diff #46586)

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

cem added inline comments.Aug 17 2018, 6:11 PM
sys/kern/subr_bus.c
2409–2410 ↗(On Diff #46586)

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.

2431 ↗(On Diff #46586)

Yeah, that's a good approach.

2454 ↗(On Diff #46586)

That's fair.

jmg planned changes to this revision.Aug 20 2018, 12:32 AM

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

jmg updated this revision to Diff 47180.Aug 23 2018, 4:45 PM

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 added inline comments.Aug 23 2018, 4:45 PM
share/man/man9/sbuf.9
28 ↗(On Diff #47180)

I'll update this before committing.

jmg added a comment.Aug 23 2018, 4:47 PM

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 added a comment.Aug 24 2018, 3:18 AM

Looking very good! Some nits and doc suggestions below.

share/man/man9/sbuf.9
195 ↗(On Diff #47180)

Drain functions should have return type int, not void.

594 ↗(On Diff #47180)

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

594–595 ↗(On Diff #47180)

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 ↗(On Diff #47180)

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

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

683–684 ↗(On Diff #47180)

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

685–686 ↗(On Diff #47180)

short lines can be combined

sys/kern/kern_sysctl.c
370 ↗(On Diff #47180)

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

sys/kern/subr_bus.c
2439 ↗(On Diff #46586)

sizeof() still :-)

2423 ↗(On Diff #47180)

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

2444 ↗(On Diff #47180)

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 ↗(On Diff #47180)

Seems like tmpint must be initialized in this case.

1323 ↗(On Diff #47180)

style nit: return (r)

jmg planned changes to this revision.Aug 24 2018, 5:02 PM
jmg marked 12 inline comments as done.

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

share/man/man9/sbuf.9
594–595 ↗(On Diff #47180)

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 ↗(On Diff #47180)

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

683–684 ↗(On Diff #47180)

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 ↗(On Diff #47180)

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

sys/kern/subr_bus.c
2444 ↗(On Diff #47180)

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 ↗(On Diff #47180)

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

1323 ↗(On Diff #47180)

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

cem added inline comments.Aug 24 2018, 8:09 PM
share/man/man9/sbuf.9
594–595 ↗(On Diff #47180)

Sounds good to me.

599 ↗(On Diff #47180)

That works for me.

683–684 ↗(On Diff #47180)

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

sys/kern/kern_sysctl.c
370 ↗(On Diff #47180)

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

sys/kern/subr_bus.c
2444 ↗(On Diff #47180)

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

sys/kern/subr_prf.c
1298 ↗(On Diff #47180)

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 ↗(On Diff #47180)

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 updated this revision to Diff 47374.Aug 28 2018, 5:00 AM

Update and address various comments.

jmg marked 22 inline comments as done.Aug 28 2018, 5:05 AM

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

jmg planned changes to this revision.Aug 28 2018, 5:16 AM
jmg marked 3 inline comments as done.

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

sys/kern/subr_bus.c
2444 ↗(On Diff #47180)

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 updated this revision to Diff 47375.Aug 28 2018, 5:19 AM
jmg marked an inline comment as done.

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

cem accepted this revision.Aug 28 2018, 6:11 PM

LGTM

share/man/man9/sbuf.9
590 ↗(On Diff #47374)

Maybe .Xr from printf.9.

sys/kern/subr_bus.c
2439 ↗(On Diff #46586)

Still there

This revision is now accepted and ready to land.Aug 28 2018, 6:11 PM
jmg added inline comments.Aug 28 2018, 9:26 PM
share/man/man9/sbuf.9
590 ↗(On Diff #47374)

Let me think about this.

sys/kern/subr_bus.c
2439 ↗(On Diff #46586)

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 added inline comments.Aug 28 2018, 9:39 PM
sys/kern/subr_bus.c
2439 ↗(On Diff #46586)

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

jmg updated this revision to Diff 47419.Aug 29 2018, 12:39 AM

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...

This revision now requires review to proceed.Aug 29 2018, 12:39 AM
cem accepted this revision.Aug 29 2018, 1:18 AM
This revision is now accepted and ready to land.Aug 29 2018, 1:18 AM
cem added a comment.May 1 2019, 11:52 PM

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

cem commandeered this revision.May 7 2019, 5:47 PM
cem edited reviewers, added: jmg; removed: cem.
This revision now requires review to proceed.May 7 2019, 5:47 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 7 2019, 5:47 PM
This revision was automatically updated to reflect the committed changes.
imp added a comment.May 8 2019, 5:02 PM

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