[Qemu-devel] [PATCH for-2.11 0/2] Improvements for the pxe tester

[Qemu-devel] [PATCH for-2.11 0/2] Improvements for the pxe tester

[Thomas Huth]

The first patch improves the buffer handling in the pxe tester a
little bit by allocating a separate buffer on the heap for each
architecture. This also gets rid of the huge pre-initialized
array in the tester, shrinking the size of the executable by
half of a megabyte!
The second patch adds s390x support to the pxe tester. Starting
with QEMU 2.10, the guest firmware on s390x can now net-boot via
TFTP, too, so we can automatically test this code in the pxe tester.

Thomas Huth (2):
  tests/boot-sector: Do not overwrite the x86 buffer on other
    architectures
  tests/pxe: Check virtio-net-ccw on s390x

 tests/Makefile.include |  1 +
 tests/boot-sector.c    | 57 ++++++++++++++++++++++++++++++++++++++------------
 tests/pxe-test.c       |  7 +++++++
 3 files changed, 52 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures

[Thomas Huth]

Re-using the boot_sector code buffer from x86 for other architectures
is not very nice, especially if we add more architectures later. It's
also ugly that the test uses a huge pre-initialized array - the size
of the executable is very huge due to this array. So let's use a
separate buffer for each architecture instead, allocated from the heap,
so that we really just use the memory that we need.

Suggested-by: Michael Tsirkin <mst@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/boot-sector.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/tests/boot-sector.c b/tests/boot-sector.c
index e3880f4..4ea1373 100644
--- a/tests/boot-sector.c
+++ b/tests/boot-sector.c
@@ -21,13 +21,12 @@
 #define SIGNATURE 0xdead
 #define SIGNATURE_OFFSET 0x10
 #define BOOT_SECTOR_ADDRESS 0x7c00
+#define SIGNATURE_ADDR (BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET)
 
-/* Boot sector code: write SIGNATURE into memory,
+/* x86 boot sector code: write SIGNATURE into memory,
  * then halt.
- * Q35 machine requires a minimum 0x7e000 bytes disk.
- * (bug or feature?)
  */
