[PATCH phosphor-host-ipmid v2] IPMI Get IP Support
Andrew Jeffery
andrew at aj.id.au
Thu Feb 11 12:04:37 AEDT 2016
Hi Adriana,
I realise this has already been merged, but I feel like I should point
out some issues below for future improvements.
On Wed, 2016-02-10 at 15:40 -0600, OpenBMC Patches wrote:
> From: Adriana Kobylak <anoo at us.ibm.com>
>
> Fix parsing of Get IP address.
> Dbus method returns additional information such as family and
> gateway, but this IPMI option just requires the IP to be returned.
> ---
> transporthandler.C | 75 ++++++++++++++++++++++++++----------------------------
> 1 file changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/transporthandler.C b/transporthandler.C
> index df2986a..d43f1f9 100644
> --- a/transporthandler.C
> +++ b/transporthandler.C
> @@ -215,9 +215,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> unsigned char prefixlen;
> unsigned char scope;
> unsigned int flags;
> - char saddr [128];
> - char gateway [128];
> - uint8_t buf[11];
> + char *saddr = NULL;
> int i = 0;
>
> printf("IPMI GET_LAN\n");
> @@ -259,57 +257,56 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> else if (reqptr->parameter == LAN_PARM_IP)
> {
> const char* device = "eth0";
> + uint8_t buf[5]; // Size of expected IPMI response msg
Might be best to use a constant and avoid the comment:
#define IPMI_IP_RESP_SIZE 5
>
> - sd_bus_message *res = NULL;
> - sd_bus *bus1 = NULL;
> - sd_bus_error err = SD_BUS_ERROR_NULL;
> -
> - rc = sd_bus_open_system(&bus1);
> - if(rc < 0)
> - {
> - fprintf(stderr,"ERROR: Getting a SYSTEM bus hook\n");
> + r = sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetAddress4");
> + if (r < 0) {
> + fprintf(stderr, "Failed to add method object: %s\n", strerror(-r));
> return -1;
> }
> -
> - rc = sd_bus_call_method(bus1, // 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)
> - {
> - fprintf(stderr, "Failed to Get IP on interface : %s\n", device);
> + r = sd_bus_message_append(m, "s", device);
> + if (r < 0) {
> + fprintf(stderr, "Failed to append message data: %s\n", strerror(-r));
> return -1;
> }
> -
> - /* rc = sd_bus_message_read(res, "a(iyyus)s", ...); */
> - rc = sd_bus_message_enter_container (res, 'a', "(iyyus)");
> + r = sd_bus_call(bus, m, 0, &error, &reply);
> + if (r < 0) {
> + fprintf(stderr, "Failed to call method: %s\n", strerror(-r));
> + return -1;
> + }
> + rc = sd_bus_message_enter_container (reply, '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)
> + rc = sd_bus_message_read(reply, "(iyyus)", &family, &prefixlen, &scope, &flags, &saddr);
> + if (rc < 0)
> {
> - fprintf(stderr, "Failed to parse gateway from response message:[%s]\n", strerror(-rc));
> + fprintf(stderr, "Failed to receive response: %s\n", strerror(-r));
> return -1;
> }
>
> + printf("%s:%d:%d:%d:%s\n", family==AF_INET?"IPv4":"IPv6", prefixlen, scope, flags, saddr);
> +
> 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]);
> +
> + // Parse IP address
Instead of hand-crafting this, why not use inet_pton()[1]? The binary
IP value is output in the s_addr member of struct in_addr. Also, we
need to do different things based on whether we have an IPv4 vs IPv6
address (assuming we care as there's detection of v4/v6 in the printf()
above), and with inet_pton() abstraction this is a change of a
parameter rather than a change of algorithm.
> + char *tokptr = NULL;
> + char* digit = strtok_r(saddr, ".", &tokptr);
> + if (digit == NULL)
> + {
> + fprintf(stderr, "Unexpected IP format: %s", saddr);
> + return IPMI_CC_RESPONSE_ERROR;
> + }
> + i = 0;
> + while (digit != NULL)
> + {
> + int resp_byte = strtoul(digit, NULL, 10);
If we continue on this path (instead of using inet_pton() as suggested
above), with strtoul() you're assigning an unsigned value into a signed
type - it's a bit of nit but resp_byte should probably be unsigned.
Additionally, if you're using strtoul() you should to do error checking
as suggested in the NOTES section of the man page[2]. The example in
the strtol() man page[3] demonstrates that actually requires quite a
bit of effort.
> + memcpy((void*)&buf[i+1], &resp_byte, 1);
I think really what you're trying to say here is that you expect the
value produced by strtoul() to be <= 255, and that you want to make use
of the parsed value. resp_byte should be tested for <= 255, as if the
parsed value is bigger it's a malformed IP. Malformed IPs should
probably result in an error being propagated to the caller.
Regardless, if we use inet_pton() instead we can just punt on the
problem of how to detect invalid IPs and deal with the error return
code (0, -1/errno) as necessary.
Additionally, the above approach locks the code to a little-endian
environment. I don't know that we care about being portable, but it's
easy to avoid the lock-in and the memcpy (assumes we've checked
resp_byte is in-range):
buf[i+1] = (uint8_t) resp_byte;
And on that, it seems that we only use i in the context of the loop, in
the assignment into buf. Can we set i=1 prior to the loop, rather than
obfuscating the code unnecessarily with i+1?
Again, these last points are not relevant if we switch to inet_pton(),
though the s_addr value will still eventually need to be memcpy()ed
into the response buffer.
> + i++;
> + digit = strtok_r(NULL, ".", &tokptr);
> + }
>
> *data_len = sizeof(buf);
> memcpy(response, &buf, *data_len);
Andrew
[1] http://man7.org/linux/man-pages/man3/inet_pton.3.html
[2] http://man7.org/linux/man-pages/man3/strtoul.3.html#NOTES
[3] http://man7.org/linux/man-pages/man3/strtol.3.html#EXAMPLE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160211/bd67a530/attachment.sig>
More information about the openbmc
mailing list