Re: [Qemu-devel] [qemu-s390x] [PULL-for-s390x 06/14] s390-ccw: parse and set boot menu options

[Collin L. Walling]

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 <address@hidden> 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?

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).

Thanks for reporting this.  Seems to be a few cases that I missed on my end.

--
- Collin L Walling