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