[Freeipa-devel] [PATCH 0093] Log memory allocation failures

[Adam Tkac]

On Fri, Nov 16, 2012 at 03:15:44PM +0100, Petr Spacek wrote:
> On 11/16/2012 02:24 PM, Simo Sorce wrote:
> >On Fri, 2012-11-16 at 13:31 +0100, Petr Spacek wrote:
> >>+#define FILE_LINE_FUNC_MSG "%-15s: %4d: %-20s"
> >>+#define FILE_LINE_FUNC_ARG __FILE__, __LINE__, __func__
> >
> >Wouldn't it be more compact and less error prone to do something like:
> >
> >#define LOG_POSITION_MSG(str, ...) \
> >do { \
> >log_error("[%-15s: %4d: %-20s] " str, \
> >	  __FILE__, __LINE__, __func__, __VA_ARGS__); \
> >} while(0);
> >
> >>  #define CLEANUP_WITH(result_code)                              \
> >>         do {                                                    \
> >>                 result = (result_code);                         \
> >>@@ -46,15 +52,21 @@
> >>                 (target_ptr) = isc_mem_allocate((m), (s));      \
> >>                 if ((target_ptr) == NULL) {                     \
> >>                         result = ISC_R_NOMEMORY;                \
> >>+                       log_error("MEMORY ALLOCATION FAILURE at
> >>"       \
> >>+                                 FILE_LINE_FUNC_MSG,           \
> >>+                                 FILE_LINE_FUNC_ARG);  \
> >
> >and here simply:
> >	LOG_POSITION_MSG("MEMORY ALLOCATION FAILURE");
> >
> >The _MSG seem misleading doesn't make you think it is a format string
> >(maybe calling it _FMT would be better if you want to keep this split in
> >pieces).
> >
> >The single form above let you write less and still do things like:
> >LOG_POSITION_MSG("This failed with %d (%s)", ret, strerror(ret));
> 
> Yes, it would. I split it to avoid dns_result_totext() duplication
> in my next patch ... I see this approach as better now. Amended
> patch is attached.

Thanks for the patch, please read my comments below.

Regards, Adam

> From ef61c2e6db88b39c8b2642b14c23fd62a3eff472 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspacek at redhat.com>
> Date: Fri, 16 Nov 2012 13:17:44 +0100
> Subject: [PATCH] Log memory allocation failures.
> 
> Signed-off-by: Petr Spacek <pspacek at redhat.com>
> ---
>  src/log.h  | 7 ++++++-
>  src/util.h | 5 +++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/log.h b/src/log.h
> index 9062a4e80786b5bab806d6c9ceba1d1a9917a3e2..d6a40151d25b6f67cf6735ec955d45e4ebe4106c 100644
> --- a/src/log.h
> +++ b/src/log.h
> @@ -23,22 +23,27 @@
>  
>  #include <isc/error.h>
>  #include <dns/log.h>
> +#include <dns/result.h>
>  
>  #ifdef LOG_AS_ERROR
>  #define GET_LOG_LEVEL(level)	ISC_LOG_ERROR
>  #else
>  #define GET_LOG_LEVEL(level)	(level)
>  #endif
>  
>  #define fatal_error(...) \
> -    isc_error_fatal(__FILE__, __LINE__, __VA_ARGS__)
> +	isc_error_fatal(__FILE__, __LINE__, __VA_ARGS__)
>  
>  #define log_bug(fmt, ...) \
>  	log_error("bug in %s(): " fmt, __func__,##__VA_ARGS__)
>  
>  #define log_error_r(fmt, ...) \
>  	log_error(fmt ": %s", ##__VA_ARGS__, dns_result_totext(result))
>  
> +#define log_error_position(format, ...)				\
> +	log_error("[%-15s: %4d: %-21s] " format, 			\

Do we really need to format the format string? I would prefer to drop the
"[...]" stuff at all.

> +		  __FILE__, __LINE__, __func__, ##__VA_ARGS__)
> +
>  /* Basic logging functions */
>  #define log_error(format, ...)	\
>  	log_write(GET_LOG_LEVEL(ISC_LOG_ERROR), format, ##__VA_ARGS__)
> diff --git a/src/util.h b/src/util.h
> index 2b8f10e59ddacdb1d0e49cf0de3e9ab8a3786090..56fd5d80b55b8f8033718afbc31e9ff598f7e257 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -21,6 +21,8 @@
>  #ifndef _LD_UTIL_H_
>  #define _LD_UTIL_H_
>  
> +#include "log.h"
> +
>  #define CLEANUP_WITH(result_code)				\
>  	do {							\
>  		result = (result_code);				\
> @@ -46,15 +48,17 @@
>  		(target_ptr) = isc_mem_allocate((m), (s));	\
>  		if ((target_ptr) == NULL) {			\
>  			result = ISC_R_NOMEMORY;		\
> +			log_error_position("MEMORY ALLOCATION FAILURE");	\

Such huge msg is not needed, I think. What about more conservative

log_error_position("Memory allocation failed")

>  			goto cleanup;				\
>  		}						\
>  	} while (0)
>  
>  #define CHECKED_MEM_GET(m, target_ptr, s)			\
>  	do {							\
>  		(target_ptr) = isc_mem_get((m), (s));		\
>  		if ((target_ptr) == NULL) {			\
>  			result = ISC_R_NOMEMORY;		\
> +			log_error_position("MEMORY ALLOCATION FAILURE");	\
>  			goto cleanup;				\
>  		}						\
>  	} while (0)
> @@ -67,6 +71,7 @@
>  		(target) = isc_mem_strdup((m), (source));	\
>  		if ((target) == NULL) {				\
>  			result = ISC_R_NOMEMORY;		\
> +			log_error_position("MEMORY ALLOCATION FAILURE");	\
>  			goto cleanup;				\
>  		}						\
>  	} while (0)
> -- 
> 1.7.11.7
> 


-- 
Adam Tkac, Red Hat, Inc.