[PATCH phosphor-host-ipmid] Using systemd dbus API to set get LAN parameters.
Cyril Bur
cyrilbur at gmail.com
Thu Jan 21 15:48:02 AEDT 2016
On Wed, 20 Jan 2016 07:20:28 -0600
OpenBMC Patches <openbmc-patches at 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 at 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;
> }
> -
More information about the openbmc
mailing list