Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230331004844.1592789-1-irogers@google.com> <20230331004844.1592789-2-irogers@google.com>
In-Reply-To: <20230331004844.1592789-2-irogers@google.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Fri, 31 Mar 2023 17:19:06 -0700
Message-ID: <CAM9d7cjXGO-xd1tKDUHUrYtG=94aXnubRrUNe6EmyuGwo3=okA@mail.gmail.com>
Subject: Re: [PATCH v1 1/4] tools api: Add io__getline
To:     Ian Rogers <irogers@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org, llvm@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Ian,

On Thu, Mar 30, 2023 at 5:49 PM Ian Rogers <irogers@google.com> wrote:
>
> Reads a line to allocated memory up to a newline following the getline
> API.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/api/io.h        | 40 +++++++++++++++++++++++++++++++++++++++
>  tools/perf/tests/api-io.c | 36 +++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
>
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index 777c20f6b604..d874e8fa8b07 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -7,7 +7,9 @@
>  #ifndef __API_IO__
>  #define __API_IO__
>
> +#include <errno.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <unistd.h>
>
>  struct io {
> @@ -112,4 +114,42 @@ static inline int io__get_dec(struct io *io, __u64 *dec)
>         }
>  }
>
> +/* Read up to and including the first newline following the pattern of getline. */
> +static inline ssize_t io__getline(char **line_out, size_t *line_len_out, struct io *io)
> +{
> +       char buf[128];
> +       int buf_pos = 0;
> +       char *line = NULL;
> +       size_t line_len = 0;
> +       int ch = 0;
> +
> +       /* TODO: reuse previously allocated memory. */
> +       free(*line_out);
> +       while (ch != '\n') {
> +               ch = io__get_char(io);
> +
> +               if (ch < 0)
> +                       break;
> +
> +               if (buf_pos == sizeof(buf)) {
> +                       line = realloc(line, line_len + sizeof(buf));
> +                       if (!line)
> +                               return -ENOMEM;

We can use a temp variable for the failure case, and also free
the original line before returning the error.


> +                       memcpy(&line[line_len], buf, sizeof(buf));
> +                       line_len += sizeof(buf);
> +                       buf_pos = 0;
> +               }
> +               buf[buf_pos++] = (char)ch;
> +       }
> +       line = realloc(line, line_len + buf_pos + 1);
> +       if (!line)
> +               return -ENOMEM;

Ditto.

Thanks,
Namhyung


> +       memcpy(&line[line_len], buf, buf_pos);
> +       line[line_len + buf_pos] = '\0';
> +       line_len += buf_pos;
> +       *line_out = line;
> +       *line_len_out = line_len;
> +       return line_len;
> +}
> +
>  #endif /* __API_IO__ */
> diff --git a/tools/perf/tests/api-io.c b/tools/perf/tests/api-io.c
> index e91cf2c127f1..0ff39cdfcb01 100644
> --- a/tools/perf/tests/api-io.c
> +++ b/tools/perf/tests/api-io.c
> @@ -289,6 +289,40 @@ static int test_get_dec(void)
>         return ret;
>  }
>
> +static int test_get_line(void)
> +{
> +       char path[PATH_MAX];
> +       struct io io;
> +       char test_string[1024];
> +       char *line = NULL;
> +       size_t i, line_len = 0;
> +       size_t buf_size = 128;
> +       int ret = 0;
> +
> +       for (i = 0; i < 512; i++)
> +               test_string[i] = 'a';
> +       test_string[512] = '\n';
> +       for (i = 513; i < 1023; i++)
> +               test_string[i] = 'b';
> +       test_string[1023] = '\0';
> +
> +       if (setup_test(path, test_string, buf_size, &io))
> +               return -1;
> +
> +       EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 513);
> +       EXPECT_EQUAL((int)strlen(line), 513);
> +       for (i = 0; i < 512; i++)
> +               EXPECT_EQUAL(line[i], 'a');
> +       EXPECT_EQUAL(line[512], '\n');
> +       EXPECT_EQUAL((int)io__getline(&line, &line_len, &io), 510);
> +       for (i = 0; i < 510; i++)
> +               EXPECT_EQUAL(line[i], 'b');
> +
> +       free(line);
> +       cleanup_test(path, &io);
> +       return ret;
> +}
> +
>  static int test__api_io(struct test_suite *test __maybe_unused,
>                         int subtest __maybe_unused)
>  {
> @@ -300,6 +334,8 @@ static int test__api_io(struct test_suite *test __maybe_unused,
>                 ret = TEST_FAIL;
>         if (test_get_dec())
>                 ret = TEST_FAIL;
> +       if (test_get_line())
> +               ret = TEST_FAIL;
>         return ret;
>  }
>
> --
> 2.40.0.348.gf938b09366-goog
>