[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
cyrilbur at gmail.com
Tue Feb 2 16:24:50 AEDT 2016
On Tue, 2 Feb 2016 15:13 Norman James <njames at 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 at 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 at mendozajonas.com>
> To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
> Cc: openbmc at lists.ozlabs.org, William <bjlinan at 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 at lists.ozlabs.org>
> ------------------------------
>
>
>
>
> Hi William, a question below;
>
> On Mon, Feb 01, 2016 at 12:40:25AM -0600, OpenBMC Patches wrote:
> > From: William <bjlinan at 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 at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/openbmc
>
> _______________________________________________
> openbmc mailing list
> openbmc at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
>
>
> _______________________________________________
> openbmc mailing list
> openbmc at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160202/4a0eefb4/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160202/4a0eefb4/attachment-0001.gif>
More information about the openbmc
mailing list