Message ID | 54902DCA.7040201@huawei.com |
---|---|
State | New |
Headers | show |
On 16 December 2014 at 13:04, Gonglei <arei.gonglei@huawei.com> wrote: > On 2014/12/16 20:42, Peter Maydell wrote: > >> On 16 December 2014 at 09:22, <arei.gonglei@huawei.com> wrote: >>> @@ -412,9 +411,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, >>> object_property_set_link(OBJECT(machine), OBJECT(s), >>> "rtc_state", &error_abort); >>> >>> - if (set_boot_dev(s, boot_device)) { >>> - exit(1); >>> - } >>> + set_boot_dev(s, boot_device, &error_abort); >> >> This turns a "print error message and exit" path into >> an abort(), which doesn't seem right (this can be triggered >> by bad user input arguments, yes?). error_abort should >> only be used in cases where you would assert() if there >> was an error (ie where it would be a QEMU bug if it >> happened). >> > > Yes, agree. How does use a incremental patch fix this, Peter? > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 99deba6..d7822b8 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -364,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, > FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE }; > static pc_cmos_init_late_arg arg; > PCMachineState *pc_machine = PC_MACHINE(machine); > + Error *local_err = NULL; > > /* various important CMOS locations needed by PC/Bochs bios */ > > @@ -411,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, > object_property_set_link(OBJECT(machine), OBJECT(s), > "rtc_state", &error_abort); > > - set_boot_dev(s, boot_device, &error_abort); > + set_boot_dev(s, boot_device, &local_err); > + if (local_err) { > + exit(1); > + } That won't print the error message at all... -- PMM
On 2014/12/16 21:23, Peter Maydell wrote: > On 16 December 2014 at 13:04, Gonglei <arei.gonglei@huawei.com> wrote: >> On 2014/12/16 20:42, Peter Maydell wrote: >> >>> On 16 December 2014 at 09:22, <arei.gonglei@huawei.com> wrote: >>>> @@ -412,9 +411,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, >>>> object_property_set_link(OBJECT(machine), OBJECT(s), >>>> "rtc_state", &error_abort); >>>> >>>> - if (set_boot_dev(s, boot_device)) { >>>> - exit(1); >>>> - } >>>> + set_boot_dev(s, boot_device, &error_abort); >>> >>> This turns a "print error message and exit" path into >>> an abort(), which doesn't seem right (this can be triggered >>> by bad user input arguments, yes?). error_abort should >>> only be used in cases where you would assert() if there >>> was an error (ie where it would be a QEMU bug if it >>> happened). >>> >> >> Yes, agree. How does use a incremental patch fix this, Peter? >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 99deba6..d7822b8 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c >> @@ -364,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, >> FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE }; >> static pc_cmos_init_late_arg arg; >> PCMachineState *pc_machine = PC_MACHINE(machine); >> + Error *local_err = NULL; >> >> /* various important CMOS locations needed by PC/Bochs bios */ >> >> @@ -411,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, >> object_property_set_link(OBJECT(machine), OBJECT(s), >> "rtc_state", &error_abort); >> >> - set_boot_dev(s, boot_device, &error_abort); >> + set_boot_dev(s, boot_device, &local_err); >> + if (local_err) { >> + exit(1); >> + } > > That won't print the error message at all... > Yes, I see. Thanks. I will send a new pull request :) Regards, -Gonglei
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 99deba6..d7822b8 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -364,6 +364,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE }; static pc_cmos_init_late_arg arg; PCMachineState *pc_machine = PC_MACHINE(machine); + Error *local_err = NULL; /* various important CMOS locations needed by PC/Bochs bios */ @@ -411,7 +412,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, object_property_set_link(OBJECT(machine), OBJECT(s), "rtc_state", &error_abort); - set_boot_dev(s, boot_device, &error_abort); + set_boot_dev(s, boot_device, &local_err); + if (local_err) { + exit(1); + } /* floppy type */ if (floppy) {