Changeset View
Standalone View
sys/kern/subr_bus.c
Show First 20 Lines • Show All 2,400 Lines • ▼ Show 20 Lines | |||||
{ | { | ||||
const char *name = device_get_name(dev); | const char *name = device_get_name(dev); | ||||
if (name == NULL) | if (name == NULL) | ||||
return (printf("unknown: ")); | return (printf("unknown: ")); | ||||
return (printf("%s%d: ", name, device_get_unit(dev))); | return (printf("%s%d: ", name, device_get_unit(dev))); | ||||
} | } | ||||
static int | |||||
sbuf_drain_printf(void *arg, const char *data, int len) | |||||
cem: We already have one of these in kern_sysctl.c — we should only have one. | |||||
jmgUnsubmitted Done Inline ActionsThat 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. jmg: That one is slightly different. It doesn't end up emulating printf by returned total printed… | |||||
cemAuthorUnsubmitted Done Inline ActionsYeah, 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. cem: Yeah, agree they are slightly different, but they are not so different they cannot be… | |||||
{ | |||||
int *retvalptr = arg; | |||||
int r; | |||||
r = printf("%.*s", len, data); | |||||
*retvalptr += r; | |||||
return r; | |||||
} | |||||
/** | /** | ||||
* @brief Print the name of the device followed by a colon, a space | * @brief Print the name of the device followed by a colon, a space | ||||
* and the result of calling vprintf() with the value of @p fmt and | * and the result of calling vprintf() with the value of @p fmt and | ||||
* the following arguments. | * the following arguments. | ||||
* | * | ||||
* @returns the number of characters printed | * @returns the number of characters printed | ||||
*/ | */ | ||||
int | int | ||||
device_printf(device_t dev, const char * fmt, ...) | device_printf(device_t dev, const char * fmt, ...) | ||||
{ | { | ||||
char buf[128]; | |||||
cemAuthorUnsubmitted Done Inline ActionsNow 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. cem: Now we're buffering twice — that's a bit unfortunate. To make this work nicely I think you'd… | |||||
jmgUnsubmitted Done Inline ActionsYeah, 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. jmg: Yeah, I thought about that. Didn't know what the correct solution was.
Putting the function… | |||||
cemAuthorUnsubmitted Done Inline ActionsYeah, that's a good approach. cem: Yeah, that's a good approach. | |||||
struct sbuf sb; | |||||
const char *name; | |||||
va_list ap; | va_list ap; | ||||
int retval; | int retval; | ||||
Done Inline ActionsThe tests and the docs say this must be a size_t to be passed as the argument for sbuf_printf_drain. cem: The tests and the docs say this must be a `size_t` to be passed as the argument for… | |||||
retval = device_print_prettyname(dev); | retval = 0; | ||||
sbuf_new(&sb, buf, sizeof buf, SBUF_FIXEDLEN); | |||||
cemAuthorUnsubmitted Done Inline Actionsstyle(9) nit: sizeof(buf) cem: style(9) nit: sizeof(buf) | |||||
jmgUnsubmitted Done Inline ActionsYeah, 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. jmg: Yeah, figured someone would catch that. This is one of the few nits I have w/ style(9). I… | |||||
cemAuthorUnsubmitted Done Inline Actionssizeof() still :-) cem: sizeof() still :-) | |||||
cemAuthorUnsubmitted Not Done Inline ActionsStill there cem: Still there | |||||
jmgUnsubmitted Not Done Inline Actionssorry, 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. jmg: sorry, wasn't trying to sneak something by, was just too aggressive in marking things done. | |||||
cemAuthorUnsubmitted Not Done Inline ActionsYeah, I figured it was something like that. No worries. cem: Yeah, I figured it was something like that. No worries. | |||||
sbuf_set_drain(&sb, sbuf_drain_printf, &retval); | |||||
name = device_get_name(dev); | |||||
if (name == NULL) | |||||
sbuf_cat(&sb, "unknown: "); | |||||
else | |||||
sbuf_printf(&sb, "%s%d: ", name, device_get_unit(dev)); | |||||
va_start(ap, fmt); | va_start(ap, fmt); | ||||
retval += vprintf(fmt, ap); | sbuf_vprintf(&sb, fmt, ap); | ||||
va_end(ap); | va_end(ap); | ||||
sbuf_finish(&sb); | |||||
sbuf_delete(&sb); | |||||
cemAuthorUnsubmitted Done Inline ActionsThis is sort of pointless for a totally static sbuf. cem: This is sort of pointless for a totally static sbuf. | |||||
jmgUnsubmitted Done Inline ActionsI know, but it's documented as required: jmg: I know, but it's documented as required:
There must be a call to sbuf_delete() for every call… | |||||
cemAuthorUnsubmitted Done Inline ActionsThat's fair. cem: That's fair. | |||||
return (retval); | return (retval); | ||||
Done Inline ActionsGiven 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. cem: Given our bytecount is `size_t` and `device_printf()` returns `int`, I'm not sure what the best… | |||||
Done Inline ActionsYeah, 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. jmg: Yeah, me either. There's also the fact that if you manage to have a device_printf that contains… | |||||
Done Inline ActionsVery much so. Plus the printf would probably never complete anyway. Maybe KASSERT + blind int cast is fine. cem: Very much so. Plus the printf would probably never complete anyway. Maybe KASSERT + blind int… | |||||
Not Done Inline ActionsThis 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: This is not an issue. len is passed in as an int. size_t is unsigned, and so should not over… | |||||
} | } | ||||
/** | /** | ||||
* @internal | * @internal | ||||
*/ | */ | ||||
static void | static void | ||||
device_set_desc_internal(device_t dev, const char* desc, int copy) | device_set_desc_internal(device_t dev, const char* desc, int copy) | ||||
{ | { | ||||
▲ Show 20 Lines • Show All 3,300 Lines • Show Last 20 Lines |
We already have one of these in kern_sysctl.c — we should only have one.