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