syslog support to xentoollog

Guys

Is there any plans to add syslog support in to xentoollog.

Is this something that xentoollog would like to adopt .

Thanks
Gihan

That is something we definitely want, patches are welcome :-)

Great I'll do some patching to get syslog working.

looking at the xentool.h, following definitions

#define XTL_STDIOSTREAM_SHOW_PID 01u
#define XTL_STDIOSTREAM_SHOW_DATE 02u
#define XTL_STDIOSTREAM_HIDE_PROGRESS 04u

I would suggest to be as
#define XTL_SHOW_PID 01u
#define XTL_SHOW_DATE 02u
#define XTL_HIDE_PROGRESS 04u
So it will be more generic for any loggers that will be implemented

also is everyone happy with following loglevel mappings between xentoollog_level and syslog

       {XTL_DEBUG,LOG_DEBUG},
       {XTL_VERBOSE,LOG_DEBUG},
       {XTL_DETAIL,LOG_DEBUG},
       {XTL_PROGRESS,LOG_DEBUG},
       {XTL_INFO,LOG_INFO},
       {XTL_NOTICE,LOG_NOTICE},
       {XTL_WARN,LOG_WARNING},
       {XTL_ERROR,LOG_ERR},
       {XTL_CRITICAL,LOG_CRIT}

Thanks
Gihan

Gihan Munasinghe writes ("Re: [Xen-devel] syslog support to xentoollog"):

Excellent.

Sure.

Note though that XTL_SHOW_DATE isn't going to be relevant for syslog since in syslog the date is put in by syslogd.

That's probably right.

Ian.

Guys

See the attach patch, this implements a xentoollog consumer that can log in to syslog

Following are the changes

xentoollog.h:
1) Generic, xtl_logger_set_minlevel and xtl_logger_adjust_flags functions to be called from outside,
2) Making logger flags to be more generic
3) Change the struct xentool_logger, each logger will implement its own "set_minlevel" and "adjust_flags" which will be invoked by xtl_logger_set_minlevel and xtl_logger_adjust_flags generic functions.
4) New function prototype to create a xentool_logger_syslogger

Going forward I would like to suggest that xentoollog.h file should only have generic functions, except for xtl_createlogger_* functions. Logger type specific function should not be included(e.g xtl_stdiostream_set_minlevel ) this will make sure outside code can switch from one logger to another (from stdiologger to syslogger vice versa) with out breaking the code.

xtl_core.c:
1) Implementation of xtl_logger_set_minlevel and xtl_logger_adjust_flags functions.

xtl_logger_syslog.c :
New logger implementation to log in to syslog.

I did some testing by changing the code in xl. This seems to work, but I would really like if more people can test this. Also could someone look @ the syslogger_progress function and add any extra bits if needed, I did a very basic implementation of this.

Thanks
Gihan

diff -Nuar a/tools/libxc/Makefile b/tools/libxc/Makefile
@@ -29,6 +29,7 @@
 CTRL_SRCS-y       += xc_memshr.c
 CTRL_SRCS-y       += xtl_core.c
 CTRL_SRCS-y       += xtl_logger_stdio.c
+CTRL_SRCS-y       += xtl_logger_syslog.c
 CTRL_SRCS-$(CONFIG_X86) += xc_pagetab.c
 CTRL_SRCS-$(CONFIG_Linux) += xc_linux.c
 CTRL_SRCS-$(CONFIG_SunOS) += xc_solaris.c

diff -Nuar a/tools/libxc/xtl_core.c b/tools/libxc/xtl_core.c
@@ -63,7 +63,23 @@
     logger->progress(logger, context, doing_what, percent, done, total);
 }
 
