[PATCH V2] ui/ncurses: Add device hierarchy
Jeremy Kerr
jk at ozlabs.org
Wed Jul 16 13:23:04 EST 2014
Hi Sam,
> Boot options are now listed under their matching boot device in the
> ncurses UI to help differentitate similar boot option names
I get the following build failure with this patch:
../../../ui/ncurses/nc-menu.c: In function 'pmenu_find_device':
../../../ui/ncurses/nc-menu.c:225:24: error: 'bd' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
struct blockdev_info *bd;
^
../../../ui/ncurses/nc-menu.c:277:10: error: 'intf' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
mac_str(intf->hwaddr, intf->hwaddr_size,
^
cc1: all warnings being treated as errors
- I think we'll need a check for matched in both of those cases.
> diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c
> index 7200a81..d67fac5 100644
> --- a/ui/ncurses/nc-cui.c
> +++ b/ui/ncurses/nc-cui.c
> @@ -407,10 +407,12 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt,
> {
> struct cui *cui = cui_from_arg(arg);
> struct cui_opt_data *cod;
> + struct pmenu_item *i, *dev_hdr = NULL;
> unsigned int insert_pt;
> int result, rows, cols;
> - struct pmenu_item *i;
> ITEM *selected;
> + const char *tab = " ";
> + char *shift;
This is super-trivial, but our convention here is to put the local
variable declarations in descending-length order. This makes it slightly
easier to scan for a variable declaration.
>
> pb_debug("%s: %p %s\n", __func__, opt, opt->id);
>
> @@ -420,9 +422,15 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt,
> if (cui->current == &cui->main->scr)
> nc_scr_unpost(cui->current);
>
> - /* Save the item in opt->ui_info for cui_device_remove() */
> + /* Check if the boot device is new */
> + dev_hdr = pmenu_find_device(cui->main, dev, opt);
> +
> + /* All actual boot entries are 'tabbed' across */
> + shift = talloc_asprintf(cui->main, "%s%s", tab, opt->name ? : "Unknown Name");
80 chars again :) (also in a couple of other places)
Not sure why this var is called 'shift' - something from the previous
version?
>
> - opt->ui_info = i = pmenu_item_create(cui->main, opt->name);
> + /* Save the item in opt->ui_info for cui_device_remove() */
> + opt->ui_info = i = pmenu_item_create(cui->main, shift);
> + talloc_free(shift);
> if (!i)
> return -1;
>
> @@ -448,8 +456,15 @@ static int cui_boot_option_add(struct device *dev, struct boot_option *opt,
> pb_log("%s: set_menu_items failed: %d\n", __func__, result);
>
> /* Insert new items at insert_pt. */
> - insert_pt = pmenu_grow(cui->main, 1);
> - pmenu_item_insert(cui->main, i, insert_pt);
> + if (dev_hdr) {
> + insert_pt = pmenu_grow(cui->main, 2);
> + pmenu_item_insert(cui->main, dev_hdr, insert_pt);
> + pb_log("%s: adding new device hierarchy %s\n",__func__,opt->device_id);
> + pmenu_item_insert(cui->main, i, insert_pt+1);
> + } else {
> + insert_pt = pmenu_grow(cui->main, 1);
> + pmenu_item_add(cui->main, i, insert_pt);
> + }
>
> pb_log("%s: adding opt '%s'\n", __func__, cod->name);
> pb_log(" image '%s'\n", cod->bd->image);
> @@ -500,6 +515,7 @@ static void cui_device_remove(struct device *dev, void *arg)
> {
> struct cui *cui = cui_from_arg(arg);
> int result;
> + unsigned int i;
> struct boot_option *opt;
>
> pb_log("%s: %p %s\n", __func__, dev, dev->id);
> @@ -515,10 +531,20 @@ static void cui_device_remove(struct device *dev, void *arg)
> pb_log("%s: set_menu_items failed: %d\n", __func__, result);
>
> list_for_each_entry(&dev->boot_options, opt, list) {
> - struct pmenu_item *i = pmenu_item_from_arg(opt->ui_info);
> + struct pmenu_item *item = pmenu_item_from_arg(opt->ui_info);
> +
> + assert(pb_protocol_device_cmp(dev, cod_from_item(item)->dev));
> + pmenu_remove(cui->main, item);
> + }
>
> - assert(pb_protocol_device_cmp(dev, cod_from_item(i)->dev));
> - pmenu_remove(cui->main, i);
> + /* Manually remove remaining device hierarchy item */
> + for (i=0; i < cui->main->item_count; i++) {
> + struct pmenu_item *item = item_userptr(cui->main->items[i]);
> + if (item && item->data) {
> + struct cui_opt_data *data = item->data;
> + if (data && data->name && !strcmp(data->name,dev->id))
> + pmenu_remove(cui->main,item);
> + }
You can save a little indentation here by using a continue instead, to
skip entries:
for (i=0; i < cui->main->item_count; i++) {
struct pmenu_item *item = item_userptr(cui->main->items[i]);
struct cui_opt_data *data;
if (!item || !item->data)
continue;
data = item->data;
if (data && data->name && !strcmp(data->name,dev->id))
pmenu_remove(cui->main,item);
}
> +void pmenu_item_add(struct pmenu *menu, struct pmenu_item *item,
> + unsigned int insert_pt)
> +{
> + unsigned int dev;
> + bool found;
> + struct cui_opt_data *cod = item->data;
> +
> + /* Items array should already be disconnected */
> +
> + for (dev=0; dev < menu->item_count; dev++) {
> + if (menu->items[dev]) {
> + struct pmenu_item *i = item_userptr(menu->items[dev]);
> + struct cui_opt_data *d = i->data;
> + if (d && !d->opt) {
> + /* Device header will have opt == NULL */
> + if (!strcmp(cod->opt->device_id,d->name)) {
> + found = true;
> + break;
> + }
> + }
> + }
same here too:
for (dev = 0; dev < menu->item_count; dev++) {
if (!menu->items[dev])
continue;
[...]
}
> +
> + if (found) {
We'll need to initialise this to false, no?
> + assert(dev < insert_pt);
> + /* Shift down entries between header and insert_pt */
> + memmove(menu->items + dev + 2, menu->items + dev + 1,
> + ((menu->items + insert_pt) - (menu->items + dev + 1))
> + * sizeof(menu->items[0]));
> + memset(menu->items + dev + 1, 0, sizeof(menu->items[0]));
> + insert_pt = dev + 1;
> + }
> + /* If for some reason we didn't find the matching device,
> + * at least add it to a valid position */
> + pmenu_item_insert(menu, item, insert_pt);
> +}
> +
> +/**
> + * pmenu_find_device - Determine if a boot option is new, and if
> + * so return a new pmenu_item to represent its parent device
> + */
> +
> +struct pmenu_item *pmenu_find_device(struct pmenu *menu, struct device *dev,
> + struct boot_option *opt)
> +{
> + unsigned int i;
> + bool newdev = true, matched = false;
> + struct cui *cui = cui_from_pmenu(menu);
> + struct pmenu_item *item, *dev_hdr = NULL;
> + struct cui_opt_data *cod;
> + struct system_info *sys;
> + struct blockdev_info *bd;
> + struct interface_info *intf;
> + char hwaddr[32];
> + char buf[256];
> +
> + for (i=0; i < menu->item_count; i++) {
> + item = item_userptr(menu->items[i]);
> + cod = item->data;
> + /* boot entries will have opt defined */
> + if (!cod || cod->opt)
> + continue;
> + if (!strcmp(cod->name, opt->device_id)) {
>
Would it be worth adding a device pointer to item->data? We're doing a
few strcmps around the place.
> + /* Create a dummy pmenu_item to represent the dev */
> + pb_debug("%s: Building new item\n",__func__);
> + sys = cui->sysinfo;
> + switch (dev->type) {
> + case DEVICE_TYPE_OPTICAL:
> + case DEVICE_TYPE_DISK:
> + /* Find block info */
> + for (i=0; sys && i < sys->n_blockdevs; i++) {
> + bd = sys->blockdevs[i];
> + if (!strcmp(opt->device_id, bd->name)) {
> + matched = true;
> + break;
> + }
> + }
> + snprintf(buf,sizeof(buf),"[%s: %s / %s]",
> + dev->type == DEVICE_TYPE_DISK ? "Disk" : "CD/DVD",
> + bd->name, bd->uuid);
> + break;
> +
> + case DEVICE_TYPE_NETWORK:
> + /* Find interface info */
> + for (i=0; sys && i < sys->n_interfaces; i++) {
> + intf = sys->interfaces[i];
> + if (!strcmp(opt->device_id, intf->name)) {
> + matched = true;
> + break;
> + }
> + }
> + mac_str(intf->hwaddr, intf->hwaddr_size,
> + hwaddr, sizeof(hwaddr));
> + snprintf(buf,sizeof(buf),"[Interface %s / %s]",
> + intf->name, hwaddr);
> + break;
> +
> + default:
> + /* Assume the device may be able to boot */
> + break;
> + }
Looks okay, but makes me think we could have a single representation of
a bootable device, with a name, type and ID. This would help out with
the 'boot a specific device' code I'm working on too.
However, this would be something for a later change, I'm happy to use
the blockdev and interface arrays as they are for this code.
Thanks!
Jeremy
More information about the Petitboot
mailing list