Create a bootstat library for external C and C++ programs

Create a bootstat library for external C and C++ programs

BUG=none
TEST=emerge-x86-generic bootstat

[jrbarnette, 2010-08-10 21:23:58]

This is one of a trio of changes to allow flimflam to report boot time metrics
for network state changes.  These are the other two CLs:

Ebuild changes:
  http://codereview.chromium.org/3106004/show

Flimflam changes:
  http://codereview.chromium.org/3117004/show

[jrbarnette, 2010-08-10 21:36:31]

On 2010/08/10 21:23:58, jrbarnette wrote:
> This is one of a trio of changes to allow flimflam to report boot time metrics
> for network state changes. [ ... ]

Note that this CL also contains an otherwise unrelated bug fix to
make disk statistics work.  The fix isn't needed as part of the
work for the flimflam change, but it *is* needed for a forthcoming
change to the 'init' package.

[rochberg, 2010-08-11 01:32:21]

LGTM, though I'd love to see a tiny little unittest

http://codereview.chromium.org/3129002/diff/5001/6002
File bootstat.h (right):

http://codereview.chromium.org/3129002/diff/5001/6002#newcode16
bootstat.h:16: // that exceed this buffer size may be truncated.
"may" seems a little soft---we should guarantee truncation.

http://codereview.chromium.org/3129002/diff/5001/6003
File bootstat_log.c (right):

http://codereview.chromium.org/3129002/diff/5001/6003#newcode69
bootstat_log.c:69: ofprefix, BOOTSTAT_MAX_EVENT_LEN - 1, event_id);
Why sizeof() - 1 ?
Similarly, why BOOTSTAT_MAX_EVENT_LEN - 1?

If you split this string mangling out, it would be easy to write a simple unit
test that would give you confidence on the corner cases here.

[adlr, 2010-08-11 02:15:00]

couple nits. i suggest you review the google c++ style guide and apply for
readability.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

http://codereview.chromium.org/3129002/diff/5001/6001
File Makefile (right):

http://codereview.chromium.org/3129002/diff/5001/6001#newcode1
Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
is there an implicit
%.o: %.c
rule or something? how do the .o files get generated?

http://codereview.chromium.org/3129002/diff/5001/6002
File bootstat.h (right):

http://codereview.chromium.org/3129002/diff/5001/6002#newcode29
bootstat.h:29: #endif // BOOTSTAT_H_
2 spaces before //

http://codereview.chromium.org/3129002/diff/5001/6003
File bootstat_log.c (right):

http://codereview.chromium.org/3129002/diff/5001/6003#newcode13
bootstat_log.c:13: #include "bootstat.h"
consider putting this as your first include, to make sure the .h file has the
required headers

