[Sheepdog] [PATCH 5/5] logger: fix the vprintf() bug

[MORITA Kazutaka]

At Thu, 13 Oct 2011 15:36:24 +0800,
Liu Yuan wrote:
> 
> On 10/13/2011 03:25 PM, AndyChen wrote:
> > 2011/10/13 Liu Yuan<namei.unix at gmail.com>:
> >> On 10/13/2011 12:12 PM, AndyChen wrote:
> >>>>> -       vprintf(SDOG_INFO "we create a new vdi, %d %s (%zd) %" PRIu64 ",
> >>>>> vid: %"
> >>>>> +       vprintf(SDOG_INFO, "we create a new vdi, %d %s (%zd) %" PRIu64
> >>>>> ", vid: %"
> >>>>>                 PRIx32 ", base %" PRIx32 ", cur %" PRIx32 " \n",
> >>>>>                 is_snapshot, name, strlen(name), size, *new_vid,
> >>>>> base_vid, cur_vid);
> >>>>>
> >>>>>         if (!copies) {
> >>>>> -               vprintf(SDOG_WARNING "qemu doesn't specify the copies...
> >>>>> %d\n",
> >>>>> +               vprintf(SDOG_WARNING, "qemu doesn't specify the
> >>>>> copies... %d\n",
> >>>>>                         sys->nr_sobjs);
> >>>>>                 copies = sys->nr_sobjs;
> >>>>>         }
> >>>> Why we need extra ',' in vprintf?
> >>> it is not extra, now  vprintf() use log_write() which defined in
> >>> sheepdog/include/logger.h,
> >>> log_write() has log level parameter 'prio', but vrpintf() does not
> >>> parse the log level, use LOG_INFO default.
> >>> you mean use vprintf() such as use printk()?
> >>>
> >> I didn't get you meant. but the definition vprintf(fmt, args...) shows that
> >> 'fmt' is the constant char *,  so I don't think we need to place ',' between
> >> two format strings. I look at the printk implementation in linux kernel,
> >> there is no such ',' between format strings either.
> > here the vprintf defined by the sheep logger, not the standard library
> > function: vprintf(fmt, args...)
> 
> #define vprintf(fmt, args...)                                           \
> do {                                                                    \
>          log_write(LOG_INFO, __func__, __LINE__, fmt, ##args);           \
> } while (0)
> 
> extern void log_write(int prio, const char *func, int line, const char 
> *fmt, ...)
>          __attribute__ ((format (printf, 4, 5)));
> 
> So I think, it is safe to say that the type of 'fmt' in vprintf() is, 
> const char *.

AndyChen changes the macro in this patch:

==
diff --git a/include/logger.h b/include/logger.h
index cdd7642..461f2d9 100644
--- a/include/logger.h
+++ b/include/logger.h
@@ -64,14 +64,14 @@ extern void log_write(int prio, const char *func, int line, const char *fmt, ...
 #define	SDOG_INFO		LOG_INFO
 #define	SDOG_DEBUG		LOG_DEBUG
 
-#define vprintf(fmt, args...)						\
+#define vprintf(level, fmt, args...)						\
 do {									\
-	log_write(LOG_INFO, __func__, __LINE__, fmt, ##args);		\
+	log_write(level, __func__, __LINE__, fmt, ##args);		\
 } while (0)
 ==

The definitions of log levels are changed to integers by the 3rd
patch.  I think the intention of this patch is not fixing a bug but
making it clear how to specify the log level.

Anyway, should we change the name of dprintf() and vprintf()?  These
names are already in POSIX functions and it is a bit confusing.


Thanks,

Kazutaka