[PATCH phosphor-host-ipmid v4] memory leak found during multiple reboots. it was found that in many cases the reply was not unref'ed

Cyril Bur cyrilbur at gmail.com
Thu Mar 10 11:09:54 AEDT 2016


On Wed, 9 Mar 2016 11:23:01 +0530
"Vishwanatha Subbanna" <vishwanath at in.ibm.com> wrote:

> Thanks for the comments.. I responded inline.
> 
> Thanks
> 
> -------------------------------------------------------------------------------------
> 
> Thanks and Regards,
> Vishwanath.
> Advisory Software Engineer,
> Power Firmware Development,
> Systems &Technology Lab,
> MG2-6F-255 , Manyata Embassy Business Park,
> Bangalore , KA , 560045
> Ph: +91-80-46678255
> E-mail: vishwanath at in.ibm.com
> ----------------------------------------------------------------------------------
> 
> Cyril Bur <cyrilbur at gmail.com> wrote on 09/03/2016 04:54:11 am:
> 
> > From: Cyril Bur <cyrilbur at gmail.com>
> > To: Vishwanatha Subbanna/India/IBM at IBMIN
> > Cc: OpenBMC Patches <openbmc-patches at stwcx.xyz>, openbmc at lists.ozlabs.org
> > Date: 09/03/2016 04:54 am
> > Subject: Re: [PATCH phosphor-host-ipmid v4] memory leak found during
> > multiple reboots. it was found that in many cases the reply was not  
> unref'ed
> >
> > On Wed,  2 Mar 2016 00:40:28 -0600
> > OpenBMC Patches <openbmc-patches at stwcx.xyz> wrote:
> >  
> > > From: vishwa <vishwanath at in.ibm.com>
> > >
> > > --signed off by vishwanath at in.ibm.com---
> > > ---
> > >  apphandler.C       | 150 ++++++++++++++++++----------------------
> > >  host-services.c    |   2 +-
> > >  ipmid.C            |  53 +++++++-------
> > >  sensorhandler.C    |   2 +-
> > >  storageaddsel.C    |   4 +-
> > >  transporthandler.C | 199 ++++++++++++++++++++++  
> > +------------------------------  
> > >  6 files changed, 182 insertions(+), 228 deletions(-)
> > >
> > > diff --git a/apphandler.C b/apphandler.C
> > > index a48059f..a921643 100644
> > > --- a/apphandler.C
> > > +++ b/apphandler.C
> > > @@ -119,11 +119,22 @@ ipmi_ret_t  
> > ipmi_app_get_device_guid(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      const char  *objname = "/org/openbmc/control/chassis0";
> > >      const char  *iface = "org.freedesktop.DBus.Properties";
> > >      const char  *chassis_iface = "org.openbmc.control.Chassis";
> > > -    sd_bus_message *reply = NULL, *m = NULL;
> > > +    sd_bus_message *reply = NULL;
> > >      sd_bus_error error = SD_BUS_ERROR_NULL;
> > >      int r = 0;
> > >      char *uuid = NULL;
> > >
> > > +    // UUID is in RFC4122 format. Ex:  
> 61a39523-78f2-11e5-9862-e6402cfc3223
> > > +    // Per IPMI Spec 2.0 need to convert to 16 hex bytes and  
> > reverse the byte order  
> > > +    // Ex: 0x2332fc2c40e66298e511f2782395a361
> > > +
> > > +    const int resp_size = 16; // Response is 16 hex bytes per IPMI  
> Spec
> > > +    uint8_t resp_uuid[resp_size]; // Array to hold the formatted  
> response
> > > +    int resp_loc = resp_size-1; // Point resp end of array to  
> > save in reverse order  
> > > +    int i = 0;
> > > +    char *tokptr = NULL;
> > > +    char *id_octet = NULL;
> > > +
> > >      // Status code.
> > >      ipmi_ret_t rc = IPMI_CC_OK;
> > >      *data_len = 0;
> > > @@ -131,49 +142,33 @@ ipmi_ret_t  
> > ipmi_app_get_device_guid(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      printf("IPMI GET DEVICE GUID\n");
> > >
> > >      // Call Get properties method with the interface and property name
> > > -    r = sd_bus_message_new_method_call  
> (bus,&m,busname,objname,iface,"Get");
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to add the Get method object: %s  
> > \n", strerror(-r));  
> > > -        return IPMI_CC_UNSPECIFIED_ERROR;
> > > -    }
> > > -    r = sd_bus_message_append(m, "ss", chassis_iface, "uuid");
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to append arguments: %s\n", strerror  
> (-r));
> > > -        return -1;
> > > -    }
> > > -    r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to call the Get method: %s\n",  
> > strerror(-r));  
> > > -        return IPMI_CC_UNSPECIFIED_ERROR;
> > > +    r = sd_bus_call_method(bus,busname,objname,iface,
> > > +                           "Get",&error, &reply, "ss",
> > > +                           chassis_iface, "uuid");
> > > +    if (r < 0)
> > > +    {
> > > +        fprintf(stderr, "Failed to call Get Method: %s\n", strerror  
> (-r));
> > > +        rc = IPMI_CC_UNSPECIFIED_ERROR;
> > > +        goto finish;
> > >      }
> > > +  
> >
> > The above looks like a rework, not really addressing a memory leak
> > specifically? The rework looks good to me but can we please start  
> splitting
> > patches up and do what the commit message says in each patch.
> >
> > This series would have been better with (at least) the above being in one
> > commit entitled something like "Rework ipmi_app_get_device_guid() dbud  
> message
> > handling" and then an explanation as to why this way is better.
> >  
> 
> 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 ?
> 

