[PATCH phosphor-host-ipmid v3] Add ipmi cold-reset command, which call a dbus method.

[PATCH phosphor-host-ipmid v3] Add ipmi cold-reset command, which call a dbus method.

[OpenBMC Patches]

1. A method dbus_cold_reset() to talk with the dbus method 'place_holder';
2. Also get service name by object mapper which is object_mapper_get_connection();
3. Register the ipmi command;
4. Add related .o to the Makefile.

https://github.com/openbmc/phosphor-host-ipmid/pull/56

William (1):
  Add ipmi coldReset command, which call a dbus method, belongs to
    NETFUN_APP.     1. A method dbus_warm_reset() to talk with the dbus
    method 'warmRest';     2. Also get service name by
    ipmid_get_sd_bus_connection() instead of
    object_mapper_get_connection();     3. Register the ipmi command;   
     4. Add related .o to the Makefile;     5. Add wildcard function.

 Makefile        |   1 +
 globalhandler.C | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 globalhandler.h |  12 ++++
 3 files changed, 184 insertions(+)
 create mode 100644 globalhandler.C
 create mode 100644 globalhandler.h

-- 
2.6.4

[PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, which call a dbus method, belongs to NETFUN_APP. 1. A method dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get service name by ipmid_get_sd_bus_connection() instead of object_mapper_get_connection(); 3. Register the ipmi command; 4. Add related .o to the Makefile; 5. Add wildcard function.

[OpenBMC Patches]

From: William <bjlinan@cn.ibm.com>

---
 Makefile        |   1 +
 globalhandler.C | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 globalhandler.h |  12 ++++
 3 files changed, 184 insertions(+)
 create mode 100644 globalhandler.C
 create mode 100644 globalhandler.h

diff --git a/Makefile b/Makefile
index 472bfaa..58e7310 100644
--- a/Makefile
+++ b/Makefile
@@ -15,6 +15,7 @@ LIB_APP_OBJ = apphandler.o     \
               ipmisensor.o     \
               storageaddsel.o  \
               transporthandler.o  \
+              globalhandler.o  
 
 LIB_HOST_SRV_OBJ = host-services.o
 
diff --git a/globalhandler.C b/globalhandler.C
new file mode 100644
index 0000000..e68ea79
--- /dev/null
+++ b/globalhandler.C
@@ -0,0 +1,171 @@
+#include "globalhandler.h"
+#include "ipmid-api.h"
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+const char  *control_object_name  =  "/org/openbmc/control/bmc0";
+const char  *control_intf_name    =  "org.openbmc.control.Bmc";
+
+const char  *objectmapper_service_name =  "org.openbmc.objectmapper";
+const char  *objectmapper_object_name  =  "/org/openbmc/objectmapper/objectmapper";
+const char  *objectmapper_intf_name    =  "org.openbmc.objectmapper.ObjectMapper";
+
+void register_netfn_global_functions() __attribute__((constructor));
+
+int obj_mapper_get_connection(char** buf, const char* obj_path)
+{
+    sd_bus_error error = SD_BUS_ERROR_NULL;
+    sd_bus_message *m = NULL;
+    sd_bus *bus = NULL;
+    char *temp_buf = NULL, *intf = NULL;
+    size_t buf_size = 0;
+    int r;
+
+    //Get the system bus where most system services are provided.
+    bus = ipmid_get_sd_bus_connection();
+
+    /*
+     * Bus, service, object path, interface and method are provided to call
+     * the method.
+     * Signatures and input arguments are provided by the arguments at the
+     * end.
+     */
+    r = sd_bus_call_method(bus,
+            objectmapper_service_name,                      /* service to contact */
+            objectmapper_object_name,                       /* object path */
+            objectmapper_intf_name,                         /* interface name */
+            "GetObject",                                    /* method name */
+            &error,                                         /* object to return error in */
+            &m,                                             /* return message on success */
+            "s",                                            /* input signature */
+            obj_path                                        /* first argument */
+            );
+
+    if (r < 0) {
+        fprintf(stderr, "Failed to issue method call: %s\n", error.message);
+        goto finish;
+    }
+
+    // Get the key, aka, the connection name
+    sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
+    
+	/* 
+     * TODO: check the return code. Currently for no reason the message
+     * parsing of object mapper is always complaining about
+     * "Device or resource busy", but the result seems OK for now. Need
+     *  further checks.
+     */
+
+    buf_size = strlen(temp_buf) + 1;
+    printf("IPMID connection name: %s\n", temp_buf);
+    *buf = (char*)malloc(buf_size);
+
+    if (*buf == NULL) {
+        fprintf(stderr, "Malloc failed for warm reset");
+        r = -1;
+        goto finish;
+    }
+
+    memcpy(*buf, temp_buf, buf_size);
+
+finish:
+    sd_bus_error_free(&error);
+    sd_bus_message_unref(m);
+
+    return r;
+}
+
+int dbus_warm_reset()
+{
+    sd_bus_error error = SD_BUS_ERROR_NULL;
+    sd_bus_message *m = NULL;
+    sd_bus *bus = NULL;
+    char* temp_buf = NULL;
+    uint8_t* get_value = NULL;
+    char* connection = NULL;
+    int r, i;
+
+    r = obj_mapper_get_connection(&connection, control_object_name);
+    if (r < 0) {
+        fprintf(stderr, "Failed to get connection, return value: %d.\n", r);
+        goto finish;
+    }
+
+    printf("connection: %s\n", connection);
+
+    // Open the system bus where most system services are provided.
+    bus = ipmid_get_sd_bus_connection();
+    
+    /*
+     * Bus, service, object path, interface and method are provided to call
+     * the method.
+     * Signatures and input arguments are provided by the arguments at the
+     * end.
+	 */
+    r = sd_bus_call_method(bus,
+            connection,                                /* service to contact */
+            control_object_name,                       /* object path */
+            control_intf_name,                         /* interface name */
+            "warmReset",                               /* method name */
+            &error,                                    /* object to return error in */
+            &m,                                        /* return message on success */
+            NULL,
+            NULL
+            );
+
+    if (r < 0) {
+        fprintf(stderr, "Failed to issue method call: %s\n", error.message);
+        goto finish;
+    }
+
+finish:
+    sd_bus_error_free(&error);
+    sd_bus_message_unref(m);
+    free(connection);
+
+    return r;
+}
+
+ipmi_ret_t ipmi_global_warm_reset(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
+                              ipmi_request_t request, ipmi_response_t response,
+                              ipmi_data_len_t data_len, ipmi_context_t context)
+{
+    printf("Handling GLOBAL warmReset Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
+
+    // TODO: call the correct dbus method for warmReset.
+    dbus_warm_reset();
+
+    // Status code.
+    ipmi_ret_t rc = IPMI_CC_OK;
+    *data_len = 0;
+    return rc;
+}
+
+ipmi_ret_t ipmi_global_wildcard_handler(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
+                              ipmi_request_t request, ipmi_response_t response,
+                              ipmi_data_len_t data_len, ipmi_context_t context)
+{
+    printf("Handling WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
+
+    // Status code.
+    ipmi_ret_t rc = IPMI_CC_OK;
+
+    *data_len = strlen("THIS IS WILDCARD");
+
+    // Now pack actual response
+    memcpy(response, "THIS IS WILDCARD", *data_len);
+
+    return rc;
+}
+
+void register_netfn_global_functions()
+{
+    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, IPMI_CMD_WARM_RESET);
+    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WARM_RESET, NULL, ipmi_global_warm_reset);
+
+    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, IPMI_CMD_WILDCARD);
+    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WILDCARD, NULL, ipmi_global_wildcard_handler);
+
+	return;
+}
diff --git a/globalhandler.h b/globalhandler.h
new file mode 100644
index 0000000..608df3b
--- /dev/null
+++ b/globalhandler.h
@@ -0,0 +1,12 @@
+#ifndef __HOST_IPMI_GLOBAL_HANDLER_H__
+#define __HOST_IPMI_GLOBAL_HANDLER_H__
+
+#include <stdint.h>
+
+// Various GLOBAL operations under a single command.
+enum ipmi_global_control_cmds : uint8_t
+{
+IPMI_CMD_WARM_RESET 			   = 0x02,
+};
+
+#endif
-- 
2.6.4

Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, which call a dbus method, belongs to NETFUN_APP. 1. A method dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get service name by ipmid_get_sd_bus_connection() instead of object_mapper_get_connection(); 3. Register the ipmi command; 4. Add related .o to the Makefile; 5. Add wildcard function.

