| Message ID | 20170726150446.20381-1-otubo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/07/2017 17:04, Eduardo Otubo wrote: > Starting qemu-system-unicore32 without the -kernel parameter results in > an assert() returns false and aborts qemu. This patch replaces it with a > proper error message followed by exit(1). > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > --- > hw/unicore32/puv3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c > index e9d1a60b6f..ff62efb4df 100644 > --- a/hw/unicore32/puv3.c > +++ b/hw/unicore32/puv3.c > @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char *kernel_filename) > if (kernel_filename == NULL && qtest_enabled()) { > return; > } > - assert(kernel_filename != NULL); > + if (kernel_filename == NULL) { > + error_report("kernel parameter cannot be empty"); > + exit(1); > + } > > /* only zImage format supported */ > size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR, > Cc: qemu-trivial@nongnu.org
On 07/26/2017 11:04 AM, Eduardo Otubo wrote: > Starting qemu-system-unicore32 without the -kernel parameter results in > an assert() returns false and aborts qemu. This patch replaces it with a > proper error message followed by exit(1). > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > --- > hw/unicore32/puv3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c > index e9d1a60b6f..ff62efb4df 100644 > --- a/hw/unicore32/puv3.c > +++ b/hw/unicore32/puv3.c > @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char *kernel_filename) > if (kernel_filename == NULL && qtest_enabled()) { > return; > } > - assert(kernel_filename != NULL); > + if (kernel_filename == NULL) { > + error_report("kernel parameter cannot be empty"); > + exit(1); > + } > How do you actually trigger this? There's already appropriate command line options handling in vl.c, which all targets (I tried) incorporate. For unicore32: $ ./unicore32-softmmu/qemu-system-unicore32 -kernel qemu-system-unicore32: -kernel: requires an argument I also see what I believe is proper handling with invalid kernels (or empty options): $ ./unicore32-softmmu/qemu-system-unicore32 -kernel '' qemu-system-unicore32: Load kernel error: '' Regards, - Cleber. > /* only zImage format supported */ > size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR, >
On 07/26/2017 11:28 AM, Cleber Rosa wrote: > > > On 07/26/2017 11:04 AM, Eduardo Otubo wrote: >> Starting qemu-system-unicore32 without the -kernel parameter results in >> an assert() returns false and aborts qemu. This patch replaces it with a >> proper error message followed by exit(1). >> >> Signed-off-by: Eduardo Otubo <otubo@redhat.com> >> --- >> hw/unicore32/puv3.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c >> index e9d1a60b6f..ff62efb4df 100644 >> --- a/hw/unicore32/puv3.c >> +++ b/hw/unicore32/puv3.c >> @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char *kernel_filename) >> if (kernel_filename == NULL && qtest_enabled()) { >> return; >> } >> - assert(kernel_filename != NULL); >> + if (kernel_filename == NULL) { >> + error_report("kernel parameter cannot be empty"); >> + exit(1); >> + } >> > > How do you actually trigger this? There's already appropriate command > line options handling in vl.c, which all targets (I tried) incorporate. > For unicore32: > Oh my, I misread your commit message. I see this is actually triggered *without* -kernel, and not without a parameter. - Cleber. > $ ./unicore32-softmmu/qemu-system-unicore32 -kernel > qemu-system-unicore32: -kernel: requires an argument > > I also see what I believe is proper handling with invalid kernels (or > empty options): > > $ ./unicore32-softmmu/qemu-system-unicore32 -kernel '' > qemu-system-unicore32: Load kernel error: '' > > Regards, > - Cleber. > >> /* only zImage format supported */ >> size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR, >> >
On 26.07.2017 17:04, Eduardo Otubo wrote: > Starting qemu-system-unicore32 without the -kernel parameter results in > an assert() returns false and aborts qemu. This patch replaces it with a > proper error message followed by exit(1). > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > --- > hw/unicore32/puv3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c > index e9d1a60b6f..ff62efb4df 100644 > --- a/hw/unicore32/puv3.c > +++ b/hw/unicore32/puv3.c > @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char *kernel_filename) > if (kernel_filename == NULL && qtest_enabled()) { > return; > } > - assert(kernel_filename != NULL); > + if (kernel_filename == NULL) { > + error_report("kernel parameter cannot be empty"); > + exit(1); > + } > > /* only zImage format supported */ > size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR, That fixes the "crash", indeed. Tested-by: Thomas Huth <thuth@redhat.com>
On 07/26/2017 12:04 PM, Eduardo Otubo wrote: > Starting qemu-system-unicore32 without the -kernel parameter results in > an assert() returns false and aborts qemu. This patch replaces it with a > proper error message followed by exit(1). > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > --- > hw/unicore32/puv3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c > index e9d1a60b6f..ff62efb4df 100644 > --- a/hw/unicore32/puv3.c > +++ b/hw/unicore32/puv3.c > @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char *kernel_filename) > if (kernel_filename == NULL && qtest_enabled()) { > return; > } > - assert(kernel_filename != NULL); > + if (kernel_filename == NULL) { > + error_report("kernel parameter cannot be empty"); > + exit(1); > + } This seems a temporary kludge for 2.10 but not the correct long-term fix. As commented in another thread [1] where I got a bit discomfited, it'd be nice if during the 2.11 schedule the industry provide some help to clean/unify the loader.c code, ideally remove all duplicated code not arch-specific and use hw/core/loader*. [1] http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06760.html > > /* only zImage format supported */ > size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR, >
On 26.07.2017 18:00, Philippe Mathieu-Daudé wrote: > On 07/26/2017 12:04 PM, Eduardo Otubo wrote: >> Starting qemu-system-unicore32 without the -kernel parameter results in >> an assert() returns false and aborts qemu. This patch replaces it with a >> proper error message followed by exit(1). >> >> Signed-off-by: Eduardo Otubo <otubo@redhat.com> >> --- >> hw/unicore32/puv3.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c >> index e9d1a60b6f..ff62efb4df 100644 >> --- a/hw/unicore32/puv3.c >> +++ b/hw/unicore32/puv3.c >> @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char >> *kernel_filename) >> if (kernel_filename == NULL && qtest_enabled()) { >> return; >> } >> - assert(kernel_filename != NULL); >> + if (kernel_filename == NULL) { >> + error_report("kernel parameter cannot be empty"); >> + exit(1); >> + } > > This seems a temporary kludge for 2.10 but not the correct long-term fix. > > As commented in another thread [1] where I got a bit discomfited, it'd > be nice if during the 2.11 schedule the industry provide some help to > clean/unify the loader.c code, ideally remove all duplicated code not > arch-specific and use hw/core/loader*. > > [1] http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06760.html Sorry, but this here *is* a machine-specific problem. I fail to see how this should be related to the hw/core/loader.c code? Thomas
On 07/26/2017 01:05 PM, Thomas Huth wrote: > On 26.07.2017 18:00, Philippe Mathieu-Daudé wrote: >> On 07/26/2017 12:04 PM, Eduardo Otubo wrote: >>> Starting qemu-system-unicore32 without the -kernel parameter results in >>> an assert() returns false and aborts qemu. This patch replaces it with a >>> proper error message followed by exit(1). >>> >>> Signed-off-by: Eduardo Otubo <otubo@redhat.com> >>> --- >>> hw/unicore32/puv3.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c >>> index e9d1a60b6f..ff62efb4df 100644 >>> --- a/hw/unicore32/puv3.c >>> +++ b/hw/unicore32/puv3.c >>> @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char >>> *kernel_filename) >>> if (kernel_filename == NULL && qtest_enabled()) { >>> return; >>> } >>> - assert(kernel_filename != NULL); >>> + if (kernel_filename == NULL) { >>> + error_report("kernel parameter cannot be empty"); >>> + exit(1); >>> + } >> >> This seems a temporary kludge for 2.10 but not the correct long-term fix. >> >> As commented in another thread [1] where I got a bit discomfited, it'd >> be nice if during the 2.11 schedule the industry provide some help to >> clean/unify the loader.c code, ideally remove all duplicated code not >> arch-specific and use hw/core/loader*. >> >> [1] http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06760.html > > Sorry, but this here *is* a machine-specific problem. I fail to see how > this should be related to the hw/core/loader.c code? Yes, this assert is a problem, same as the one in xen_load_linux() and this is a surgical fix, indeed. I probably misused the "kludge" word, sorry. My comment was about long-term use of -kernel. Maybe I didn't point to the correct file, I was thinking about the various hw/$arch/boot.c. My feeling is the -kernel parameter started as something simple years ago then grow and now his usage is somehow abused. I'd rather see a common hw/core/boot.c with each arch just adding arch-related fixes. For example in hw/arm/boot.c, see arm_load_kernel_notify(), except features parsing, initrd_start and the fixupcontext part, most of the code is multiarch and could benefit other archs less lov^H^H^Hsupported. arm_load_kernel() is generic. In load_dtb() only the acells/scells is arm-specific and could replace dup microblaze code, work on mips, etc. Regards, Phil.
diff --git a/hw/unicore32/puv3.c b/hw/unicore32/puv3.c index e9d1a60b6f..ff62efb4df 100644 --- a/hw/unicore32/puv3.c +++ b/hw/unicore32/puv3.c @@ -92,7 +92,10 @@ static void puv3_load_kernel(const char *kernel_filename) if (kernel_filename == NULL && qtest_enabled()) { return; } - assert(kernel_filename != NULL); + if (kernel_filename == NULL) { + error_report("kernel parameter cannot be empty"); + exit(1); + } /* only zImage format supported */ size = load_image_targphys(kernel_filename, KERNEL_LOAD_ADDR,
Starting qemu-system-unicore32 without the -kernel parameter results in an assert() returns false and aborts qemu. This patch replaces it with a proper error message followed by exit(1). Signed-off-by: Eduardo Otubo <otubo@redhat.com> --- hw/unicore32/puv3.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)