[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct |
Date: |
Mon, 29 Jan 2018 21:02:10 +0100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 29.01.2018 um 19:21 hat Anatol Pomozov geschrieben:
> Hi
>
> On Mon, Jan 15, 2018 at 6:49 AM, Kevin Wolf <address@hidden> wrote:
> > Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
> >> Using C structs makes the code more readable and prevents type conversion
> >> errors.
> >>
> >> Borrow multiboot1 header from GRUB project.
> >>
> >> Signed-off-by: Anatol Pomozov <address@hidden>
> >> ---
> >> hw/i386/multiboot.c | 124 +++++++++------------
> >> hw/i386/multiboot_header.h | 254
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >> tests/multiboot/mmap.c | 14 +--
> >> tests/multiboot/mmap.out | 10 ++
> >> tests/multiboot/modules.c | 12 ++-
> >> tests/multiboot/modules.out | 10 ++
> >> tests/multiboot/multiboot.h | 66 ------------
> >> 7 files changed, 339 insertions(+), 151 deletions(-)
> >> create mode 100644 hw/i386/multiboot_header.h
> >> delete mode 100644 tests/multiboot/multiboot.h
> >
> > This is a good change in general. However, I'm not sure if we should
> > really replace the header used in the tests. After all, the tests also
> > test whether things are at the right place, and if there is an error in
> > the header file, we can only catch it if the tests keep using their own
> > definitions.
>
> The added multibooh.h is from GRUB - the developers of multiboot. I
> think we can trust it. Having a single header will make future tests
> maintainence easier.
>
> But if you feel strongly that qemu tests should use it's own version
> of multiboot header then I can add it back.
No, I don't feel strongly, just wanted to mention that there is an
advantage of having a separate header in case you had not thought of it.
The decision is up to you.
Kevin