[Sam Mendoza-Jonas]

Hi William, a question below;

On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
> From: William <bjlinan@cn.ibm.com>
> 
> ---
>  Makefile        |   1 +
>  globalhandler.C | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  globalhandler.h |  12 ++++
>  3 files changed, 184 insertions(+)
>  create mode 100644 globalhandler.C
>  create mode 100644 globalhandler.h
> 
> diff --git a/Makefile b/Makefile
> index 472bfaa..58e7310 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,6 +15,7 @@ LIB_APP_OBJ = apphandler.o     \
>                ipmisensor.o     \
>                storageaddsel.o  \
>                transporthandler.o  \
> +              globalhandler.o  
>  
>  LIB_HOST_SRV_OBJ = host-services.o
>  
> diff --git a/globalhandler.C b/globalhandler.C
> new file mode 100644
> index 0000000..e68ea79
> --- /dev/null
> +++ b/globalhandler.C
> @@ -0,0 +1,171 @@
> +#include "globalhandler.h"
> +#include "ipmid-api.h"
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +const char  *control_object_name  =  "/org/openbmc/control/bmc0";
> +const char  *control_intf_name    =  "org.openbmc.control.Bmc";
> +
> +const char  *objectmapper_service_name =  "org.openbmc.objectmapper";
> +const char  *objectmapper_object_name  =  "/org/openbmc/objectmapper/objectmapper";
> +const char  *objectmapper_intf_name    =  "org.openbmc.objectmapper.ObjectMapper";
> +
> +void register_netfn_global_functions() __attribute__((constructor));
> +
> +int obj_mapper_get_connection(char** buf, const char* obj_path)
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char *temp_buf = NULL, *intf = NULL;
> +    size_t buf_size = 0;
> +    int r;
> +
> +    //Get the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +
> +    /*
> +     * Bus, service, object path, interface and method are provided to call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at the
> +     * end.
> +     */
> +    r = sd_bus_call_method(bus,
> +            objectmapper_service_name,                      /* service to contact */
> +            objectmapper_object_name,                       /* object path */
> +            objectmapper_intf_name,                         /* interface name */
> +            "GetObject",                                    /* method name */
> +            &error,                                         /* object to return error in */
> +            &m,                                             /* return message on success */
> +            "s",                                            /* input signature */
> +            obj_path                                        /* first argument */
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n", error.message);
> +        goto finish;
> +    }
> +
> +    // Get the key, aka, the connection name
> +    sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> +    
> +	/* 
> +     * TODO: check the return code. Currently for no reason the message
> +     * parsing of object mapper is always complaining about
> +     * "Device or resource busy", but the result seems OK for now. Need
> +     *  further checks.
> +     */

I've seen this comment before and it worries me a bit. Why are we
getting the error? How do you know temp_buf is allocated properly? What
does it mean that the result "seems OK"?
You check return codes before and after this, but if you get unlucky
here you could easily segfault or read nonsense.

I think this issue should be figured out before merging the code, both
here and wherever else it appears.

Thanks,
Sam

