Re: [Qemu-devel] [qemu-s390x] [PATCH v1 3/5] s390-ccw: parse and set boot menu options

[Collin L. Walling]

On 11/28/2017 06:45 AM, Thomas Huth wrote:
> On 27.11.2017 21:55, Collin L. Walling wrote:
> > [...]
> >   
> > +static void s390_ipl_set_boot_menu(uint8_t *boot_menu_enabled,
> > +                                   uint16_t *boot_menu_timeout)
> > +{
> > +    QemuOptsList *plist = qemu_find_opts("boot-opts");
> > +    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> > +    const char *p;
> > +    long result = 0;
> > +
> > +    *boot_menu_enabled = qemu_opt_get_bool(opts, "menu", false);
> > +
> > +    if (*boot_menu_enabled) {
> > +        p = qemu_opt_get(opts, "splash-time");
> > +        qemu_strtol(p, NULL, 10, &result);
> It's maybe better to check the return code of qemu_strtol to see whether
> the option contained a valid number? An additional check for negative
> numbers would also be fine, I think.

Agreed.  I'll add some integer overflow checking as well.

> > [...]
> >   
> >   struct IplBlockCcw {
> >       uint64_t netboot_start_addr;
> > -    uint8_t  reserved0[77];
> > +    uint8_t  reserved0[74];
> > +    uint8_t  boot_menu_enabled;
> > +    uint16_t boot_menu_timeout;
> Could you please try to keep the fields aligned to their natural
> alignment? I.e. swap the two new entries, so that timeout comes before
> enabled?

Can do.

> >       uint8_t  ssid;
> >       uint16_t devno;
> >       uint8_t  vm_flags;
> > @@ -51,7 +53,9 @@ struct IplBlockQemuScsi {
> >       uint32_t lun;
> >       uint16_t target;
> >       uint16_t channel;
> > -    uint8_t  reserved0[77];
> > +    uint8_t  reserved0[74];
> > +    uint8_t  boot_menu_enabled;
> > +    uint16_t boot_menu_timeout;
> dito
>
> >       uint8_t  ssid;
> >       uint16_t devno;
> >   } QEMU_PACKED;
>   Thomas

--
- Collin L Walling