<html><body><p>The intent of this patch is to *replace* the popen () calls with dbus api calls.<br><br>The functional equivalent to add an IP address (previously a system call to ip addr add ...) would be:<br><br><tt>rc = </tt><tt><b>sd_bus_call_method</b></tt><tt>(bus, // On the System Bus<br>> + app, // Service to contact<br>> + obj, // Object path <br>> + ifc, // Interface name<br>> + "</tt><tt><b>AddAddress4</b></tt><tt>", // Method to be called<br>> + &err, // object to return error<br>> + &res, // Response message on success<br>> + "ssss", // input message (dev,ip,nm,gw)<br>> + "eth0",<br>> + new_ipaddr,<br>> + new_netmask,<br>> + new_gateway);</tt><br><br>hth,<br>rhari !<br><br>Hariharasubramanian R.<br>Power Firmware Development<br>IBM India Systems & Technology Lab, Bangalore, India<br>Phone: +91 80 4025 6950 <br><br><img width="16" height="16" src="cid:1__=8FBBF5D2DF8897A88f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for Cyril Bur ---01/21/2016 10:18:35 AM---On Wed, 20 Jan 2016 07:20:28 -0600 OpenBMC Patches <openbmc-pat"><font color="#424282">Cyril Bur ---01/21/2016 10:18:35 AM---On Wed, 20 Jan 2016 07:20:28 -0600 OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:</font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">Cyril Bur <cyrilbur@gmail.com></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">OpenBMC Patches <openbmc-patches@stwcx.xyz></font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">openbmc@lists.ozlabs.org</font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">01/21/2016 10:18 AM</font><br><font size="2" color="#5F5F5F">Subject: </font><font size="2">Re: [PATCH phosphor-host-ipmid] Using systemd dbus API to set get LAN parameters.</font><br><font size="2" color="#5F5F5F">Sent by: </font><font size="2">"openbmc" <openbmc-bounces+hramasub=in.ibm.com@lists.ozlabs.org></font><br><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br><br><tt>On Wed, 20 Jan 2016 07:20:28 -0600<br>OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br><br>Hi Hariharasubramanian,<br><br>I have a few questions about this patch.<br><br><br>Firstly, I notice you've removed all the calls to popen() and I can't see any<br>functional equivalent. Unfortunately there is no commit message so I don't know<br>if you meant to remove them and not replace them. Thought I had to check.<br><br><br>> From: Hariharasubramanian R <hramasub@in.ibm.com><br>> <br>> ---<br>> transporthandler.C | 233 +++++++++++++++++++++++++++++++++++++++++++----------<br>> 1 file changed, 191 insertions(+), 42 deletions(-)<br>> <br>> diff --git a/transporthandler.C b/transporthandler.C<br>> index 0f7a730..dac8fe8 100644<br>> --- a/transporthandler.C<br>> +++ b/transporthandler.C<br>> @@ -1,11 +1,31 @@<br>> #include <stdio.h><br>> #include <string.h><br>> #include <stdint.h><br>> +#include <arpa/inet.h><br>> <br>> #include "ipmid-api.h"<br>> #include "ipmid.H"<br>> #include "transporthandler.h"<br>> <br>> +#define SYSTEMD_NETWORKD_DBUS 1<br>> +<br>> +#ifdef SYSTEMD_NETWORKD_DBUS<br>> +#include <systemd/sd-bus.h><br>> +#endif<br><br>Why even have the SYSTEMD_NETWORKD_DBUS?<br><br>> +<br>> +// OpenBMC System Manager dbus framework<br>> +const char *app = "org.openbmc.NetworkManager";<br>> +const char *obj = "/org/openbmc/NetworkManager/Interface";<br>> +const char *ifc = "org.openbmc.NetworkManager";<br>> +<br>> +char cur_ipaddr [16] = "";<br>> +char cur_netmask [16] = "";<br>> +char cur_gateway [16] = "";<br>> +<br>> +char new_ipaddr [16] = "";<br>> +char new_netmask [16] = "";<br>> +char new_gateway [16] = "";<br>> +<br><br>Why are you declaring these globally?<br><br>> void register_netfn_transport_functions() __attribute__((constructor));<br>> <br>> ipmi_ret_t ipmi_transport_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> @@ -31,7 +51,6 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> {<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> *data_len = 0;<br>> - char syscmd[128];<br>> <br>> printf("IPMI SET_LAN\n");<br>> <br>> @@ -43,18 +62,88 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> <br>> if (reqptr->parameter == 3) // IP<br>> {<br>> - sprintf(syscmd, "ifconfig eth0 %d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);<br>> - system(syscmd);<br>> + sprintf(new_ipaddr, "%d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);<br>> }<br>> else if (reqptr->parameter == 6) // Subnet<br>> {<br>> - sprintf(syscmd, "ifconfig eth0 netmask %d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);<br>> - system(syscmd);<br>> + sprintf(new_netmask, "%d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);<br>> }<br>> else if (reqptr->parameter == 12) // Gateway<br>> {<br>> - sprintf(syscmd, "route add default gw %d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);<br>> - system(syscmd);<br>> + sprintf(new_gateway, "%d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);<br>> + }<br>> + else if (reqptr->parameter == 0) // Apply config<br>> + {<br>> + int rc = 0;<br>> + sd_bus_message *req = NULL;<br>> + sd_bus_message *res = NULL;<br>> + sd_bus *bus = NULL;<br>> + sd_bus_error err = SD_BUS_ERROR_NULL;<br>> + <br>> + if (!strcmp(new_ipaddr, "") || !strcmp (new_netmask, "") || !strcmp (new_gateway, ""))<br>> + {<br><br><br><br>> + fprintf(stderr,"ERROR: Incomplete LAN Parameters\n");<br>> + return -1;<br>> + }<br>> + <br>> + rc = sd_bus_open_system(&bus);<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(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>> + &err, // object to return error<br>> + &res, // Response message on success<br>> + "ssss", // input message (dev,ip,nm,gw)<br>> + "eth0",<br>> + cur_ipaddr,<br>> + cur_netmask,<br>> + cur_gateway);<br><br>So here it becomes quite clear that all that (possibly error prone) sprintf<br>earlier was simply so you can send the addresses over dbus.<br><br>Perhaps you chould just pass the reqptr->data[] values as they are?<br><br><br>> + }<br>> +<br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr, "Failed to remove existing IP %s: %s\n", cur_ipaddr, err.message);<br>> + return -1;<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(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>> + &err, // object to return error<br>> + &res, // Response message on success<br>> + "ssss", // input message (dev,ip,nm,gw)<br>> + "eth0",<br>> + new_ipaddr,<br>> + new_netmask,<br>> + new_gateway);<br><br>See my previous comment.<br><br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr, "Failed to set IP %s: %s\n", new_ipaddr, err.message);<br>> + return -1;<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>> @@ -78,10 +167,16 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> {<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> *data_len = 0;<br>> -<br>> + sd_bus_error err = SD_BUS_ERROR_NULL; /* fixme */<br><br>What is going to need fixing exactly?<br><br>> const uint8_t current_revision = 0x11; // Current rev per IPMI Spec 2.0<br>> - char syscmd[128];<br>> - int i = 0;<br>> +<br>> + int family;<br>> + unsigned char prefixlen;<br>> + unsigned char scope;<br>> + unsigned int flags;<br>> + char saddr [128];<br>> + char gateway [128];<br>> + uint8_t buf[11];<br>> <br>> printf("IPMI GET_LAN\n");<br>> <br>> @@ -121,56 +216,111 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> }<br>> else if (reqptr->parameter == 3) // IP<br><br>See my other comment about this<br><br>> {<br>> - //string to parse: inet xx.xx.xxx.xxx/xx<br>> + const char* device = "eth0";<br>> <br>> - uint8_t buf[5];<br>> - memcpy((void*)&buf[0], ¤t_revision, 1);<br>> + sd_bus_message *res = NULL;<br>> + sd_bus *bus = NULL;<br>> + sd_bus_error err = SD_BUS_ERROR_NULL;<br>> +<br>> + rc = sd_bus_open_system(&bus);<br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");<br>> + return -1;<br>> + }<br>> <br>> - for (i=0; i<4; i++)<br>> + rc = sd_bus_call_method(bus, // On the System Bus<br>> + app, // Service to contact<br>> + obj, // Object path <br>> + ifc, // Interface name<br>> + "GetAddress4", // Method to be called<br>> + &err, // object to return error<br>> + &res, // Response message on success<br>> + "s", // input message (dev,ip,nm,gw)<br>> + "eth0");<br>> + if(rc < 0)<br>> {<br>> - char ip[5];<br>> -<br>> - sprintf(syscmd, "ip address show dev eth0|grep inet|cut -d'/' -f1|cut -d' ' -f 6|cut -d'.' -f%d|head -n1", i+1);<br>> - FILE *fp = popen(syscmd, "r");<br>> -<br>> - memset(ip,0,sizeof(ip));<br>> - while (fgets(ip, sizeof(ip), fp) != 0)<br>> - {<br>> - int tmpip = strtoul(ip, NULL, 10);<br>> - memcpy((void*)&buf[i+1], &tmpip, 1);<br>> - }<br>> - pclose(fp);<br>> + fprintf(stderr, "Failed to Get IP on interface : %s\n", device);<br>> + return -1;<br>> + }<br>> +<br>> + /* rc = sd_bus_message_read(res, "a(iyyus)s", ...); */<br><br>Delete that line?<br><br>> + rc = sd_bus_message_enter_container (res, 'a', "(iyyus)");<br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr, "Failed to parse response message:[%s]\n", strerror(-rc));<br>> + return -1;<br>> + }<br>> +<br>> + while ((rc = sd_bus_message_read(res, "(iyyus)", &family, &prefixlen, &scope, &flags, &saddr)) > 0) {<br>> + printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6", prefixlen, scope, flags, saddr);<br>> }<br><br>Again, I bet there is someone sprintf()ing in order to pass saddr as a string<br>requiring you to do a sscanf() just a bit further, this makes no sense!<br><br>> <br>> + rc = sd_bus_message_read (res, "s", &gateway);<br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr, "Failed to parse gateway from response message:[%s]\n", strerror(-rc));<br>> + return -1;<br>> + }<br>> +<br>> + memcpy((void*)&buf[0], ¤t_revision, 1);<br>> + sscanf (saddr, "%c.%c.%c.%c", &buf[1], &buf[2], &buf[3], &buf[4]);<br>> + buf[5] = family;<br>> + buf[6] = prefixlen;<br>> + sscanf (gateway, "%c.%c.%c.%c", &buf[7], &buf[8], &buf[9], &buf[10]);<br>> +<br><br>From man 3 scanf:<br><br> c Matches a sequence of characters whose length is specified by the<br> maximum field width (default 1); the next pointer must be a pointer to<br> char, and there must be enough room for all the charac- ters (no<br> terminating null byte is added). The usual skip of leading white space<br> is suppressed. To skip white space first, use an explicit space in the<br> format.<br><br>To conclude the %c will only match 0-9. Unless someone is passing around<br>'addresses' in the form of A.B.C.D which is horrifying me but I suppose could<br>work... but NO!<br><br>However, this isn't the biggest problem we have here - I ask the question<br>again, why are strings being used to represent these when dbus is perfectly<br>capable of a variety of different types.<br><br>> *data_len = sizeof(buf);<br>> memcpy(response, &buf, *data_len);<br>> +<br>> return IPMI_CC_OK;<br>> }<br>> else if (reqptr->parameter == 5) // MAC<br>> {<br><br>I know this isn't in the scope of this patch but perhaps we could stop with the<br>magic numbers...<br><br>> //string to parse: link/ether xx:xx:xx:xx:xx:xx<br>> <br>> - uint8_t buf[7];<br>> - memcpy((void*)&buf[0], ¤t_revision, 1);<br>> + const char* device = "eth0";<br><br>Why did you even declare device? Perhaps consider #defineing it?<br><br>> + char eaddr [12];<br>> + uint8_t buf[7];<br>> +<br>> + sd_bus_message *res = NULL;<br>> + sd_bus *bus = NULL;<br>> + sd_bus_error err = SD_BUS_ERROR_NULL;<br>> +<br>> + rc = sd_bus_open_system(&bus);<br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");<br>> + return -1;<br>> + }<br>> +<br>> + rc = sd_bus_call_method(bus, // On the System Bus<br>> + app, // Service to contact<br>> + obj, // Object path <br>> + ifc, // Interface name<br>> + "GetHwAddress", // Method to be called<br>> + &err, // object to return error<br>> + &res, // Response message on success<br>> + "s", // input message (dev,ip,nm,gw)<br>> + device);<br>> + if(rc < 0)<br>> + {<br>> + fprintf(stderr, "Failed to Get HW address of device : %s\n", device);<br>> + return -1;<br>> + }<br>> <br>> - for (i=0; i<6; i++)<br>> + rc = sd_bus_message_read (res, "s", &eaddr);<br>> + if(rc < 0)<br>> {<br>> - char mac[4];<br>> -<br>> - sprintf(syscmd, "ip address show dev eth0|grep link|cut -d' ' -f 6|cut -d':' -f%d", i+1);<br>> - FILE *fp = popen(syscmd, "r");<br>> -<br>> - memset(mac,0,sizeof(mac));<br>> - while (fgets(mac, sizeof(mac), fp) != 0)<br>> - {<br>> - int tmpmac = strtoul(mac, NULL, 16);<br>> - memcpy((void*)&buf[i+1], &tmpmac, 1);<br>> - }<br>> - pclose(fp);<br>> + fprintf(stderr, "Failed to parse gateway from response message:[%s]\n", strerror(-rc));<br>> + return -1;<br>> }<br>> <br>> + memcpy((void*)&buf[0], ¤t_revision, 1);<br>> + sscanf (eaddr, "%x:%x:%x:%x:%x:%x", &buf[1], &buf[2], &buf[3], &buf[4], &buf[5], &buf[6]);<br>> +<br><br>... and again ...<br><br>> *data_len = sizeof(buf);<br>> memcpy(response, &buf, *data_len);<br>> +<br>> return IPMI_CC_OK;<br>> }<br>> else<br>> @@ -195,4 +345,3 @@ void register_netfn_transport_functions()<br>> <br>> return;<br>> }<br>> -<br><br>_______________________________________________<br>openbmc mailing list<br>openbmc@lists.ozlabs.org<br></tt><tt><a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a></tt><tt><br></tt><br><BR>
</body></html>