> +
> +    buf_size = strlen(temp_buf) + 1;
> +    printf("IPMID connection name: %s\n", temp_buf);
> +    *buf = (char*)malloc(buf_size);
> +
> +    if (*buf == NULL) {
> +        fprintf(stderr, "Malloc failed for warm reset");
> +        r = -1;
> +        goto finish;
> +    }
> +
> +    memcpy(*buf, temp_buf, buf_size);
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +
> +    return r;
> +}
> +
> +int dbus_warm_reset()
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char* temp_buf = NULL;
> +    uint8_t* get_value = NULL;
> +    char* connection = NULL;
> +    int r, i;
> +
> +    r = obj_mapper_get_connection(&connection, control_object_name);
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to get connection, return value: %d.\n", r);
> +        goto finish;
> +    }
> +
> +    printf("connection: %s\n", connection);
> +
> +    // Open the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +    
> +    /*
> +     * Bus, service, object path, interface and method are provided to call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at the
> +     * end.
> +	 */
> +    r = sd_bus_call_method(bus,
> +            connection,                                /* service to contact */
> +            control_object_name,                       /* object path */
> +            control_intf_name,                         /* interface name */
> +            "warmReset",                               /* method name */
> +            &error,                                    /* object to return error in */
> +            &m,                                        /* return message on success */
> +            NULL,
> +            NULL
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n", error.message);
> +        goto finish;
> +    }
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +    free(connection);
> +
> +    return r;
> +}
> +
> +ipmi_ret_t ipmi_global_warm_reset(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t response,
> +                              ipmi_data_len_t data_len, ipmi_context_t context)
> +{
> +    printf("Handling GLOBAL warmReset Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // TODO: call the correct dbus method for warmReset.
> +    dbus_warm_reset();
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +    *data_len = 0;
> +    return rc;
> +}
> +
> +ipmi_ret_t ipmi_global_wildcard_handler(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t response,
> +                              ipmi_data_len_t data_len, ipmi_context_t context)
> +{
> +    printf("Handling WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +
> +    *data_len = strlen("THIS IS WILDCARD");
> +
> +    // Now pack actual response
> +    memcpy(response, "THIS IS WILDCARD", *data_len);
> +
> +    return rc;
> +}
> +
> +void register_netfn_global_functions()
> +{
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, IPMI_CMD_WARM_RESET);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WARM_RESET, NULL, ipmi_global_warm_reset);
> +
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, IPMI_CMD_WILDCARD);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WILDCARD, NULL, ipmi_global_wildcard_handler);
> +
> +	return;
> +}
> diff --git a/globalhandler.h b/globalhandler.h
> new file mode 100644
> index 0000000..608df3b
> --- /dev/null
> +++ b/globalhandler.h
> @@ -0,0 +1,12 @@
> +#ifndef __HOST_IPMI_GLOBAL_HANDLER_H__
> +#define __HOST_IPMI_GLOBAL_HANDLER_H__
> +
> +#include <stdint.h>
> +
> +// Various GLOBAL operations under a single command.
> +enum ipmi_global_control_cmds : uint8_t
> +{
> +IPMI_CMD_WARM_RESET 			   = 0x02,
> +};
> +
> +#endif
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, which call a dbus method, belongs to NETFUN_APP. 1. A method dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get service name by ipmid_get_sd_bus_connection() instead of object_mapper_get_connection(); 3. Register the ipmi command; 4. Add related .o to the Makefile; 5. Add wildcard function.

[Norman James]

[-- Attachment #1.1: Type: text/plain, Size: 9520 bytes --]


Too late.  Cyril had already approved this code.   If you want to step up
the priority on fixing the TODO, you could open an issue so we can make
sure it gets addressed in a timely fashion.


Regards,
Norman James
IBM - POWER Systems Architect
Phone: 1-512-286-6807 (T/L: 363-6807)
Internet: njames@us.ibm.com




From:	Sam Mendoza-Jonas <sam@mendozajonas.com>
To:	OpenBMC Patches <openbmc-patches@stwcx.xyz>
Cc:	openbmc@lists.ozlabs.org, William <bjlinan@cn.ibm.com>
Date:	02/01/2016 09:44 PM
Subject:	Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command,
            which call a dbus method, belongs to NETFUN_APP. 1. A method
            dbus_warm_reset() to talk with the dbus method 'warmRest'; 2.
            Also get service name by ipmid_get_sd_bus_connection() instead
            of object_mapper_get_connection(); 3. Register the ipmi
            command; 4. Add related .o to the Makefile; 5. Add wildcard
            function.
Sent by:	"openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org>



Hi William, a question below;

On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
> From: William <bjlinan@cn.ibm.com>
>
> ---
>  Makefile        |   1 +
>  globalhandler.C | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++
++++++
>  globalhandler.h |  12 ++++
>  3 files changed, 184 insertions(+)
>  create mode 100644 globalhandler.C
>  create mode 100644 globalhandler.h
>
> diff --git a/Makefile b/Makefile
> index 472bfaa..58e7310 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,6 +15,7 @@ LIB_APP_OBJ = apphandler.o     \
>                ipmisensor.o     \
>                storageaddsel.o  \
>                transporthandler.o  \
> +              globalhandler.o
>
>  LIB_HOST_SRV_OBJ = host-services.o
>
> diff --git a/globalhandler.C b/globalhandler.C
> new file mode 100644
> index 0000000..e68ea79
> --- /dev/null
> +++ b/globalhandler.C
> @@ -0,0 +1,171 @@
> +#include "globalhandler.h"
> +#include "ipmid-api.h"
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +const char  *control_object_name  =  "/org/openbmc/control/bmc0";
> +const char  *control_intf_name    =  "org.openbmc.control.Bmc";
> +
> +const char  *objectmapper_service_name =  "org.openbmc.objectmapper";
> +const char  *objectmapper_object_name  =
"/org/openbmc/objectmapper/objectmapper";
> +const char  *objectmapper_intf_name    =
"org.openbmc.objectmapper.ObjectMapper";
> +
> +void register_netfn_global_functions() __attribute__((constructor));
> +
> +int obj_mapper_get_connection(char** buf, const char* obj_path)
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char *temp_buf = NULL, *intf = NULL;
> +    size_t buf_size = 0;
> +    int r;
> +
> +    //Get the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +
> +    /*
> +     * Bus, service, object path, interface and method are provided to
call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at
the
> +     * end.
> +     */
> +    r = sd_bus_call_method(bus,
> +            objectmapper_service_name,                      /* service
to contact */
> +            objectmapper_object_name,                       /* object
path */
> +            objectmapper_intf_name,                         /* interface
name */
> +            "GetObject",                                    /* method
name */
> +            &error,                                         /* object to
return error in */
> +            &m,                                             /* return
message on success */
> +            "s",                                            /* input
signature */
> +            obj_path                                        /* first
argument */
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n",
error.message);
> +        goto finish;
> +    }
> +
> +    // Get the key, aka, the connection name
> +    sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> +
> +		 /*
> +     * TODO: check the return code. Currently for no reason the message
> +     * parsing of object mapper is always complaining about
> +     * "Device or resource busy", but the result seems OK for now. Need
> +     *  further checks.
> +     */

I've seen this comment before and it worries me a bit. Why are we
getting the error? How do you know temp_buf is allocated properly? What
does it mean that the result "seems OK"?
You check return codes before and after this, but if you get unlucky
here you could easily segfault or read nonsense.

I think this issue should be figured out before merging the code, both
here and wherever else it appears.

Thanks,
Sam