+void xtl_logger_set_minlevel(struct xentoollog_logger *logger,
+                             xentoollog_level min_level){
+    if (!logger || !logger->set_minlevel) return; 
+
+    logger->set_minlevel(logger,min_level);
+}
+    
+void xtl_logger_adjust_flags(struct xentoollog_logger *logger,
+                             unsigned set_flags, unsigned clear_flags){
+    if (!logger || !logger->adjust_flags) return; 
+
+    logger->adjust_flags(logger,set_flags,clear_flags);
+}
+
+
 void xtl_logger_destroy(struct xentoollog_logger *logger) {
-    if (!logger) return;
+    if (!logger || !logger->destroy) return;
+
     logger->destroy(logger);
 }

diff -Nuar a/tools/libxc/xtl_logger_stdio.c b/tools/libxc/xtl_logger_stdio.c
@@ -124,6 +124,20 @@
     lg->flags = new_flags;
 }
 
+static void stdiostream_set_minlevel(struct xentoollog_logger *logger_in,
+                                  xentoollog_level min_level){
+
+    xentoollog_logger_stdiostream *lg = (void*)logger_in;
+    xtl_stdiostream_set_minlevel(lg,min_level);
+}
+
+static void stdiostream_adjust_flags(struct xentoollog_logger *logger_in,
+                                  unsigned set_flags, unsigned clear_flags) {
+
+    xentoollog_logger_stdiostream *lg = (void*)logger_in;
+    xtl_stdiostream_adjust_flags(lg,set_flags,clear_flags); 
+}
+
 xentoollog_logger_stdiostream *xtl_createlogger_stdiostream
         (FILE *f, xentoollog_level min_level, unsigned flags) {
     xentoollog_logger_stdiostream newlogger;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

I'll wait for Ian to review this one before applying it.

-- Keir

Gihan Munasinghe writes ("[PATCH] syslog support to xentoollog"):

In general this is a good idea but it's not sensible to make it a hard and fast rule. Eg, if you had a function xtl_syslog_change_facility() it wouldn't make any sense for it to be implemented by stdiostream.

Ian.

Gihan Munasinghe writes ("[PATCH] syslog support to xentoollog"):

I think it should at the very least limit the messages until the percentage changes by at least (say) 5%. The progress function may be called very very often.

Ian.

Keir Fraser writes ("Re: [Xen-devel] [PATCH] syslog support to xentoollog"):

Great, thanks. I think there are a few things that need doing.

Ian.

Should a call like xtl_syslog_change_facility() given out directly to the library users. this call can be wrapped with in a more generic call like xtl_cahnge_log_place(struct xentool_logger , void *new_place);

In stdiostream implementation this can be caste to a stream and in syslogger implementation this can be cast as a facility. If some logger doesn't want to implement that we can have empty implementation

Well may be it should not be a hard and fast rule, but more of a best practice scenario then.

Thanks
Gihan

Is this the only change that need to happen, are you happy with the rest of the patch.

Tanks
Gihan

Gihan Munasinghe writes ("[PATCH] syslog support to xentoollog"):

Thanks. Here are my comments after reading the code in detail:

None of your code seems to implement min_level. At the very least you should be able to suppress logging of progress messages to syslog.

Could you please to use the same comment style as the rest of this file ? Switching styles in a single file makes the it particularly hard to read.

If we are going to make these methods of every logger, you can just delete these two functions. This logging stuff is new enough that we don't care about backward API compatibility. Also "deprecated" is spelt thus.

This should be a #define, surely. If not, you forgot "static const". But it's only used in one place. Perhaps you should put it there.

sizeof(char) is 1 by definition.

You should not cast the result from calloc.

syslog already knows how to add the pid to messages.

...

This formatting is different to that produced by stdiostream. Surely the messages should try to be as similar as possible.

This is wrong. What if the context or the errno value contain "%s" or something ? You can prepend a single "%s" to the format if you want to prepend a fixed string.

Gihan Munasinghe writes ("Re: [PATCH] syslog support to xentoollog"):

This is a bad idea because the type and semantics of new_place are not defined.

Yuck.

Ian.