Page MenuHomeFreeBSD

make device_printf use sbuf
ClosedPublic

Authored by cem on Aug 12 2018, 3:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 10:04 AM
Unknown Object (File)
Fri, Mar 22, 6:39 PM
Unknown Object (File)
Fri, Mar 22, 6:39 PM
Unknown Object (File)
Fri, Mar 22, 6:39 PM
Unknown Object (File)
Fri, Mar 22, 11:38 AM
Unknown Object (File)
Fri, Mar 22, 11:38 AM
Unknown Object (File)
Fri, Mar 22, 11:38 AM
Unknown Object (File)
Fri, Mar 8, 6:32 AM
Subscribers

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19251
Build 18864: arc lint + arc unit

Event Timeline

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

sys/kern/subr_bus.c
2424–2425

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

2434

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.

2442

style(9) nit: sizeof(buf)

2457

This is sort of pointless for a totally static sbuf.

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
2424–2425

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.

2434

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.

2442

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.

2457

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

sys/kern/subr_bus.c
2424–2425

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.

2434

Yeah, that's a good approach.

2457

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.

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.

share/man/man9/sbuf.9
28

I'll update this before committing.

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.

Looking very good! Some nits and doc suggestions below.

share/man/man9/sbuf.9
201

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

607

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

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

690–693

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

692–693

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
2438

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

2442

sizeof() still :-)

2459

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
1300

Seems like tmpint must be initialized in this case.

1325

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

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.

607

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

690–693

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
2459

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
1300

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

1325

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

share/man/man9/sbuf.9
594–595

Sounds good to me.

607

That works for me.

690–693

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
2459

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

sys/kern/subr_prf.c
1300

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

1325

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

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
2459

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

LGTM

share/man/man9/sbuf.9
590

Maybe .Xr from printf.9.

sys/kern/subr_bus.c
2442

Still there

This revision is now accepted and ready to land.Aug 28 2018, 6:11 PM
share/man/man9/sbuf.9
590

Let me think about this.

sys/kern/subr_bus.c
2442

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.

sys/kern/subr_bus.c
2442

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

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
This revision is now accepted and ready to land.Aug 29 2018, 1:18 AM

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

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.

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