From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhNhC-00085Y-7u for qemu-devel@nongnu.org; Tue, 06 Sep 2016 17:14:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhNh6-0007sC-DL for qemu-devel@nongnu.org; Tue, 06 Sep 2016 17:14:17 -0400 Received: from 19.mo1.mail-out.ovh.net ([178.32.97.206]:52778) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhNh6-0007qt-5E for qemu-devel@nongnu.org; Tue, 06 Sep 2016 17:14:12 -0400 Received: from player726.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo1.mail-out.ovh.net (Postfix) with ESMTP id CFEACFF890F for ; Tue, 6 Sep 2016 23:14:09 +0200 (CEST) Date: Tue, 6 Sep 2016 23:13:59 +0200 From: Greg Kurz Message-ID: <20160906231359.35c17ecb@bahia> In-Reply-To: <96a46d89-5066-3c8e-b755-b6df4dd84db6@redhat.com> References: <1473167877-2545-1-git-send-email-lvivier@redhat.com> <1473167877-2545-2-git-send-email-lvivier@redhat.com> <20160906175529.12869cc3@bahia> <96a46d89-5066-3c8e-b755-b6df4dd84db6@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 1/3] qtest: replace strtoXX() by qemu_strtoXX() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: david@gibson.dropbear.id.au, thuth@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org On Tue, 6 Sep 2016 20:17:00 +0200 Laurent Vivier wrote: > On 06/09/2016 17:55, Greg Kurz wrote: > > On Tue, 6 Sep 2016 15:17:55 +0200 > > Laurent Vivier wrote: > > > >> Signed-off-by: Laurent Vivier > >> --- > > > > The patch also adds error checking and assertions. Maybe worth to be mentioned > > in the changelog... > > In case of a new version of the patch, I will... > > > > >> v4: > >> - add this patch in the series to change all strtoXX() in qtest.c > >> > >> qtest.c | 49 ++++++++++++++++++++++++++----------------------- > >> 1 file changed, 26 insertions(+), 23 deletions(-) > >> > >> diff --git a/qtest.c b/qtest.c > >> index da4826c..4c94708 100644 > >> --- a/qtest.c > >> +++ b/qtest.c > >> @@ -27,6 +27,7 @@ > >> #include "qemu/config-file.h" > >> #include "qemu/option.h" > >> #include "qemu/error-report.h" > >> +#include "qemu/cutils.h" > >> > >> #define MAX_IRQ 256 > >> > >> @@ -324,12 +325,13 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> } else if (strcmp(words[0], "outb") == 0 || > >> strcmp(words[0], "outw") == 0 || > >> strcmp(words[0], "outl") == 0) { > >> - uint16_t addr; > >> - uint32_t value; > >> + unsigned long addr; > >> + unsigned long value; > >> > >> g_assert(words[1] && words[2]); > >> - addr = strtoul(words[1], NULL, 0); > >> - value = strtoul(words[2], NULL, 0); > >> + g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0); > >> + g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0); > >> + g_assert(addr <= 0xffff); > >> > >> if (words[0][3] == 'b') { > >> cpu_outb(addr, value); > >> @@ -343,11 +345,12 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> } else if (strcmp(words[0], "inb") == 0 || > >> strcmp(words[0], "inw") == 0 || > >> strcmp(words[0], "inl") == 0) { > >> - uint16_t addr; > >> + unsigned long addr; > >> uint32_t value = -1U; > >> > >> g_assert(words[1]); > >> - addr = strtoul(words[1], NULL, 0); > >> + g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0); > >> + g_assert(addr <= 0xffff); > >> > >> if (words[0][2] == 'b') { > >> value = cpu_inb(addr); > >> @@ -366,8 +369,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> uint64_t value; > >> > >> g_assert(words[1] && words[2]); > >> - addr = strtoull(words[1], NULL, 0); > >> - value = strtoull(words[2], NULL, 0); > >> + g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0); > >> + g_assert(qemu_strtoull(words[2], NULL, 0, &value) == 0); > >> > >> if (words[0][5] == 'b') { > >> uint8_t data = value; > >> @@ -395,7 +398,7 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> uint64_t value = UINT64_C(-1); > >> > >> g_assert(words[1]); > >> - addr = strtoull(words[1], NULL, 0); > >> + g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0); > >> > >> if (words[0][4] == 'b') { > >> uint8_t data; > >> @@ -421,8 +424,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> char *enc; > >> > >> g_assert(words[1] && words[2]); > >> - addr = strtoull(words[1], NULL, 0); > >> - len = strtoull(words[2], NULL, 0); > >> + g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0); > >> + g_assert(qemu_strtoull(words[2], NULL, 0, &len) == 0); > >> > >> data = g_malloc(len); > >> cpu_physical_memory_read(addr, data, len); > >> @@ -443,8 +446,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> gchar *b64_data; > >> > >> g_assert(words[1] && words[2]); > >> - addr = strtoull(words[1], NULL, 0); > >> - len = strtoull(words[2], NULL, 0); > >> + g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0); > >> + g_assert(qemu_strtoull(words[2], NULL, 0, &len) == 0); > >> > >> data = g_malloc(len); > >> cpu_physical_memory_read(addr, data, len); > >> @@ -460,8 +463,8 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> size_t data_len; > >> > >> g_assert(words[1] && words[2] && words[3]); > >> - addr = strtoull(words[1], NULL, 0); > >> - len = strtoull(words[2], NULL, 0); > >> + g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0); > >> + g_assert(qemu_strtoull(words[2], NULL, 0, &len) == 0); > >> > >> data_len = strlen(words[3]); > >> if (data_len < 3) { > >> @@ -486,12 +489,12 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > >> } else if (strcmp(words[0], "memset") == 0) { > >> uint64_t addr, len; > >> uint8_t *data; > >> - uint8_t pattern; > >> + unsigned long pattern; > >> > >> g_assert(words[1] && words[2] && words[3]); > >> - addr = strtoull(words[1], NULL, 0); > >> - len = strtoull(words[2], NULL, 0); > >> - pattern = strtoull(words[3], NULL, 0); > >> + g_assert(qemu_strtoull(words[1], NULL, 0, &addr) == 0); > >> + g_assert(qemu_strtoull(words[2], NULL, 0, &len) == 0); > >> + g_assert(qemu_strtoul(words[3], NULL, 0, &pattern) == 0); > > > > And: > > > > g_assert(pattern <= 0xff) > > I think pattern > 0xff is valid as memset() takes an "int" and only uses > the byte value (for instance to use -1 to fill memory with 0xff). It > can't do bad things... > Of course... sorry for the noise :) > In the previous case ("g_assert(addr <= 0xffff)"), if addr > 0xffff, > cpu_out/in can write at a bad address. We could just ignore the upper > part of the word, but to debug test case I think it's good to have an > assert in this case. > It makes sense indeed. > Thanks, > Laurent Cheers. -- Greg