On Wed, Feb 21, 2024 at 09:50:35AM -0800, Al Chu wrote: > Yeah, to verify you'd need a BMC that has the ability to configure a dynamic > IP address.
I found that I have such a system, actually, so I tried the old and new (with yout patch below) build. Old: # ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type Section Lan6_Conf ## READ-ONLY ## IPv6_Dynamic_Address_Source_Type EndSection # valgrind ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type ==26033== ERROR SUMMARY: 264 errors from 46 contexts (suppressed: 0 from 0) New: # ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type Section Lan6_Conf ## READ-ONLY ## IPv6_Dynamic_Address_Source_Type SLAAC EndSection # valgrind ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type ==25466== ERROR SUMMARY: 256 errors from 38 contexts (suppressed: 0 from 0) Certainly looks like an improvement! Thank you, Pavel > here's the patch i just commited upstream > > https://github.com/chu11/freeipmi-mirror/commit/41d0d70f09b4becfceef0517543cbf335c0e927a > > Thanks for the catch > > Oh and I got an e-mail that said you subscribed to freeipmi-devel today? > I'm not sure what was up with gnu's mailman, but hopefully it's fixed now. > > Al > > On 2/21/24 09:25, Pavel Cahyna wrote: > > On Wed, Feb 21, 2024 at 09:04:13AM -0800, Al Chu wrote: > > > On 2/21/24 03:37, Pavel Cahyna wrote: > > > > [ please Cc: me in replies, I am not subscribed and my attempt to > > > > subscribe hits "Bug in Mailman version 2.1.29" .] > > > > > > > > Hello Al, > > > > > > > > > certain fields are only used for static vs dynamic addresses, so only > > > > > specific ones need to be initialized as long as things are programmed > > > > > correctly. > > > > > > > > > > IIRC, the patch below fixed the mistake that I used "source" instead > > > > > of "source_type" for dynamic addresses. > > > > that's my point - the patch below initializes "source_type" and leaves > > > > "source" uninitialized, but then it keeps using "ipv6_data.source". To > > > > me, the patch seems incomplete and > > > > > > > > get_dynamic_address_source_type_string (ipv6_data.source)) < 0) > > > > looks like it should be changed to > > > > > > > > get_dynamic_address_source_type_string (ipv6_data.source_type)) < 0) > > > Ohhh, I get it now. I misread the prior e-mail. Yeah, I think you're > > > right, that doesn't look right at all. > > > > > > Thanks for the catch. I can get this fixed sometime today. > > > > > > I assume you hit this bug? Is it critical for a new release? > > I have not hit the bug, I am not using ipmi-config myself, actually. The > > bug was found by a static code scanner. Do you know how would one > > reproduce it or in general test this code? I suppose this needs a BMC > > with an IPv6 address configured? > > > > It is not entirely critical, but I would prefer not to update to a > > release which has a coding error added. If you have a patch, I will add > > it to the CentOS Stream and Fedora packages. > > > > Regards, Pavel > > > > > Al > > > > > > > What do you think? If not, I don't see where does ipv6_data.source get > > > > initialized. > > > > > > > > Regards, Pavel > > > > > > > > On Tue, Feb 20, 2024 at 05:03:46PM +0100, Pavel Cahyna wrote: > > > > > Hello, > > > > > > > > > > I have a question about this commit: > > > > > > > > > > commit 478ffaa7d2ffe240a0afaf9d7d5189342c94bd4d > > > > > Author: Albert Chu <ch...@llnl.gov> > > > > > Date: Wed Jan 26 21:48:47 2022 -0800 > > > > > > > > > > ipmi-config: correct IPv6 dynamic address checkout error > > > > > > > > > > > > > > > It contains this change: > > > > > > > > > > ----- > > > > > @@ -1089,15 +1090,15 @@ _get_ipv6_dynamic_address > > > > > (ipmi_config_state_data_t *state_data, > > > > > goto cleanup; > > > > > } > > > > > > > > > > - if (FIID_OBJ_GET (obj_cmd_rs, "source", &val) < 0) > > > > > + if (FIID_OBJ_GET (obj_cmd_rs, "source_type", &val) < 0) > > > > > { > > > > > pstdout_fprintf (state_data->pstate, > > > > > stderr, > > > > > - "fiid_obj_get: 'source': %s\n", > > > > > + "fiid_obj_get: 'source_type': %s\n", > > > > > fiid_obj_errormsg (obj_cmd_rs)); > > > > > goto cleanup; > > > > > } > > > > > - ipv6_data->source = val; > > > > > + ipv6_data->source_type = val; > > > > > > > > > > if (fiid_obj_get_data (obj_cmd_rs, > > > > > "address", > > > > > ----- > > > > > > > > > > > > > > > which, IIUC, leaves ipv6_data->source uninitialized. > > > > > > > > > > It then contains this change: > > > > > ----- > > > > > @@ -1186,7 +1187,7 @@ ipv6_dynamic_address_source_checkout > > > > > (ipmi_config_state_data_t *state_data, > > > > > > > > > > if (ipmi_config_section_update_keyvalue_output (state_data, > > > > > kv, > > > > > - > > > > > get_dynamic_address_source_string (ipv6_data.source)) < 0) > > > > > + > > > > > get_dynamic_address_source_type_string (ipv6_data.source)) < 0) > > > > > return (IPMI_CONFIG_ERR_FATAL_ERROR); > > > > > > > > > > rv = IPMI_CONFIG_ERR_SUCCESS; > > > > > ----- > > > > > > > > > > which keeps using ipv6_data.source, which looks uninitialized. This > > > > > looks suspect, but I admit I > > > > > don't really understand the code. > > > > > > > > > > Best regards, Pavel > > > -- > > > Al Chu > > > Livermore Computing > > > Lawrence Livermore National Laboratory > > > > -- > Al Chu > Livermore Computing > Lawrence Livermore National Laboratory >