[PATCH phosphor-host-ipmid] Get MAC address
Cyril Bur
cyrilbur at gmail.com
Tue Feb 9 12:10:39 AEDT 2016
On Mon, 8 Feb 2016 17:40:27 -0600
OpenBMC Patches <openbmc-patches at stwcx.xyz> wrote:
> From: Adriana Kobylak <anoo at us.ibm.com>
>
> Support for Get MAC address command.
> Address review comments from Set MAC command.
Hi Adriana, thanks for doing the cleanup.
It looks like you've done a few things in the same patch.
Could you please separate out the response to Joel and Daniels review from your
new work?
Comments below,
Cyril
> ---
> transporthandler.C | 100 ++++++++++++++++++++++++++++++-----------------------
> transporthandler.h | 9 +++++
> 2 files changed, 66 insertions(+), 43 deletions(-)
>
> diff --git a/transporthandler.C b/transporthandler.C
> index c38adea..0ef818d 100644
> --- a/transporthandler.C
> +++ b/transporthandler.C
I think Joel mentioned something about not declaring sd_bus as extern?
> @@ -52,6 +52,10 @@ 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;
> + sd_bus_message *reply = NULL, *m = NULL;
> + sd_bus_error error = SD_BUS_ERROR_NULL;
> + int r = 0;
> +
>
> printf("IPMI SET_LAN\n");
>
> @@ -61,18 +65,15 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> // TODO Add the rest of the parameters like setting auth type
> // TODO Add error handling
>
> - if (reqptr->parameter == 3) // IP
> + if (reqptr->parameter == LAN_PARM_IP)
> {
> sprintf(new_ipaddr, "%d.%d.%d.%d", reqptr->data[0], reqptr->data[1], reqptr->data[2], reqptr->data[3]);
> }
> - else if (reqptr->parameter == 5) // MAC
> + else if (reqptr->parameter == LAN_PARM_MAC)
> {
> char mac[18];
> - sd_bus_message *reply = NULL, *m = NULL;
> - sd_bus_error error = SD_BUS_ERROR_NULL;
> - int r = 0;
>
I'm pretty sure that isn't what Joel meant in his review
> - sprintf(mac, "%x:%x:%x:%x:%x:%x",
> + snprintf(mac, 18, "%02x:%02x:%02x:%02x:%02x:%02x",
Hardcoded 18? How sure are you that this works now?
> reqptr->data[0],
> reqptr->data[1],
> reqptr->data[2],
> @@ -96,15 +97,15 @@ ipmi_ret_t ipmi_transport_set_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> return -1;
> }
> }
> - else if (reqptr->parameter == 6) // Subnet
> + else if (reqptr->parameter == LAN_PARM_SUBNET)
> {
> 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
> + else if (reqptr->parameter == LAN_PARM_GATEWAY)
> {
> 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
> + else if (reqptr->parameter == LAN_PARM_INPROGRESS) // Apply config
It is wonderful that you've gone through and given names to these constants and
that you've done this everywhere. I hesitate to mention this but if you're
going to go through and address existing issues, perhaps you could address them
all, those sprintf lines could also benefit from snprintf...
> {
> int rc = 0;
> sd_bus_message *req = NULL;
> @@ -199,7 +200,10 @@ 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 */
> + sd_bus_message *reply = NULL, *m = NULL;
> + sd_bus_error error = SD_BUS_ERROR_NULL;
> + int r = 0;
> +
> const uint8_t current_revision = 0x11; // Current rev per IPMI Spec 2.0
>
> int family;
> @@ -209,6 +213,7 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> char saddr [128];
> char gateway [128];
> uint8_t buf[11];
> + int i = 0;
>
> printf("IPMI GET_LAN\n");
>
> @@ -225,28 +230,28 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> // TODO Use dbus interface once available. For now use ip cmd.
> // TODO Add the rest of the parameters, like gateway
>
> - if (reqptr->parameter == 0) // In progress
> + if (reqptr->parameter == LAN_PARM_INPROGRESS)
> {
> uint8_t buf[] = {current_revision,0};
> *data_len = sizeof(buf);
> memcpy(response, &buf, *data_len);
> return IPMI_CC_OK;
> }
> - else if (reqptr->parameter == 1) // Authentication support
> + else if (reqptr->parameter == LAN_PARM_AUTHSUPPORT)
> {
> uint8_t buf[] = {current_revision,0x04};
> *data_len = sizeof(buf);
> memcpy(response, &buf, *data_len);
> return IPMI_CC_OK;
> }
> - else if (reqptr->parameter == 2) // Authentication enables
> + else if (reqptr->parameter == LAN_PARM_AUTHENABLES)
> {
> uint8_t buf[] = {current_revision,0x04,0x04,0x04,0x04,0x04};
> *data_len = sizeof(buf);
> memcpy(response, &buf, *data_len);
> return IPMI_CC_OK;
> }
> - else if (reqptr->parameter == 3) // IP
> + else if (reqptr->parameter == LAN_PARM_IP)
> {
> const char* device = "eth0";
>
> @@ -306,49 +311,58 @@ ipmi_ret_t ipmi_transport_get_lan(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>
> return IPMI_CC_OK;
> }
> - else if (reqptr->parameter == 5) // MAC
> + else if (reqptr->parameter == LAN_PARM_MAC)
> {
> //string to parse: link/ether xx:xx:xx:xx:xx:xx
>
> const char* device = "eth0";
> - char eaddr [12];
> uint8_t buf[7];
> + char *eaddr1 = NULL;
>
> - 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");
> + r = sd_bus_message_new_method_call(bus,&m,app,obj,ifc,"GetHwAddress");
> + if (r < 0) {
> + fprintf(stderr, "Failed to add method object: %s\n", strerror(-r));
> 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);
> + 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, "s", &eaddr);
> - if(rc < 0)
> - {
> - fprintf(stderr, "Failed to parse gateway from response message:[%s]\n", strerror(-rc));
> + r = sd_bus_call(bus, m, 0, &error, &reply);
> + if (r < 0) {
> + fprintf(stderr, "Failed to call method: %s\n", strerror(-r));
> return -1;
> }
> + r = sd_bus_message_read(reply, "s", &eaddr1);
> + if (r < 0) {
> + fprintf(stderr, "Failed to get a response: %s", strerror(-r));
> + return IPMI_CC_RESPONSE_ERROR;
> + }
> + if (eaddr1 == NULL)
> + {
> + fprintf(stderr, "Failed to get a valid response: %s", strerror(-r));
> + return IPMI_CC_RESPONSE_ERROR;
> + }
>
> 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]);
> +
> + char *tokptr = NULL;
> + char* digit = strtok_r(eaddr1, ":", &tokptr);
> + if (digit == NULL)
> + {
> + fprintf(stderr, "Unexpected MAC format: %s", eaddr1);
> + return IPMI_CC_RESPONSE_ERROR;
> + }
> +
> + i=0;
> + while (digit != NULL)
> + {
> + int resp_byte = strtoul(digit, NULL, 16);
> + memcpy((void*)&buf[i+1], &resp_byte, 1);
> + i++;
> + digit = strtok_r(NULL, ":", &tokptr);
> + }
>
> *data_len = sizeof(buf);
> memcpy(response, &buf, *data_len);
> diff --git a/transporthandler.h b/transporthandler.h
> index 49b1d95..ce7842b 100644
> --- a/transporthandler.h
> +++ b/transporthandler.h
> @@ -15,4 +15,13 @@ enum ipmi_transport_return_codes
> IPMI_CC_PARM_NOT_SUPPORTED = 0x80,
> };
>
> +// Parameters
> +static const int LAN_PARM_INPROGRESS = 0;
> +static const int LAN_PARM_AUTHSUPPORT = 1;
> +static const int LAN_PARM_AUTHENABLES = 2;
> +static const int LAN_PARM_IP = 3;
> +static const int LAN_PARM_MAC = 5;
> +static const int LAN_PARM_SUBNET = 6;
> +static const int LAN_PARM_GATEWAY = 12;
> +
> #endif
More information about the openbmc
mailing list