Code review comment for lp:~christof-mroz/hipl/mock-functions

> > === modified file 'Makefile.am'
> > --- Makefile.am 2011-03-25 17:51:40 +0000
> > +++ Makefile.am 2011-04-03 22:12:27 +0000
> > @@ -23,7 +23,7 @@
> >
> > ACLOCAL_AMFLAGS = -I m4
> >
> > -HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h
> test/*/*.h test/*/*/*.h
> > +HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h
> test/*/*.h test/*/*/*.h test/mocks.h
>
> Use test/*.h to prepare for future headers.

OK (my idea was to prevent accidental inclusion of a stray header: test/ is currently not supposed to contain other headers than mocks.h)

> > +
> > +/*** time(2) ***/
> > +
>
> Why do you need to overload time()? Contrary to system(), this should not have
> side-effects when executed from a unit-test.

To simulate clock skews, for example (which are handled by periodic cleanup).

> > +/*** system(3) ***/
> > +
> > +bool mock_system = false;
> > +char *mock_system_last = NULL;
> > +int mock_system_exit = EXIT_SUCCESS;
> > +
> > +/**
> > + * system(3) mock function. Controlled by the ::mock_system flag.
> > + * Stores a copy of @a command in ::mock_system_last, if enabled.
>
> mock_system_exit seems to be more interesting to talk about here.

Oops, forgot to document the globals...

> > + * @param command A copy of this string will be stored in
> ::mock_system_last of
> > + * non-NULL. Otherwise, ::mock_system_last is set to NULL.
> > + * @return The value of ::mock_system_exit if @a command was non-
> NULL, -1
> > + * otherwise.
> > + */
>
> The usage is unclear to me. Why do I need mock_system_last?

See the hipfw-esp-speedup branch for an example: There, we expect a certain command line to be built, which can be checked by comparing the last command executed.