<html><body><p>Thanks for the comments.. I responded inline.<br><br>Thanks<br><br>-------------------------------------------------------------------------------------<br>Thanks and Regards,<br>Vishwanath.<br>Advisory Software Engineer,<br>Power Firmware Development, <br>Systems &Technology Lab,<br>MG2-6F-255 , Manyata Embassy Business Park, <br>Bangalore , KA , 560045<br>Ph: +91-80-46678255<br>E-mail: vishwanath@in.ibm.com<br>----------------------------------------------------------------------------------<br><br><tt>Cyril Bur <cyrilbur@gmail.com> wrote on 09/03/2016 04:54:11 am:<br><br>> From: Cyril Bur <cyrilbur@gmail.com></tt><br><tt>> To: Vishwanatha Subbanna/India/IBM@IBMIN</tt><br><tt>> Cc: OpenBMC Patches <openbmc-patches@stwcx.xyz>, openbmc@lists.ozlabs.org</tt><br><tt>> Date: 09/03/2016 04:54 am</tt><br><tt>> Subject: Re: [PATCH phosphor-host-ipmid v4] memory leak found during<br>> multiple reboots. it was found that in many cases the reply was not unref'ed</tt><br><tt>> <br>> On Wed, 2 Mar 2016 00:40:28 -0600<br>> OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br>> <br>> > From: vishwa <vishwanath@in.ibm.com><br>> > <br>> > --signed off by vishwanath@in.ibm.com---<br>> > ---<br>> > apphandler.C | 150 ++++++++++++++++++----------------------<br>> > host-services.c | 2 +-<br>> > ipmid.C | 53 +++++++-------<br>> > sensorhandler.C | 2 +-<br>> > storageaddsel.C | 4 +-<br>> > transporthandler.C | 199 ++++++++++++++++++++++<br>> +------------------------------<br>> > 6 files changed, 182 insertions(+), 228 deletions(-)<br>> > <br>> > diff --git a/apphandler.C b/apphandler.C<br>> > index a48059f..a921643 100644<br>> > --- a/apphandler.C<br>> > +++ b/apphandler.C<br>> > @@ -119,11 +119,22 @@ ipmi_ret_t <br>> ipmi_app_get_device_guid(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > const char *objname = "/org/openbmc/control/chassis0";<br>> > const char *iface = "org.freedesktop.DBus.Properties";<br>> > const char *chassis_iface = "org.openbmc.control.Chassis";<br>> > - sd_bus_message *reply = NULL, *m = NULL;<br>> > + sd_bus_message *reply = NULL;<br>> > sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > int r = 0;<br>> > char *uuid = NULL;<br>> > <br>> > + // UUID is in RFC4122 format. Ex: 61a39523-78f2-11e5-9862-e6402cfc3223<br>> > + // Per IPMI Spec 2.0 need to convert to 16 hex bytes and <br>> reverse the byte order<br>> > + // Ex: 0x2332fc2c40e66298e511f2782395a361<br>> > +<br>> > + const int resp_size = 16; // Response is 16 hex bytes per IPMI Spec<br>> > + uint8_t resp_uuid[resp_size]; // Array to hold the formatted response<br>> > + int resp_loc = resp_size-1; // Point resp end of array to <br>> save in reverse order<br>> > + int i = 0;<br>> > + char *tokptr = NULL;<br>> > + char *id_octet = NULL;<br>> > +<br>> > // Status code.<br>> > ipmi_ret_t rc = IPMI_CC_OK;<br>> > *data_len = 0;<br>> > @@ -131,49 +142,33 @@ ipmi_ret_t <br>> ipmi_app_get_device_guid(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > printf("IPMI GET DEVICE GUID\n");<br>> > <br>> > // Call Get properties method with the interface and property name<br>> > - r = sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"Get");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add the Get method object: %s<br>> \n", strerror(-r));<br>> > - return IPMI_CC_UNSPECIFIED_ERROR;<br>> > - }<br>> > - r = sd_bus_message_append(m, "ss", chassis_iface, "uuid");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to append arguments: %s\n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to call the Get method: %s\n", <br>> strerror(-r));<br>> > - return IPMI_CC_UNSPECIFIED_ERROR;<br>> > + r = sd_bus_call_method(bus,busname,objname,iface,<br>> > + "Get",&error, &reply, "ss",<br>> > + chassis_iface, "uuid");<br>> > + if (r < 0)<br>> > + {<br>> > + fprintf(stderr, "Failed to call Get Method: %s\n", strerror(-r));<br>> > + rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> > + goto finish;<br>> > }<br>> > +<br>> <br>> The above looks like a rework, not really addressing a memory leak<br>> specifically? The rework looks good to me but can we please start splitting<br>> patches up and do what the commit message says in each patch.<br>> <br>> This series would have been better with (at least) the above being in one<br>> commit entitled something like "Rework ipmi_app_get_device_guid() dbud message<br>> handling" and then an explanation as to why this way is better.<br>> </tt><br><br><tt>Okay... I had 2 approaches in front of me. Handle memory leak in the old way first and then in the next patch, undo what I had done before and do it using new API and handle the leak also in the new approach. I am not totally agreeing on this since there is a do-undo-redo effort involved here. BTW, is doing a code rework also not a method to fix the leaks if we had some issues in the way it was before ?</tt><br><tt><br>> > r = sd_bus_message_read(reply, "v", "s", &uuid);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to get a response: %s", strerror(-r));<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > - }<br>> > - if (uuid == NULL)<br>> > + if (r < 0 || uuid == NULL)<br>> > {<br>> > - fprintf(stderr, "Failed to get a valid response: %s", <br>> strerror(-r));<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > + fprintf(stderr, "Failed to get a response: %s", strerror(-r));<br>> > + rc = IPMI_CC_RESPONSE_ERROR;<br>> > + goto finish;<br>> > }<br>> > <br>> > - // UUID is in RFC4122 format. Ex: 61a39523-78f2-11e5-9862-e6402cfc3223<br>> > - // Per IPMI Spec 2.0 need to convert to 16 hex bytes and <br>> reverse the byte order<br>> > - // Ex: 0x2332fc2c40e66298e511f2782395a361<br>> > -<br>> > - const int resp_size = 16; // Response is 16 hex bytes per IPMI Spec<br>> > - uint8_t resp_uuid[resp_size]; // Array to hold the formatted response<br>> > - int resp_loc = resp_size-1; // Point resp end of array to <br>> save in reverse order<br>> > - int i = 0;<br>> > - char *tokptr = NULL;<br>> > -<br>> > // Traverse the UUID<br>> > - char* id_octet = strtok_r(uuid, "-", &tokptr); // Get the <br>> UUID octects separated by dash<br>> > + id_octet = strtok_r(uuid, "-", &tokptr); // Get the UUID <br>> octects separated by dash<br>> > <br>> > if (id_octet == NULL)<br>> > - { // Error<br>> > + {<br>> > + // Error<br>> > fprintf(stderr, "Unexpected UUID format: %s", uuid);<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > + rc = IPMI_CC_RESPONSE_ERROR;<br>> > + goto finish;<br>> > }<br>> > <br>> > while (id_octet != NULL)<br>> > @@ -201,8 +196,9 @@ ipmi_ret_t <br>> ipmi_app_get_device_guid(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > // Pack the actual response<br>> > memcpy(response, &resp_uuid, *data_len);<br>> > <br>> > +finish:<br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> > <br>> > return rc;<br>> > }<br>> > @@ -246,15 +242,13 @@ ipmi_ret_t <br>> ipmi_app_set_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > const char *busname = "org.openbmc.watchdog.Host";<br>> > const char *objname = "/org/openbmc/watchdog/host0";<br>> > const char *iface = "org.openbmc.Watchdog";<br>> > - sd_bus_message *reply = NULL, *m = NULL;<br>> > + sd_bus_message *reply = NULL;<br>> > sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > int r = 0;<br>> > <br>> > set_wd_data_t *reqptr = (set_wd_data_t*) request;<br>> > uint16_t timer = 0;<br>> > uint32_t timer_ms = 0;<br>> > - // Status code.<br>> > - ipmi_ret_t rc = IPMI_CC_OK;<br>> > <br>> > *data_len = 0;<br>> > <br>> > @@ -266,53 +260,45 @@ ipmi_ret_t <br>> ipmi_app_set_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > printf("WATCHDOG SET Timer:[0x%X] 100ms intervals\n",timer);<br>> > <br>> > // Set watchdog timer<br>> > - r = sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"set");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add the set method object: %s<br>> \n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_message_append(m, "i", timer_ms);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add timer value: %s\n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to call the set method: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > + r = sd_bus_call_method(bus, busname, objname, iface,<br>> > + "set", &error, &reply, "i", timer_ms);<br>> > + if(r < 0)<br>> > + {<br>> > + fprintf(stderr, "Failed to call the SET method: %s\n", <br>> strerror(-r));<br>> > + goto finish;<br>> > }<br>> > <br>> > + sd_bus_error_free(&error);<br>> > + reply = sd_bus_message_unref(reply);<br>> > +<br>> <br>> So here you're sort of reworking AND fixing the leaks.<br>> In terms of fixing the leaks can't you just pass NULL instead of <br>> reply? I could<br>> be wrong here but I believe that sd_bus will handle passing NULL and realise<br>> that you don't care about the reply (which, AFAICT, you don't).<br>> </tt><br><br><tt>Sorry but I don't understand "Pass the NULL instead of reply". Pass the NULL where ?. </tt><br><tt>The way its done here is per one of the approaches followed in systemd code and I followed the same.</tt><br><tt>I may have not consumed the reply here but just thinking futuristic to avoid any dangling pointer issues if they were to be re used.</tt><br><br><tt>from sd-bus/bus-message.C</tt><br><tt>---------------------------</tt><br><br><tt>_public_ sd_bus_message* sd_bus_message_unref(sd_bus_message *m) {</tt><br><br><tt> if (!m)</tt><br><tt> return NULL;</tt><br><br><tt> assert(m->n_ref > 0);</tt><br><tt> m->n_ref--;</tt><br><br><tt> if (m->n_ref > 0)</tt><br><tt> return NULL;</tt><br><br><tt> message_free(m);</tt><br><tt> return NULL;</tt><br><tt>}</tt><br><br><tt><br>> > // Stop the current watchdog if any<br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"stop");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add the start method object: <br>> %s\n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to call the start method: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > + r = sd_bus_call_method(bus, busname, objname, iface,<br>> > + "stop", &error, &reply, NULL);<br>> > + if(r < 0)<br>> > + {<br>> > + fprintf(stderr, "Failed to call the STOP method: %s\n", <br>> strerror(-r));<br>> > + goto finish;<br>> > }<br>> > <br>> > - // Start the watchdog if requested<br>> > if (reqptr->t_use & 0x40)<br>> > {<br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"start");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add the start method <br>> object: %s\n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to call the start method: %s<br>> \n", strerror(-r));<br>> > - return -1;<br>> > + sd_bus_error_free(&error);<br>> > + reply = sd_bus_message_unref(reply);<br>> > +<br>> > + // Start the watchdog if requested<br>> > + r = sd_bus_call_method(bus, busname, objname, iface,<br>> > + "start", &error, &reply, NULL);<br>> > + if(r < 0)<br>> > + {<br>> > + fprintf(stderr, "Failed to call the START method: %s<br>> \n", strerror(-r));<br>> > }<br>> > }<br>> > <br>> > +finish:<br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> > <br>> > - return rc;<br>> > + return (r < 0) ? -1 : IPMI_CC_OK;<br>> > }<br>> > <br>> > <br>> > @@ -323,7 +309,7 @@ ipmi_ret_t <br>> ipmi_app_reset_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > const char *busname = "org.openbmc.watchdog.Host";<br>> > const char *objname = "/org/openbmc/watchdog/host0";<br>> > const char *iface = "org.openbmc.Watchdog";<br>> > - sd_bus_message *reply = NULL, *m = NULL;<br>> > + sd_bus_message *reply = NULL;<br>> > sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > int r = 0;<br>> > <br>> > @@ -334,19 +320,15 @@ ipmi_ret_t <br>> ipmi_app_reset_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > printf("WATCHDOG RESET\n");<br>> > <br>> > // Refresh watchdog<br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"poke");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add the method object: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > + r = sd_bus_call_method(bus, busname, objname, iface,<br>> > + "poke", &error, &reply, NULL);<br>> > if (r < 0) {<br>> > - fprintf(stderr, "Failed to call the method: %s\n", strerror(-r));<br>> > - return -1;<br>> > + fprintf(stderr, "Failed to add reset watchdog: %s\n", <br>> strerror(-r));<br>> > + rc = -1;<br>> > }<br>> > <br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> > <br>> > return rc;<br>> > }<br>> > diff --git a/host-services.c b/host-services.c<br>> > index 23aa55e..cc47439 100644<br>> > --- a/host-services.c<br>> > +++ b/host-services.c<br>> > @@ -68,7 +68,7 @@ static int soft_power_off(sd_bus_message *m, <br>> void *userdata, sd_bus_error *ret_e<br>> > <br>> > finish:<br>> > sd_bus_error_free(&bus_error);<br>> > - sd_bus_message_unref(response);<br>> > + response = sd_bus_message_unref(response);<br>> > <br>> > if(rc < 0)<br>> > {<br>> > diff --git a/ipmid.C b/ipmid.C<br>> > index 0f4139c..06fbf05 100644<br>> > --- a/ipmid.C<br>> > +++ b/ipmid.C<br>> > @@ -225,9 +225,8 @@ static int send_ipmi_message(sd_bus_message <br>> *req, unsigned char seq, unsigned ch<br>> > <br>> > final:<br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > - sd_bus_message_unref(reply);<br>> > -<br>> > + m = sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> <br>> Why this change? You don't check m or reply/... the unref already <br>> happens. The<br>> only thing I can think that you're doing is fixing warnings (in which case,<br>> separate patch and please say so) but quite frankly you shouldn't be enabling<br>> those warnings, they're a pain.<br>> </tt><br><br><tt>I agree I am not consuming it here / passing the pointer back.. but it was more of a good practise that I was following.</tt><br><tt>I am not sure which warning are you saying is a pain ?. Do you have an example ? -or- are you generally saying</tt><br><tt>compiler should not warn for these types of message usages ?.</tt><br><br><tt>BTW, I did not fix any warnings.. That code was just being futuristic.</tt><br><tt><br>> > <br>> > return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;<br>> > }<br>> > @@ -246,13 +245,13 @@ static int <br>> handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error<br>> > r = sd_bus_message_read(m, "yyyy", &sequence, &netfn, &lun, &cmd);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to parse signal message: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > + goto final;<br>> > }<br>> > <br>> > r = sd_bus_message_read_array(m, 'y', &request, &sz );<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to parse signal message: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > + goto final;<br>> > }<br>> > <br>> > fprintf(ipmiio, "IPMI Incoming: Seq 0x%02x, NetFn 0x%02x, <br>> CMD: 0x%02x \n", sequence, netfn, cmd);<br>> > @@ -268,6 +267,7 @@ static int handle_ipmi_command(sd_bus_message <br>> *m, void *user_data, sd_bus_error<br>> > if(r != 0)<br>> > {<br>> > fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:<br>> [0x%X]\n",r, netfn, cmd);<br>> > + goto final;<br>> > }<br>> > <br>> > fprintf(ipmiio, "IPMI Response:\n");<br>> > @@ -278,11 +278,12 @@ static int <br>> handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error<br>> > ((unsigned char *)response) + 1, resplen - 1);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to send the response message\n");<br>> > - return -1;<br>> > }<br>> > <br>> > +final:<br>> > + m = sd_bus_message_unref(m);<br>> > <br>> > - return 0;<br>> > + return (r < 0) ? -1 : 0;<br>> > }<br>> > <br>> > <br>> > @@ -470,6 +471,7 @@ int <br>> find_interface_property_fru_type(dbus_interface_t *interface, const char *pr<br>> > fprintf(stderr, "Failed to create a method call: %s", <br>> strerror(-r));<br>> > fprintf(stderr,"Bus: %s Path: %s Interface: %s \n",<br>> > interface->bus, interface->path, interface->interface);<br>> > + goto final;<br>> > }<br>> > <br>> > r = sd_bus_message_append(m, "ss", <br>> "org.openbmc.InventoryItem", property_name);<br>> > @@ -477,6 +479,7 @@ int <br>> find_interface_property_fru_type(dbus_interface_t *interface, const char *pr<br>> > fprintf(stderr, "Failed to create a input parameter: %s",<br>> strerror(-r));<br>> > fprintf(stderr,"Bus: %s Path: %s Interface: %s \n",<br>> > interface->bus, interface->path, interface->interface);<br>> > + goto final;<br>> > }<br>> > <br>> > r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > @@ -496,7 +499,8 @@ int <br>> find_interface_property_fru_type(dbus_interface_t *interface, const char *pr<br>> > final:<br>> > <br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + m = sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> > <br>> > return r;<br>> > }<br>> > @@ -512,29 +516,18 @@ int find_openbmc_path(const char *type, <br>> const uint8_t num, dbus_interface_t *int<br>> > <br>> > char *str1, *str2, *str3;<br>> > sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > - sd_bus_message *reply = NULL, *m=NULL;<br>> > + sd_bus_message *reply = NULL;<br>> > <br>> > <br>> > int r;<br>> > <br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,busname,objname,busname,"getObjectFromByteId");<br>> > + r = sd_bus_call_method(bus,busname,objname,busname, <br>> "getObjectFromByteId",<br>> > + &error, &reply, "sy", type, num);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to create a method call: %s", <br>> strerror(-r));<br>> > - }<br>> > -<br>> > - r = sd_bus_message_append(m, "sy", type, num);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to create a input parameter: %s",<br>> strerror(-r));<br>> > - }<br>> > -<br>> > - // Call the IPMI responder on the bus so the message can be <br>> sent to the CEC<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to call the method: %s", strerror(-r));<br>> > goto final;<br>> > }<br>> > <br>> > -<br>> > r = sd_bus_message_read(reply, "(sss)", &str1, &str2, &str3);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to get a response: %s", strerror(-r));<br>> > @@ -550,7 +543,7 @@ int find_openbmc_path(const char *type, const <br>> uint8_t num, dbus_interface_t *int<br>> > final:<br>> > <br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> > <br>> > return r;<br>> > }<br>> > @@ -577,11 +570,13 @@ int set_sensor_dbus_state_s(uint8_t number, <br>> const char *method, const char *valu<br>> > r = <br>> sd_bus_message_new_method_call(bus,&m,a.bus,a.path,a.interface,method);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to create a method call: %s", <br>> strerror(-r));<br>> > + goto final;<br>> > }<br>> > <br>> > r = sd_bus_message_append(m, "v", "s", value);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to create a input parameter: %s",<br>> strerror(-r));<br>> > + goto final;<br>> > }<br>> > <br>> > <br>> > @@ -590,9 +585,9 @@ int set_sensor_dbus_state_s(uint8_t number, <br>> const char *method, const char *valu<br>> > fprintf(stderr, "Failed to call the method: %s", strerror(-r));<br>> > }<br>> > <br>> > -<br>> > +final:<br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + m = sd_bus_message_unref(m);<br>> > <br>> > return 0;<br>> > }<br>> > @@ -612,11 +607,13 @@ int set_sensor_dbus_state_y(uint8_t number, <br>> const char *method, const uint8_t va<br>> > r = <br>> sd_bus_message_new_method_call(bus,&m,a.bus,a.path,a.interface,method);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to create a method call: %s", <br>> strerror(-r));<br>> > + goto final;<br>> > }<br>> > <br>> > r = sd_bus_message_append(m, "v", "y", value);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to create a input parameter: %s",<br>> strerror(-r));<br>> > + goto final;<br>> > }<br>> > <br>> > <br>> > @@ -625,9 +622,9 @@ int set_sensor_dbus_state_y(uint8_t number, <br>> const char *method, const uint8_t va<br>> > fprintf(stderr, "12 Failed to call the method: %s", strerror(-r));<br>> > }<br>> > <br>> > -<br>> > +final:<br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > + m = sd_bus_message_unref(m);<br>> > <br>> > return 0;<br>> > -}<br>> > \ No newline at end of file<br>> > +}<br>> > diff --git a/sensorhandler.C b/sensorhandler.C<br>> > index d171bf5..90bfd0f 100644<br>> > --- a/sensorhandler.C<br>> > +++ b/sensorhandler.C<br>> > @@ -221,7 +221,7 @@ ipmi_ret_t <br>> ipmi_sen_get_sensor_reading(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > }<br>> > <br>> > <br>> > - sd_bus_message_unref(reply);<br>> > + reply = sd_bus_message_unref(reply);<br>> > <br>> > return rc;<br>> > }<br>> > diff --git a/storageaddsel.C b/storageaddsel.C<br>> > index 51edc59..cb90aab 100644<br>> > --- a/storageaddsel.C<br>> > +++ b/storageaddsel.C<br>> > @@ -193,8 +193,8 @@ int send_esel_to_dbus(const char *desc, const <br>> char *sev, const char *details, ui<br>> > <br>> > finish:<br>> > sd_bus_error_free(&error);<br>> > - sd_bus_message_unref(m);<br>> > - sd_bus_message_unref(reply);<br>> > + m = sd_bus_message_unref(m);<br>> > + reply = sd_bus_message_unref(reply);<br>> > return r;<br>> > }<br>> > <br>> > diff --git a/transporthandler.C b/transporthandler.C<br>> > index d43f1f9..0fd25f9 100644<br>> > --- a/transporthandler.C<br>> > +++ b/transporthandler.C<br>> > @@ -55,7 +55,7 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t <br>> netfn, ipmi_cmd_t cmd,<br>> > ipmi_ret_t rc = IPMI_CC_OK;<br>> > *data_len = 0;<br>> > sd_bus *bus = ipmid_get_sd_bus_connection();<br>> > - sd_bus_message *reply = NULL, *m = NULL;<br>> > + sd_bus_message *reply = NULL;<br>> > sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > int r = 0;<br>> > <br>> > @@ -84,20 +84,10 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t<br>> netfn, ipmi_cmd_t cmd,<br>> > reqptr->data[4],<br>> > reqptr->data[5]);<br>> > <br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"SetHwAddress");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add method object: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_message_append(m, "s", mac);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to append message data: %s<br>> \n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > + r = sd_bus_call_method(bus,app,obj,ifc,"SetHwAddress",<br>> > + &error, &reply, "s", mac);<br>> > if (r < 0) {<br>> > fprintf(stderr, "Failed to call method: %s\n", strerror(-r));<br>> > - return -1;<br>> > }<br>> > }<br>> > else if (reqptr->parameter == LAN_PARM_SUBNET)<br>> > @@ -112,84 +102,72 @@ ipmi_ret_t <br>> ipmi_transport_set_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > }<br>> > else if (reqptr->parameter == LAN_PARM_INPROGRESS) // Apply config<br>> > {<br>> > - int rc = 0;<br>> > - sd_bus_message *req = NULL;<br>> > - sd_bus_message *res = NULL;<br>> > - sd_bus *bus1 = NULL;<br>> > - sd_bus_error err = SD_BUS_ERROR_NULL;<br>> > - <br>> > if (!strcmp(new_ipaddr, "") || !strcmp (new_netmask, "") <br>> || !strcmp (new_gateway, ""))<br>> > {<br>> > fprintf(stderr,"ERROR: Incomplete LAN Parameters\n");<br>> > return -1;<br>> > }<br>> > - <br>> > - rc = sd_bus_open_system(&bus1);<br>> > - if(rc < 0)<br>> > - {<br>> > - fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");<br>> > - return -1;<br>> > - }<br>> > <br>> > if (strcmp(cur_ipaddr, ""))<br>> > {<br>> > - sd_bus_error_free(&err);<br>> > - sd_bus_message_unref(req);<br>> > - sd_bus_message_unref(res);<br>> > -<br>> > - rc = sd_bus_call_method(bus1, // On the System Bus<br>> > - app, // Service to contact<br>> > - obj, // Object path <br>> > - ifc, // Interface name<br>> > - "DelAddress4", // Method to be called<br>> > - &err, // object to <br>> return error<br>> > - &res, // Response <br>> message on success<br>> > - "ssss", // input <br>> message (dev,ip,nm,gw)<br>> > - "eth0",<br>> > - cur_ipaddr,<br>> > - cur_netmask,<br>> > - cur_gateway);<br>> > + r = sd_bus_call_method(bus, // On the System Bus<br>> > + app, // Service to contact<br>> > + obj, // Object path<br>> > + ifc, // Interface name<br>> > + "DelAddress4", // Method to be called<br>> > + &error, //
object to <br>> return error<br>> > + &reply, // Response <br>> message on success<br>> > + "ssss", // input <br>> message (dev,ip,nm,gw)<br>> > + "eth0",<br>> > + cur_ipaddr,<br>> > + cur_netmask,<br>> > + cur_gateway);<br>> > }<br>> > <br>> > - if(rc < 0)<br>> > + if(r < 0)<br>> > {<br>> > - fprintf(stderr, "Failed to remove existing IP %s: %s<br>> \n", cur_ipaddr, err.message);<br>> > - return -1;<br>> > + fprintf(stderr, "Failed to remove existing IP %s: %s<br>> \n", cur_ipaddr, error.message);<br>> > + goto finish;<br>> > }<br>> > <br>> > - sd_bus_error_free(&err);<br>> > - sd_bus_message_unref(req);<br>> > - sd_bus_message_unref(res);<br>> > -<br>> > - rc = sd_bus_call_method(bus1, // On the System Bus<br>> > - app, // Service to contact<br>> > - obj, // Object path </tt><br><tt>> > - ifc, // Interface name<br>> > - "AddAddress4", // Method to be called<br>> > - &err, // object to return error<br>> > - &res, // Response <br>> message on success<br>> > - "ssss", // input message <br>> (dev,ip,nm,gw)<br>> > - "eth0",<br>> > - new_ipaddr,<br>> > - new_netmask,<br>> > - new_gateway);<br>> > - if(rc < 0)<br>> > + sd_bus_error_free(&error);<br>> > + reply = sd_bus_message_unref(reply);<br>> > +<br>> > + r = sd_bus_call_method(bus, // On the System Bus<br>> > + app, // Service to contact<br>> > + obj, // Object path<br>> > + ifc, // Interface name<br>> > + "AddAddress4", // Method to be called<br>> > + &error, // object to return error<br>> > + &reply, // Response <br>> message on success<br>> > + "ssss", // input message <br>> (dev,ip,nm,gw)<br>> > + "eth0",<br>> > + new_ipaddr,<br>> > + new_netmask,<br>> > + new_gateway);<br>> > + if(r < 0)<br>> > {<br>> > - fprintf(stderr, "Failed to set IP %s: %s\n", <br>> new_ipaddr, err.message);<br>> > - return -1;<br>> > + fprintf(stderr, "Failed to set IP %s: %s\n", <br>> new_ipaddr, error.message);<br>> > + }<br>> > + else<br>> > + {<br>> > + strcpy (cur_ipaddr, new_ipaddr);<br>> > + strcpy (cur_netmask, new_netmask);<br>> > + strcpy (cur_gateway, new_gateway);<br>> > }<br>> > -<br>> > - strcpy (cur_ipaddr, new_ipaddr);<br>> > - strcpy (cur_netmask, new_netmask);<br>> > - strcpy (cur_gateway, new_gateway);<br>> > }<br>> > else<br>> > {<br>> > fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> > - return IPMI_CC_PARM_NOT_SUPPORTED;<br>> > + rc = IPMI_CC_PARM_NOT_SUPPORTED;<br>> > }<br>> > <br>> > - return rc;<br>> > +finish:<br>> > + // Clenaup the resources allocated reply and error<br>> > + sd_bus_error_free(&error);<br>> > + reply = sd_bus_message_unref(reply);<br>> > +<br>> > + return (r < 0) ? -1 : rc;<br>> > }<br>> > <br>> > struct get_lan_t {<br>> > @@ -206,7 +184,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t<br>> netfn, ipmi_cmd_t cmd,<br>> > ipmi_ret_t rc = IPMI_CC_OK;<br>> > *data_len = 0;<br>> > sd_bus *bus = ipmid_get_sd_bus_connection();<br>> > - sd_bus_message *reply = NULL, *m = NULL;<br>> > + sd_bus_message *reply = NULL;<br>> > sd_bus_error error = SD_BUS_ERROR_NULL;<br>> > int r = 0;<br>> > const uint8_t current_revision = 0x11; // Current rev per IPMI Spec 2.0<br>> > @@ -259,32 +237,27 @@ ipmi_ret_t <br>> ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > const char* device = "eth0";<br>> > uint8_t buf[5]; // Size of expected IPMI response msg<br>> > <br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetAddress4");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add method object: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_message_append(m, "s", device);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to append message data: %s<br>> \n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > + r = sd_bus_call_method(bus,app,obj,ifc,"GetAddress4",<br>> > + &error, &reply, "s", device);<br>> > + if (r < 0)<br>> > + {<br>> > fprintf(stderr, "Failed to call method: %s\n", strerror(-r));<br>> > - return -1;<br>> > + rc = -1;<br>> > + goto finish;<br>> > }<br>> > - rc = sd_bus_message_enter_container (reply, 'a', "(iyyus)");<br>> > - if(rc < 0)<br>> > + r = sd_bus_message_enter_container (reply, 'a', "(iyyus)");<br>> > + if(r < 0)<br>> > {<br>> > fprintf(stderr, "Failed to parse response message:<br>> [%s]\n", strerror(-rc));<br>> > - return -1;<br>> > + rc = -1;<br>> > + goto finish;<br>> > }<br>> > - rc = sd_bus_message_read(reply, "(iyyus)", &family, <br>> &prefixlen, &scope, &flags, &saddr);<br>> > - if (rc < 0)<br>> > + r = sd_bus_message_read(reply, "(iyyus)", &family, <br>> &prefixlen, &scope, &flags, &saddr);<br>> > + if (r < 0)<br>> > {<br>> > fprintf(stderr, "Failed to receive response: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > + rc = -1;<br>> > + goto finish;<br>> > }<br>> > <br>> > printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6",<br>> prefixlen, scope, flags, saddr);<br>> > @@ -297,7 +270,8 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t<br>> netfn, ipmi_cmd_t cmd,<br>> > if (digit == NULL)<br>> > {<br>> > fprintf(stderr, "Unexpected IP format: %s", saddr);<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > + rc = IPMI_CC_RESPONSE_ERROR;<br>> > + goto finish;<br>> > }<br>> > i = 0;<br>> > while (digit != NULL)<br>> > @@ -311,7 +285,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t<br>> netfn, ipmi_cmd_t cmd,<br>> > *data_len = sizeof(buf);<br>> > memcpy(response, &buf, *data_len);<br>> > <br>> > - return IPMI_CC_OK;<br>> > + rc = IPMI_CC_OK;<br>> > }<br>> > else if (reqptr->parameter == LAN_PARM_MAC)<br>> > {<br>> > @@ -321,30 +295,26 @@ ipmi_ret_t <br>> ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > uint8_t buf[7];<br>> > char *eaddr1 = NULL;<br>> > <br>> > - r = <br>> sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetHwAddress");<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to add method object: %s\n", <br>> strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_message_append(m, "s", device);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to append message data: %s<br>> \n", strerror(-r));<br>> > - return -1;<br>> > - }<br>> > - r = sd_bus_call(bus, m, 0, &error, &reply);<br>> > - if (r < 0) {<br>> > - fprintf(stderr, "Failed to call method: %s\n", strerror(-r));<br>> > - return -1;<br>> > + r = sd_bus_call_method(bus,app,obj,ifc,"GetHwAddress",<br>> > + &error, &reply, "s", device);<br>> > + if (r < 0)<br>> > + {<br>> > + fprintf(stderr, "Failed to call GetHwAddress: %s\n", <br>> strerror(-r));<br>> > + rc = -1;<br>> > + goto finish;<br>> > }<br>> > r = sd_bus_message_read(reply, "s", &eaddr1);<br>> > - if (r < 0) {<br>> > + if (r < 0)<br>> > + {<br>> > fprintf(stderr, "Failed to get a response: %s", strerror(-r));<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > + rc = IPMI_CC_RESPONSE_ERROR;<br>> > + goto finish;<br>> > }<br>> > if (eaddr1 == NULL)<br>> > {<br>> > fprintf(stderr, "Failed to get a valid response: %s",<br>> strerror(-r));<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > + rc = IPMI_CC_RESPONSE_ERROR;<br>> > + goto finish;<br>> > }<br>> > <br>> > memcpy((void*)&buf[0], ¤t_revision, 1);<br>> > @@ -354,7 +324,8 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t<br>> netfn, ipmi_cmd_t cmd,<br>> > if (digit == NULL)<br>> > {<br>> > fprintf(stderr, "Unexpected MAC format: %s", eaddr1);<br>> > - return IPMI_CC_RESPONSE_ERROR;<br>> > + rc = IPMI_CC_RESPONSE_ERROR;<br>> > + goto finish;<br>> > }<br>> > <br>> > i=0;<br>> > @@ -369,14 +340,18 @@ ipmi_ret_t <br>> ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > *data_len = sizeof(buf);<br>> > memcpy(response, &buf, *data_len);<br>> > <br>> > - return IPMI_CC_OK;<br>> > + rc = IPMI_CC_OK;<br>> > }<br>> > else<br>> > {<br>> > fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> > - return IPMI_CC_PARM_NOT_SUPPORTED;<br>> > + rc = IPMI_CC_PARM_NOT_SUPPORTED;<br>> > }<br>> > <br>> > +finish:<br>> > + sd_bus_error_free(&error);<br>> > + reply = sd_bus_message_unref(reply);<br>> > +<br>> > return rc;<br>> > }<br>> > <br>> <br></tt><BR>
</body></html>