[PATCH 1/2] ui/ncurses: Add function to insert boot entries in specific position

Jeremy Kerr jk at ozlabs.org
Thu Jul 3 16:52:27 EST 2014


Hi Sam,

> pmenu_item_add() is added in preparation for changes to the ncurses
> UI where boot entries are grouped underneath the name of the matching
> boot device

I'd be fine with merging this with the previous patch.

Just a couple of minimal things:

> +void pmenu_item_add(struct pmenu *menu, struct pmenu_item *item, unsigned int insert_pt)

Could trim this to 80 chars? Might also be good to mention that the menu
must have preallocated space for the new item.

> +        int found = 0;

May as well make this a bool

> +        struct cui_opt_data *cod = item->data;;

Duplicate semicolon here.

> +                if(menu->items[dev]){

Just needs a couple of spaces around the brackets:

		if (menu->items[dev]) {

- same applies to other places in your two patches.

> +        if(found){
> +                assert(dev<insert_pt);

Really minor, but spaces around the operator here make it a little more
readable.

> +                /* 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(ITEM *));

I'd have a slight preference for sizeof(menu->items[0]) here, so we
don't have problems if the type of the items array changes. Also, 80
chars.


> +                memset(menu->items + dev + 1, 0, sizeof(ITEM *));
> +                pmenu_item_insert(menu, item, dev + 1);
> +        } else {
> +                /* 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);
> +        }
> +}

Since we need to do a pmenu_item_insert in both cases, you could just do
a:

	if (found) {
		[...]
		insert_pt = dev + 1;
	}

	pmenu_item_insert(menu, item, insert_pt);


Cheers,


Jeremy


More information about the Petitboot mailing list