http://codereview.chromium.org/3129002/diff/5001/6003#newcode45
bootstat_log.c:45: static void append_logdata(const char *ifname,
one arg per line if not all args on one line

http://codereview.chromium.org/3129002/diff/5001/6003#newcode68
bootstat_log.c:68: (void) snprintf(ofpath, sizeof (ofpath) - 1, "/tmp/%s-%.*s",
one arg per line

http://codereview.chromium.org/3129002/diff/5001/6003#newcode70
bootstat_log.c:70: ofpath[sizeof (ofpath) - 1] = '\0';
no space after sizeof

[jrbarnette, 2010-08-11 02:27:47]

In the interest of clarifying and streamlining my outstanding review requests,
I've moved some reviewers to the Cc: list.

The four outstanding reviews all interact, so if you're listed as a reviewer for
one, you'll benefit from awareness of the other three.  Additionally, some of
you have in interest in using the code I'm adding, so you're included on
principle.

[jrbarnette, 2010-08-11 03:26:55]

On Aug 10, 2010, at 7:15 PM, adlr@chromium.org wrote:

> couple nits. i suggest you review the google c++ style guide and  
> apply for
> readability.
>
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
>
I did, actually.  Seems that some of the rules are specified
a bit obliquely.  :-(


>
>
> http://codereview.chromium.org/3129002/diff/5001/6001
> File Makefile (right):
>
> http://codereview.chromium.org/3129002/diff/5001/6001#newcode1
> Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights
> reserved.
> is there an implicit
> %.o: %.c
> rule or something? how do the .o files get generated?
>
Yup, implicit rule.  I could make it explicit, if it'll make
you feel better.


> http://codereview.chromium.org/3129002/diff/5001/6002
> File bootstat.h (right):
>
> http://codereview.chromium.org/3129002/diff/5001/6002#newcode29
> bootstat.h:29: #endif // BOOTSTAT_H_
> 2 spaces before //
>
Yes, I see now in the template in the style guide that there are
two spaces.  It's only visible if you know to look for it, and
the text doesn't say anything.  :-(


> http://codereview.chromium.org/3129002/diff/5001/6003
> File bootstat_log.c (right):
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode13
> bootstat_log.c:13: #include "bootstat.h"
> consider putting this as your first include, to make sure the .h file
> has the required headers
>
Um, I'm not especially keen on that on principle.  Beyond my personal
opinion, the style guide specifies an ordering, which says
"your project's .h" goes last...


> http://codereview.chromium.org/3129002/diff/5001/6003#newcode45
> bootstat_log.c:45: static void append_logdata(const char *ifname,
> one arg per line if not all args on one line
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode68
> bootstat_log.c:68: (void) snprintf(ofpath, sizeof (ofpath) - 1,
> "/tmp/%s-%.*s",
> one arg per line
>
The style guide cites four formats as valid.  In order:
   foo(a, b, c);
   foo(a,
       b, c);
   foo(a,
       b,
       c);
   foo(
     a,
     b,
     c);

It seems to suggest (by implication) picking the first of these that
actually fits in the available space.  If it still bothers you, I'm
not wedded to any particular format.


> http://codereview.chromium.org/3129002/diff/5001/6003#newcode70
> bootstat_log.c:70: ofpath[sizeof (ofpath) - 1] = '\0';
> no space after sizeof
>
Another whitespace requirement spelled out merely in the template
and not the text.  :-(


> http://codereview.chromium.org/3129002/show

-- jrb

[petkov, 2010-08-11 04:44:29]

A few style nits and some random remarks related to these general comments:

- Using C++ would help you reduce the meaningful line count of this utility even
further by leveraging routines from libbase.

- I don't think it's a good idea to add new code to the Chromium OS code base
with no unit tests. You should at least consider adding a BVT autotest.

http://codereview.chromium.org/3129002/diff/5001/6002
File bootstat.h (right):

http://codereview.chromium.org/3129002/diff/5001/6002#newcode22
bootstat.h:22: #define BOOTSTAT_MAX_EVENT_LEN  64
C++ style discourages macros.

http://codereview.chromium.org/3129002/diff/5001/6003
File bootstat_log.c (right):

http://codereview.chromium.org/3129002/diff/5001/6003#newcode19
bootstat_log.c:19: #define UPTIME    "/proc/uptime"
No macros, per style.

static const char kUptime[] = "/proc/uptime";

http://codereview.chromium.org/3129002/diff/5001/6003#newcode22
bootstat_log.c:22: #define ROOTDEV   "sda"
No macros, per style.

static const char kRootDev[] = "sda";

http://codereview.chromium.org/3129002/diff/5001/6003#newcode38
bootstat_log.c:38: #define MAX_STAT_PATH  128
No macros, per style.

http://codereview.chromium.org/3129002/diff/5001/6003#newcode43
bootstat_log.c:43: #define OFMODE 
(S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)
No macros, per style.

http://codereview.chromium.org/3129002/diff/5001/6003#newcode45
bootstat_log.c:45: static void append_logdata(const char *ifname,
stars to the left, per Chromium C++ style.

const char* ifname

http://codereview.chromium.org/3129002/diff/5001/6003#newcode68
bootstat_log.c:68: (void) snprintf(ofpath, sizeof (ofpath) - 1, "/tmp/%s-%.*s",
StringPrintf from libbase would reduce some of the complexity.

http://codereview.chromium.org/3129002/diff/5001/6003#newcode71
bootstat_log.c:71: ofd = open(ofpath, O_WRONLY | O_APPEND | O_CREAT, OFMODE);
All the code from here down can be replaced by CopyFile from libbase.

[jrbarnette, 2010-08-11 17:44:23]

On 2010/08/11 01:32:21, rochberg wrote:
> LGTM, though I'd love to see a tiny little unittest
> 
Yeah, the truncation boundary case could use a test.
My main hold up has been knowing what's the right
framework to put in place so the tests can grow.


> http://codereview.chromium.org/3129002/diff/5001/6002
> File bootstat.h (right):
> 
> http://codereview.chromium.org/3129002/diff/5001/6002#newcode16
> bootstat.h:16: // that exceed this buffer size may be truncated.
> "may" seems a little soft---we should guarantee truncation.
> 
Well, I chose the word deliberately, but maybe not for a good reason.
My thinking was to binary compatibility in the event of a change:
If we increase the maximum, code compiled against the old version
of the header (this version) that also depended on truncation would
become invalid.  The word "may" solves that issue by telling clients
not to rely on truncation.  However, it might make more sense just
to expect that all clients will be recompiled in the event of a change.


> http://codereview.chromium.org/3129002/diff/5001/6003
> File bootstat_log.c (right):
> 
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode69
> bootstat_log.c:69: ofprefix, BOOTSTAT_MAX_EVENT_LEN - 1, event_id);
> Why sizeof() - 1 ?

It's wrong.  I was assuming that snprintf() wouldn't add a NUL to
a full buffer.  ITOT snprintf() guarantees termination.


> Similarly, why BOOTSTAT_MAX_EVENT_LEN - 1?
> 
BOOTSTAT_MAX_EVENT_LEN includes 1 for a terminating
NUL character.  The "%.*s" output won't include the NUL,
so we need the -1.  It's a nuisance, but the alternative is
to require all clients to have a '+1' for allocations, to account
for the NUL byte.


> If you split this string mangling out, it would be easy to write a simple unit
> test that would give you confidence on the corner cases here.

The mangling can easily be tested against the code as is.  I've already
tested it, and I have full confidence in the truncation.  That said, a unit
test to prove it would help; I'll be asking around today to figure out the
best way to incorporate it.

[jrbarnette, 2010-08-11 18:15:27]

On Aug 10, 2010, at 9:44 PM, petkov@chromium.org wrote:

> A few style nits and some random remarks related to these general  
> comments:
>
> - Using C++ would help you reduce the meaningful line count of this  
> utility even
> further by leveraging routines from libbase.
>
Whether or not it reduced the meaningful line count, it would
probably about double the unmeaningful line count.  The library
must have both C and C++ bindings.  Currently, that problem is
solved by writing the code in C, and using the same header for
both bindings.  If the library is in C++, there would have to
be a separate header and source to provide the separate bindings.
For this code in its current state, I don't see the extra
boilerplate as a win.

It would be good to have a pointer to the libbase spec; I expect
sooner or later it will be useful.


> - I don't think it's a good idea to add new code to the Chromium OS  
> code base
> with no unit tests. You should at least consider adding a BVT  
> autotest.
>
There's already a BVT test:  platform_BootPerf.  The point
of the current exercise is to add bootperf without breaking
that test case or the code in chrome that depends on the
timestamp files.  Eventually, the platform_BootPerf test
needs to change to use a bootstat interface for extracting
the data created by bootstat_log().  That interface doesn't
exist yet (because there's no client for it until we change
autotest or chrome).

Once there's a specified interface for extracting data
from bootstat, there'll be a very real need for unit tests
that prove that the data going into bootstat_log() can be
extracted properly; until that time, there's not a lot of
behavior for unit tests to cover.

>
>
> http://codereview.chromium.org/3129002/diff/5001/6002
> File bootstat.h (right):
>
> http://codereview.chromium.org/3129002/diff/5001/6002#newcode22
> bootstat.h:22: #define BOOTSTAT_MAX_EVENT_LEN  64
> C++ style discourages macros.
>
For this header to be usable by C code, the constant needs
to be defined as a macro.  A variable declared as
"const int" can't be used as the size for a statically
allocated array.


> http://codereview.chromium.org/3129002/diff/5001/6003
> File bootstat_log.c (right):
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode19
> bootstat_log.c:19: #define UPTIME    "/proc/uptime"
> No macros, per style.
>
> static const char kUptime[] = "/proc/uptime";
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode22
> bootstat_log.c:22: #define ROOTDEV   "sda"
> No macros, per style.
>
> static const char kRootDev[] = "sda";
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode38
> bootstat_log.c:38: #define MAX_STAT_PATH  128
> No macros, per style.
>
As long as this code is implemented in C, it's better that
this be a macro (see my comment about about array sizes).


> http://codereview.chromium.org/3129002/diff/5001/6003#newcode43
> bootstat_log.c:43: #define OFMODE
> (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)
> No macros, per style.
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode45
> bootstat_log.c:45: static void append_logdata(const char *ifname,
> stars to the left, per Chromium C++ style.
>
> const char* ifname
>
I've scoured the style guide, and I couldn't find this requirement.
Could you point me to which section?


> http://codereview.chromium.org/3129002/diff/5001/6003#newcode68
> bootstat_log.c:68: (void) snprintf(ofpath, sizeof (ofpath) - 1,
> "/tmp/%s-%.*s",
> StringPrintf from libbase would reduce some of the complexity.
>
> http://codereview.chromium.org/3129002/diff/5001/6003#newcode71
> bootstat_log.c:71: ofd = open(ofpath, O_WRONLY | O_APPEND | O_CREAT,
> OFMODE);
> All the code from here down can be replaced by CopyFile from libbase.
>
> http://codereview.chromium.org/3129002/show

-- jrb

[petkov, 2010-08-11 18:26:03]

It's helpful if you respond to reviews by clicking Reply on each individual
comment.

On 2010/08/11 18:15:27, jrbarnette wrote:
> On Aug 10, 2010, at 9:44 PM, mailto:petkov@chromium.org wrote:
> 
> > A few style nits and some random remarks related to these general  
> > comments:
> >
> > - Using C++ would help you reduce the meaningful line count of this  
> > utility even
> > further by leveraging routines from libbase.
> >
> Whether or not it reduced the meaningful line count, it would
> probably about double the unmeaningful line count.  The library
> must have both C and C++ bindings.  Currently, that problem is
> solved by writing the code in C, and using the same header for
> both bindings.  If the library is in C++, there would have to
> be a separate header and source to provide the separate bindings.
> For this code in its current state, I don't see the extra
> boilerplate as a win.
> 
> It would be good to have a pointer to the libbase spec; I expect
> sooner or later it will be useful.
> 
> 
> > - I don't think it's a good idea to add new code to the Chromium OS  
> > code base
> > with no unit tests. You should at least consider adding a BVT  
> > autotest.
> >
> There's already a BVT test:  platform_BootPerf.  The point

OK. Definitely worse than a unit test but better than no explicit testing at
all.

> of the current exercise is to add bootperf without breaking
> that test case or the code in chrome that depends on the
> timestamp files.  Eventually, the platform_BootPerf test
> needs to change to use a bootstat interface for extracting
> the data created by bootstat_log().  That interface doesn't
> exist yet (because there's no client for it until we change
> autotest or chrome).
> 
> Once there's a specified interface for extracting data
> from bootstat, there'll be a very real need for unit tests
> that prove that the data going into bootstat_log() can be
> extracted properly; until that time, there's not a lot of
> behavior for unit tests to cover.
> 
> >
> >
> > http://codereview.chromium.org/3129002/diff/5001/6002
> > File bootstat.h (right):
> >
> > http://codereview.chromium.org/3129002/diff/5001/6002#newcode22
> > bootstat.h:22: #define BOOTSTAT_MAX_EVENT_LEN  64
> > C++ style discourages macros.
> >
> For this header to be usable by C code, the constant needs
> to be defined as a macro.  A variable declared as
> "const int" can't be used as the size for a statically
> allocated array.
> 
> 
> > http://codereview.chromium.org/3129002/diff/5001/6003
> > File bootstat_log.c (right):
> >
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode19
> > bootstat_log.c:19: #define UPTIME    "/proc/uptime"
> > No macros, per style.
> >
> > static const char kUptime[] = "/proc/uptime";
> >
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode22
> > bootstat_log.c:22: #define ROOTDEV   "sda"
> > No macros, per style.
> >
> > static const char kRootDev[] = "sda";
> >
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode38
> > bootstat_log.c:38: #define MAX_STAT_PATH  128
> > No macros, per style.
> >
> As long as this code is implemented in C, it's better that
> this be a macro (see my comment about about array sizes).
> 
> 
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode43
> > bootstat_log.c:43: #define OFMODE
> > (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)
> > No macros, per style.
> >
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode45
> > bootstat_log.c:45: static void append_logdata(const char *ifname,
> > stars to the left, per Chromium C++ style.
> >
> > const char* ifname
> >
> I've scoured the style guide, and I couldn't find this requirement.
> Could you point me to which section?

I don't know if it's explicitly written anywhere.

Given that this whole utility deviates significantly from the C++ style, you
probably don't care anyway.

> 
> 
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode68
> > bootstat_log.c:68: (void) snprintf(ofpath, sizeof (ofpath) - 1,
> > "/tmp/%s-%.*s",
> > StringPrintf from libbase would reduce some of the complexity.
> >
> > http://codereview.chromium.org/3129002/diff/5001/6003#newcode71
> > bootstat_log.c:71: ofd = open(ofpath, O_WRONLY | O_APPEND | O_CREAT,
> > OFMODE);
> > All the code from here down can be replaced by CopyFile from libbase.
> >
> > http://codereview.chromium.org/3129002/show
> 
> -- jrb
> 
>

[jrbarnette, 2010-08-11 20:57:26]

On 2010/08/11 18:26:03, petkov wrote:
[ ... ]
> > > http://codereview.chromium.org/3129002/diff/5001/6003#newcode45
> > > bootstat_log.c:45: static void append_logdata(const char *ifname,
> > > stars to the left, per Chromium C++ style.
> > >
> > > const char* ifname
> > >
> > I've scoured the style guide, and I couldn't find this requirement.
> > Could you point me to which section?
> 
> I don't know if it's explicitly written anywhere.
> 
Um, if it's not written down in the coding style guide, there's
an argument to be made that it's not part of the coding style.
That said...

> Given that this whole utility deviates significantly from the C++ style, you
> probably don't care anyway.
> 
I do care about the style guide.  To the extent that there's a clear standard
that makes sense for C, that's what I want to use.

In this case I'm torn:  although the style guide is silent on the point, other
C++ code in the Chromium OS source base leans towards "char* foo" over
"char *foo".  OTOH, C code (in Chromium OS and elsewhere) leans strongly
towards the latter convention, and for the moment, this is still C code.

In the long run this is transitional code:  it's not worth any extended
discussion,
as I expect the code to be substantially rewritten as it grows.  With that
thought
in mind, I've uploaded changes to address any comment where the cost of
further discussion clearly exceeds the cost of the change.

[petkov, 2010-08-11 21:20:44]

Mostly style nits, LGTM otherwise.

I just have hard time believing that this code will be temporary and, as it
grows, somebody will rewrite it in C++ with appropriate unit tests. I hope I'm
wrong :-)

http://codereview.chromium.org/3129002/diff/5001/6001
File Makefile (right):

http://codereview.chromium.org/3129002/diff/5001/6001#newcode1
Makefile:1: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved.
On 2010/08/11 02:15:00, adlr wrote:
> is there an implicit
> %.o: %.c
> rule or something? how do the .o files get generated?

while there's an implicit rule, it's seems like a good idea to include the rule
explicitly in the makefile

http://codereview.chromium.org/3129002/diff/20001/21003
File bootstat.h (right):

http://codereview.chromium.org/3129002/diff/20001/21003#newcode24
bootstat.h:24: extern void bootstat_log(const char *);
const char*

FWIW, I prefer stars to the right too.

http://codereview.chromium.org/3129002/diff/20001/21004
File bootstat_log.c (right):

http://codereview.chromium.org/3129002/diff/20001/21004#newcode13
bootstat_log.c:13: #include "bootstat.h"
per style, this should be the first include because bootstat_log.c implements
bootstat.h.

speaking of that, the files are a bit misnamed. this file name (bootstat_log.c)
should match the name of its header (bootstat.h)

http://codereview.chromium.org/3129002/diff/20001/21004#newcode46
bootstat_log.c:46: char      ofpath[MAX_STAT_PATH];
per style (examples), don't align variables

http://codereview.chromium.org/3129002/diff/20001/21004#newcode48
bootstat_log.c:48: ssize_t   nbytes;
per style, this is probably a bad variable name due to ambiguous abbreviation

http://codereview.chromium.org/3129002/diff/20001/21004#newcode72
bootstat_log.c:72: while ((nbytes = read(ifd, buffer, sizeof (buffer))) > 0) {
remove space after sizeof?

[jrbarnette, 2010-08-11 23:11:29]

On 2010/08/11 21:20:44, petkov wrote:
[ ... ]
> http://codereview.chromium.org/3129002/diff/20001/21004#newcode13
> bootstat_log.c:13: #include "bootstat.h"
> per style, this should be the first include because bootstat_log.c implements
> bootstat.h.
> 
> speaking of that, the files are a bit misnamed. this file name
(bootstat_log.c)
> should match the name of its header (bootstat.h)
> 
Ah!  There's a substantive question here:  I anticipate that the most
significant addition to this component will be various routines for extracting
data logged via bootstat_log().  That will mean a library with a two-pronged
interface:
  * A logging interface for clients that want to log events.
  * An extraction interface for clients that want to report
    previously logged statistics.

I anticipate almost no growth in the bootstat_log() interface; I think the
single function is enough.  The extraction interface is likely to have lots
of functions.  I also expect that most clients will use one interface or the
other; that is, in a single source module, I would not expect to find code
that both logs events, and extracts them.

W.r.t. the API, we could have either a single header "bootstat.h", or two
headers, e.g. "bootstat_log.h" and "bootstat_read.h".  I've been assuming
one header.  Two headers might make more sense, but I have a concern
of hidden interactions.  In particular, BOOTSTAT_MAX_EVENT_LEN is a
name that's likely to be common to *both* headers, which argues for
a single header.

IWBN if we could get this particular decision right at the outset; making
the bootstat API stable would help avoid a repeat of the current multiple
CL odyssey on which I'm currently embarked.  I still prefer a single header
name, because I know it won't run afoul of issues like the location of
BOOTSTAT_MAX_EVENT_LEN, but there may be some consideration I've
overlooked.

[petkov-google, 2010-08-11 23:25:15]

On 2010/08/11 23:11:29, jrbarnette wrote:
> On 2010/08/11 21:20:44, petkov wrote:
> [ ... ]
> > http://codereview.chromium.org/3129002/diff/20001/21004#newcode13
> > bootstat_log.c:13: #include "bootstat.h"
> > per style, this should be the first include because bootstat_log.c
implements
> > bootstat.h.
> > 
> > speaking of that, the files are a bit misnamed. this file name
> (bootstat_log.c)
> > should match the name of its header (bootstat.h)
> > 
> Ah!  There's a substantive question here:  I anticipate that the most
> significant addition to this component will be various routines for extracting
> data logged via bootstat_log().  That will mean a library with a two-pronged
> interface:
>   * A logging interface for clients that want to log events.
>   * An extraction interface for clients that want to report
>     previously logged statistics.
> 
> I anticipate almost no growth in the bootstat_log() interface; I think the
> single function is enough.  The extraction interface is likely to have lots
> of functions.  I also expect that most clients will use one interface or the
> other; that is, in a single source module, I would not expect to find code
> that both logs events, and extracts them.
> 
> W.r.t. the API, we could have either a single header "bootstat.h", or two
> headers, e.g. "bootstat_log.h" and "bootstat_read.h".  I've been assuming
> one header.  Two headers might make more sense, but I have a concern
> of hidden interactions.  In particular, BOOTSTAT_MAX_EVENT_LEN is a
> name that's likely to be common to *both* headers, which argues for
> a single header.
> 
> IWBN if we could get this particular decision right at the outset; making
> the bootstat API stable would help avoid a repeat of the current multiple
> CL odyssey on which I'm currently embarked.  I still prefer a single header
> name, because I know it won't run afoul of issues like the location of
> BOOTSTAT_MAX_EVENT_LEN, but there may be some consideration I've
> overlooked.

I don't care much and that's why I put the comment as a side note. It is usually
better to have similarly named interface and implementation files.

As far as BOOTSTAT_MAX_EVENT_LEN is concerned, you could put it in a separate
header file, if necessary.

Although, it's hard for me to comment here because I would use a C++ interface
with "string" parameters, and then a very basic C wrapper API for the few C
clients :-)

[petkov, 2010-08-11 23:26:14]

On 2010/08/11 23:25:15, petkov-google wrote:
> On 2010/08/11 23:11:29, jrbarnette wrote:
> > On 2010/08/11 21:20:44, petkov wrote:
> > [ ... ]
> > > http://codereview.chromium.org/3129002/diff/20001/21004#newcode13
> > > bootstat_log.c:13: #include "bootstat.h"
> > > per style, this should be the first include because bootstat_log.c
> implements
> > > bootstat.h.
> > > 
> > > speaking of that, the files are a bit misnamed. this file name
> > (bootstat_log.c)
> > > should match the name of its header (bootstat.h)
> > > 
> > Ah!  There's a substantive question here:  I anticipate that the most
> > significant addition to this component will be various routines for
extracting
> > data logged via bootstat_log().  That will mean a library with a two-pronged
> > interface:
> >   * A logging interface for clients that want to log events.
> >   * An extraction interface for clients that want to report
> >     previously logged statistics.
> > 
> > I anticipate almost no growth in the bootstat_log() interface; I think the
> > single function is enough.  The extraction interface is likely to have lots
> > of functions.  I also expect that most clients will use one interface or the
> > other; that is, in a single source module, I would not expect to find code
> > that both logs events, and extracts them.
> > 
> > W.r.t. the API, we could have either a single header "bootstat.h", or two
> > headers, e.g. "bootstat_log.h" and "bootstat_read.h".  I've been assuming
> > one header.  Two headers might make more sense, but I have a concern
> > of hidden interactions.  In particular, BOOTSTAT_MAX_EVENT_LEN is a
> > name that's likely to be common to *both* headers, which argues for
> > a single header.
> > 
> > IWBN if we could get this particular decision right at the outset; making
> > the bootstat API stable would help avoid a repeat of the current multiple
> > CL odyssey on which I'm currently embarked.  I still prefer a single header
> > name, because I know it won't run afoul of issues like the location of
> > BOOTSTAT_MAX_EVENT_LEN, but there may be some consideration I've
> > overlooked.
> 
> I don't care much and that's why I put the comment as a side note. It is
usually
> better to have similarly named interface and implementation files.
> 
> As far as BOOTSTAT_MAX_EVENT_LEN is concerned, you could put it in a separate
> header file, if necessary.
> 
> Although, it's hard for me to comment here because I would use a C++ interface
> with "string" parameters, and then a very basic C wrapper API for the few C
> clients :-)

[jrbarnette, 2010-08-16 18:51:07]

PTAL.

I've updated the code with a gtest-based unit test.

I manually verified the test by temporarily introducing an off-by-one
bug in bootstat_log() in both possible directions, and confirming that
this caused a test failure.  (And of course, the test passes in its current
form).

[petkov, 2010-08-16 19:19:47]

LGTM

Glad to see basic unit tests! :-)

http://codereview.chromium.org/3129002/diff/8003/31006
File log_unit_tests.cc (right):

http://codereview.chromium.org/3129002/diff/8003/31006#newcode37
log_unit_tests.cc:37: static void TestEventByName(const string& event_name) {
You could probably also test that the event file is not empty, a single line,
something like that...

http://codereview.chromium.org/3129002/diff/8003/31006#newcode39
log_unit_tests.cc:39: string truncated_event(event_name.substr(0,
BOOTSTAT_MAX_EVENT_LEN-1));
spaces around -

http://codereview.chromium.org/3129002/diff/8003/31006#newcode51
log_unit_tests.cc:51: TestEventByName(very_long.substr(0,
BOOTSTAT_MAX_EVENT_LEN-1));
spaces around -

[jrbarnette, 2010-08-16 20:26:41]

On 2010/08/16 19:19:47, petkov wrote:
[ ... ]
> http://codereview.chromium.org/3129002/diff/8003/31006#newcode37
> log_unit_tests.cc:37: static void TestEventByName(const string& event_name) {
> You could probably also test that the event file is not empty, a single line,
> something like that...
> 
I considered it.  I decided against it in the interest of wrapping this up and
enabling more immediately valuable work.