"Guilherme G. Piccoli" <gpicc...@linux.vnet.ibm.com> writes:
> Commit 15fdeaa65af2362d94e64ddd604b68f588495a05 ("Add
> support to configurable rebuild rate in iprconfig.") added
> support to specify the array rebuild rate on command-line
> interface of iprconfig. Here we add the same functionality
> on the ncurses interface.
Thanks for doing this! A few comments inline.
> /**
> @@ -10966,68 +10967,95 @@ int ioa_config_menu(struct ioa_config_attr
> *ioa_config_attr,
> same line as the field in which the menu is opened for*/
> start_row += 2; /* for title */ /* FIXME */
>
> - if (ioa_config_attr->option == 1 || ioa_config_attr->option == 2
> - || ioa_config_attr->option == 3 || ioa_config_attr->option == 4
> - || ioa_config_attr->option == 5) {
> - num_menu_items = 4;
> - menu_item = malloc(sizeof(ITEM **) * (num_menu_items + 1));
> - userptr = malloc(sizeof(int) * num_menu_items);
> + if (ioa_config_attr->option == 6) {
> + rc = display_input(start_row + 1,
> + &ioa_config_attr->rebuild_rate);
> + /* (start_row + 1) to correct window location */
I really dislike the way the ncurses code is growing. I think it could
use some refactoring anytime soon.
I see your patch moves a lot of code, making things harder to review.
It's hard to see what has changed. I think moving this hunk to the
'else' condition would help git diff. Just a thought.
Is it possible for ioa_config_attr->option to be something higher than
6? If so, we should catch it with an else.
>
> - menu_index = 0;
> - if (ioa_config_attr->option == 1)
> - menu_item[menu_index] = new_item("None","");
> - else if (ioa_config_attr->option == 2)
> - menu_item[menu_index] = new_item("RAID","");
> - else if (ioa_config_attr->option == 3)
> - menu_item[menu_index] = new_item("Disabled","");
> - else if (ioa_config_attr->option == 5)
> - menu_item[menu_index] = new_item("Enabled","");
> - else
> - menu_item[menu_index] = new_item("Default","");
> + if (ioa_config_attr->rebuild_rate > 100)
> + ioa_config_attr->rebuild_rate = 100;
>
> - userptr[menu_index] = 0;
> - set_item_userptr(menu_item[menu_index],
> - (char *)&userptr[menu_index]);
> + if (ioa_config_attr->rebuild_rate < 0)
> + ioa_config_attr->rebuild_rate = 0;
>
> - menu_index++;
> + ioa_config_attr->rebuild_rate =
> + (ioa_config_attr->rebuild_rate * 15) / 100;
~~~^~~~
Wrong identation here.
This information is provided by the user, right? Maybe we could throw
an Invalid Value error message here, instead of automatically fixing it.
What do you think?
Other than that, and a trailing whitespace when applying it, the patch
looks good to me.
--
Gabriel Krisman Bertazi
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