-static uint8_t boot_sector[0x7e000] = {
+static uint8_t x86_boot_sector[512] = {
     /* The first sector will be placed at RAM address 00007C00, and
      * the BIOS transfers control to 00007C00
      */
@@ -50,8 +49,8 @@ static uint8_t boot_sector[0x7e000] = {
     [0x07] = HIGH(SIGNATURE),
     /* 7c08:  mov %ax,0x7c10 */
     [0x08] = 0xa3,
-    [0x09] = LOW(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
-    [0x0a] = HIGH(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
+    [0x09] = LOW(SIGNATURE_ADDR),
+    [0x0a] = HIGH(SIGNATURE_ADDR),
 
     /* 7c0b cli */
     [0x0b] = 0xfa,
@@ -72,7 +71,9 @@ static uint8_t boot_sector[0x7e000] = {
 int boot_sector_init(char *fname)
 {
     int fd, ret;
-    size_t len = sizeof boot_sector;
+    size_t len;
+    char *boot_code;
+    const char *arch = qtest_get_arch();
 
     fd = mkstemp(fname);
     if (fd < 0) {
@@ -80,16 +81,26 @@ int boot_sector_init(char *fname)
         return 1;
     }
 
-    /* For Open Firmware based system, we can use a Forth script instead */
-    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
-        len = sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x c!\n",
-                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
-                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 1);
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
+        /* Q35 requires a minimum 0x7e000 bytes disk (bug or feature?) */
+        len = 0x7e000;
+        boot_code = g_malloc(len);
+        memcpy(boot_code, x86_boot_sector, sizeof x86_boot_sector);
+    } else if (g_str_equal(arch, "ppc64")) {
+        /* For Open Firmware based system, use a Forth script */
+        boot_code = g_strdup_printf("\\ Bootscript\n%x %x c! %x %x c!\n",
+                                    LOW(SIGNATURE), SIGNATURE_ADDR,
+                                    HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
+        len = strlen(boot_code);
+    } else {
+        g_assert_not_reached();
     }
 
-    ret = write(fd, boot_sector, len);
+    ret = write(fd, boot_code, len);
     close(fd);
 
+    g_free(boot_code);
+
     if (ret != len) {
         fprintf(stderr, "Could not write \"%s\"", fname);
         return 1;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] tests/pxe: Check virtio-net-ccw on s390x

[Thomas Huth]

Now that we've got a firmware that can do TFTP booting on s390x (i.e.
the pc-bios/s390-netboot.img), we can enable the PXE tester for this
architecture, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |  1 +
 tests/boot-sector.c    | 20 ++++++++++++++++++++
 tests/pxe-test.c       |  7 +++++++
 3 files changed, 28 insertions(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index eb4895f..639371e4 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -337,6 +337,7 @@ check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
 check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
+check-qtest-s390x-y += tests/pxe-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
diff --git a/tests/boot-sector.c b/tests/boot-sector.c
index 4ea1373..bc5837a 100644
--- a/tests/boot-sector.c
+++ b/tests/boot-sector.c
@@ -67,6 +67,21 @@ static uint8_t x86_boot_sector[512] = {
     [0x1FF] = 0xAA,
 };
 
+/* For s390x, use a mini "kernel" with the appropriate signature */
+static const uint8_t s390x_psw[] = {
+    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+};
+static const uint8_t s390x_code[] = {
+    0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */
+    0x00, 0x00, 0x00, 0x00,
+    'S', '3', '9', '0',
+    'E', 'P', 0x00, 0x01,
+    0xa7, 0x38, HIGH(SIGNATURE_ADDR), LOW(SIGNATURE_ADDR), /* lhi r3,0x7c10 */
+    0xa7, 0x48, LOW(SIGNATURE), HIGH(SIGNATURE),           /* lhi r4,0xadde */
+    0x40, 0x40, 0x30, 0x00,                                /* sth r4,0(r3) */
+    0xa7, 0xf4, 0xff, 0xfa                                 /* j 0x10010 */
+};
+
 /* Create boot disk file.  */
 int boot_sector_init(char *fname)
 {
@@ -92,6 +107,11 @@ int boot_sector_init(char *fname)
                                     LOW(SIGNATURE), SIGNATURE_ADDR,
                                     HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
         len = strlen(boot_code);
+    } else if (g_str_equal(arch, "s390x")) {
+        len = 0x10000 + sizeof s390x_code;
+        boot_code = g_malloc(len);
+        memcpy(boot_code, s390x_psw, sizeof s390x_psw);
+        memcpy(&boot_code[0x10000], s390x_code, sizeof s390x_code);
     } else {
         g_assert_not_reached();
     }
diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index cf6e225..0d70afc 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -51,6 +51,11 @@ static void test_pxe_spapr_vlan(void)
     test_pxe_one("-device spapr-vlan,netdev=" NETNAME, true);
 }
 
+static void test_pxe_virtio_ccw(void)
+{
+    test_pxe_one("-device virtio-net-ccw,bootindex=1,netdev=" NETNAME, false);
+}
+
 int main(int argc, char *argv[])
 {
     int ret;
@@ -68,6 +73,8 @@ int main(int argc, char *argv[])
     } else if (strcmp(arch, "ppc64") == 0) {
         qtest_add_func("pxe/virtio", test_pxe_virtio_pci);
         qtest_add_func("pxe/spapr-vlan", test_pxe_spapr_vlan);
+    } else if (g_str_equal(arch, "s390x")) {
+        qtest_add_func("pxe/virtio-ccw", test_pxe_virtio_ccw);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures

[Cornelia Huck]

On Wed,  9 Aug 2017 06:59:37 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Re-using the boot_sector code buffer from x86 for other architectures
> is not very nice, especially if we add more architectures later. It's
> also ugly that the test uses a huge pre-initialized array - the size
> of the executable is very huge due to this array. So let's use a
> separate buffer for each architecture instead, allocated from the heap,
> so that we really just use the memory that we need.
> 
> Suggested-by: Michael Tsirkin <mst@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/boot-sector.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index e3880f4..4ea1373 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -21,13 +21,12 @@
>  #define SIGNATURE 0xdead
>  #define SIGNATURE_OFFSET 0x10
>  #define BOOT_SECTOR_ADDRESS 0x7c00
> +#define SIGNATURE_ADDR (BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET)

Do you want to use this new #define in boot_sector_test() as well?

>  
> -/* Boot sector code: write SIGNATURE into memory,
> +/* x86 boot sector code: write SIGNATURE into memory,
>   * then halt.
> - * Q35 machine requires a minimum 0x7e000 bytes disk.
> - * (bug or feature?)
>   */
> -static uint8_t boot_sector[0x7e000] = {
> +static uint8_t x86_boot_sector[512] = {
>      /* The first sector will be placed at RAM address 00007C00, and
>       * the BIOS transfers control to 00007C00
>       */
> @@ -50,8 +49,8 @@ static uint8_t boot_sector[0x7e000] = {
>      [0x07] = HIGH(SIGNATURE),
>      /* 7c08:  mov %ax,0x7c10 */
>      [0x08] = 0xa3,
> -    [0x09] = LOW(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
> -    [0x0a] = HIGH(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
> +    [0x09] = LOW(SIGNATURE_ADDR),
> +    [0x0a] = HIGH(SIGNATURE_ADDR),
>  
>      /* 7c0b cli */
>      [0x0b] = 0xfa,
> @@ -72,7 +71,9 @@ static uint8_t boot_sector[0x7e000] = {
>  int boot_sector_init(char *fname)
>  {
>      int fd, ret;
> -    size_t len = sizeof boot_sector;
> +    size_t len;
> +    char *boot_code;
> +    const char *arch = qtest_get_arch();
>  
>      fd = mkstemp(fname);
>      if (fd < 0) {
> @@ -80,16 +81,26 @@ int boot_sector_init(char *fname)
>          return 1;
>      }
>  
> -    /* For Open Firmware based system, we can use a Forth script instead */
> -    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
> -        len = sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x c!\n",
> -                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
> -                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 1);
> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> +        /* Q35 requires a minimum 0x7e000 bytes disk (bug or feature?) */
> +        len = 0x7e000;

Use the maximum of (0x7e000, sizeof(x86_boot_sector))? (Not that it is
likely that the boot sector will ever grow, but I think it is cleaner.)

> +        boot_code = g_malloc(len);

Would g_malloc_0() be better?

> +        memcpy(boot_code, x86_boot_sector, sizeof x86_boot_sector);

sizeof(x86_boot_sector)?

> +    } else if (g_str_equal(arch, "ppc64")) {
> +        /* For Open Firmware based system, use a Forth script */
> +        boot_code = g_strdup_printf("\\ Bootscript\n%x %x c! %x %x c!\n",
> +                                    LOW(SIGNATURE), SIGNATURE_ADDR,
> +                                    HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
> +        len = strlen(boot_code);
> +    } else {
> +        g_assert_not_reached();
>      }
>  
> -    ret = write(fd, boot_sector, len);
> +    ret = write(fd, boot_code, len);
>      close(fd);
>  
> +    g_free(boot_code);
> +
>      if (ret != len) {
>          fprintf(stderr, "Could not write \"%s\"", fname);
>          return 1;

This makes the code much nicer :)

Re: [Qemu-devel] [PATCH 2/2] tests/pxe: Check virtio-net-ccw on s390x

[Cornelia Huck]

On Wed,  9 Aug 2017 06:59:38 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Now that we've got a firmware that can do TFTP booting on s390x (i.e.
> the pc-bios/s390-netboot.img), we can enable the PXE tester for this
> architecture, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |  1 +
>  tests/boot-sector.c    | 20 ++++++++++++++++++++
>  tests/pxe-test.c       |  7 +++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index eb4895f..639371e4 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -337,6 +337,7 @@ check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
>  
>  check-qtest-s390x-y = tests/boot-serial-test$(EXESUF)
> +check-qtest-s390x-y += tests/pxe-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index 4ea1373..bc5837a 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -67,6 +67,21 @@ static uint8_t x86_boot_sector[512] = {
>      [0x1FF] = 0xAA,
>  };
>  
> +/* For s390x, use a mini "kernel" with the appropriate signature */
> +static const uint8_t s390x_psw[] = {
> +    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
> +};
> +static const uint8_t s390x_code[] = {
> +    0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */
> +    0x00, 0x00, 0x00, 0x00,
> +    'S', '3', '9', '0',
> +    'E', 'P', 0x00, 0x01,
> +    0xa7, 0x38, HIGH(SIGNATURE_ADDR), LOW(SIGNATURE_ADDR), /* lhi r3,0x7c10 */
> +    0xa7, 0x48, LOW(SIGNATURE), HIGH(SIGNATURE),           /* lhi r4,0xadde */
> +    0x40, 0x40, 0x30, 0x00,                                /* sth r4,0(r3) */
> +    0xa7, 0xf4, 0xff, 0xfa                                 /* j 0x10010 */
> +};
> +
>  /* Create boot disk file.  */
>  int boot_sector_init(char *fname)
>  {
> @@ -92,6 +107,11 @@ int boot_sector_init(char *fname)
>                                      LOW(SIGNATURE), SIGNATURE_ADDR,
>                                      HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
>          len = strlen(boot_code);
> +    } else if (g_str_equal(arch, "s390x")) {
> +        len = 0x10000 + sizeof s390x_code;
> +        boot_code = g_malloc(len);

g_malloc_0()?

> +        memcpy(boot_code, s390x_psw, sizeof s390x_psw);
> +        memcpy(&boot_code[0x10000], s390x_code, sizeof s390x_code);

I'd prefer the sizeof() notation.

>      } else {
>          g_assert_not_reached();
>      }
> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> index cf6e225..0d70afc 100644
> --- a/tests/pxe-test.c
> +++ b/tests/pxe-test.c
> @@ -51,6 +51,11 @@ static void test_pxe_spapr_vlan(void)
>      test_pxe_one("-device spapr-vlan,netdev=" NETNAME, true);
>  }
>  
> +static void test_pxe_virtio_ccw(void)
> +{
> +    test_pxe_one("-device virtio-net-ccw,bootindex=1,netdev=" NETNAME, false);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      int ret;
> @@ -68,6 +73,8 @@ int main(int argc, char *argv[])
>      } else if (strcmp(arch, "ppc64") == 0) {
>          qtest_add_func("pxe/virtio", test_pxe_virtio_pci);
>          qtest_add_func("pxe/spapr-vlan", test_pxe_spapr_vlan);
> +    } else if (g_str_equal(arch, "s390x")) {
> +        qtest_add_func("pxe/virtio-ccw", test_pxe_virtio_ccw);
>      }
>      ret = g_test_run();
>      boot_sector_cleanup(disk);

Else, looks good.

Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures

[Thomas Huth]

On 09.08.2017 11:05, Cornelia Huck wrote:
> On Wed,  9 Aug 2017 06:59:37 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> Re-using the boot_sector code buffer from x86 for other architectures
>> is not very nice, especially if we add more architectures later. It's
>> also ugly that the test uses a huge pre-initialized array - the size
>> of the executable is very huge due to this array. So let's use a
>> separate buffer for each architecture instead, allocated from the heap,
>> so that we really just use the memory that we need.
>>
>> Suggested-by: Michael Tsirkin <mst@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/boot-sector.c | 37 ++++++++++++++++++++++++-------------
>>  1 file changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
>> index e3880f4..4ea1373 100644
>> --- a/tests/boot-sector.c
>> +++ b/tests/boot-sector.c
>> @@ -21,13 +21,12 @@
>>  #define SIGNATURE 0xdead
>>  #define SIGNATURE_OFFSET 0x10
>>  #define BOOT_SECTOR_ADDRESS 0x7c00
>> +#define SIGNATURE_ADDR (BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET)
> 
> Do you want to use this new #define in boot_sector_test() as well?

Yes, sounds like a good idea.

>>  
>> -/* Boot sector code: write SIGNATURE into memory,
>> +/* x86 boot sector code: write SIGNATURE into memory,
>>   * then halt.
>> - * Q35 machine requires a minimum 0x7e000 bytes disk.
>> - * (bug or feature?)
>>   */
>> -static uint8_t boot_sector[0x7e000] = {
>> +static uint8_t x86_boot_sector[512] = {
>>      /* The first sector will be placed at RAM address 00007C00, and
>>       * the BIOS transfers control to 00007C00
>>       */
>> @@ -50,8 +49,8 @@ static uint8_t boot_sector[0x7e000] = {
>>      [0x07] = HIGH(SIGNATURE),
>>      /* 7c08:  mov %ax,0x7c10 */
>>      [0x08] = 0xa3,
>> -    [0x09] = LOW(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
>> -    [0x0a] = HIGH(BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET),
>> +    [0x09] = LOW(SIGNATURE_ADDR),
>> +    [0x0a] = HIGH(SIGNATURE_ADDR),
>>  
>>      /* 7c0b cli */
>>      [0x0b] = 0xfa,
>> @@ -72,7 +71,9 @@ static uint8_t boot_sector[0x7e000] = {
>>  int boot_sector_init(char *fname)
>>  {
>>      int fd, ret;
>> -    size_t len = sizeof boot_sector;
>> +    size_t len;
>> +    char *boot_code;
>> +    const char *arch = qtest_get_arch();
>>  
>>      fd = mkstemp(fname);
>>      if (fd < 0) {
>> @@ -80,16 +81,26 @@ int boot_sector_init(char *fname)
>>          return 1;
>>      }
>>  
>> -    /* For Open Firmware based system, we can use a Forth script instead */
>> -    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
>> -        len = sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x c!\n",
>> -                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
>> -                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 1);
>> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
>> +        /* Q35 requires a minimum 0x7e000 bytes disk (bug or feature?) */
>> +        len = 0x7e000;
> 
> Use the maximum of (0x7e000, sizeof(x86_boot_sector))? (Not that it is
> likely that the boot sector will ever grow, but I think it is cleaner.)

Sounds like a little bit of too much sanity checking for me, but ok, I
can add it.

>> +        boot_code = g_malloc(len);
> 
> Would g_malloc_0() be better?

Good idea, the test is likely more predictable if we don't have random
data in the file here (it should not really matter, but let's better be
safe than sorry).

>> +        memcpy(boot_code, x86_boot_sector, sizeof x86_boot_sector);
> 
> sizeof(x86_boot_sector)?

The original code uses sizeof without parenthesis, so I think we should
stay with that coding style.

>> +    } else if (g_str_equal(arch, "ppc64")) {
>> +        /* For Open Firmware based system, use a Forth script */
>> +        boot_code = g_strdup_printf("\\ Bootscript\n%x %x c! %x %x c!\n",
>> +                                    LOW(SIGNATURE), SIGNATURE_ADDR,
>> +                                    HIGH(SIGNATURE), SIGNATURE_ADDR + 1);
>> +        len = strlen(boot_code);
>> +    } else {
>> +        g_assert_not_reached();
>>      }
>>  
>> -    ret = write(fd, boot_sector, len);
>> +    ret = write(fd, boot_code, len);
>>      close(fd);
>>  
>> +    g_free(boot_code);
>> +
>>      if (ret != len) {
>>          fprintf(stderr, "Could not write \"%s\"", fname);
>>          return 1;
> 
> This makes the code much nicer :)

Thanks for the review!

I'll wait for some more feedback, then send a v2...

 Thomas

Re: [Qemu-devel] [PATCH 1/2] tests/boot-sector: Do not overwrite the x86 buffer on other architectures

[Cornelia Huck]

On Wed, 9 Aug 2017 11:18:33 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 09.08.2017 11:05, Cornelia Huck wrote:
> > On Wed,  9 Aug 2017 06:59:37 +0200
> > Thomas Huth <thuth@redhat.com> wrote:

> >> @@ -80,16 +81,26 @@ int boot_sector_init(char *fname)
> >>          return 1;
> >>      }
> >>  
> >> -    /* For Open Firmware based system, we can use a Forth script instead */
> >> -    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
> >> -        len = sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x c!\n",
> >> -                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
> >> -                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 1);
> >> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
> >> +        /* Q35 requires a minimum 0x7e000 bytes disk (bug or feature?) */
> >> +        len = 0x7e000;  
> > 
> > Use the maximum of (0x7e000, sizeof(x86_boot_sector))? (Not that it is
> > likely that the boot sector will ever grow, but I think it is cleaner.)  
> 
> Sounds like a little bit of too much sanity checking for me, but ok, I
> can add it.

It's probably a bit paranoid, but I don't think it hurts. 

> 
> >> +        boot_code = g_malloc(len);  
> > 
> > Would g_malloc_0() be better?  
> 
> Good idea, the test is likely more predictable if we don't have random
> data in the file here (it should not really matter, but let's better be
> safe than sorry).
> 
> >> +        memcpy(boot_code, x86_boot_sector, sizeof x86_boot_sector);  
> > 
> > sizeof(x86_boot_sector)?  
> 
> The original code uses sizeof without parenthesis, so I think we should
> stay with that coding style.

After your patch, the original sizeof callers are gone, no? (I really
prefer the sizeof() variant...)