> +
> +    buf_size = strlen(temp_buf) + 1;
> +    printf("IPMID connection name: %s\n", temp_buf);
> +    *buf = (char*)malloc(buf_size);
> +
> +    if (*buf == NULL) {
> +        fprintf(stderr, "Malloc failed for warm reset");
> +        r = -1;
> +        goto finish;
> +    }
> +
> +    memcpy(*buf, temp_buf, buf_size);
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +
> +    return r;
> +}
> +
> +int dbus_warm_reset()
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char* temp_buf = NULL;
> +    uint8_t* get_value = NULL;
> +    char* connection = NULL;
> +    int r, i;
> +
> +    r = obj_mapper_get_connection(&connection, control_object_name);
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to get connection, return value: %d.\n",
r);
> +        goto finish;
> +    }
> +
> +    printf("connection: %s\n", connection);
> +
> +    // Open the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +
> +    /*
> +     * Bus, service, object path, interface and method are provided to
call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at
the
> +     * end.
> +		  */
> +    r = sd_bus_call_method(bus,
> +            connection,                                /* service to
contact */
> +            control_object_name,                       /* object path */
> +            control_intf_name,                         /* interface name
*/
> +            "warmReset",                               /* method name */
> +            &error,                                    /* object to
return error in */
> +            &m,                                        /* return message
on success */
> +            NULL,
> +            NULL
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n",
error.message);
> +        goto finish;
> +    }
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +    free(connection);
> +
> +    return r;
> +}
> +
> +ipmi_ret_t ipmi_global_warm_reset(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t
response,
> +                              ipmi_data_len_t data_len, ipmi_context_t
context)
> +{
> +    printf("Handling GLOBAL warmReset Netfn:[0x%X], Cmd:[0x%X]\n",netfn,
cmd);
> +
> +    // TODO: call the correct dbus method for warmReset.
> +    dbus_warm_reset();
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +    *data_len = 0;
> +    return rc;
> +}
> +
> +ipmi_ret_t ipmi_global_wildcard_handler(ipmi_netfn_t netfn, ipmi_cmd_t
cmd,
> +                              ipmi_request_t request, ipmi_response_t
response,
> +                              ipmi_data_len_t data_len, ipmi_context_t
context)
> +{
> +    printf("Handling WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +
> +    *data_len = strlen("THIS IS WILDCARD");
> +
> +    // Now pack actual response
> +    memcpy(response, "THIS IS WILDCARD", *data_len);
> +
> +    return rc;
> +}
> +
> +void register_netfn_global_functions()
> +{
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP,
IPMI_CMD_WARM_RESET);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WARM_RESET, NULL,
ipmi_global_warm_reset);
> +
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP,
IPMI_CMD_WILDCARD);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WILDCARD, NULL,
ipmi_global_wildcard_handler);
> +
> +		 return;
> +}
> diff --git a/globalhandler.h b/globalhandler.h
> new file mode 100644
> index 0000000..608df3b
> --- /dev/null
> +++ b/globalhandler.h
> @@ -0,0 +1,12 @@
> +#ifndef __HOST_IPMI_GLOBAL_HANDLER_H__
> +#define __HOST_IPMI_GLOBAL_HANDLER_H__
> +
> +#include <stdint.h>
> +
> +// Various GLOBAL operations under a single command.
> +enum ipmi_global_control_cmds : uint8_t
> +{
> +IPMI_CMD_WARM_RESET 		 		 		    = 0x02,
> +};
> +
> +#endif
> --
> 2.6.4
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc


