On 10/8/23 15:38, David Gow wrote:
On Wed, 9 Aug 2023 at 23:54, Richard Fitzgerald rf@opensource.cirrus.com wrote:
Add handling to kunit_log_append() for log messages that are longer than a single buffer fragment.
The initial implementation of fragmented buffers did not change the logic of the original kunit_log_append(). A consequence was that it still had the original assumption that a log line will fit into one buffer.
This patch checks for log messages that are larger than one fragment buffer. In that case, kvasprintf() is used to format it into a temporary buffer and that content is then split across as many fragments as necessary.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
I think this looks good (and is a long-overdue addition to the logging functionality).
One thought I have (and I'm kicking myself for not thinking of it earlier) is that this is starting to get very similar to the "string stream" functionality in lib/kunit/string-stream.{h,c}. Now, I actually think this implementation is much more efficient (using larger fragments, whereas the string stream uses variable-sized ones). Particularly since there are a lot of cases where string streams are created, converted to a string, and then logged, there's almost certainly a bunch of redundant work being done here.
My gut feeling is that we should stick with this as-is, and maybe try to either work out some better integration between string streams and logging (to avoid that extra string allocation) or find some way of combining them.
I completely failed to notice string_stream. I could re-implement this to use string_stream. I wonder whether appending newlines gets a bit inefficient with the current string_stream implementation. Could add newline support to string_stream and alloc one extra byte for each fragment just in case we need to add a newline.
The string_stream implementation would waste a lot a memory if you log many short lines. My current code wastes memory if you log lots of lines that don't fit in available space in the current fragment - though it's simple to shuffle the formatted string backwards to fill up the previous fragment (I just haven't done that yet).