Re: [qemu-s390x] [PATCH 3/4] pc-bios/s390-ccw: fix non-sequential boot entries (eckd)

[Collin Walling]

On 04/13/2018 02:14 AM, Thomas Huth wrote:
> On 10.04.2018 17:01, Collin Walling wrote:
>> zIPL boot menu entries can be non-sequential. Let's account
>> for this issue for the s390 zIPL boot menu. Since this boot
>> menu is actually an imitation and is not completely capable
>> of everything the real zIPL menu can do, let's also print a
>> different banner to the user.
>>
>> Signed-off-by: Collin Walling <address@hidden>
>> Reported-by: Vasily Gorbik <address@hidden>
>> ---
>>  pc-bios/s390-ccw/menu.c | 32 ++++++++++++++++++++------------
>>  1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index 96eec81..083405f 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -158,7 +158,7 @@ static void boot_menu_prompt(bool retry)
>>      }
>>  }
>>  
>> -static int get_boot_index(int entries)
>> +static int get_boot_index(bool *valid_entries)
>>  {
>>      int boot_index;
>>      bool retry = false;
>> @@ -168,7 +168,8 @@ static int get_boot_index(int entries)
>>          boot_menu_prompt(retry);
>>          boot_index = get_index();
>>          retry = true;
>> -    } while (boot_index < 0 || boot_index >= entries);
>> +    } while (boot_index < 0 || boot_index >= MAX_BOOT_ENTRIES ||
>> +             !valid_entries[boot_index]);
>>  
>>      sclp_print("\nBooting entry #");
>>      sclp_print(uitoa(boot_index, tmp, sizeof(tmp)));
>> @@ -176,21 +177,28 @@ static int get_boot_index(int entries)
>>      return boot_index;
>>  }
>>  
>> -static void zipl_println(const char *data, size_t len)
>> +static void zipl_println(const char *data, size_t len, bool *valid_entries)
>>  {
>>      char buf[len + 2];
>> +    int entry;
>>  
>>      ebcdic_to_ascii(data, buf, len);
>>      buf[len] = '\n';
>>      buf[len + 1] = '\0';
>>  
>>      sclp_print(buf);
>> +
>> +    entry = buf[0] == ' ' ? atoui(buf + 1) : atoui(buf);
>> +    valid_entries[entry] = true;
> 
> zipl_println is now doing more than its name suggests - it now also
> populates the valid_entries array. So I think you should either put an
> explaining comment in front of zipl_println, or (what I'd prefer), move
> this valid_entries populating code rather to the while loop in
> menu_get_zipl_boot_index below instead.

Fair enough. I'll spin up v2 for the list after this change and some 
reassurance testing.

Thanks for the feedback and r-b's.


> 
>> +    if (entry == 0)
>> +        sclp_print("\n");
>>  }
>>  
>>  int menu_get_zipl_boot_index(const char *menu_data)
>>  {
>>      size_t len;
>> -    int entries;
>> +    bool valid_entries[MAX_BOOT_ENTRIES] = {false};
>>      uint16_t zipl_flag = *(uint16_t *)(menu_data - ZIPL_FLAG_OFFSET);
>>      uint16_t zipl_timeout = *(uint16_t *)(menu_data - ZIPL_TIMEOUT_OFFSET);
>>  
>> @@ -202,19 +210,19 @@ int menu_get_zipl_boot_index(const char *menu_data)
>>          timeout = zipl_timeout * 1000;
>>      }
>>  
>> -    /* Print and count all menu items, including the banner */
>> -    for (entries = 0; *menu_data; entries++) {
>> +    /* Print banner */
>> +    sclp_print("s390-ccw zIPL Boot Menu\n\n");
>> +    menu_data += strlen(menu_data) + 1;
>> +
>> +    /* Print entries */
>> +    while (*menu_data) {
>>          len = strlen(menu_data);
>> -        zipl_println(menu_data, len);
>> +        zipl_println(menu_data, len, valid_entries);
>>          menu_data += len + 1;
>> -
>> -        if (entries < 2) {
>> -            sclp_print("\n");
>> -        }
>>      }
>>  
>>      sclp_print("\n");
>> -    return get_boot_index(entries - 1); /* subtract 1 to exclude banner */
>> +    return get_boot_index(valid_entries);
>>  }
> 
>  Thomas
> 


-- 
Respectfully,
- Collin Walling