[-- Attachment #1.2: Type: text/html, Size: 15452 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]

Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, which call a dbus method, belongs to NETFUN_APP. 1. A method dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get service name by ipmid_get_sd_bus_connection() instead of object_mapper_get_connection(); 3. Register the ipmi command; 4. Add related .o to the Makefile; 5. Add wildcard function.

[Nan KX Li]

[-- Attachment #1: Type: text/plain, Size: 9277 bytes --]

Hi,
Thanks Sam.
I will think of it.

Regards,

William Li ( Li Nan, 李楠 ) 

Firmware Engineering Professional
OpenPower AE Team | IBM System & Technology Lab
Mobile: +86-186-1081 6605, +86-138-1188 5954

Beijing, China



From:   Sam Mendoza-Jonas <sam@mendozajonas.com>
To:     OpenBMC Patches <openbmc-patches@stwcx.xyz>
Cc:     openbmc@lists.ozlabs.org, Nan KX Li/China/IBM@IBMCN
Date:   02/02/2016 11:44
Subject:        Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset 
command, which call a dbus method, belongs to NETFUN_APP. 1. A method 
dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get 
service name by ipmid_get_sd_bus_connection() instead of 
object_mapper_get_connection(); 3. Register the ipmi command; 4. Add 
related .o to the Makefile; 5. Add wildcard function.



Hi William, a question below;

On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
> From: William <bjlinan@cn.ibm.com>
> 
> ---
>  Makefile        |   1 +
>  globalhandler.C | 171 
++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  globalhandler.h |  12 ++++
>  3 files changed, 184 insertions(+)
>  create mode 100644 globalhandler.C
>  create mode 100644 globalhandler.h
> 
> diff --git a/Makefile b/Makefile
> index 472bfaa..58e7310 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,6 +15,7 @@ LIB_APP_OBJ = apphandler.o     \
>                ipmisensor.o     \
>                storageaddsel.o  \
>                transporthandler.o  \
> +              globalhandler.o 
> 
>  LIB_HOST_SRV_OBJ = host-services.o
> 
> diff --git a/globalhandler.C b/globalhandler.C
> new file mode 100644
> index 0000000..e68ea79
> --- /dev/null
> +++ b/globalhandler.C
> @@ -0,0 +1,171 @@
> +#include "globalhandler.h"
> +#include "ipmid-api.h"
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +const char  *control_object_name  =  "/org/openbmc/control/bmc0";
> +const char  *control_intf_name    =  "org.openbmc.control.Bmc";
> +
> +const char  *objectmapper_service_name =  "org.openbmc.objectmapper";
> +const char  *objectmapper_object_name  = 
"/org/openbmc/objectmapper/objectmapper";
> +const char  *objectmapper_intf_name    = 
"org.openbmc.objectmapper.ObjectMapper";
> +
> +void register_netfn_global_functions() __attribute__((constructor));
> +
> +int obj_mapper_get_connection(char** buf, const char* obj_path)
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char *temp_buf = NULL, *intf = NULL;
> +    size_t buf_size = 0;
> +    int r;
> +
> +    //Get the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +
> +    /*
> +     * Bus, service, object path, interface and method are provided to 
call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at 
the
> +     * end.
> +     */
> +    r = sd_bus_call_method(bus,
> +            objectmapper_service_name,                      /* service 
to contact */
> +            objectmapper_object_name,                       /* object 
path */
> +            objectmapper_intf_name,                         /* 
interface name */
> +            "GetObject",                                    /* method 
name */
> +            &error,                                         /* object 
to return error in */
> +            &m,                                             /* return 
message on success */
> +            "s",                                            /* input 
signature */
> +            obj_path                                        /* first 
argument */
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n", 
error.message);
> +        goto finish;
> +    }
> +
> +    // Get the key, aka, the connection name
> +    sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> + 
> +              /* 
> +     * TODO: check the return code. Currently for no reason the message
> +     * parsing of object mapper is always complaining about
> +     * "Device or resource busy", but the result seems OK for now. Need
> +     *  further checks.
> +     */

I've seen this comment before and it worries me a bit. Why are we
getting the error? How do you know temp_buf is allocated properly? What
does it mean that the result "seems OK"?
You check return codes before and after this, but if you get unlucky
here you could easily segfault or read nonsense.

I think this issue should be figured out before merging the code, both
here and wherever else it appears.

Thanks,
Sam

> +
> +    buf_size = strlen(temp_buf) + 1;
> +    printf("IPMID connection name: %s\n", temp_buf);
> +    *buf = (char*)malloc(buf_size);
> +
> +    if (*buf == NULL) {
> +        fprintf(stderr, "Malloc failed for warm reset");
> +        r = -1;
> +        goto finish;
> +    }
> +
> +    memcpy(*buf, temp_buf, buf_size);
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +
> +    return r;
> +}
> +
> +int dbus_warm_reset()
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char* temp_buf = NULL;
> +    uint8_t* get_value = NULL;
> +    char* connection = NULL;
> +    int r, i;
> +
> +    r = obj_mapper_get_connection(&connection, control_object_name);
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to get connection, return value: 
%d.\n", r);
> +        goto finish;
> +    }
> +
> +    printf("connection: %s\n", connection);
> +
> +    // Open the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> + 
> +    /*
> +     * Bus, service, object path, interface and method are provided to 
call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at 
the
> +     * end.
> +               */
> +    r = sd_bus_call_method(bus,
> +            connection,                                /* service to 
contact */
> +            control_object_name,                       /* object path 
*/
> +            control_intf_name,                         /* interface 
name */
> +            "warmReset",                               /* method name 
*/
> +            &error,                                    /* object to 
return error in */
> +            &m,                                        /* return 
message on success */
> +            NULL,
> +            NULL
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n", 
error.message);
> +        goto finish;
> +    }
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +    free(connection);
> +
> +    return r;
> +}
> +
> +ipmi_ret_t ipmi_global_warm_reset(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t 
response,
> +                              ipmi_data_len_t data_len, ipmi_context_t 
context)
> +{
> +    printf("Handling GLOBAL warmReset Netfn:[0x%X], 
Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // TODO: call the correct dbus method for warmReset.
> +    dbus_warm_reset();
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +    *data_len = 0;
> +    return rc;
> +}
> +
> +ipmi_ret_t ipmi_global_wildcard_handler(ipmi_netfn_t netfn, ipmi_cmd_t 
cmd,
> +                              ipmi_request_t request, ipmi_response_t 
response,
> +                              ipmi_data_len_t data_len, ipmi_context_t 
context)
> +{
> +    printf("Handling WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +
> +    *data_len = strlen("THIS IS WILDCARD");
> +
> +    // Now pack actual response
> +    memcpy(response, "THIS IS WILDCARD", *data_len);
> +
> +    return rc;
> +}
> +
> +void register_netfn_global_functions()
> +{
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, 
IPMI_CMD_WARM_RESET);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WARM_RESET, NULL, 
ipmi_global_warm_reset);
> +
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, 
IPMI_CMD_WILDCARD);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WILDCARD, NULL, 
ipmi_global_wildcard_handler);
> +
> +              return;
> +}
> diff --git a/globalhandler.h b/globalhandler.h
> new file mode 100644
> index 0000000..608df3b
> --- /dev/null
> +++ b/globalhandler.h
> @@ -0,0 +1,12 @@
> +#ifndef __HOST_IPMI_GLOBAL_HANDLER_H__
> +#define __HOST_IPMI_GLOBAL_HANDLER_H__
> +
> +#include <stdint.h>
> +
> +// Various GLOBAL operations under a single command.
> +enum ipmi_global_control_cmds : uint8_t
> +{
> +IPMI_CMD_WARM_RESET                                              = 
0x02,
> +};
> +
> +#endif
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc




