[PATCH phosphor-event] Event Logs to survive a reboot
Chris Austen
austenc at us.ibm.com
Wed Feb 3 17:22:19 AEDT 2016
Thanks for you insights, I issues this pull request specifically to get feedback. So here goes...
Chris Austen
POWER Systems Enablement Manager
(512) 286-5184 (T/L: 363-5184)
-----"openbmc" <openbmc-bounces+austenc=us.ibm.com at lists.ozlabs.org> wrote: -----
To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
From: Joel Stanley
Sent by: "openbmc"
Date: 02/03/2016 12:09AM
Cc: OpenBMC Maillist <openbmc at lists.ozlabs.org>
Subject: Re: [PATCH phosphor-event] Event Logs to survive a reboot
On Wed, Feb 3, 2016 at 3:40 AM, OpenBMC Patches
<openbmc-patches at stwcx.xyz> wrote:
> From: Chris Austen <austenc at us.ibm.com>
>
> The event service stores the events from the host. Before this commit all the code was in memory. That meant once a reboot occured you lost the logs. With this commit I am storing all logs to the file system. This refactoring also enables a test suite. The suite can be invoked by 'make test; ./test'.
'make check' is commonly used instead of test. Either way, the make
target should run the test for you.
CA: Sure I can make that change
>
> 1) /var/sys/obmc/events/x
/var/log is generally where logs are stored.
CA: Typical location for any log from any service? I would imagine a single log file is standard and not a bunch of small ones.
> 2) logs are stored in binary and saved
> 3) Each log now has a timestamp of when the log reached the service
> 4) I added a caching mechanism to reduce the io traffic if the the method is simply being called to capture the next parameter
> 5) You will notice a mixture of c/c++. This is because I want the test infrastructure to take advantage of c++ features
>
> Useful REST calls
> ~~~~~~~~~~~~~~~~~
> Note, dont forget to login first
>
> List all events
> curl -k https://bmc/org/openbmc/records/events/
>
> Create an event log for testing purposes
> curl -c cjar -b cjar -k -H "Content-Type: application/json" -X POST -d "{\"data\": []}" https://bmc/org/openbmc/records/events/action/acceptTestMessage
>
> Delete an event
> curl -c cjar -b cjar -k -H "Content-Type: application/json" -X POST -d "{\"data\": []}" https://bmc/org/openbmc/records/events/1/action/delete
>
> Delete all events
> curl -c cjar -b cjar -k -H "Content-Type: application/json" -X POST -d "{\"data\": []}" https://bmc/org/openbmc/records/events/action/clear
Should this go into a README?
CA: Yes it will
> ---
> Makefile | 29 ++--
> event_messaged.C | 44 +++++
> event_messaged.c | 341 ---------------------------------------
It looks like you've moved and modified this file in the same commit.
The move should be a separate commit, so we can review the code
changes.
CA: huge refactor best to review that in its entirety. I could simply rename it in a commit but that is kind of lame
> event_messaged_sdbus.c | 425 +++++++++++++++++++++++++++++++++++++++++++++++++
> event_messaged_sdbus.h | 14 ++
> message.C | 224 ++++++++++++++++++++++++++
> message.H | 60 +++++++
> test.C | 92 +++++++++++
> 8 files changed, 878 insertions(+), 351 deletions(-)
> create mode 100644 event_messaged.C
> delete mode 100644 event_messaged.c
> create mode 100644 event_messaged_sdbus.c
> create mode 100644 event_messaged_sdbus.h
> create mode 100644 message.C
> create mode 100644 message.H
> create mode 100644 test.C
>
> diff --git a/Makefile b/Makefile
> index 4c2ba9b..903ac06 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,27 +1,36 @@
> -TEST = listfunc
> -OBJS_TEST = $(TEST).o
>
> -EXE = event_messaged
> +TEST = test
>
> -OBJS = $(EXE).o \
> - list.o \
> +EXE = event_messaged
> +EXE_OBJS = $(EXE).o message.o event_messaged_sdbus.o list.o
>
> +OBJS_TEST = $(TEST).o message.o
> +
> +CPPFLAGS += -g -fpic -Wall -std=c++11
>
> DEPPKGS = libsystemd
> -CC ?= $(CROSS_COMPILE)gcc
> INCLUDES += $(shell pkg-config --cflags $(DEPPKGS))
> LIBS += $(shell pkg-config --libs $(DEPPKGS))
>
> +
> +
> all: $(EXE)
>
> %.o : %.c
> $(CC) -c $< $(CFLAGS) $(INCLUDES) -o $@
>
> -$(EXE): $(OBJS)
> - $(CC) $^ $(LDFLAGS) $(LIBS) -o $@
> +%.o : %.C
> + $(CXX) -c $< $(CPPFLAGS) -o $@
> +
> +
> +$(EXE): $(EXE_OBJS)
> + $(CXX) $^ $(LDFLAGS) $(LIBS) -o $@
> +
>
> $(TEST): $(OBJS_TEST)
> - $(CC) $^ $(LDFLAGS) $(LIBS) -o $@
> + $(CXX) $^ $(LDFLAGS) -o $@
>
> clean:
> - rm -f $(OBJS) $(EXE) *.o *.d
> + rm -f $(TEST) *.o *.so $(EXE)
> +
> +
> diff --git a/event_messaged.C b/event_messaged.C
> new file mode 100644
> index 0000000..4376431
> --- /dev/null
> +++ b/event_messaged.C
This file only contains C, so perhaps make it a C file?
> @@ -0,0 +1,44 @@
> +#include <iostream>
> +#include "message.H"
> +#include "event_messaged_sdbus.h"
> +
> +const char *glogname = "/var/lib/obmc/events";
> +
> +using namespace std;
> +
> +
Extra whitespace.
> +// Walk through all the saved logs and reload them in to dbus objects
> +// Needed because on a reboot the existing logs need to be reloaded
> +int load_existing_events(void) {
> +
> + event_log_list log_list;
> + uint16_t logid=0;
Don't initalise this here.
CA: ok
> +
> + while (message_get_next_log(&logid, &log_list)) {
> + send_log_to_dbus(logid);
> + }
> +
> + return 0;
> +}
> +
> +int main(int argc, char *argv[]) {
> +
> + int rc = 0;
Don't initalise this here.
CA: ok
> +
> + if ((rc = build_bus()) < 0) {
This is more readable:
CA: ok
rc = build_bus();
if (rc < 0)
> + fprintf(stderr, "Event Messager failed to connect to dbus rc=%d", rc);
> + goto finish;
> + }
> +
> + if ((rc = load_existing_events()) < 0) {
> + fprintf(stderr, "Event Messager failed add previous logs to dbus rc=%d", rc);
> + goto finish;
> + }
> +
> + rc = start_event_monitor();
> +
> + finish:
This should not be indented.
CA: ok
> + cleanup_event_monitor();
> +
> + return rc;
> +}
> \ No newline at end of file
> diff --git a/message.C b/message.C
> new file mode 100644
> index 0000000..31a545a
> --- /dev/null
> +++ b/message.C
> @@ -0,0 +1,224 @@
> +#include <iostream>
> +#include <fstream>
> +#include <iomanip>
> +#include <cstdint>
> +#include <string>
> +#include <sys/types.h>
> +#include <dirent.h>
> +#include <sstream>
> +#include <sys/stat.h>
> +#include <cstring>
> +#include "message.H"
> +#include <time.h>
> +#include <stddef.h>
> +#include <cstdio>
> +
> +using namespace std;
> +
> +const uint32_t g_eyecatcher = 0x4F424D43; // OBMC
> +const uint16_t g_version = 1;
> +
> +struct logheader_t {
> + uint32_t eyecatcher;
> + uint16_t version;
> + uint16_t logid;
> + time_t timestamp;
> + uint16_t detailsoffset;
> + uint16_t messagelen;
> + uint16_t severitylen;
> + uint16_t associationlen;
> + uint16_t reportedbylen;
> + uint16_t debugdatalen;
> +};
> +
> +uint16_t g_logid = 0;
> +
> +extern const char *glogname;
> +
> +static uint16_t get_new_log_number(void) {
> + return ++g_logid;
> +}
> +
> +static void update_latest_logid(uint16_t logid) {
> + if (logid > g_logid)
> + g_logid = logid;
> + return;
> +}
> +
> +inline uint16_t GETLEN(const char *s) {
> + return (uint16_t) (1 + strlen(s));
> +}
This should be lower case.
CA: ok
> +
> +static uint16_t create_log_event(event_record_t *rec) {
> +
> + std::ostringstream buffer;
> + ofstream myfile;
> + logheader_t hdr;
_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc
More information about the openbmc
mailing list