On Mon, 26 Feb 2018 14:44:45 -0500 "Collin L. Walling" <wall...@linux.vnet.ibm.com> wrote:
> On 02/26/2018 02:29 PM, Collin L. Walling wrote: > > On 02/26/2018 01:48 PM, Cornelia Huck wrote: > >> On Mon, 26 Feb 2018 11:42:29 +0100 > >> Thomas Huth <th...@redhat.com> wrote: > >> > >> [...] > >> > >>> 3 files changed, 66 insertions(+), 4 deletions(-) > >>> +static void s390_ipl_set_boot_menu(S390IPLState *ipl) > >>> +{ > >>> + QemuOptsList *plist = qemu_find_opts("boot-opts"); > >>> + QemuOpts *opts = QTAILQ_FIRST(&plist->head); > >>> + uint8_t *flags = &ipl->qipl.qipl_flags; > >>> + uint32_t *timeout = &ipl->qipl.boot_menu_timeout; > >>> + const char *tmp; > >>> + unsigned long splash_time = 0; > >>> + > >>> + if (!get_boot_device(0)) { > >>> + if (boot_menu) { > >>> + error_report("boot menu requires a bootindex to be > >>> specified for " > >>> + "the IPL device."); > >>> + } > >>> + return; > >>> + } > >>> + > >>> + switch (ipl->iplb.pbt) { > >>> + case S390_IPL_TYPE_CCW: > >>> + break; > >>> + default: > >>> + error_report("boot menu is not supported for this device > >>> type."); > >> If I specify both a bootindex for a device and a -kernel parameter, I > >> get this error message. Looks a tad odd, but not sure how to avoid it. > > > > Hmm... perhaps an additional check if no IPLB, then skip trying to set > > any boot menu data? > > > >> [...] > > > > > Something like: > > if (!ipl->iplb.len) { > return; > } > > placed just below the if (!get_boot_device(0)) chunk fixed it for me.If > no IPLB was set, > then the IPLB fields should just all be zeros. Why not just check if > the length is 0 to > determine that we did not set an IPLB at all? Yes, that makes the message go away for me. > > also: > > if (!ipl->iplb.len) { > if (boot_menu) { > error_report("boot menu requires an IPLB to function"); > } > return; > } > > if you think an error message is needed (use a better message, mine is > not helpful but I > just wanted to demonstrate that the if (boot_menu) check should be > nested first). I'm not sure we want an error message for a command line that booted without any message previously. It's not like I'd expect a boot menu when I pass in a kernel anyway... > > Thanks for reporting this. Seems to be a few cases that I missed on my end. The usual result of different people trying things :) How shall we proceed? I'd be willing to pull this now and then apply fixups on top, or we can have a respin. Thomas?