[-- Attachment #2: Type: text/html, Size: 15550 bytes --]

Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, which call a dbus method, belongs to NETFUN_APP. 1. A method dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get service name by ipmid_get_sd_bus_connection() instead of object_mapper_get_connection(); 3. Register the ipmi command; 4. Add related .o to the Makefile; 5. Add wildcard function.

[Cyril Bur]

[-- Attachment #1.1: Type: text/plain, Size: 10858 bytes --]

On Tue, 2 Feb 2016 15:13 Norman James <njames@us.ibm.com> wrote:

> Too late. Cyril had already approved this code. If you want to step up the
> priority on fixing the TODO, you could open an issue so we can make sure it
> gets addressed in a timely fashion.
>

I never 'approved' this patch. I reviewed it and pointed out that it needed
work in several places.

By the looks of things not all my concerns were addressed or explained to
me in a way that makes me feel comfortable with the patch as it is.

Furthermore even if this version had addressed all my issues with the
previous version, that does not mean I automatically approve this patch, I
still need to give my actual approval to each patch no matter what version
we get to.

You'll know when I 'approve' a patch as I will reply to it with my
"reviewed-by" tag.

Regards,
Cyril Bur
Open Source Developer


>
> Regards,
> Norman James
> IBM - POWER Systems Architect
> Phone: 1-512-286-6807 (T/L: 363-6807)
> Internet: njames@us.ibm.com
>
>
> [image: Inactive hide details for Sam Mendoza-Jonas ---02/01/2016 09:44:21
> PM---Hi William, a question below; On Mon, Feb 01, 2016 at 1]Sam
> Mendoza-Jonas ---02/01/2016 09:44:21 PM---Hi William, a question below; On
> Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
>
> From: Sam Mendoza-Jonas <sam@mendozajonas.com>
> To: OpenBMC Patches <openbmc-patches@stwcx.xyz>
> Cc: openbmc@lists.ozlabs.org, William <bjlinan@cn.ibm.com>
> Date: 02/01/2016 09:44 PM
> Subject: Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command,
> which call a dbus method, belongs to NETFUN_APP. 1. A method
> dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get
> service name by ipmid_get_sd_bus_connection() instead of
> object_mapper_get_connection(); 3. Register the ipmi command; 4. Add
> related .o to the Makefile; 5. Add wildcard function.
> Sent by: "openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org>
> ------------------------------
>
>
>
>
> Hi William, a question below;
>
> On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
> > From: William <bjlinan@cn.ibm.com>
> >
> > ---
> >  Makefile        |   1 +
> >  globalhandler.C | 171
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  globalhandler.h |  12 ++++
> >  3 files changed, 184 insertions(+)
> >  create mode 100644 globalhandler.C
> >  create mode 100644 globalhandler.h
> >
> > diff --git a/Makefile b/Makefile
> > index 472bfaa..58e7310 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -15,6 +15,7 @@ LIB_APP_OBJ = apphandler.o     \
> >                ipmisensor.o     \
> >                storageaddsel.o  \
> >                transporthandler.o  \
> > +              globalhandler.o
> >
> >  LIB_HOST_SRV_OBJ = host-services.o
> >
> > diff --git a/globalhandler.C b/globalhandler.C
> > new file mode 100644
> > index 0000000..e68ea79
> > --- /dev/null
> > +++ b/globalhandler.C
> > @@ -0,0 +1,171 @@
> > +#include "globalhandler.h"
> > +#include "ipmid-api.h"
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdint.h>
> > +
> > +const char  *control_object_name  =  "/org/openbmc/control/bmc0";
> > +const char  *control_intf_name    =  "org.openbmc.control.Bmc";
> > +
> > +const char  *objectmapper_service_name =  "org.openbmc.objectmapper";
> > +const char  *objectmapper_object_name  =
>  "/org/openbmc/objectmapper/objectmapper";
> > +const char  *objectmapper_intf_name    =
>  "org.openbmc.objectmapper.ObjectMapper";
> > +
> > +void register_netfn_global_functions() __attribute__((constructor));
> > +
> > +int obj_mapper_get_connection(char** buf, const char* obj_path)
> > +{
> > +    sd_bus_error error = SD_BUS_ERROR_NULL;
> > +    sd_bus_message *m = NULL;
> > +    sd_bus *bus = NULL;
> > +    char *temp_buf = NULL, *intf = NULL;
> > +    size_t buf_size = 0;
> > +    int r;
> > +
> > +    //Get the system bus where most system services are provided.
> > +    bus = ipmid_get_sd_bus_connection();
> > +
> > +    /*
> > +     * Bus, service, object path, interface and method are provided to
> call
> > +     * the method.
> > +     * Signatures and input arguments are provided by the arguments at
> the
> > +     * end.
> > +     */
> > +    r = sd_bus_call_method(bus,
> > +            objectmapper_service_name,                      /* service
> to contact */
> > +            objectmapper_object_name,                       /* object
> path */
> > +            objectmapper_intf_name,                         /*
> interface name */
> > +            "GetObject",                                    /* method
> name */
> > +            &error,                                         /* object
> to return error in */
> > +            &m,                                             /* return
> message on success */
> > +            "s",                                            /* input
> signature */
> > +            obj_path                                        /* first
> argument */
> > +            );
> > +
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to issue method call: %s\n",
> error.message);
> > +        goto finish;
> > +    }
> > +
> > +    // Get the key, aka, the connection name
> > +    sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> > +
> > + /*
> > +     * TODO: check the return code. Currently for no reason the message
> > +     * parsing of object mapper is always complaining about
> > +     * "Device or resource busy", but the result seems OK for now. Need
> > +     *  further checks.
> > +     */
>
> I've seen this comment before and it worries me a bit. Why are we
> getting the error? How do you know temp_buf is allocated properly? What
> does it mean that the result "seems OK"?
> You check return codes before and after this, but if you get unlucky
> here you could easily segfault or read nonsense.
>
> I think this issue should be figured out before merging the code, both
> here and wherever else it appears.
>
> Thanks,
> Sam
>
> > +
> > +    buf_size = strlen(temp_buf) + 1;
> > +    printf("IPMID connection name: %s\n", temp_buf);
> > +    *buf = (char*)malloc(buf_size);
> > +
> > +    if (*buf == NULL) {
> > +        fprintf(stderr, "Malloc failed for warm reset");
> > +        r = -1;
> > +        goto finish;
> > +    }
> > +
> > +    memcpy(*buf, temp_buf, buf_size);
> > +
> > +finish:
> > +    sd_bus_error_free(&error);
> > +    sd_bus_message_unref(m);
> > +
> > +    return r;
> > +}
> > +
> > +int dbus_warm_reset()
> > +{
> > +    sd_bus_error error = SD_BUS_ERROR_NULL;
> > +    sd_bus_message *m = NULL;
> > +    sd_bus *bus = NULL;
> > +    char* temp_buf = NULL;
> > +    uint8_t* get_value = NULL;
> > +    char* connection = NULL;
> > +    int r, i;
> > +
> > +    r = obj_mapper_get_connection(&connection, control_object_name);
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to get connection, return value:
> %d.\n", r);
> > +        goto finish;
> > +    }
> > +
> > +    printf("connection: %s\n", connection);
> > +
> > +    // Open the system bus where most system services are provided.
> > +    bus = ipmid_get_sd_bus_connection();
> > +
> > +    /*
> > +     * Bus, service, object path, interface and method are provided to
> call
> > +     * the method.
> > +     * Signatures and input arguments are provided by the arguments at
> the
> > +     * end.
> > +  */
> > +    r = sd_bus_call_method(bus,
> > +            connection,                                /* service to
> contact */
> > +            control_object_name,                       /* object path */
> > +            control_intf_name,                         /* interface
> name */
> > +            "warmReset",                               /* method name */
> > +            &error,                                    /* object to
> return error in */
> > +            &m,                                        /* return
> message on success */
> > +            NULL,
> > +            NULL
> > +            );
> > +
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to issue method call: %s\n",
> error.message);
> > +        goto finish;
> > +    }
> > +
> > +finish:
> > +    sd_bus_error_free(&error);
> > +    sd_bus_message_unref(m);
> > +    free(connection);
> > +
> > +    return r;
> > +}
> > +
> > +ipmi_ret_t ipmi_global_warm_reset(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> > +                              ipmi_request_t request, ipmi_response_t
> response,
> > +                              ipmi_data_len_t data_len, ipmi_context_t
> context)
> > +{
> > +    printf("Handling GLOBAL warmReset Netfn:[0x%X],
> Cmd:[0x%X]\n",netfn, cmd);
> > +
> > +    // TODO: call the correct dbus method for warmReset.
> > +    dbus_warm_reset();
> > +
> > +    // Status code.
> > +    ipmi_ret_t rc = IPMI_CC_OK;
> > +    *data_len = 0;
> > +    return rc;
> > +}
> > +
> > +ipmi_ret_t ipmi_global_wildcard_handler(ipmi_netfn_t netfn, ipmi_cmd_t
> cmd,
> > +                              ipmi_request_t request, ipmi_response_t
> response,
> > +                              ipmi_data_len_t data_len, ipmi_context_t
> context)
> > +{
> > +    printf("Handling WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> > +
> > +    // Status code.
> > +    ipmi_ret_t rc = IPMI_CC_OK;
> > +
> > +    *data_len = strlen("THIS IS WILDCARD");
> > +
> > +    // Now pack actual response
> > +    memcpy(response, "THIS IS WILDCARD", *data_len);
> > +
> > +    return rc;
> > +}
> > +
> > +void register_netfn_global_functions()
> > +{
> > +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP,
> IPMI_CMD_WARM_RESET);
> > +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WARM_RESET, NULL,
> ipmi_global_warm_reset);
> > +
> > +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP,
> IPMI_CMD_WILDCARD);
> > +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WILDCARD, NULL,
> ipmi_global_wildcard_handler);
> > +
> > + return;
> > +}
> > diff --git a/globalhandler.h b/globalhandler.h
> > new file mode 100644
> > index 0000000..608df3b
> > --- /dev/null
> > +++ b/globalhandler.h
> > @@ -0,0 +1,12 @@
> > +#ifndef __HOST_IPMI_GLOBAL_HANDLER_H__
> > +#define __HOST_IPMI_GLOBAL_HANDLER_H__
> > +
> > +#include <stdint.h>
> > +
> > +// Various GLOBAL operations under a single command.
> > +enum ipmi_global_control_cmds : uint8_t
> > +{
> > +IPMI_CMD_WARM_RESET    = 0x02,
> > +};
> > +
> > +#endif
> > --
> > 2.6.4
> >
> >
> > _______________________________________________
> > openbmc mailing list
> > openbmc@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
>
>
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
>

