[hipl-dev] Re: [Merge] lp:~fahad-aizaz/hipl/logging into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+65788@xxxxxxxxxxxxxxxxxx
- Date: Fri, 24 Jun 2011 15:12:57 -0000
Review: Needs Information
review needs-info
On Fri, Jun 24, 2011 at 01:38:15PM +0000, Fahad Aizaz wrote:
>
> For more details, see:
> https://code.launchpad.net/~fahad-aizaz/hipl/logging/+merge/65788
>
> The SYSLOG functionality has been added. A new logdebug level has been added.
IMO these are two independent things that should really be reviewed
and decided upon separately in two independent merge proposals.
> --- firewall/main.c 2011-06-22 13:25:54 +0000
> +++ firewall/main.c 2011-06-24 13:38:10 +0000
> @@ -230,6 +230,8 @@
> if (fork() > 0) {
> return EXIT_SUCCESS;
> }
> + } else {
> + hip_set_logtype(LOGTYPE_STDERR_SYSLOG);
> }
>
> --- hipd/hipd.c 2011-05-16 11:39:06 +0000
> +++ hipd/hipd.c 2011-06-24 13:38:10 +0000
> @@ -165,7 +165,7 @@
> fprintf(stderr, " -f set debug type format to short\n");
> - fprintf(stderr, " -d set the initial (pre-config) debug level to ALL
> (default is MEDIUM)\n");
> + fprintf(stderr, " -d set the initial (pre-config) debug level to ALL
> (default is LOW)\n");
> fprintf(stderr, " -D <module name> disable this module. " \
> "Use additional -D for additional modules.\n");
> @@ -326,7 +326,7 @@
>
> /* Configuration is valid! Fork a daemon, if so configured */
> if (flags & HIPD_START_FOREGROUND) {
> - hip_set_logtype(LOGTYPE_STDERR);
> + hip_set_logtype(LOGTYPE_STDERR_SYSLOG);
> HIP_DEBUG("foreground\n");
> } else {
> hip_set_logtype(LOGTYPE_SYSLOG);
> @@ -446,7 +446,7 @@
>
> /* set the initial verbosity level */
> - hip_set_logdebug(LOGDEBUG_MEDIUM);
> + hip_set_logdebug(LOGDEBUG_LOW);
>
> @@ -457,7 +457,7 @@
>
> /* 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;
I wonder why this is so different between hipfw and hipd.
> --- hipd/user.c 2011-05-16 11:39:06 +0000
> +++ hipd/user.c 2011-06-24 13:38:10 +0000
> @@ -335,6 +335,12 @@
> HIP_IFEL(hip_set_logdebug(LOGDEBUG_MEDIUM), -1,
> "Error when setting daemon DEBUG status to MEDIUM\n");
> break;
> + case HIP_MSG_SET_DEBUG_LOW:
> + /* Removes debugging messages. */
> + HIP_DEBUG("Handling DEBUG LOW user message.\n");
> + HIP_IFEL(hip_set_logdebug(LOGDEBUG_LOW), -1,
> + "Error when setting daemon DEBUG status to LOW\n");
> + break;
> case HIP_MSG_SET_DEBUG_NONE:
> /* Removes debugging messages. */
> HIP_DEBUG("Handling DEBUG NONE user message.\n");
The comment seems to make no sense but rather looks like it was blindly
copy-pasted from below.
> --- lib/core/debug.c 2011-04-05 16:44:22 +0000
> +++ lib/core/debug.c 2011-06-24 13:38:10 +0000
> @@ -216,13 +228,14 @@
> case LOGTYPE_NOLOG:
> break;
> case LOGTYPE_STDERR:
> - if (strlen(prefix) > 0) {
> +
> + if (strlen(prefix)) {
> printed = fprintf(stderr, "%s: ", prefix);
Why?
> if (printed < 0) {
> goto err;
> }
> } else {
> - /* LOGFMT_SHORT: no prefix */
> + // LOGFMT_SHORT: no prefix
Why?
> @@ -230,25 +243,39 @@
> goto err;
> }
> break;
> + case LOGTYPE_STDERR_SYSLOG:
> +
> + printed = vsnprintf(syslog_msg, DEBUG_MSG_MAX_LEN, fmt, args);
> + if (strlen(prefix)) {
> + syslog(syslog_level | SYSLOG_FACILITY, "%s %s", prefix,
> syslog_msg);
> + } else {
> + syslog(syslog_level | SYSLOG_FACILITY, "%s", syslog_msg);
> + }
> +
> + if (printed < 0) {
> + goto err;
> + }
Logging before error checking? This is suspicous.
> + break;
> case LOGTYPE_SYSLOG:
> - openlog(NULL, LOG_PID, SYSLOG_FACILITY);
> +
> printed = vsnprintf(syslog_msg, DEBUG_MSG_MAX_LEN, fmt, args);
Why did you add all those empty lines to the case statements?
It looks like a few have it now, but most don't.
> syslog(syslog_level | SYSLOG_FACILITY, "%s %s", prefix, syslog_msg);
> - /* the result of vsnprintf depends on glibc version; handle them both
> + /**
> + * the result of vsnprintf depends on glibc version; handle them both
Why? This does not look like something that should be in Doxygen.
> * (note about barriers: printed has \0 excluded,
> - * DEBUG_MSG_MAX_LEN has \0 included) */
> + * DEBUG_MSG_MAX_LEN has \0 included)
> + */
Why?
> if (printed < 0 || printed > DEBUG_MSG_MAX_LEN - 1) {
> syslog(syslog_level | SYSLOG_FACILITY,
> - "%s", "previous msg was truncated!!!");
> + "previous msg was truncated!!!");
Looks unrelated.
> - closelog();
> break;
> default:
> printed = fprintf(stderr, "hip_vlog(): undefined logtype: %d",
> logtype);
> exit(1);
> }
>
> - /* logging was succesful */
> + /* logging was successful */
This is an unrelated typo fix that should IMO be pushed to trunk
right away.
> @@ -276,10 +303,28 @@
> va_list args;
> va_start(args, fmt);
> - if ((debug_level == DEBUG_LEVEL_INFO && logdebug != LOGDEBUG_NONE) ||
> - (debug_level == DEBUG_LEVEL_DEBUG && logdebug == LOGDEBUG_ALL) ||
> + /**
> + * Following matrix is implemented below
The following matrix ...
> --- 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 @@
> -#ifdef CONFIG_HIP_DEBUG
> +
> #define HIP_DEBUG(...) hip_print_str(DEBUG_LEVEL_DEBUG, __FILE__, __LINE__,
> __FUNCTION__, __VA_ARGS__)
> #define HIP_HEXDUMP(prefix, str, len) \
> @@ -245,15 +245,6 @@
>
> -#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
Why?
Diego
--
https://code.launchpad.net/~fahad-aizaz/hipl/logging/+merge/65788
Your team HIPL core team is subscribed to branch lp:hipl.
Other related posts:
