[Openais] [PATCH] V3 Fix debug function in logsys.c (Was Re: Remove unused functions from logsys.c)

[Steven Dake]

Andreas,

Thanks for all the work on this, but your intuition of removing the code 
completely (your first patch) is correct.  This is why the patch was 
applied without argument.  I don't want ifdef's in the code around debug 
output.  Libraries in general, such as logsys, should never print any 
type of output (unless that is their purpose in the case of stderr mode 
of operation).  If a field deployment can't turn optional code with a 
runtime (vs build time) configuration option, then it doesn't belong in 
the codebase.

In some cases we make exceptions but they are rare and this type of 
debug output doesn't warrant one of those exceptions.

Going forward I want to reduce lines of code and simplify the code base. 
  Deleting unused symbols is one way to achieve that.

responses to log_printf_to_logs inline in your message below:

Regards
-steve

   On 06/12/2010 06:43 AM, Andreas Florath wrote:
> Hello!
>
> Attached you can find version 3 of fixing the buffer overflow in the
> decode_mode() function of exec/logsys.c.
>
> @Colin: Thanks for the hints:
>   o You're right with the mstrncpy() - adapted it now
>     for exactly the use case from decode_mode_append(), where no \0 is
>     needed during setting up the string.
>   o use used_buf as a normal variable and returning the new value
>     is much cleaner: adapted the decode_mode_append() function.
>
> @Fabio:
> You wrote:
>
>   "It is enough to fix the buffer overflow in the old code. This
>   debugging piece of infrastructure is not performance critical. There
>   is little to no point to implement all those buffers/strlen
>   optimizations."
>
> There are no optimizations for speed or space in this code.  The only
> optimizations I did is writing understandable and maintainable code
> without code duplication.
>
> Each of the functions is simple - each of the functions is easy to
> understand - and each function follows the guideline: Let one function
> to one thing.
>
> IMHO functions like 'log_printf_to_logs()' with about 21 lines of
> parameter and variable declarations and about 200 lines of code are
> one major reason of the current problems with the logsys (see [2]).
> It takes days to understand those functions and it is mostly impossible
> to find bugs.

Clearly this function needs some simplification.

>
> Please have a look at many many web pages and books which discuss
> length of functions and methods.  Nowadays it's recommended that a
> method / function should have not more than seven lines of code.
>

good goal but not always achievable.  Where we can, we do try to break 
up functions as much as possible so they are not overloaded.  As you can 
see, log_printf_to_logs didn't have this approach applied to it.  There 
are other parts of the code where this can be done as well (have a look 
at service.c if you really want to twist your brain around).

I really want to simplify the design going forward.  You can help with 
that if your interested.

Regards
-steve

