lkml.org 
[lkml]   [2023]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 7/7] kunit: Don't waste first attempt to format string in kunit_log_append()
    On Wed, Aug 9, 2023 at 10:54 AM Richard Fitzgerald
    <rf@opensource.cirrus.com> wrote:
    >
    > It's wasteful to call vsnprintf() only to figure out the length of the
    > string. The string might fit in the available buffer space but we have to
    > vsnprintf() again to do that.
    >
    > Instead, attempt to format the string to the available buffer at the same
    > time as finding the string length. Only if the string didn't fit the
    > available space is it necessary to extend the log and format the string
    > again to a new fragment buffer.
    >
    > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

    Hello!

    I find this version slightly more confusing but I realize that the
    extra vsnprintf() call is unnecessary. So I am ok with this version
    except for one question below.

    I am curious what David Gow thinks and it would also be good to have
    another set of eyes here.

    My testing of this showed no problems.

    Thanks!
    -Rae

    > ---
    > lib/kunit/test.c | 33 +++++++++++++++++----------------
    > 1 file changed, 17 insertions(+), 16 deletions(-)
    >
    > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
    > index 28d0048d6171..230ec5e9824f 100644
    > --- a/lib/kunit/test.c
    > +++ b/lib/kunit/test.c
    > @@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
    > if (!log)
    > return;
    >
    > - /* Evaluate length of line to add to log */
    > + frag = list_last_entry(log, struct kunit_log_frag, list);
    > + log_len = strlen(frag->buf);
    > + len_left = sizeof(frag->buf) - log_len - 1;
    > +
    > + /* Attempt to print formatted line to current fragment */
    > va_start(args, fmt);
    > - len = vsnprintf(NULL, 0, fmt, args) + 1;
    > + len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1;
    > va_end(args);
    >
    > + if (len <= len_left)
    > + goto out_newline;
    > +
    > + /* Abandon the truncated result of vsnprintf */
    > + frag->buf[log_len] = '\0';
    > +
    > if (len > sizeof(frag->buf) - 1) {
    > va_start(args, fmt);
    > tmp = kvasprintf(GFP_KERNEL, fmt, args);
    > @@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
    > return;
    > }
    >
    > - frag = list_last_entry(log, struct kunit_log_frag, list);
    > - log_len = strlen(frag->buf);
    > - len_left = sizeof(frag->buf) - log_len - 1;
    > -
    > - if (len > len_left) {
    > - frag = kunit_log_extend(log);
    > - if (!frag)
    > - return;
    > -
    > - len_left = sizeof(frag->buf) - 1;
    > - log_len = 0;
    > - }
    > + frag = kunit_log_extend(log);

    Are we meant to be using the remainder of the last fragment to its
    capacity or just start again with a new fragment and leave some of the
    last fragment empty? I worry we abandoned using the last fragment or
    is that the intention? Let me know if I understand this correctly.

    > + if (!frag)
    > + return;
    >
    > /* Print formatted line to the log */
    > va_start(args, fmt);
    > - vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args);
    > + vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args);
    > va_end(args);
    > -
    > +out_newline:
    > /* Add newline to end of log if not already present. */
    > kunit_log_newline(frag);
    > }
    > --
    > 2.30.2
    >

    \
     
     \ /
      Last update: 2023-08-11 01:54    [W:5.147 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site