Brian King <brk...@linux.vnet.ibm.com> writes:
> Fix a memory leak in update-all-ucodes.
>
> Print a status summary to the user if, when chosing to update all devices
> to the latest microcode level using the ncurses interface, all devices are
> update
> which says "All devices up to date"
>
Hi Brian,
Thanks for the patch. Other than some minor issues, it looks good to
me. Please fix before pushing.
> ---
>
> iprconfig.c | 24 +++++++++++++++++-------
> iprconfig.h | 2 ++
> 2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff -puN iprconfig.c~iprutils_update_all_ucodes_fixes iprconfig.c
> --- iprutils.patch/iprconfig.c~iprutils_update_all_ucodes_fixes
> 2015-09-16 15:51:55.413526784 -0500
> +++ iprutils.patch-bjking1/iprconfig.c 2015-09-16 16:23:16.847135630
> -0500
> @@ -11549,6 +11549,7 @@ int download_all_ucode(i_container *i_co
> int header_lines;
> char line[BUFSIZ];
> char *body;
> + int rc = 93;
Please use 'RC_93_All_Up_To_Date' here.
>
> processing();
> i_con = free_i_con(i_con);
> @@ -11564,11 +11565,15 @@ int download_all_ucode(i_container *i_co
> continue;
>
> lfw = get_latest_fw_image(dev);
> - if (!lfw || lfw->version <= get_fw_version(dev))
> + if (!lfw || lfw->version <= get_fw_version(dev)) {
> + free(lfw);
> continue;
> + }
>
> - if (ioa->is_secondary)
> + if (ioa->is_secondary) {
> + free(lfw);
> continue;
> + }
Nice catch. Sorry I missed it. :)
>
> sprintf(line, "%-6s %-10X %-10s %s\n",
> basename(dev->gen_name), lfw->version,
> @@ -11585,22 +11590,27 @@ int download_all_ucode(i_container *i_co
> }
> }
>
> - n_confirm_download_all_ucode.body = body;
> - s_out = screen_driver(&n_confirm_download_all_ucode, header_lines,
> i_con);
> + if (update_list) {
> + n_confirm_download_all_ucode.body = body;
> + s_out = screen_driver(&n_confirm_download_all_ucode,
> header_lines, i_con);
> + }
>
> - if (s_out && s_out->rc != CANCEL_FLAG) {
> + if (update_list && (!s_out || s_out->rc != CANCEL_FLAG)) {
Adding update_list to this condition is redundant with the while below.
Also, It think the only way for s_out to be NULL but update_list to
exist, is malloc failing in screen_driver, which would cause us to
segfault before reaching this code (we gotta fix that). Either way, if
s_out is NULL but update_list isn't, we've hit a bug and we should fail
execution instead of attempting the update.
I recommend keeping the original check here.
> while (update_list) {
> elem = update_list;
> update_list = elem->next;
> update_ucode(elem->dev, elem->lfw);
> + free(elem->lfw);
> free(elem);
> }
> }
>
> - if (s_out)
> + if (s_out) {
> + rc = s_out->rc;
> free(s_out);
> + }
By making the call to screen_driver conditional and with this change,
s_out can be used without being initialized, since screen_driver will no
longer be called if update_list == NULL. This causes a Segfault in my
system if there isn't any devices to update. Initializing s_out to NULL
will fix this.
Thanks!
--
Gabriel Krisman Bertazi
------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
