[PATCH phosphor-host-ipmid] Add ipmi cold-reset command, which call a dbus method. 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.
Cyril Bur
cyrilbur at gmail.com
Tue Jan 5 11:01:48 AEDT 2016
On Thu, 31 Dec 2015 11:50:25 -0600
OpenBMC Patches <openbmc-patches at stwcx.xyz> wrote:
> From: William <bjlinan at cn.ibm.com>
>
Hi William,
I reviewed "[PATCH phosphor-host-ipmid v3] Add get/set ipmid command support
with correct DBUS property handling." yesterday and a lot of the comments I
wrote in there apply here, also I notice a lot of identical code, perhaps
instead of duplicating code some effort could be put into writing routines that
could be shared amongst all these handlers?
> ---
> Makefile | 2 +-
> globalhandler.C | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> globalhandler.h | 12 +++++
> 3 files changed, 173 insertions(+), 1 deletion(-)
> create mode 100644 globalhandler.C
> create mode 100644 globalhandler.h
>
> diff --git a/Makefile b/Makefile
> index 57ad6fe..2c9b37e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,7 +15,7 @@ LIB_APP_OBJ = apphandler.o \
> ipmisensor.o \
> storageaddsel.o \
> transporthandler.o \
> -
> + globalhandler.o \
>
The last one probably shouldn't escape the newline...
> TESTADDSEL_OBJ = $(TESTADDSEL).o \
> storageaddsel.o
> diff --git a/globalhandler.C b/globalhandler.C
> new file mode 100644
> index 0000000..0e2aec4
> --- /dev/null
> +++ b/globalhandler.C
> @@ -0,0 +1,160 @@
> +#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;
> +
> + // Open the system bus where most system services are provided.
> + r = sd_bus_open_system(&bus);
> + if (r < 0) {
> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));
> + goto finish;
> + }
> +
> + // 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.
So how sure are you that what you're doing now is robust?
> + //r = sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> + //if (r < 0) {
> + // fprintf(stderr, "Failed to parse response message: %s\n", strerror(-r));
> + // goto finish;
> + //}
Please don't send commented code
> +
> + 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 get_sys_boot_options");
> + r = -1;
> + goto finish;
> + }
> +
> + memcpy(*buf, temp_buf, buf_size);
> +
> +finish:
> + sd_bus_error_free(&error);
> + sd_bus_message_unref(m);
> + sd_bus_unref(bus);
> +
> + return r;
> +}
> +
> +int dbus_cold_reset()
> +{
While they aren't identical, dbus_cold_reset() and dbus_get_property() (from
the previous series I reviewed share a lot, is this the approach the best way
of doing this, how many more commands will share this very similar code.
> + 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.
> + r = sd_bus_open_system(&bus);
> + if (r < 0) {
> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));
> + goto finish;
> + }
> +
> + // 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.
> + // TODO: the method is a place holder, not cold reset, need to call the correct
> + // method.
So this patch doesn't actually do what it sets out to do?
> + r = sd_bus_call_method(bus,
> + connection, /* service to contact */
> + control_object_name, /* object path */
> + control_intf_name, /* interface name */
> + "place_holder", /* 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);
> + sd_bus_unref(bus);
> + free(connection);
> +
> + return r;
> +}
> +
> +ipmi_ret_t ipmi_global_cold_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 COLD RESET Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> +
> + // TODO: call the correct dbus method for cold reset.
> + // Currently the reboot method called is just a placeholder.
> + dbus_cold_reset();
> +
> + // Status code.
> + ipmi_ret_t rc = IPMI_CC_OK;
> + *data_len = 0;
> + return rc;
> +}
> +
> +void register_netfn_global_functions()
> +{
> + printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_APP, IPMI_CMD_COLD_RESET);
> + ipmi_register_callback(NETFUN_APP, IPMI_CMD_COLD_RESET, NULL, ipmi_global_cold_reset);
> +}
> diff --git a/globalhandler.h b/globalhandler.h
> new file mode 100644
> index 0000000..185e7df
> --- /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_COLD_RESET = 0x02,
> +};
> +
> +#endif
More information about the openbmc
mailing list