Sounds like your commit message probably should have been about the rework to
address the currently inefficient way of doing things and the leakiness.

Not a super big deal really but it's just when I read a commit message that
starts with "memory leak" I expect to see just fixing of memory leaks....

> > >      r = sd_bus_message_read(reply, "v", "s", &uuid);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to get a response: %s", strerror(-r));
> > > -        return IPMI_CC_RESPONSE_ERROR;
> > > -    }
> > > -    if (uuid == NULL)
> > > +    if (r < 0 || uuid == NULL)
> > >      {
> > > -        fprintf(stderr, "Failed to get a valid response: %s",  
> > strerror(-r));  
> > > -        return IPMI_CC_RESPONSE_ERROR;
> > > +        fprintf(stderr, "Failed to get a response: %s", strerror(-r));
> > > +        rc = IPMI_CC_RESPONSE_ERROR;
> > > +        goto finish;
> > >      }
> > >
> > > -    // UUID is in RFC4122 format. Ex:  
> 61a39523-78f2-11e5-9862-e6402cfc3223
> > > -    // Per IPMI Spec 2.0 need to convert to 16 hex bytes and  
> > reverse the byte order  
> > > -    // Ex: 0x2332fc2c40e66298e511f2782395a361
> > > -
> > > -    const int resp_size = 16; // Response is 16 hex bytes per IPMI  
> Spec
> > > -    uint8_t resp_uuid[resp_size]; // Array to hold the formatted  
> response
> > > -    int resp_loc = resp_size-1; // Point resp end of array to  
> > save in reverse order  
> > > -    int i = 0;
> > > -    char *tokptr = NULL;
> > > -
> > >      // Traverse the UUID
> > > -    char* id_octet = strtok_r(uuid, "-", &tokptr); // Get the  
> > UUID octects separated by dash  
> > > +    id_octet = strtok_r(uuid, "-", &tokptr); // Get the UUID  
> > octects separated by dash  
> > >
> > >      if (id_octet == NULL)
> > > -    { // Error
> > > +    {
> > > +        // Error
> > >          fprintf(stderr, "Unexpected UUID format: %s", uuid);
> > > -        return IPMI_CC_RESPONSE_ERROR;
> > > +        rc = IPMI_CC_RESPONSE_ERROR;
> > > +        goto finish;
> > >      }
> > >
> > >      while (id_octet != NULL)
> > > @@ -201,8 +196,9 @@ ipmi_ret_t  
> > ipmi_app_get_device_guid(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      // Pack the actual response
> > >      memcpy(response, &resp_uuid, *data_len);
> > >
> > > +finish:
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);
> > >
> > >      return rc;
> > >  }
> > > @@ -246,15 +242,13 @@ ipmi_ret_t  
> > ipmi_app_set_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      const char  *busname = "org.openbmc.watchdog.Host";
> > >      const char  *objname = "/org/openbmc/watchdog/host0";
> > >      const char  *iface = "org.openbmc.Watchdog";
> > > -    sd_bus_message *reply = NULL, *m = NULL;
> > > +    sd_bus_message *reply = NULL;
> > >      sd_bus_error error = SD_BUS_ERROR_NULL;
> > >      int r = 0;
> > >
> > >      set_wd_data_t *reqptr = (set_wd_data_t*) request;
> > >      uint16_t timer = 0;
> > >      uint32_t timer_ms = 0;
> > > -    // Status code.
> > > -    ipmi_ret_t rc = IPMI_CC_OK;
> > >
> > >      *data_len = 0;
> > >
> > > @@ -266,53 +260,45 @@ ipmi_ret_t  
> > ipmi_app_set_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      printf("WATCHDOG SET Timer:[0x%X] 100ms intervals\n",timer);
> > >
> > >      // Set watchdog timer
> > > -    r = sd_bus_message_new_method_call  
> (bus,&m,busname,objname,iface,"set");
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to add the set method object: %s  
> > \n", strerror(-r));  
> > > -        return -1;
> > > -    }
> > > -    r = sd_bus_message_append(m, "i", timer_ms);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to add timer value: %s\n", strerror  
> (-r));
> > > -        return -1;
> > > -    }
> > > -    r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to call the set method: %s\n",  
> > strerror(-r));  
> > > -        return -1;
> > > +    r = sd_bus_call_method(bus, busname, objname, iface,
> > > +                           "set", &error, &reply, "i", timer_ms);
> > > +    if(r < 0)
> > > +    {
> > > +        fprintf(stderr, "Failed to call the SET method: %s\n",  
> > strerror(-r));  
> > > +        goto finish;
> > >      }
> > >
> > > +    sd_bus_error_free(&error);
> > > +    reply = sd_bus_message_unref(reply);
> > > +  
> >
> > So here you're sort of reworking AND fixing the leaks.
> > In terms of fixing the leaks can't you just pass NULL instead of
> > reply? I could
> > be wrong here but I believe that sd_bus will handle passing NULL and  
> realise
> > that you don't care about the reply (which, AFAICT, you don't).
> >  
> 
> Sorry but I don't understand "Pass the NULL instead of reply". Pass the
> NULL where ?.

