[{"id":1763118,"web_url":"http://patchwork.ozlabs.org/comment/1763118/","msgid":"<172f4b48-f791-2f36-3df5-0a2bd4c859c3@redhat.com>","list_archive_url":null,"date":"2017-09-05T09:22:14","subject":"Re: [Qemu-devel] [PATCH v6 10/29] libqtest: Topologically sort\n\tfunctions","submitter":{"id":66152,"url":"http://patchwork.ozlabs.org/api/people/66152/","name":"Thomas Huth","email":"thuth@redhat.com"},"content":"On 01.09.2017 20:03, Eric Blake wrote:\n> Put static functions prior to public ones, in part so that\n> improvements to qtest_start() can benefit from the static\n> helpers without needing forward references. Code motion, with\n> no semantic change.\n> \n> Signed-off-by: Eric Blake \n> ---\n> tests/libqtest.c | 263 +++++++++++++++++++++++++++----------------------------\n> 1 file changed, 131 insertions(+), 132 deletions(-)\n> \n> diff --git a/tests/libqtest.c b/tests/libqtest.c\n> index 438a22678d..5d16351e24 100644\n> --- a/tests/libqtest.c\n> +++ b/tests/libqtest.c\n> @@ -49,7 +49,6 @@ static struct sigaction sigact_old;\n> g_assert_cmpint(ret, !=, -1); \\\n> } while (0)\n> \n> -static int qtest_query_target_endianness(QTestState *s);\n> \n> static int init_socket(const char *socket_path)\n> {\n> @@ -128,6 +127,137 @@ static void setup_sigabrt_handler(void)\n> sigaction(SIGABRT, &sigact, &sigact_old);\n> }\n> \n> +static void socket_send(int fd, const char *buf, ssize_t size)\n> +{\n> + size_t offset;\n> +\n> + if (size < 0) {\n> + size = strlen(buf);\n> + }\n> + offset = 0;\n> + while (offset < size) {\n> + ssize_t len;\n> +\n> + len = write(fd, buf + offset, size - offset);\n> + if (len == -1 && errno == EINTR) {\n> + continue;\n> + }\n> +\n> + g_assert_no_errno(len);\n> + g_assert_cmpint(len, >, 0);\n> +\n> + offset += len;\n> + }\n> +}\n> +\n> +static void socket_sendf(int fd, const char *fmt, va_list ap)\n> +{\n> + gchar *str = g_strdup_vprintf(fmt, ap);\n> +\n> + socket_send(fd, str, -1);\n> + g_free(str);\n> +}\n> +\n> +static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)\n> +{\n> + va_list ap;\n> +\n> + va_start(ap, fmt);\n> + socket_sendf(s->fd, fmt, ap);\n> + va_end(ap);\n> +}\n> +\n> +static GString *qtest_recv_line(QTestState *s)\n> +{\n> + GString *line;\n> + size_t offset;\n> + char *eol;\n> +\n> + while ((eol = strchr(s->rx->str, '\\n')) == NULL) {\n> + ssize_t len;\n> + char buffer[1024];\n> +\n> + len = read(s->fd, buffer, sizeof(buffer));\n> + if (len == -1 && errno == EINTR) {\n> + continue;\n> + }\n> +\n> + if (len == -1 || len == 0) {\n> + fprintf(stderr, \"Broken pipe\\n\");\n> + exit(1);\n> + }\n> +\n> + g_string_append_len(s->rx, buffer, len);\n> + }\n> +\n> + offset = eol - s->rx->str;\n> + line = g_string_new_len(s->rx->str, offset);\n> + g_string_erase(s->rx, 0, offset + 1);\n> +\n> + return line;\n> +}\n> +\n> +static gchar **qtest_rsp(QTestState *s, int expected_args)\n> +{\n> + GString *line;\n> + gchar **words;\n> + int i;\n> +\n> +redo:\n> + line = qtest_recv_line(s);\n> + words = g_strsplit(line->str, \" \", 0);\n> + g_string_free(line, TRUE);\n> +\n> + if (strcmp(words[0], \"IRQ\") == 0) {\n> + long irq;\n> + int ret;\n> +\n> + g_assert(words[1] != NULL);\n> + g_assert(words[2] != NULL);\n> +\n> + ret = qemu_strtol(words[2], NULL, 0, &irq);\n> + g_assert(!ret);\n> + g_assert_cmpint(irq, >=, 0);\n> + g_assert_cmpint(irq, <, MAX_IRQ);\n> +\n> + if (strcmp(words[1], \"raise\") == 0) {\n> + s->irq_level[irq] = true;\n> + } else {\n> + s->irq_level[irq] = false;\n> + }\n> +\n> + g_strfreev(words);\n> + goto redo;\n> + }\n> +\n> + g_assert(words[0] != NULL);\n> + g_assert_cmpstr(words[0], ==, \"OK\");\n> +\n> + if (expected_args) {\n> + for (i = 0; i < expected_args; i++) {\n> + g_assert(words[i] != NULL);\n> + }\n> + } else {\n> + g_strfreev(words);\n> + }\n> +\n> + return words;\n> +}\n> +\n> +static int qtest_query_target_endianness(QTestState *s)\n> +{\n> + gchar **args;\n> + int big_endian;\n> +\n> + qtest_sendf(s, \"endianness\\n\");\n> + args = qtest_rsp(s, 1);\n> + g_assert(strcmp(args[1], \"big\") == 0 || strcmp(args[1], \"little\") == 0);\n> + big_endian = strcmp(args[1], \"big\") == 0;\n> + g_strfreev(args);\n> +\n> + return big_endian;\n> +}\n> +\n> static void cleanup_sigabrt_handler(void)\n> {\n> sigaction(SIGABRT, &sigact_old, NULL);\n> @@ -252,137 +382,6 @@ void qtest_quit(QTestState *s)\n> g_free(s);\n> }\n> \n> -static void socket_send(int fd, const char *buf, ssize_t size)\n> -{\n> - size_t offset;\n> -\n> - if (size < 0) {\n> - size = strlen(buf);\n> - }\n> - offset = 0;\n> - while (offset < size) {\n> - ssize_t len;\n> -\n> - len = write(fd, buf + offset, size - offset);\n> - if (len == -1 && errno == EINTR) {\n> - continue;\n> - }\n> -\n> - g_assert_no_errno(len);\n> - g_assert_cmpint(len, >, 0);\n> -\n> - offset += len;\n> - }\n> -}\n> -\n> -static void socket_sendf(int fd, const char *fmt, va_list ap)\n> -{\n> - gchar *str = g_strdup_vprintf(fmt, ap);\n> -\n> - socket_send(fd, str, -1);\n> - g_free(str);\n> -}\n> -\n> -static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)\n> -{\n> - va_list ap;\n> -\n> - va_start(ap, fmt);\n> - socket_sendf(s->fd, fmt, ap);\n> - va_end(ap);\n> -}\n> -\n> -static GString *qtest_recv_line(QTestState *s)\n> -{\n> - GString *line;\n> - size_t offset;\n> - char *eol;\n> -\n> - while ((eol = strchr(s->rx->str, '\\n')) == NULL) {\n> - ssize_t len;\n> - char buffer[1024];\n> -\n> - len = read(s->fd, buffer, sizeof(buffer));\n> - if (len == -1 && errno == EINTR) {\n> - continue;\n> - }\n> -\n> - if (len == -1 || len == 0) {\n> - fprintf(stderr, \"Broken pipe\\n\");\n> - exit(1);\n> - }\n> -\n> - g_string_append_len(s->rx, buffer, len);\n> - }\n> -\n> - offset = eol - s->rx->str;\n> - line = g_string_new_len(s->rx->str, offset);\n> - g_string_erase(s->rx, 0, offset + 1);\n> -\n> - return line;\n> -}\n> -\n> -static gchar **qtest_rsp(QTestState *s, int expected_args)\n> -{\n> - GString *line;\n> - gchar **words;\n> - int i;\n> -\n> -redo:\n> - line = qtest_recv_line(s);\n> - words = g_strsplit(line->str, \" \", 0);\n> - g_string_free(line, TRUE);\n> -\n> - if (strcmp(words[0], \"IRQ\") == 0) {\n> - long irq;\n> - int ret;\n> -\n> - g_assert(words[1] != NULL);\n> - g_assert(words[2] != NULL);\n> -\n> - ret = qemu_strtol(words[2], NULL, 0, &irq);\n> - g_assert(!ret);\n> - g_assert_cmpint(irq, >=, 0);\n> - g_assert_cmpint(irq, <, MAX_IRQ);\n> -\n> - if (strcmp(words[1], \"raise\") == 0) {\n> - s->irq_level[irq] = true;\n> - } else {\n> - s->irq_level[irq] = false;\n> - }\n> -\n> - g_strfreev(words);\n> - goto redo;\n> - }\n> -\n> - g_assert(words[0] != NULL);\n> - g_assert_cmpstr(words[0], ==, \"OK\");\n> -\n> - if (expected_args) {\n> - for (i = 0; i < expected_args; i++) {\n> - g_assert(words[i] != NULL);\n> - }\n> - } else {\n> - g_strfreev(words);\n> - }\n> -\n> - return words;\n> -}\n> -\n> -static int qtest_query_target_endianness(QTestState *s)\n> -{\n> - gchar **args;\n> - int big_endian;\n> -\n> - qtest_sendf(s, \"endianness\\n\");\n> - args = qtest_rsp(s, 1);\n> - g_assert(strcmp(args[1], \"big\") == 0 || strcmp(args[1], \"little\") == 0);\n> - big_endian = strcmp(args[1], \"big\") == 0;\n> - g_strfreev(args);\n> -\n> - return big_endian;\n> -}\n> -\n> typedef struct {\n> JSONMessageParser parser;\n> QDict *response;\n> \n\nThat's a *lot* of code motion - just to get rid of one forward\ndeclaration. IMHO getting rid of just one forward declaration does not\njustify this code churn. But if we really, really want to get rid of the\nforward declaration here, it's maybe better to move the\nqtest_init_without_qmp_handshake() and qtest_init() function to the end\nof the file instead?\n\n Thomas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=thuth@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmh8h5sfHz9sNr\n\tfor ;\n\tTue, 5 Sep 2017 19:23:16 +1000 (AEST)","from localhost ([::1]:57661 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpA4g-0000zW-PR\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 05:23:14 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:40970)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpA3r-0000vD-A1\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 05:22:28 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpA3m-0002Zp-2t\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 05:22:23 -0400","from mx1.redhat.com ([209.132.183.28]:42242)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpA3l-0002ZT-Q2\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 05:22:17 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id D8C084620F\n\tfor ; Tue, 5 Sep 2017 09:22:16 +0000 (UTC)","from [10.36.116.114] (ovpn-116-114.ams2.redhat.com [10.36.116.114])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 986228513C;\n\tTue, 5 Sep 2017 09:22:15 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com D8C084620F","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170901180340.30009-1-eblake@redhat.com>\n\t<20170901180340.30009-11-eblake@redhat.com>","From":"Thomas Huth ","Message-ID":"<172f4b48-f791-2f36-3df5-0a2bd4c859c3@redhat.com>","Date":"Tue, 5 Sep 2017 11:22:14 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170901180340.30009-11-eblake@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tTue, 05 Sep 2017 09:22:17 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v6 10/29] libqtest: Topologically sort\n\tfunctions","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"pbonzini@redhat.com, armbru@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1763167,"web_url":"http://patchwork.ozlabs.org/comment/1763167/","msgid":"<87vakxxy3v.fsf@dusky.pond.sub.org>","list_archive_url":null,"date":"2017-09-05T10:01:56","subject":"Re: [Qemu-devel] [PATCH v6 10/29] libqtest: Topologically sort\n\tfunctions","submitter":{"id":2645,"url":"http://patchwork.ozlabs.org/api/people/2645/","name":"Markus Armbruster","email":"armbru@redhat.com"},"content":"Thomas Huth writes:\n\n> On 01.09.2017 20:03, Eric Blake wrote:\n>> Put static functions prior to public ones, in part so that\n>> improvements to qtest_start() can benefit from the static\n>> helpers without needing forward references. Code motion, with\n>> no semantic change.\n>> \n>> Signed-off-by: Eric Blake \n>> ---\n>> tests/libqtest.c | 263 +++++++++++++++++++++++++++----------------------------\n>> 1 file changed, 131 insertions(+), 132 deletions(-)\n[...]\n> That's a *lot* of code motion - just to get rid of one forward\n> declaration. IMHO getting rid of just one forward declaration does not\n> justify this code churn. But if we really, really want to get rid of the\n> forward declaration here, it's maybe better to move the\n> qtest_init_without_qmp_handshake() and qtest_init() function to the end\n> of the file instead?\n\nI've never understood why people hate forward declarations so much.\nJust because the compiler needs to see a declaration before any use\ndoesn't mean a human reader profits from seeing the definition before\nany use. Sometimes the code is easier to understand in top-down order.\n\nFor code I maintain, I evaluate code motion proposals strictly on\nreadability merits, with complete disregard for the number of forward\ndeclarations added or deleted.\n\nI'm very much not maintaining this code, though :)","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=armbru@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xmj267344z9s9Y\n\tfor ;\n\tTue, 5 Sep 2017 20:02:37 +1000 (AEST)","from localhost ([::1]:57862 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpAgl-00006M-2c\n\tfor incoming@patchwork.ozlabs.org; Tue, 05 Sep 2017 06:02:35 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:37990)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpAgN-00005d-5h\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 06:02:15 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpAgB-00086t-7C\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 06:02:06 -0400","from mx1.redhat.com ([209.132.183.28]:34348)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpAgB-00085v-2A\n\tfor qemu-devel@nongnu.org; Tue, 05 Sep 2017 06:01:59 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 20AF0C04BD2C\n\tfor ; Tue, 5 Sep 2017 10:01:58 +0000 (UTC)","from blackfin.pond.sub.org (ovpn-116-75.ams2.redhat.com\n\t[10.36.116.75])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id E23CD82400;\n\tTue, 5 Sep 2017 10:01:57 +0000 (UTC)","by blackfin.pond.sub.org (Postfix, from userid 1000)\n\tid 672201138645; Tue, 5 Sep 2017 12:01:56 +0200 (CEST)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 20AF0C04BD2C","From":"Markus Armbruster ","To":"Thomas Huth ","References":"<20170901180340.30009-1-eblake@redhat.com>\n\t<20170901180340.30009-11-eblake@redhat.com>\n\t<172f4b48-f791-2f36-3df5-0a2bd4c859c3@redhat.com>","Date":"Tue, 05 Sep 2017 12:01:56 +0200","In-Reply-To":"<172f4b48-f791-2f36-3df5-0a2bd4c859c3@redhat.com> (Thomas Huth's\n\tmessage of \"Tue, 5 Sep 2017 11:22:14 +0200\")","Message-ID":"<87vakxxy3v.fsf@dusky.pond.sub.org>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tTue, 05 Sep 2017 10:01:58 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v6 10/29] libqtest: Topologically sort\n\tfunctions","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"pbonzini@redhat.com, qemu-devel@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]