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 <pspa...@redhat.com>
> Date: Fri, 16 Nov 2012 13:17:44 +0100
> Subject: [PATCH] Log memory allocation failures.
>
> Signed-off-by: Petr Spacek <pspa...@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.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
