| Message ID | 1364364631-24665-1-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 27, 2013 at 02:10:31PM +0800, liguang wrote: > if head magic is missing or wrong unexpectedly, we'd > better to reject booting. > e.g. > I make a mistake to boot a vmlinuz for MIPS(which > I think it's for x86) like this: > qemu-system-x86_64 -kernel vmlinuz -initrd demord > then qemu report: > "qemu: linux kernel too old to load a ram disk" > that's misleading. > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > --- > hw/i386/pc.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index b1e06fa..2b78dfc 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -683,8 +683,10 @@ static void load_linux(void *fw_cfg, > if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, > kernel_cmdline, kernel_size, header)) { > return; > + } else { > + fprintf(stderr, "please assure specicified kernel is for x86!\n"); > + exit(1); load_multiboot() can fail for other reasons so this error messing is misleading. Giving QEMU a non-x86 kernel is just one scenario where this may fail. > } > - protocol = 0; > } Why did you drop protocol = 0? Stefan
在 2013-03-28四的 10:42 +0100，Stefan Hajnoczi写道： > On Wed, Mar 27, 2013 at 02:10:31PM +0800, liguang wrote: > > if head magic is missing or wrong unexpectedly, we'd > > better to reject booting. > > e.g. > > I make a mistake to boot a vmlinuz for MIPS(which > > I think it's for x86) like this: > > qemu-system-x86_64 -kernel vmlinuz -initrd demord > > then qemu report: > > "qemu: linux kernel too old to load a ram disk" > > that's misleading. > > > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > > --- > > hw/i386/pc.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index b1e06fa..2b78dfc 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -683,8 +683,10 @@ static void load_linux(void *fw_cfg, > > if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, > > kernel_cmdline, kernel_size, header)) { > > return; > > + } else { > > + fprintf(stderr, "please assure specicified kernel is for x86!\n"); > > + exit(1); > > load_multiboot() can fail for other reasons so this error messing is > misleading. Giving QEMU a non-x86 kernel is just one scenario where > this may fail. according to my check of load_mutiboot function, mostly it will return 0 if it's not multboot, or 1 it's a multiboot, so print this message, or can I just print "wrong kernel image!" ? > > > } > > - protocol = 0; > > } > > Why did you drop protocol = 0? I think we only want either normal or multi boot linux kernel, I can't see meaning let other case go on. so, here, if a normal kernel, OK, go on, if multiboot, OK, go on, others, NO, end up. so, "protocol = 0" is meaningless here.
On Fri, Mar 29, 2013 at 1:38 AM, li guang <lig.fnst@cn.fujitsu.com> wrote: > 在 2013-03-28四的 10:42 +0100，Stefan Hajnoczi写道： >> On Wed, Mar 27, 2013 at 02:10:31PM +0800, liguang wrote: >> > if head magic is missing or wrong unexpectedly, we'd >> > better to reject booting. >> > e.g. >> > I make a mistake to boot a vmlinuz for MIPS(which >> > I think it's for x86) like this: >> > qemu-system-x86_64 -kernel vmlinuz -initrd demord >> > then qemu report: >> > "qemu: linux kernel too old to load a ram disk" >> > that's misleading. >> > >> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> >> > --- >> > hw/i386/pc.c | 4 +++- >> > 1 files changed, 3 insertions(+), 1 deletions(-) >> > >> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> > index b1e06fa..2b78dfc 100644 >> > --- a/hw/i386/pc.c >> > +++ b/hw/i386/pc.c >> > @@ -683,8 +683,10 @@ static void load_linux(void *fw_cfg, >> > if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, >> > kernel_cmdline, kernel_size, header)) { >> > return; >> > + } else { >> > + fprintf(stderr, "please assure specicified kernel is for x86!\n"); >> > + exit(1); >> >> load_multiboot() can fail for other reasons so this error messing is >> misleading. Giving QEMU a non-x86 kernel is just one scenario where >> this may fail. > > according to my check of load_mutiboot function, > mostly it will return 0 if it's not multboot, > or 1 it's a multiboot, so print this message, > or can I just print "wrong kernel image!" ? Yes, load_multiboot() fails if the image is not a valid multiboot image. An error message like "not a valid multiboot image" is good. >> >> > } >> > - protocol = 0; >> > } >> >> Why did you drop protocol = 0? > > I think we only want either normal or multi boot linux kernel, > I can't see meaning let other case go on. > so, here, if a normal kernel, OK, go on, > if multiboot, OK, go on, > others, NO, end up. > so, "protocol = 0" is meaningless here. I looked in more detail now and here is why we cannot drop this line of code: uint16_t protocol; Note that the variable is not initialized. [...] if (ldl_p(header+0x202) == 0x53726448) protocol = lduw_p(header+0x206); else { /* This looks like a multiboot kernel. If it is, let's stop treating it like a Linux kernel. */ if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, kernel_cmdline, kernel_size, header)) return; protocol = 0; Set it to 0 here so it's initialized } if (protocol < 0x200 || !(header[0x211] & 0x01)) { Use variable here - so we *must* initialize it before use. Stefan
OK, Thanks! I will update this patch as your comment. 在 2013-03-29五的 15:46 +0100，Stefan Hajnoczi写道： > On Fri, Mar 29, 2013 at 1:38 AM, li guang <lig.fnst@cn.fujitsu.com> wrote: > > 在 2013-03-28四的 10:42 +0100，Stefan Hajnoczi写道： > >> On Wed, Mar 27, 2013 at 02:10:31PM +0800, liguang wrote: > >> > if head magic is missing or wrong unexpectedly, we'd > >> > better to reject booting. > >> > e.g. > >> > I make a mistake to boot a vmlinuz for MIPS(which > >> > I think it's for x86) like this: > >> > qemu-system-x86_64 -kernel vmlinuz -initrd demord > >> > then qemu report: > >> > "qemu: linux kernel too old to load a ram disk" > >> > that's misleading. > >> > > >> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > >> > --- > >> > hw/i386/pc.c | 4 +++- > >> > 1 files changed, 3 insertions(+), 1 deletions(-) > >> > > >> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >> > index b1e06fa..2b78dfc 100644 > >> > --- a/hw/i386/pc.c > >> > +++ b/hw/i386/pc.c > >> > @@ -683,8 +683,10 @@ static void load_linux(void *fw_cfg, > >> > if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, > >> > kernel_cmdline, kernel_size, header)) { > >> > return; > >> > + } else { > >> > + fprintf(stderr, "please assure specicified kernel is for x86!\n"); > >> > + exit(1); > >> > >> load_multiboot() can fail for other reasons so this error messing is > >> misleading. Giving QEMU a non-x86 kernel is just one scenario where > >> this may fail. > > > > according to my check of load_mutiboot function, > > mostly it will return 0 if it's not multboot, > > or 1 it's a multiboot, so print this message, > > or can I just print "wrong kernel image!" ? > > Yes, load_multiboot() fails if the image is not a valid multiboot > image. An error message like "not a valid multiboot image" is good. > > >> > >> > } > >> > - protocol = 0; > >> > } > >> > >> Why did you drop protocol = 0? > > > > I think we only want either normal or multi boot linux kernel, > > I can't see meaning let other case go on. > > so, here, if a normal kernel, OK, go on, > > if multiboot, OK, go on, > > others, NO, end up. > > so, "protocol = 0" is meaningless here. > > I looked in more detail now and here is why we cannot drop this line of code: > > uint16_t protocol; > > Note that the variable is not initialized. > > [...] > if (ldl_p(header+0x202) == 0x53726448) > protocol = lduw_p(header+0x206); > else { > /* This looks like a multiboot kernel. If it is, let's stop > treating it like a Linux kernel. */ > if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, > kernel_cmdline, kernel_size, header)) > return; > protocol = 0; > > Set it to 0 here so it's initialized > > } > if (protocol < 0x200 || !(header[0x211] & 0x01)) { > > Use variable here - so we *must* initialize it before use. > > Stefan
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index b1e06fa..2b78dfc 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -683,8 +683,10 @@ static void load_linux(void *fw_cfg, if (load_multiboot(fw_cfg, f, kernel_filename, initrd_filename, kernel_cmdline, kernel_size, header)) { return; + } else { + fprintf(stderr, "please assure specicified kernel is for x86!\n"); + exit(1); } - protocol = 0; } if (protocol < 0x200 || !(header[0x211] & 0x01)) {
if head magic is missing or wrong unexpectedly, we'd better to reject booting. e.g. I make a mistake to boot a vmlinuz for MIPS(which I think it's for x86) like this: qemu-system-x86_64 -kernel vmlinuz -initrd demord then qemu report: "qemu: linux kernel too old to load a ram disk" that's misleading. Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- hw/i386/pc.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)