[-- Attachment #1.2: Type: text/html, Size: 15047 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]

Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, which call a dbus method, belongs to NETFUN_APP. 1. A method dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get service name by ipmid_get_sd_bus_connection() instead of object_mapper_get_connection(); 3. Register the ipmi command; 4. Add related .o to the Makefile; 5. Add wildcard function.

[Nan KX Li]

[-- Attachment #1.1: Type: text/plain, Size: 11577 bytes --]

Hi Cyril,
I had modified the code following your recommended except for using the 
dbus_cold_reset function( replaced by dbus_warm_reset ). Because I haven't 
found out a best way to do it. Please give some detailed solution.

Regards,

William Li ( Li Nan, 李楠 ) 

Firmware Engineering Professional
OpenPower AE Team | IBM System & Technology Lab
Mobile: +86-186-1081 6605, +86-138-1188 5954

Beijing, China



From:   Cyril Bur <cyrilbur@gmail.com>
To:     Norman James <njames@us.ibm.com>, Sam Mendoza-Jonas 
<sam@mendozajonas.com>
Cc:     OpenBMC Patches <openbmc-patches@stwcx.xyz>, Nan KX 
Li/China/IBM@IBMCN, openbmc@lists.ozlabs.org
Date:   02/02/2016 13:26
Subject:        Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset 
command, which call a dbus method, belongs to NETFUN_APP. 1. A method 
dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get 
service name by ipmid_get_sd_bus_connection() instead of 
object_mapper_get_connection(); 3. Register the ipmi command; 4. Add 
related .o to the Makefile; 5. Add wildcard function.





On Tue, 2 Feb 2016 15:13 Norman James <njames@us.ibm.com> wrote:
Too late. Cyril had already approved this code. If you want to step up the 
priority on fixing the TODO, you could open an issue so we can make sure 
it gets addressed in a timely fashion. 

I never 'approved' this patch. I reviewed it and pointed out that it 
needed work in several places.

By the looks of things not all my concerns were addressed or explained to 
me in a way that makes me feel comfortable with the patch as it is.

Furthermore even if this version had addressed all my issues with the 
previous version, that does not mean I automatically approve this patch, I 
still need to give my actual approval to each patch no matter what version 
we get to.

You'll know when I 'approve' a patch as I will reply to it with my 
"reviewed-by" tag.

Regards,
Cyril Bur
Open Source Developer



Regards,
Norman James
IBM - POWER Systems Architect
Phone: 1-512-286-6807 (T/L: 363-6807)
Internet: njames@us.ibm.com


Sam Mendoza-Jonas ---02/01/2016 09:44:21 PM---Hi William, a question 
below; On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:

From: Sam Mendoza-Jonas <sam@mendozajonas.com>
To: OpenBMC Patches <openbmc-patches@stwcx.xyz>
Cc: openbmc@lists.ozlabs.org, William <bjlinan@cn.ibm.com>
Date: 02/01/2016 09:44 PM
Subject: Re: [PATCH phosphor-host-ipmid v3] Add ipmi coldReset command, 
which call a dbus method, belongs to NETFUN_APP. 1. A method 
dbus_warm_reset() to talk with the dbus method 'warmRest'; 2. Also get 
service name by ipmid_get_sd_bus_connection() instead of 
object_mapper_get_connection(); 3. Register the ipmi command; 4. Add 
related .o to the Makefile; 5. Add wildcard function.
Sent by: "openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org>




Hi William, a question below;

On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
> From: William <bjlinan@cn.ibm.com>
> 
> ---
>  Makefile        |   1 +
>  globalhandler.C | 171 
++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  globalhandler.h |  12 ++++
>  3 files changed, 184 insertions(+)
>  create mode 100644 globalhandler.C
>  create mode 100644 globalhandler.h
> 
> diff --git a/Makefile b/Makefile
> index 472bfaa..58e7310 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,6 +15,7 @@ LIB_APP_OBJ = apphandler.o     \
>                ipmisensor.o     \
>                storageaddsel.o  \
>                transporthandler.o  \
> +              globalhandler.o  
>  
>  LIB_HOST_SRV_OBJ = host-services.o
>  
> diff --git a/globalhandler.C b/globalhandler.C
> new file mode 100644
> index 0000000..e68ea79
> --- /dev/null
> +++ b/globalhandler.C
> @@ -0,0 +1,171 @@
> +#include "globalhandler.h"
> +#include "ipmid-api.h"
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdint.h>
> +
> +const char  *control_object_name  =  "/org/openbmc/control/bmc0";
> +const char  *control_intf_name    =  "org.openbmc.control.Bmc";
> +
> +const char  *objectmapper_service_name =  "org.openbmc.objectmapper";
> +const char  *objectmapper_object_name  = 
 "/org/openbmc/objectmapper/objectmapper";
> +const char  *objectmapper_intf_name    = 
 "org.openbmc.objectmapper.ObjectMapper";
> +
> +void register_netfn_global_functions() __attribute__((constructor));
> +
> +int obj_mapper_get_connection(char** buf, const char* obj_path)
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char *temp_buf = NULL, *intf = NULL;
> +    size_t buf_size = 0;
> +    int r;
> +
> +    //Get the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +
> +    /*
> +     * Bus, service, object path, interface and method are provided to 
call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at 
the
> +     * end.
> +     */
> +    r = sd_bus_call_method(bus,
> +            objectmapper_service_name,                      /* service 
to contact */
> +            objectmapper_object_name,                       /* object 
path */
> +            objectmapper_intf_name,                         /* 
interface name */
> +            "GetObject",                                    /* method 
name */
> +            &error,                                         /* object 
to return error in */
> +            &m,                                             /* return 
message on success */
> +            "s",                                            /* input 
signature */
> +            obj_path                                        /* first 
argument */
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n", 
error.message);
> +        goto finish;
> +    }
> +
> +    // Get the key, aka, the connection name
> +    sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> +    
> + /* 
> +     * TODO: check the return code. Currently for no reason the message
> +     * parsing of object mapper is always complaining about
> +     * "Device or resource busy", but the result seems OK for now. Need
> +     *  further checks.
> +     */

I've seen this comment before and it worries me a bit. Why are we
getting the error? How do you know temp_buf is allocated properly? What
does it mean that the result "seems OK"?
You check return codes before and after this, but if you get unlucky
here you could easily segfault or read nonsense.

I think this issue should be figured out before merging the code, both
here and wherever else it appears.

Thanks,
Sam

> +
> +    buf_size = strlen(temp_buf) + 1;
> +    printf("IPMID connection name: %s\n", temp_buf);
> +    *buf = (char*)malloc(buf_size);
> +
> +    if (*buf == NULL) {
> +        fprintf(stderr, "Malloc failed for warm reset");
> +        r = -1;
> +        goto finish;
> +    }
> +
> +    memcpy(*buf, temp_buf, buf_size);
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +
> +    return r;
> +}
> +
> +int dbus_warm_reset()
> +{
> +    sd_bus_error error = SD_BUS_ERROR_NULL;
> +    sd_bus_message *m = NULL;
> +    sd_bus *bus = NULL;
> +    char* temp_buf = NULL;
> +    uint8_t* get_value = NULL;
> +    char* connection = NULL;
> +    int r, i;
> +
> +    r = obj_mapper_get_connection(&connection, control_object_name);
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to get connection, return value: 
%d.\n", r);
> +        goto finish;
> +    }
> +
> +    printf("connection: %s\n", connection);
> +
> +    // Open the system bus where most system services are provided.
> +    bus = ipmid_get_sd_bus_connection();
> +    
> +    /*
> +     * Bus, service, object path, interface and method are provided to 
call
> +     * the method.
> +     * Signatures and input arguments are provided by the arguments at 
the
> +     * end.
> +  */
> +    r = sd_bus_call_method(bus,
> +            connection,                                /* service to 
contact */
> +            control_object_name,                       /* object path 
*/
> +            control_intf_name,                         /* interface 
name */
> +            "warmReset",                             /* method name */
> +            &error,                                    /* object to 
return error in */
> +            &m,                                        /* return 
message on success */
> +            NULL,
> +            NULL
> +            );
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Failed to issue method call: %s\n", 
error.message);
> +        goto finish;
> +    }
> +
> +finish:
> +    sd_bus_error_free(&error);
> +    sd_bus_message_unref(m);
> +    free(connection);
> +
> +    return r;
> +}
> +
> +ipmi_ret_t ipmi_global_warm_reset(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                              ipmi_request_t request, ipmi_response_t 
response,
> +                              ipmi_data_len_t data_len, ipmi_context_t 
context)
> +{
> +    printf("Handling GLOBAL warmReset Netfn:[0x%X], 
Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // TODO: call the correct dbus method for warmReset.
> +    dbus_warm_reset();
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +    *data_len = 0;
> +    return rc;
> +}
> +
> +ipmi_ret_t ipmi_global_wildcard_handler(ipmi_netfn_t netfn, ipmi_cmd_t 
cmd,
> +                              ipmi_request_t request, ipmi_response_t 
response,
> +                              ipmi_data_len_t data_len, ipmi_context_t 
context)
> +{
> +    printf("Handling WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> +
> +    // Status code.
> +    ipmi_ret_t rc = IPMI_CC_OK;
> +
> +    *data_len = strlen("THIS IS WILDCARD");
> +
> +    // Now pack actual response
> +    memcpy(response, "THIS IS WILDCARD", *data_len);
> +
> +    return rc;
> +}
> +
> +void register_netfn_global_functions()
> +{
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, 
IPMI_CMD_WARM_RESET);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WARM_RESET, NULL, 
ipmi_global_warm_reset);
> +
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, 
IPMI_CMD_WILDCARD);
> +    ipmi_register_callback(NETFUN_APP, IPMI_CMD_WILDCARD, NULL, 
ipmi_global_wildcard_handler);
> +
> + return;
> +}
> diff --git a/globalhandler.h b/globalhandler.h
> new file mode 100644
> index 0000000..608df3b
> --- /dev/null
> +++ b/globalhandler.h
> @@ -0,0 +1,12 @@
> +#ifndef __HOST_IPMI_GLOBAL_HANDLER_H__
> +#define __HOST_IPMI_GLOBAL_HANDLER_H__
> +
> +#include <stdint.h>
> +
> +// Various GLOBAL operations under a single command.
> +enum ipmi_global_control_cmds : uint8_t
> +{
> +IPMI_CMD_WARM_RESET    = 0x02,
> +};
> +
> +#endif
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc


_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc



[-- Attachment #1.2: Type: text/html, Size: 20201 bytes --]

[-- Attachment #2: Type: image/gif, Size: 105 bytes --]