[PATCH phosphor-event] Event Logs to survive a reboot

[Joel Stanley]

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.

>
> 1) /var/sys/obmc/events/x

/var/log is generally where logs are stored.

> 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?

> ---
>  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.

>  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.

> +
> +    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.

> +
> +    if ((rc = build_bus()) < 0) {

This is more readable:

 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.

> +    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.

> +
> +static uint16_t create_log_event(event_record_t *rec) {
> +
> +    std::ostringstream buffer;
> +    ofstream myfile;
> +    logheader_t hdr;