Code review comment for lp:~fahad-aizaz/hipl/logging

Hi Fahad!

> === modified file 'hipd/hipd.c'

Please don't forget to update the Copyright dates when you modify files with
Copyright headers. Scatterbrains like myself may want to try out the pre-commit
hook in https://code.launchpad.net/~stefan.goetz/hipl/commitguard that
automatically checks for that.

> /* We need to recreate the NAT UDP sockets to bind to the new port. */
> if (getuid()) {
> - HIP_ERROR("hipd must be started as root!\n");
> + HIP_DIE("hipd must be started as root!\n");
> return EXIT_FAILURE;
> }

[L] Does this change really belong into this branch? It changes the behavior of hipd in a way that is not relate to logging policy.

> === modified file 'hipd/user.c'

Please don't forget to update the Copyright dates when you modify files with
Copyright headers. Scatterbrains like myself may want to try out the pre-commit
hook in https://code.launchpad.net/~stefan.goetz/hipl/commitguard that
automatically checks for that.

> === modified file 'lib/core/conf.c'
> + * @param debug all contains a new option "low" which includes messages
> + * with priority die/assert and error.

[M] Please remove 'new' here - once this code is committed, the option is no longer new and it's irrelevant whether it is new or not, anyway.

> === modified file 'lib/core/debug.c'
> --- lib/core/debug.c 2011-04-05 16:44:22 +0000
> +++ lib/core/debug.c 2011-06-24 13:38:10 +0000
> @@ -139,6 +139,18 @@
> */
> void hip_set_logtype(int new_logtype)

[L] policy: for boyscouting, it would be nice to make 'new_logtype' 'const'. Also, it should be of type 'enum logtype'.

> @@ -216,13 +228,14 @@
> case LOGTYPE_NOLOG:
> break;
> case LOGTYPE_STDERR:
> - if (strlen(prefix) > 0) {
> +
> + if (strlen(prefix)) {

[L] This change is ok, but then, I don't see the point of it.

> } else {
> - /* LOGFMT_SHORT: no prefix */
> + // LOGFMT_SHORT: no prefix

[L] same - this cosmetic cleanup has nothing to do with this branch

> === modified file 'lib/core/debug.h'
> --- lib/core/debug.h 2011-01-10 15:13:47 +0000
> +++ lib/core/debug.h 2011-06-24 13:38:10 +0000
> @@ -233,7 +233,7 @@
> * messed up. You can find the implementation of these macros from lib/core/debug.h.
> * @{
> */
> -#ifdef CONFIG_HIP_DEBUG
> +
> #define HIP_DEBUG(...) hip_print_str(DEBUG_LEVEL_DEBUG, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__)
> #define HIP_HEXDUMP(prefix, str, len) \
> hip_hexdump(__FILE__, __LINE__, __FUNCTION__, prefix, str, len)
> @@ -245,15 +245,6 @@
> #define HIP_DEBUG_GL(debug_group, debug_level, ...) \
> hip_debug_gl(debug_group, debug_level, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__)
>
> -#else
> -#define HIP_DEBUG(...) do {} while (0)
> -#define HIP_HEXDUMP(prefix, str, len) do {} while (0)
> -#define HIP_DUMP_PACKET(prefix, str, len) do {} while (0)
> -#define HIP_DEBUG_SOCKADDR(prefix, sockaddr) do {} while (0)
> -#define HIP_DUMP_MSG(msg) do {} while (0)
> -#define HIP_DEBUG_GL(debug_group, debug_level, ...) do {} while (0)
> -#endif
> -

[L] Is it really necessary to remove the facility to completely disable debug output? I believe this code could stay and might be useful, e.g., for performance measurements.

> === modified file 'lib/core/icomm.h'

Please don't forget to update the Copyright dates when you modify files with
Copyright headers. Scatterbrains like myself may want to try out the pre-commit
hook in https://code.launchpad.net/~stefan.goetz/hipl/commitguard that
automatically checks for that.

Cheers,
 Stefan