[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] 2/8 logging header and core implementation



On Wed, Dec 17, 2008 at 04:12:20PM +0100, Daniel Veillard wrote:
> diff -u -r1.1 logging.h
> --- src/logging.h	6 Nov 2008 16:36:07 -0000	1.1
> +++ src/logging.h	17 Dec 2008 14:25:57 -0000
> @@ -30,16 +30,87 @@
>   * defined at runtime of from the libvirt daemon configuration file
>   */
>  #ifdef ENABLE_DEBUG
> -extern int debugFlag;
>  #define VIR_DEBUG(category, fmt,...)                                    \
> -    do { if (debugFlag) fprintf (stderr, "DEBUG: %s: %s (" fmt ")\n", category, __func__, __VA_ARGS__); } while (0)
> +    virLogMessage(category, VIR_LOG_DEBUG, 0, fmt, __VA_ARGS__)
> +#define VIR_INFO(category, fmt,...)                                    \
> +    virLogMessage(category, VIR_LOG_INFO, 0, fmt, __VA_ARGS__)
> +#define VIR_WARN(category, fmt,...)                                    \
> +    virLogMessage(category, VIR_LOG_WARN, 0, fmt, __VA_ARGS__)
> +#define VIR_ERROR(category, fmt,...)                                    \
> +    virLogMessage(category, VIR_LOG_ERROR, 0, fmt, __VA_ARGS__)
>  #else
>  #define VIR_DEBUG(category, fmt,...) \
>      do { } while (0)
> +#define VIR_INFO(category, fmt,...) \
> +    do { } while (0)
> +#define VIR_WARN(category, fmt,...) \
> +    do { } while (0)
> +#define VIR_ERROR(category, fmt,...) \
> +    do { } while (0)
>  #endif /* !ENABLE_DEBUG */
>  
>  #define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
>  #define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)
> +#define INFO(fmt,...) VIR_INFO(__FILE__, fmt, __VA_ARGS__)
> +#define INFO0(msg) VIR_INFO(__FILE__, "%s", msg)
> +#define WARN(fmt,...) VIR_WARN(__FILE__, fmt, __VA_ARGS__)
> +#define WARN0(msg) VIR_WARN(__FILE__, "%s", msg)
> +#define ERROR(fmt,...) VIR_ERROR(__FILE__, fmt, __VA_ARGS__)
> +#define ERROR0(msg) VIR_ERROR(__FILE__, "%s", msg)

I think we should add a prefix to the filename to give a
little scope to the namespace. This would let people use
non-filename based namespaces without risk of clashing, 
eg, perhaps

   #define ERROR(fmt,...) VIR_ERROR("file." __FILE__, fmt, __VA_ARGS__)

thus turning into    file.libvirt.c

> +/**
> + * virLogOutputFunc:
> + * @data: extra output logging data
> + * @category: the category for the message
> + * @priority: the priority for the message
> + * @msg: the message to log, preformatted and zero terminated
> + * @len: the lenght of the message in bytes without the terminating zero
> + *
> + * Callback function used to output messages
> + *
> + * Returns the number of bytes written or -1 in case of error
> + */
> +typedef int (*virLogOutputFunc) (void *data, const char *category,
> +                                 int priority, const char *str, int len);

I have a preference for 'void *data' parameters to callbacks being 
at end of the param list.

> +extern int virLogStartup(void);
> +extern int virLogReset(void);
> +extern void virLogShutdown(void);
> +extern int virLogParseFilters(const char *filters);
> +extern int virLogParseOutputs(const char *output);
> +extern void virLogMessage(const char *category, int priority, int flags,
> +                          const char *fmt, ...) ATTRIBUTE_FORMAT(printf, 4, 5);

I think it would be a good idea to have virLogMesage take a 
function name, and line number too. Likewise for the 
virLogOutputFunc() function.

