* [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.