Re: [Qemu-devel] [PATCH v6 10/29] libqtest: Topologically sort functions

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Put static functions prior to public ones, in part so that
> improvements to qtest_start() can benefit from the static
> helpers without needing forward references.  Code motion, with
> no semantic change.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/libqtest.c | 263 
> +++++++++++++++++++++++++++----------------------------
>  1 file changed, 131 insertions(+), 132 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 438a22678d..5d16351e24 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -49,7 +49,6 @@ static struct sigaction sigact_old;
>      g_assert_cmpint(ret, !=, -1); \
>  } while (0)
> 
> -static int qtest_query_target_endianness(QTestState *s);
> 
>  static int init_socket(const char *socket_path)
>  {
> @@ -128,6 +127,137 @@ static void setup_sigabrt_handler(void)
>      sigaction(SIGABRT, &sigact, &sigact_old);
>  }
> 
> +static void socket_send(int fd, const char *buf, ssize_t size)
> +{
> +    size_t offset;
> +
> +    if (size < 0) {
> +        size = strlen(buf);
> +    }
> +    offset = 0;
> +    while (offset < size) {
> +        ssize_t len;
> +
> +        len = write(fd, buf + offset, size - offset);
> +        if (len == -1 && errno == EINTR) {
> +            continue;
> +        }
> +
> +        g_assert_no_errno(len);
> +        g_assert_cmpint(len, >, 0);
> +
> +        offset += len;
> +    }
> +}
> +
> +static void socket_sendf(int fd, const char *fmt, va_list ap)
> +{
> +    gchar *str = g_strdup_vprintf(fmt, ap);
> +
> +    socket_send(fd, str, -1);
> +    g_free(str);
> +}
> +
> +static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, 
> ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    socket_sendf(s->fd, fmt, ap);
> +    va_end(ap);
> +}
> +
> +static GString *qtest_recv_line(QTestState *s)
> +{
> +    GString *line;
> +    size_t offset;
> +    char *eol;
> +
> +    while ((eol = strchr(s->rx->str, '\n')) == NULL) {
> +        ssize_t len;
> +        char buffer[1024];
> +
> +        len = read(s->fd, buffer, sizeof(buffer));
> +        if (len == -1 && errno == EINTR) {
> +            continue;
> +        }
> +
> +        if (len == -1 || len == 0) {
> +            fprintf(stderr, "Broken pipe\n");
> +            exit(1);
> +        }
> +
> +        g_string_append_len(s->rx, buffer, len);
> +    }
> +
> +    offset = eol - s->rx->str;
> +    line = g_string_new_len(s->rx->str, offset);
> +    g_string_erase(s->rx, 0, offset + 1);
> +
> +    return line;
> +}
> +
> +static gchar **qtest_rsp(QTestState *s, int expected_args)
> +{
> +    GString *line;
> +    gchar **words;
> +    int i;
> +
> +redo:
> +    line = qtest_recv_line(s);
> +    words = g_strsplit(line->str, " ", 0);
> +    g_string_free(line, TRUE);
> +
> +    if (strcmp(words[0], "IRQ") == 0) {
> +        long irq;
> +        int ret;
> +
> +        g_assert(words[1] != NULL);
> +        g_assert(words[2] != NULL);
> +
> +        ret = qemu_strtol(words[2], NULL, 0, &irq);
> +        g_assert(!ret);
> +        g_assert_cmpint(irq, >=, 0);
> +        g_assert_cmpint(irq, <, MAX_IRQ);
> +
> +        if (strcmp(words[1], "raise") == 0) {
> +            s->irq_level[irq] = true;
> +        } else {
> +            s->irq_level[irq] = false;
> +        }
> +
> +        g_strfreev(words);
> +        goto redo;
> +    }
> +
> +    g_assert(words[0] != NULL);
> +    g_assert_cmpstr(words[0], ==, "OK");
> +
> +    if (expected_args) {
> +        for (i = 0; i < expected_args; i++) {
> +            g_assert(words[i] != NULL);
> +        }
> +    } else {
> +        g_strfreev(words);
> +    }
> +
> +    return words;
> +}
> +
> +static int qtest_query_target_endianness(QTestState *s)
> +{
> +    gchar **args;
> +    int big_endian;
> +
> +    qtest_sendf(s, "endianness\n");
> +    args = qtest_rsp(s, 1);
> +    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> +    big_endian = strcmp(args[1], "big") == 0;
> +    g_strfreev(args);
> +
> +    return big_endian;
> +}
> +
>  static void cleanup_sigabrt_handler(void)
>  {
>      sigaction(SIGABRT, &sigact_old, NULL);
> @@ -252,137 +382,6 @@ void qtest_quit(QTestState *s)
>      g_free(s);
>  }
> 
> -static void socket_send(int fd, const char *buf, ssize_t size)
> -{
> -    size_t offset;
> -
> -    if (size < 0) {
> -        size = strlen(buf);
> -    }
> -    offset = 0;
> -    while (offset < size) {
> -        ssize_t len;
> -
> -        len = write(fd, buf + offset, size - offset);
> -        if (len == -1 && errno == EINTR) {
> -            continue;
> -        }
> -
> -        g_assert_no_errno(len);
> -        g_assert_cmpint(len, >, 0);
> -
> -        offset += len;
> -    }
> -}
> -
> -static void socket_sendf(int fd, const char *fmt, va_list ap)
> -{
> -    gchar *str = g_strdup_vprintf(fmt, ap);
> -
> -    socket_send(fd, str, -1);
> -    g_free(str);
> -}
> -
> -static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, 
> ...)
> -{
> -    va_list ap;
> -
> -    va_start(ap, fmt);
> -    socket_sendf(s->fd, fmt, ap);
> -    va_end(ap);
> -}
> -
> -static GString *qtest_recv_line(QTestState *s)
> -{
> -    GString *line;
> -    size_t offset;
> -    char *eol;
> -
> -    while ((eol = strchr(s->rx->str, '\n')) == NULL) {
> -        ssize_t len;
> -        char buffer[1024];
> -
> -        len = read(s->fd, buffer, sizeof(buffer));
> -        if (len == -1 && errno == EINTR) {
> -            continue;
> -        }
> -
> -        if (len == -1 || len == 0) {
> -            fprintf(stderr, "Broken pipe\n");
> -            exit(1);
> -        }
> -
> -        g_string_append_len(s->rx, buffer, len);
> -    }
> -
> -    offset = eol - s->rx->str;
> -    line = g_string_new_len(s->rx->str, offset);
> -    g_string_erase(s->rx, 0, offset + 1);
> -
> -    return line;
> -}
> -
> -static gchar **qtest_rsp(QTestState *s, int expected_args)
> -{
> -    GString *line;
> -    gchar **words;
> -    int i;
> -
> -redo:
> -    line = qtest_recv_line(s);
> -    words = g_strsplit(line->str, " ", 0);
> -    g_string_free(line, TRUE);
> -
> -    if (strcmp(words[0], "IRQ") == 0) {
> -        long irq;
> -        int ret;
> -
> -        g_assert(words[1] != NULL);
> -        g_assert(words[2] != NULL);
> -
> -        ret = qemu_strtol(words[2], NULL, 0, &irq);
> -        g_assert(!ret);
> -        g_assert_cmpint(irq, >=, 0);
> -        g_assert_cmpint(irq, <, MAX_IRQ);
> -
> -        if (strcmp(words[1], "raise") == 0) {
> -            s->irq_level[irq] = true;
> -        } else {
> -            s->irq_level[irq] = false;
> -        }
> -
> -        g_strfreev(words);
> -        goto redo;
> -    }
> -
> -    g_assert(words[0] != NULL);
> -    g_assert_cmpstr(words[0], ==, "OK");
> -
> -    if (expected_args) {
> -        for (i = 0; i < expected_args; i++) {
> -            g_assert(words[i] != NULL);
> -        }
> -    } else {
> -        g_strfreev(words);
> -    }
> -
> -    return words;
> -}
> -
> -static int qtest_query_target_endianness(QTestState *s)
> -{
> -    gchar **args;
> -    int big_endian;
> -
> -    qtest_sendf(s, "endianness\n");
> -    args = qtest_rsp(s, 1);
> -    g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
> -    big_endian = strcmp(args[1], "big") == 0;
> -    g_strfreev(args);
> -
> -    return big_endian;
> -}
> -
>  typedef struct {
>      JSONMessageParser parser;
>      QDict *response;
> 

That's a *lot* of code motion - just to get rid of one forward
declaration. IMHO getting rid of just one forward declaration does not
justify this code churn. But if we really, really want to get rid of the
forward declaration here, it's maybe better to move the
qtest_init_without_qmp_handshake() and qtest_init() function to the end
of the file instead?

 Thomas