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 <address@hidden> wrote:

> On 09.08.2017 11:05, Cornelia Huck wrote:
> > On Wed,  9 Aug 2017 06:59:37 +0200
> > Thomas Huth <address@hidden> 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...)