To sd_bus_call_method(), if you plan to ignore or you don't expect a response
to the method, you can pass NULL as the reply parameter and sd_bus will do what
you want it to do (unref it if it gets one).
See:
http://code.metager.de/source/xref/freedesktop/systemd/systemd/src/libsystemd/sd-bus/sd-bus.c#2014

If you read around there, you can actually do the same with the error parameter.

> The way its done here is per one of the approaches followed in systemd code
> and I followed the same.
> 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.
> 
> from sd-bus/bus-message.C
> ---------------------------
> 
> _public_ sd_bus_message* sd_bus_message_unref(sd_bus_message *m) {
> 
>         if (!m)
>                 return NULL;
> 
>         assert(m->n_ref > 0);
>         m->n_ref--;
> 
>         if (m->n_ref > 0)
>                 return NULL;
> 
>         message_free(m);
>         return NULL;
> }
> 

I responded to some of this below...

> 
> > >      // Stop the current watchdog if any
> > > -    r =  
> > sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"stop");  
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to add the start method object:  
> > %s\n", strerror(-r));  
> > > -        return -1;
> > > -    }
> > > -    r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to call the start method: %s\n",  
> > strerror(-r));  
> > > -        return -1;
> > > +    r = sd_bus_call_method(bus, busname, objname, iface,
> > > +                           "stop", &error, &reply, NULL);
> > > +    if(r < 0)
> > > +    {
> > > +        fprintf(stderr, "Failed to call the STOP method: %s\n",  
> > strerror(-r));  
> > > +        goto finish;
> > >      }
> > >
> > > -    // Start the watchdog if requested
> > >      if (reqptr->t_use & 0x40)
> > >      {
> > > -        r =  
> > sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"start");  
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to add the start method  
> > object: %s\n", strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to call the start method: %s  
> > \n", strerror(-r));  
> > > -            return -1;
> > > +        sd_bus_error_free(&error);
> > > +        reply = sd_bus_message_unref(reply);
> > > +
> > > +        // Start the watchdog if requested
> > > +        r = sd_bus_call_method(bus, busname, objname, iface,
> > > +                               "start", &error, &reply, NULL);
> > > +        if(r < 0)
> > > +        {
> > > +            fprintf(stderr, "Failed to call the START method: %s  
> > \n", strerror(-r));  
> > >          }
> > >      }
> > >
> > > +finish:
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);
> > >
> > > -    return rc;
> > > +    return (r < 0) ? -1 : IPMI_CC_OK;
> > >  }
> > >
> > >
> > > @@ -323,7 +309,7 @@ ipmi_ret_t  
> > ipmi_app_reset_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      const char  *busname = "org.openbmc.watchdog.Host";
> > >      const char  *objname = "/org/openbmc/watchdog/host0";
> > >      const char  *iface = "org.openbmc.Watchdog";
> > > -    sd_bus_message *reply = NULL, *m = NULL;
> > > +    sd_bus_message *reply = NULL;
> > >      sd_bus_error error = SD_BUS_ERROR_NULL;
> > >      int r = 0;
> > >
> > > @@ -334,19 +320,15 @@ ipmi_ret_t  
> > ipmi_app_reset_watchdog(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      printf("WATCHDOG RESET\n");
> > >
> > >      // Refresh watchdog
> > > -    r =  
> > sd_bus_message_new_method_call(bus,&m,busname,objname,iface,"poke");  
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to add the method object: %s\n",  
> > strerror(-r));  
> > > -        return -1;
> > > -    }
> > > -    r = sd_bus_call(bus, m, 0, &error, &reply);
> > > +    r = sd_bus_call_method(bus, busname, objname, iface,
> > > +                           "poke", &error, &reply, NULL);
> > >      if (r < 0) {
> > > -        fprintf(stderr, "Failed to call the method: %s\n", strerror  
> (-r));
> > > -        return -1;
> > > +        fprintf(stderr, "Failed to add reset  watchdog: %s\n",  
> > strerror(-r));  
> > > +        rc = -1;
> > >      }
> > >
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);
> > >
> > >      return rc;
> > >  }
> > > diff --git a/host-services.c b/host-services.c
> > > index 23aa55e..cc47439 100644
> > > --- a/host-services.c
> > > +++ b/host-services.c
> > > @@ -68,7 +68,7 @@ static int soft_power_off(sd_bus_message *m,  
> > void *userdata, sd_bus_error *ret_e  
> > >
> > >  finish:
> > >      sd_bus_error_free(&bus_error);
> > > -    sd_bus_message_unref(response);
> > > +    response = sd_bus_message_unref(response);
> > >
> > >      if(rc < 0)
> > >      {
> > > diff --git a/ipmid.C b/ipmid.C
> > > index 0f4139c..06fbf05 100644
> > > --- a/ipmid.C
> > > +++ b/ipmid.C
> > > @@ -225,9 +225,8 @@ static int send_ipmi_message(sd_bus_message  
> > *req, unsigned char seq, unsigned ch  
> > >
> > >  final:
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > -    sd_bus_message_unref(reply);
> > > -
> > > +    m = sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);  
> >
> > Why this change? You don't check m or reply/...  the unref already
> > happens. The
> > only thing I can think that you're doing is fixing warnings (in which  
> case,
> > separate patch and please say so) but quite frankly you shouldn't be  
> enabling
> > those warnings, they're a pain.
> >  
> 
> I agree I am not consuming it here / passing the pointer back.. but it was
> more of a good practise that I was following.
> I am not sure which warning are you saying is a pain ?. Do you have an
> example ? -or- are you generally saying
> compiler should not warn for these types of message usages ?.
> 

You can get GCC to warn you about ignoring the return values of functions which
can be useful but in practice just gets in the way, I was wondering if you were
trying to avoid this message.

So I agree with you that it is wise to sanitise pointers. However, when the
pointer if function local this precaution is kind of overkill, anyone modifying
this function won't be reusing that pointer and if they do well they probably
shouldn't be working on it because they clearly don't understand whats going
on, also, this is why we have code review.

What I think when I see those lines is "Oh he has the intention of using the
return value of sd_bus_message_unref(). Wait, what? The function returns right
away, why did he do that?"...

> BTW, I did not fix any warnings.. That code was just being futuristic.
> 
> > >
> > >      return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
> > >  }
> > > @@ -246,13 +245,13 @@ static int  
> > handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error  
> > >      r = sd_bus_message_read(m, "yyyy",  &sequence, &netfn, &lun,  
> &cmd);
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to parse signal message: %s\n",  
> > strerror(-r));  
> > > -        return -1;
> > > +        goto final;
> > >      }
> > >
> > >      r = sd_bus_message_read_array(m, 'y',  &request, &sz );
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to parse signal message: %s\n",  
> > strerror(-r));  
> > > -        return -1;
> > > +        goto final;
> > >      }
> > >
> > >      fprintf(ipmiio, "IPMI Incoming: Seq 0x%02x, NetFn 0x%02x,  
> > CMD: 0x%02x \n", sequence, netfn, cmd);  
> > > @@ -268,6 +267,7 @@ static int handle_ipmi_command(sd_bus_message  
> > *m, void *user_data, sd_bus_error  
> > >      if(r != 0)
> > >      {
> > >          fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:  
> > [0x%X]\n",r, netfn, cmd);  
> > > +        goto final;
> > >      }
> > >
> > >      fprintf(ipmiio, "IPMI Response:\n");
> > > @@ -278,11 +278,12 @@ static int  
> > handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error  
> > >            ((unsigned char *)response) + 1, resplen - 1);
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to send the response message\n");
> > > -        return -1;
> > >      }
> > >
> > > +final:
> > > +    m = sd_bus_message_unref(m);
> > >
> > > -    return 0;
> > > +    return (r < 0) ? -1 : 0;
> > >  }
> > >
> > >
> > > @@ -470,6 +471,7 @@ int  
> > find_interface_property_fru_type(dbus_interface_t *interface, const char  
> *pr
> > >          fprintf(stderr, "Failed to create a method call: %s",  
> > strerror(-r));  
> > >          fprintf(stderr,"Bus: %s Path: %s Interface: %s \n",
> > >                  interface->bus, interface->path, interface->  
> interface);
> > > +        goto final;
> > >      }
> > >
> > >      r = sd_bus_message_append(m, "ss",  
> > "org.openbmc.InventoryItem", property_name);  
> > > @@ -477,6 +479,7 @@ int  
> > find_interface_property_fru_type(dbus_interface_t *interface, const char  
> *pr
> > >          fprintf(stderr, "Failed to create a input parameter: %s",  
> > strerror(-r));  
> > >          fprintf(stderr,"Bus: %s Path: %s Interface: %s \n",
> > >                  interface->bus, interface->path, interface->  
> interface);
> > > +        goto final;
> > >      }
> > >
> > >      r = sd_bus_call(bus, m, 0, &error, &reply);
> > > @@ -496,7 +499,8 @@ int  
> > find_interface_property_fru_type(dbus_interface_t *interface, const char  
> *pr
> > >  final:
> > >
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    m = sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);
> > >
> > >      return r;
> > >  }
> > > @@ -512,29 +516,18 @@ int find_openbmc_path(const char *type,  
> > const uint8_t num, dbus_interface_t *int  
> > >
> > >      char  *str1, *str2, *str3;
> > >      sd_bus_error error = SD_BUS_ERROR_NULL;
> > > -    sd_bus_message *reply = NULL, *m=NULL;
> > > +    sd_bus_message *reply = NULL;
> > >
> > >
> > >      int r;
> > >
> > > -    r =  
> > sd_bus_message_new_method_call  
> (bus,&m,busname,objname,busname,"getObjectFromByteId");
> > > +    r = sd_bus_call_method(bus,busname,objname,busname,  
> > "getObjectFromByteId",  
> > > +                           &error, &reply, "sy", type, num);
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to create a method call: %s",  
> > strerror(-r));  
> > > -    }
> > > -
> > > -    r = sd_bus_message_append(m, "sy", type, num);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to create a input parameter: %s",  
> > strerror(-r));  
> > > -    }
> > > -
> > > -    // Call the IPMI responder on the bus so the message can be  
> > sent to the CEC  
> > > -    r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -    if (r < 0) {
> > > -        fprintf(stderr, "Failed to call the method: %s", strerror  
> (-r));
> > >          goto final;
> > >      }
> > >
> > > -
> > >      r = sd_bus_message_read(reply, "(sss)", &str1, &str2, &str3);
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to get a response: %s", strerror(-r));
> > > @@ -550,7 +543,7 @@ int find_openbmc_path(const char *type, const  
> > uint8_t num, dbus_interface_t *int  
> > >  final:
> > >
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);
> > >
> > >      return r;
> > >  }
> > > @@ -577,11 +570,13 @@ int set_sensor_dbus_state_s(uint8_t number,  
> > const char *method, const char *valu  
> > >      r =  
> > sd_bus_message_new_method_call(bus,&m,a.bus,a.path,a.interface,method);  
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to create a method call: %s",  
> > strerror(-r));  
> > > +        goto final;
> > >      }
> > >
> > >      r = sd_bus_message_append(m, "v", "s", value);
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to create a input parameter: %s",  
> > strerror(-r));  
> > > +        goto final;
> > >      }
> > >
> > >
> > > @@ -590,9 +585,9 @@ int set_sensor_dbus_state_s(uint8_t number,  
> > const char *method, const char *valu  
> > >          fprintf(stderr, "Failed to call the method: %s", strerror  
> (-r));
> > >      }
> > >
> > > -
> > > +final:
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    m = sd_bus_message_unref(m);
> > >
> > >      return 0;
> > >  }
> > > @@ -612,11 +607,13 @@ int set_sensor_dbus_state_y(uint8_t number,  
> > const char *method, const uint8_t va  
> > >      r =  
> > sd_bus_message_new_method_call(bus,&m,a.bus,a.path,a.interface,method);  
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to create a method call: %s",  
> > strerror(-r));  
> > > +        goto final;
> > >      }
> > >
> > >      r = sd_bus_message_append(m, "v", "y", value);
> > >      if (r < 0) {
> > >          fprintf(stderr, "Failed to create a input parameter: %s",  
> > strerror(-r));  
> > > +        goto final;
> > >      }
> > >
> > >
> > > @@ -625,9 +622,9 @@ int set_sensor_dbus_state_y(uint8_t number,  
> > const char *method, const uint8_t va  
> > >          fprintf(stderr, "12 Failed to call the method: %s", strerror  
> (-r));
> > >      }
> > >
> > > -
> > > +final:
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > +    m = sd_bus_message_unref(m);
> > >
> > >      return 0;
> > > -}
> > > \ No newline at end of file
> > > +}
> > > diff --git a/sensorhandler.C b/sensorhandler.C
> > > index d171bf5..90bfd0f 100644
> > > --- a/sensorhandler.C
> > > +++ b/sensorhandler.C
> > > @@ -221,7 +221,7 @@ ipmi_ret_t  
> > ipmi_sen_get_sensor_reading(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      }
> > >
> > >
> > > -    sd_bus_message_unref(reply);
> > > +    reply = sd_bus_message_unref(reply);
> > >
> > >      return rc;
> > >  }
> > > diff --git a/storageaddsel.C b/storageaddsel.C
> > > index 51edc59..cb90aab 100644
> > > --- a/storageaddsel.C
> > > +++ b/storageaddsel.C
> > > @@ -193,8 +193,8 @@ int send_esel_to_dbus(const char *desc, const  
> > char *sev, const char *details, ui  
> > >
> > >  finish:
> > >      sd_bus_error_free(&error);
> > > -    sd_bus_message_unref(m);
> > > -    sd_bus_message_unref(reply);
> > > +    m = sd_bus_message_unref(m);
> > > +    reply = sd_bus_message_unref(reply);
> > >      return r;
> > >  }
> > >
> > > diff --git a/transporthandler.C b/transporthandler.C
> > > index d43f1f9..0fd25f9 100644
> > > --- a/transporthandler.C
> > > +++ b/transporthandler.C
> > > @@ -55,7 +55,7 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t  
> > netfn, ipmi_cmd_t cmd,  
> > >      ipmi_ret_t rc = IPMI_CC_OK;
> > >      *data_len = 0;
> > >      sd_bus *bus = ipmid_get_sd_bus_connection();
> > > -    sd_bus_message *reply = NULL, *m = NULL;
> > > +    sd_bus_message *reply = NULL;
> > >      sd_bus_error error = SD_BUS_ERROR_NULL;
> > >      int r = 0;
> > >
> > > @@ -84,20 +84,10 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t  
> > netfn, ipmi_cmd_t cmd,  
> > >                  reqptr->data[4],
> > >                  reqptr->data[5]);
> > >
> > > -        r =  
> > sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"SetHwAddress");  
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to add method object: %s\n",  
> > strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_message_append(m, "s", mac);
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to append message data: %s  
> > \n", strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_call(bus, m, 0, &error, &reply);
> > > +        r = sd_bus_call_method(bus,app,obj,ifc,"SetHwAddress",
> > > +                               &error, &reply, "s", mac);
> > >          if (r < 0) {
> > >              fprintf(stderr, "Failed to call method: %s\n", strerror  
> (-r));
> > > -            return -1;
> > >          }
> > >      }
> > >      else if (reqptr->parameter == LAN_PARM_SUBNET)
> > > @@ -112,84 +102,72 @@ ipmi_ret_t  
> > ipmi_transport_set_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >      }
> > >      else if (reqptr->parameter == LAN_PARM_INPROGRESS) // Apply config
> > >      {
> > > -        int rc = 0;
> > > -        sd_bus_message *req = NULL;
> > > -        sd_bus_message *res = NULL;
> > > -        sd_bus *bus1        = NULL;
> > > -        sd_bus_error err    = SD_BUS_ERROR_NULL;
> > > -
> > >          if (!strcmp(new_ipaddr, "") || !strcmp (new_netmask, "")  
> > || !strcmp (new_gateway, ""))  
> > >          {
> > >              fprintf(stderr,"ERROR: Incomplete LAN Parameters\n");
> > >              return -1;
> > >          }
> > > -
> > > -        rc = sd_bus_open_system(&bus1);
> > > -        if(rc < 0)
> > > -        {
> > > -            fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");
> > > -            return -1;
> > > -        }
> > >
> > >          if (strcmp(cur_ipaddr, ""))
> > >          {
> > > -            sd_bus_error_free(&err);
> > > -            sd_bus_message_unref(req);
> > > -            sd_bus_message_unref(res);
> > > -
> > > -            rc = sd_bus_call_method(bus1,            // On the System  
> Bus
> > > -                                    app,            // Service to  
> contact
> > > -                                    obj,            // Object path
> > > -                                    ifc,            // Interface name
> > > -                                    "DelAddress4",  // Method to be  
> called
> > > -                                    &err,           // object to  
> > return error  
> > > -                                    &res,           // Response  
> > message on success  
> > > -                                    "ssss",         // input  
> > message (dev,ip,nm,gw)  
> > > -                                    "eth0",
> > > -                                    cur_ipaddr,
> > > -                                    cur_netmask,
> > > -                                    cur_gateway);
> > > +            r = sd_bus_call_method(bus,           // On the System Bus
> > > +                                   app,            // Service to  
> contact
> > > +                                   obj,            // Object path
> > > +                                   ifc,            // Interface name
> > > +                                   "DelAddress4",  // Method to be  
> called
> > > +                                   &error,         // object to  
> > return error  
> > > +                                   &reply,         // Response  
> > message on success  
> > > +                                   "ssss",         // input  
> > message (dev,ip,nm,gw)  
> > > +                                   "eth0",
> > > +                                   cur_ipaddr,
> > > +                                   cur_netmask,
> > > +                                   cur_gateway);
> > >          }
> > >
> > > -        if(rc < 0)
> > > +        if(r < 0)
> > >          {
> > > -            fprintf(stderr, "Failed to remove existing IP %s: %s  
> > \n", cur_ipaddr, err.message);  
> > > -            return -1;
> > > +            fprintf(stderr, "Failed to remove existing IP %s: %s  
> > \n", cur_ipaddr, error.message);  
> > > +            goto finish;
> > >          }
> > >
> > > -        sd_bus_error_free(&err);
> > > -        sd_bus_message_unref(req);
> > > -        sd_bus_message_unref(res);
> > > -
> > > -        rc = sd_bus_call_method(bus1,            // On the System Bus
> > > -                                app,            // Service to contact
> > > -                                obj,            // Object path
> > > -                                ifc,            // Interface name
> > > -                                "AddAddress4",  // Method to be called
> > > -                                &err,           // object to return  
> error
> > > -                                &res,           // Response  
> > message on success  
> > > -                                "ssss",         // input message  
> > (dev,ip,nm,gw)  
> > > -                                "eth0",
> > > -                                new_ipaddr,
> > > -                                new_netmask,
> > > -                                new_gateway);
> > > -        if(rc < 0)
> > > +        sd_bus_error_free(&error);
> > > +        reply = sd_bus_message_unref(reply);
> > > +
> > > +        r = sd_bus_call_method(bus,            // On the System Bus
> > > +                               app,            // Service to contact
> > > +                               obj,            // Object path
> > > +                               ifc,            // Interface name
> > > +                               "AddAddress4",  // Method to be called
> > > +                               &error,         // object to return  
> error
> > > +                               &reply,         // Response  
> > message on success  
> > > +                               "ssss",         // input message  
> > (dev,ip,nm,gw)  
> > > +                               "eth0",
> > > +                               new_ipaddr,
> > > +                               new_netmask,
> > > +                               new_gateway);
> > > +        if(r < 0)
> > >          {
> > > -            fprintf(stderr, "Failed to set IP %s: %s\n",  
> > new_ipaddr, err.message);  
> > > -            return -1;
> > > +            fprintf(stderr, "Failed to set IP %s: %s\n",  
> > new_ipaddr, error.message);  
> > > +        }
> > > +        else
> > > +        {
> > > +            strcpy (cur_ipaddr, new_ipaddr);
> > > +            strcpy (cur_netmask, new_netmask);
> > > +            strcpy (cur_gateway, new_gateway);
> > >          }
> > > -
> > > -        strcpy (cur_ipaddr, new_ipaddr);
> > > -        strcpy (cur_netmask, new_netmask);
> > > -        strcpy (cur_gateway, new_gateway);
> > >      }
> > >      else
> > >      {
> > >          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->  
> parameter);
> > > -        return IPMI_CC_PARM_NOT_SUPPORTED;
> > > +        rc = IPMI_CC_PARM_NOT_SUPPORTED;
> > >      }
> > >
> > > -    return rc;
> > > +finish:
> > > +    // Clenaup the resources allocated reply and error
> > > +    sd_bus_error_free(&error);
> > > +    reply = sd_bus_message_unref(reply);
> > > +
> > > +    return (r < 0) ? -1 : rc;
> > >  }
> > >
> > >  struct get_lan_t {
> > > @@ -206,7 +184,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t  
> > netfn, ipmi_cmd_t cmd,  
> > >      ipmi_ret_t rc = IPMI_CC_OK;
> > >      *data_len = 0;
> > >      sd_bus *bus = ipmid_get_sd_bus_connection();
> > > -    sd_bus_message *reply = NULL, *m = NULL;
> > > +    sd_bus_message *reply = NULL;
> > >      sd_bus_error error = SD_BUS_ERROR_NULL;
> > >      int r = 0;
> > >      const uint8_t current_revision = 0x11; // Current rev per IPMI  
> Spec 2.0
> > > @@ -259,32 +237,27 @@ ipmi_ret_t  
> > ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >          const char*         device             = "eth0";
> > >          uint8_t buf[5]; // Size of expected IPMI response msg
> > >
> > > -        r =  
> > sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetAddress4");  
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to add method object: %s\n",  
> > strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_message_append(m, "s", device);
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to append message data: %s  
> > \n", strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -        if (r < 0) {
> > > +        r = sd_bus_call_method(bus,app,obj,ifc,"GetAddress4",
> > > +                               &error, &reply, "s", device);
> > > +        if (r < 0)
> > > +        {
> > >              fprintf(stderr, "Failed to call method: %s\n", strerror  
> (-r));
> > > -            return -1;
> > > +            rc = -1;
> > > +            goto finish;
> > >          }
> > > -        rc = sd_bus_message_enter_container (reply, 'a', "(iyyus)");
> > > -        if(rc < 0)
> > > +        r = sd_bus_message_enter_container (reply, 'a', "(iyyus)");
> > > +        if(r < 0)
> > >          {
> > >              fprintf(stderr, "Failed to parse response message:  
> > [%s]\n", strerror(-rc));  
> > > -            return -1;
> > > +            rc = -1;
> > > +            goto finish;
> > >          }
> > > -        rc = sd_bus_message_read(reply, "(iyyus)", &family,  
> > &prefixlen, &scope, &flags, &saddr);  
> > > -        if (rc < 0)
> > > +        r = sd_bus_message_read(reply, "(iyyus)", &family,  
> > &prefixlen, &scope, &flags, &saddr);  
> > > +        if (r < 0)
> > >          {
> > >              fprintf(stderr, "Failed to receive response: %s\n",  
> > strerror(-r));  
> > > -            return -1;
> > > +            rc = -1;
> > > +            goto finish;
> > >          }
> > >
> > >          printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6",  
> > prefixlen, scope, flags, saddr);  
> > > @@ -297,7 +270,8 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t  
> > netfn, ipmi_cmd_t cmd,  
> > >          if (digit == NULL)
> > >          {
> > >              fprintf(stderr, "Unexpected IP format: %s", saddr);
> > > -            return IPMI_CC_RESPONSE_ERROR;
> > > +            rc = IPMI_CC_RESPONSE_ERROR;
> > > +            goto finish;
> > >          }
> > >          i = 0;
> > >          while (digit != NULL)
> > > @@ -311,7 +285,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t  
> > netfn, ipmi_cmd_t cmd,  
> > >          *data_len = sizeof(buf);
> > >          memcpy(response, &buf, *data_len);
> > >
> > > -        return IPMI_CC_OK;
> > > +        rc = IPMI_CC_OK;
> > >      }
> > >      else if (reqptr->parameter == LAN_PARM_MAC)
> > >      {
> > > @@ -321,30 +295,26 @@ ipmi_ret_t  
> > ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >          uint8_t             buf[7];
> > >          char *eaddr1 = NULL;
> > >
> > > -        r =  
> > sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetHwAddress");  
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to add method object: %s\n",  
> > strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_message_append(m, "s", device);
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to append message data: %s  
> > \n", strerror(-r));  
> > > -            return -1;
> > > -        }
> > > -        r = sd_bus_call(bus, m, 0, &error, &reply);
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "Failed to call method: %s\n", strerror  
> (-r));
> > > -            return -1;
> > > +        r = sd_bus_call_method(bus,app,obj,ifc,"GetHwAddress",
> > > +                               &error, &reply, "s", device);
> > > +        if (r < 0)
> > > +        {
> > > +            fprintf(stderr, "Failed to call GetHwAddress: %s\n",  
> > strerror(-r));  
> > > +            rc = -1;
> > > +            goto finish;
> > >          }
> > >          r = sd_bus_message_read(reply, "s", &eaddr1);
> > > -        if (r < 0) {
> > > +        if (r < 0)
> > > +        {
> > >              fprintf(stderr, "Failed to get a response: %s", strerror  
> (-r));
> > > -            return IPMI_CC_RESPONSE_ERROR;
> > > +            rc = IPMI_CC_RESPONSE_ERROR;
> > > +            goto finish;
> > >          }
> > >          if (eaddr1 == NULL)
> > >          {
> > >              fprintf(stderr, "Failed to get a valid response: %s",  
> > strerror(-r));  
> > > -            return IPMI_CC_RESPONSE_ERROR;
> > > +            rc = IPMI_CC_RESPONSE_ERROR;
> > > +            goto finish;
> > >          }
> > >
> > >          memcpy((void*)&buf[0], &current_revision, 1);
> > > @@ -354,7 +324,8 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t  
> > netfn, ipmi_cmd_t cmd,  
> > >          if (digit == NULL)
> > >          {
> > >              fprintf(stderr, "Unexpected MAC format: %s", eaddr1);
> > > -            return IPMI_CC_RESPONSE_ERROR;
> > > +            rc = IPMI_CC_RESPONSE_ERROR;
> > > +            goto finish;
> > >          }
> > >
> > >          i=0;
> > > @@ -369,14 +340,18 @@ ipmi_ret_t  
> > ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,  
> > >          *data_len = sizeof(buf);
> > >          memcpy(response, &buf, *data_len);
> > >
> > > -        return IPMI_CC_OK;
> > > +        rc = IPMI_CC_OK;
> > >      }
> > >      else
> > >      {
> > >          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->  
> parameter);
> > > -        return IPMI_CC_PARM_NOT_SUPPORTED;
> > > +        rc = IPMI_CC_PARM_NOT_SUPPORTED;
> > >      }
> > >
> > > +finish:
> > > +    sd_bus_error_free(&error);
> > > +    reply = sd_bus_message_unref(reply);
> > > +
> > >      return rc;
> > >  }
> > >  
> >  



More information about the openbmc mailing list