> The Linux kernel coding style has a rule about the maximum line length
> (see [1] Chapter 2: Breaking long lines and strings).  If you have
> small functions it's mostly impossible that more than 80 chars are
> needed.  When longer lines are needed because of the indentation of
> the many blocks, it's mostly a hint that the function can and should
> be split up (also see 'log_printf_to_logs()').
>
> Kind regards
>
> Andreas Florath
>
> [1] http://www.kernel.org/doc/Documentation/CodingStyle
> [2] http://marc.info/?t=127612244700006&r=1&w=2
>
> Patch against svn version 2943.
>
> Signed-off-by: Andreas Florath<gnu4u at flonatel dot org>
>
> ---
>
> Index: exec/logsys.c
> ===================================================================
> --- exec/logsys.c	(revision 2943)
> +++ exec/logsys.c	(working copy)
> @@ -217,6 +217,135 @@
>   /* forward declarations */
>   static void logsys_close_logfile(int subsysid);
>
> +#ifdef LOGSYS_DEBUG
> +struct st_ls2str
> +{
> +	unsigned long  flag;
> +	char const *   name;
> +}
> +	ls2str[] =
> +	{
> +		{ LOGSYS_MODE_OUTPUT_FILE,     "FILE" },
> +		{ LOGSYS_MODE_OUTPUT_STDERR,   "STDERR" },
> +		{ LOGSYS_MODE_OUTPUT_SYSLOG,   "SYSLOG" },
> +		{ LOGSYS_MODE_FORK,            "FORK" },
> +		{ LOGSYS_MODE_THREADED,        "THREADED" }
> +	};
> +
> +/* This is a copy of the strncpy(3) function but it returns the
> + * index to the end of the dest string.
> + * This eliminates the need for additional strlen() calls.
> + * Also it does never append a \0 at the end of the string -
> + * this must be done by the higher level functions. */
> +static size_t mstrncpy(char *dest, const char *src, size_t n)
> +{
> +	size_t i;
> +
> +	for (i=0; i<n&&  src[i]!='\0'; ++i)
> +		dest[i] = src[i];
> +
> +	return i;
> +}
> +
> +/* Appends exactly one string to the buffer.
> + * It takes care about the maximal size of buf and the comma
> + * handling between the words.
> + * The function recognizes the fact, that a comma must be inserted, on
> + * the condition whether the used_buf is 0 (->no comma prepended) or
> + * 1 (->comma prepended). */
> +static size_t decode_mode_append(
> +	unsigned int const mode, char * const buf, size_t const buflen,
> +	struct st_ls2str const * const l2si, size_t used_buf)
> +{
> +	if (mode&  l2si->flag)
> +	{
> +		if (used_buf) buf[used_buf++] = ',';
> +		return used_buf + mstrncpy(buf+used_buf, l2si->name,
> +					   buflen-used_buf-1);
> +	}
> +
> +	return used_buf;
> +}
> +
> +/* Appends all modes with an index less than the given 'maxidx' to the
> + * buffer. */
> +static char * decode_mode_maxidx(
> +	unsigned int mode, char *buf, size_t buflen, int maxidx)
> +{
> +	int idx;
> +	size_t used_buf = 0;
> +
> +	/* buflen-1: the \0 must also fit in the buffer */
> +	for (idx=0; idx<maxidx&&  used_buf<buflen-1; ++idx)
> +		used_buf = decode_mode_append(
> +			mode, buf, buflen,&ls2str[idx], used_buf);
> +
> +	buf[used_buf] = '\0';
> +	return buf;
> +}
> +
> +static char * decode_mode(int subsysid, char *buf, size_t buflen)
> +{
> +	return decode_mode_maxidx(
> +		logsys_loggers[subsysid].mode, buf, buflen,
> +		subsysid == LOGSYS_MAX_SUBSYS_COUNT ? 5 : 3);
> +}
> +
> +static const char *decode_debug(int subsysid)
> +{
> +       if (logsys_loggers[subsysid].debug)
> +               return "on";
> +
> +       return "off";
> +}
> +
> +static const char *decode_status(int subsysid)
> +{
> +       if (!logsys_loggers[subsysid].init_status)
> +               return "INIT_DONE";
> +
> +       return "NEEDS_INIT";
> +}
> +
> +static void dump_subsys_config(int subsysid)
> +{
> +       char modebuf[1024];
> +
> +       fprintf(stderr,
> +               "ID: %d\n"
> +               "subsys: %s\n"
> +               "logfile: %s\n"
> +               "logfile_fp: %p\n"
> +               "mode: %s\n"
> +               "debug: %s\n"
> +               "syslog_fac: %s\n"
> +               "syslog_pri: %s\n"
> +               "logfile_pri: %s\n"
> +               "init_status: %s\n",
> +               subsysid,
> +               logsys_loggers[subsysid].subsys,
> +               logsys_loggers[subsysid].logfile,
> +               logsys_loggers[subsysid].logfile_fp,
> +               decode_mode(subsysid, modebuf, sizeof(modebuf)),
> +               decode_debug(subsysid),
> +               logsys_facility_name_get(logsys_loggers[subsysid].syslog_facility),
> +               logsys_priority_name_get(logsys_loggers[subsysid].syslog_priority),
> +               logsys_priority_name_get(logsys_loggers[subsysid].logfile_priority),
> +               decode_status(subsysid));
> +}
> +
> +static void dump_full_config(void)
> +{
> +       int i;
> +
> +       for (i = 0; i<= LOGSYS_MAX_SUBSYS_COUNT; i++) {
> +               if (strlen(logsys_loggers[i].subsys)>  0)
> +                       dump_subsys_config(i);
> +       }
> +}
> +#endif
> +
> +
>   static uint32_t circular_memory_map (void **buf, size_t bytes)
>   {
>   	void *addr_orig;
>
>