Re: GAPK merge request

[Vadim Yanitskiy]

Hi Harald,

> I've done some casual review and it looks great to me.

Thanks.

> I would suggest internal symbols (those not with osmo_gapk_ prefix,
> like gapk_log_subsys, LOGPAPK, ...) use an internal header file which
> is not installed in the system. The header files installed during
> 'make install' should only describe the external API of the library

The mentioned logging related symbols are not installed:

> noinst_HEADERS = \
> osmocom/gapk/logging.h \
> ...

The only thing I am not sure about is 'get_cycles.h' header. At the
moment, it is installed together with other headers, because it's
a dependency of 'benchmark.h'. But, it seems I just got an idea,
how to avoid this dependency - simply use 'unsigned long long'
instead of the 'cycles_t', which is imported from 'get_cycles.h'.

> the source/sink/coec category name stringification should not
> define those strings in every file, but have a value_string
> or at least some #defines or the like.

Thanks, good idea.

> I suggest you simply rebase your chain of commits on current
> master and push it into gerrit.

But this way, each commit of the whole chain would require +2
and V+1, because 'make check' was implemented at the end of
the chain.

What if I merge 'fixeria/lib' into a local 'master' and send
it for review? I think, this would be simpler. Or would this
anyway send the whole chain to review? :)


With best regards,
Vadim Yanitskiy.

Re: GAPK merge request

[Harald Welte]

Hi Vadim,

just some feedback "from the side":

On Sun, Jan 14, 2018 at 02:19:46AM +0600, Vadim Yanitskiy wrote:
> http://git.osmocom.org/gapk/log/?h=fixeria/lib

I've done some casual review and it looks great to me.  There are
some minor issues:

* I would suggest internal symbols (those not with osmo_gapk_ prefix,
  like gapk_log_subsys, LOGPAPK, ...)
  use an internal header file which is not installed in the system.
  The header files installed during 'make install' should only describe
  the external API of the library

* the source/sink/coec category name stringification at
  
http://git.osmocom.org/gapk/commit/?h=fixeria/lib=459791c488c6b66a5cd0d7cff9392a7a0b8ca733
  should not define those strings in every file, but have a value_string
  or at least some #defines or the like.

This is just my point of view, detailed review of course depends on Sylvain.

I suggest you simply rebase your chain of commits on current master and push
it into gerrit.  That's what we have it for, and as indicated in
http://lists.osmocom.org/pipermail/openbsc/2017-December/011523.html
gapk has been introduced to gerrit.

Regards,
Harald
-- 
- Harald Welte    http://laforge.gnumonks.org/

"Privacy in residential applications is a desirable marketing option."
  (ETSI EN 300 175-7 Ch. A6)