The VIR_ERROR/WARN/INFO/DEBUG macros would automatically include
the __func__ and __line__  macros. eg

   #define VIR_INFO(category, fmt,...)                                    \
    virLogMessage(category, __func__, __line__, VIR_LOG_INFO, 0, fmt, 
                  __VA_ARGS__)

So when we process the 'char fmt,....' into an actual string we could
have some flag to turn on/off inclusion of the function & line data.

Cole did a similar thing for error reporting internal APIs when he
added this to virterror_internal.h:

  void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode,
                            const char *filename,
                            const char *funcname,
                            long long linenr,
                            const char *fmt, ...)
  ATTRIBUTE_FORMAT(printf, 7, 8);

Though, the actual implementation currently ignores filename, funcname
and linenr.

> +
> +/*
> + * Macro used to format the message as a string in virLogMessage
> + * and borrowed from libxml2 (also used in virRaiseError)
> + */
> +#define VIR_GET_VAR_STR(msg, str) {				\
> +    int       size, prev_size = -1;				\
> +    int       chars;						\
> +    char      *larger;						\
> +    va_list   ap;						\
> +                                                                \
> +    str = (char *) malloc(150);					\
> +    if (str != NULL) {						\
> +                                                                \
> +    size = 150;							\
> +                                                                \
> +    while (1) {							\
> +        va_start(ap, msg);					\
> +        chars = vsnprintf(str, size, msg, ap);			\
> +        va_end(ap);						\
> +        if ((chars > -1) && (chars < size)) {			\
> +            if (prev_size == chars) {				\
> +                break;						\
> +            } else {						\
> +                prev_size = chars;				\
> +            }							\
> +        }							\
> +        if (chars > -1)						\
> +            size += chars + 1;					\
> +        else							\
> +            size += 100;					\
> +        if ((larger = (char *) realloc(str, size)) == NULL) {	\
> +            break;						\
> +        }							\
> +        str = larger;						\
> +    }}								\
> +}

Wonder if we should make this use  virBuffer instead of
char/malloc/realloc. Could then add this definition to
buf.h and share it between the logging & error routines.

> +static const char *virLogPriorityString(virLogPriority lvl) {
> +    switch (lvl) {
> +        case VIR_LOG_DEBUG:
> +            return("debug");
> +        case VIR_LOG_INFO:
> +            return("info");
> +        case VIR_LOG_WARN:
> +            return("warning");
> +        case VIR_LOG_ERROR:
> +            return("error");
> +    }
> +    return("unknown");
> +}
> +
> +static int virLogInitialized = 0;
> +
> +/**
> + * virLogStartup:
> + *
> + * Initialize the logging module
> + *
> + * Returns 0 if successful, and -1 in case or error
> + */
> +int virLogStartup(void) {
> +    if (virLogInitialized)
> +        return(-1);
> +    virLogInitialized = 1;

Strictly speaking we should use  pthread_once() for such
initializations, or an atomic test-and-set operation.

> +    /*
> +     * serialize the error message, add level and timestamp
> +     */
> +    VIR_GET_VAR_STR(fmt, str);
> +    if (str == NULL)
> +        return;
> +    gettimeofday(&cur_time, NULL);
> +    localtime_r(&cur_time.tv_sec, &time_info);
> +
> +    if (asprintf(&msg, "%02d:%02d:%02d.%03d: %s : %s\n",
> +                 time_info.tm_hour, time_info.tm_min,
> +                 time_info.tm_sec, (int) cur_time.tv_usec / 1000,
> +                 virLogPriorityString(priority), str) < 0) {

This would be the place where we'd add in (optional) inclusion of
the function name & line number data.

> +
> +#define IS_SPACE(cur)                                                   \
> +    ((*cur == ' ') || (*cur == '\t') || (*cur == '\n') ||               \
> +     (*cur == '\r') || (*cur == '\\'))

GNULIB already provides this with  c_isspace() - unless we really
need the check for '\\' too ?

All basically looks good to me.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]