[PATCH phosphor-host-ipmid] Using systemd dbus API to set get LAN parameters.
Hariharasubramanian Ramasubramanian
hramasub at in.ibm.com
Thu Jan 21 15:59:05 AEDT 2016
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 at gmail.com>
To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
Cc: openbmc at 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 at lists.ozlabs.org>
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;
> }
> -
_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160121/3e99f7c5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160121/3e99f7c5/attachment-0001.gif>
More information about the openbmc
mailing list