Guys Is there any plans to add syslog support in to xentoollog. Is this something that xentoollog would like to adopt . Thanks Gihan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wed, 2 Jun 2010, Gihan Munasinghe wrote:> Guys > > Is there any plans to add syslog support in to xentoollog. > > Is this something that xentoollog would like to adopt . >That is something we definitely want, patches are welcome :-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini wrote:> On Wed, 2 Jun 2010, Gihan Munasinghe wrote: > >> Guys >> >> Is there any plans to add syslog support in to xentoollog. >> >> Is this something that xentoollog would like to adopt . >> >> > > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gihan Munasinghe writes ("Re: [Xen-devel] syslog support to
xentoollog"):> Great I''ll do some patching to get syslog working.
Excellent.
> 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
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.
> 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}
That''s probably right.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
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. Signed-off-by : Gihan Munasinghe <GMunasinghe@flexiant.com> Thanks Gihan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 05/06/2010 01:51, "Gihan Munasinghe" <GMunasinghe@flexiant.com> wrote:> Guys > > See the attach patch, this implements a xentoollog consumer that can log > in to syslogI''ll wait for Ian to review this one before applying it. -- Keir> 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. > > Signed-off-by : Gihan Munasinghe <GMunasinghe@flexiant.com> > > > Thanks > Gihan_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gihan Munasinghe writes ("[PATCH] syslog support to
xentoollog"):> 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.
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gihan Munasinghe writes ("[PATCH] syslog support to
xentoollog"):> Also could someone look @ the syslogger_progress function and add any
> extra bits if needed, I did a very basic implementation of this.
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser writes ("Re: [Xen-devel] [PATCH] syslog support to
xentoollog"):> I''ll wait for Ian to review this one before applying it.
Great, thanks. I think there are a few things that need doing.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gihan Munasinghe
2010-Jun-07 11:36 UTC
[Xen-devel] Re: [PATCH] syslog support to xentoollog
Ian Jackson wrote:> Gihan Munasinghe writes ("[PATCH] syslog support to xentoollog"): > >> 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. >> > > 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. > >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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gihan Munasinghe
2010-Jun-07 11:37 UTC
[Xen-devel] Re: [PATCH] syslog support to xentoollog
Ian Jackson wrote:> Gihan Munasinghe writes ("[PATCH] syslog support to xentoollog"): > >> Also could someone look @ the syslogger_progress function and add any >> extra bits if needed, I did a very basic implementation of this. >> > > 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. > >Is this the only change that need to happen, are you happy with the rest of the patch. Tanks Gihan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Gihan Munasinghe writes ("[PATCH] syslog support to
xentoollog"):> See the attach patch, this implements a xentoollog consumer that can log
> in to syslog
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.
> +//-- Should be depricated not generic enough --//
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.
> +/**
> +Create a logger that will log to syslog
> +syslog_facility is the facilities that are in the sys/syslog.h
> +edit /etc/syslog.conf file to indicate where you wansyslog to send log
data
Block comments should be
/*
* Like this
*/
so that they can be easily distinguished from code.
> +//This method need to be depricated use more generic
xtl_logger_set_minlevel method insted
> void xtl_stdiostream_set_minlevel(xentoollog_logger_stdiostream*,
> xentoollog_level min_level);
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.
> +void xtl_logger_set_minlevel(struct xentoollog_logger *logger,
> + xentoollog_level min_level){
> + if (!logger || !logger->set_minlevel) return;
I think the test for !logger is wrong: I think if you call this
function and pass it logger=NULL you should get a crash. But it''s
fine to skip the call for loggers that don''t support it (ie have 0 in
the vtable).
> void xtl_logger_destroy(struct xentoollog_logger *logger) {
> - if (!logger) return;
> + if (!logger || !logger->destroy) return;
This is wrong because every logger must definitely have a ->destroy
method, since it at least has to free up the logger structure. Any
which didn''t must either be broken, or be some kind of static logger
which should never be destroyed.
> +typedef struct _log_map{
> + xentoollog_level xen_log;
> + int syslog_level;
> +}LOG_MAP;
Do not use identifiers which start with "_"; they are reserved for the
C implementation. (In this case it''s OK I think but it''s a
very bad
habit.) There seems to be no need for this struct to have a struct
tag as well as a typedef.
Also, as a matter of style, I think the use of all caps for type names
is a bad idea. Capslock is normally used for macros.
> +LOG_MAP log_mappings[] =
> + {
You forgot "static const".
> +int maptable_len = sizeof(log_mappings)/sizeof(LOG_MAP);
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.
> + buffer = (char *)calloc(1024,sizeof(char));
sizeof(char) is 1 by definition.
You should not cast the result from calloc.
> + if (context && (lg->flags & XTL_SHOW_PID)){
syslog already knows how to add the pid to messages.
> + sprintf(new_format,"[%s] : ",context);
...> + sprintf(new_format,"[%s] : ",strerror(errnoval));
This formatting is different to that produced by stdiostream. Surely
the messages should try to be as similar as possible.
> + char *new_format = (char *) calloc(7+strlen(context),sizeof(char));
> + sprintf(new_format,"[%s] : ",context);
> + strcat(buffer,new_format);
Use asprintf. In fact you could profitably replace much of the meat
of this function with a single asprintf call.
> + strcat(buffer,format);
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.
> + syslog(get_syslog_level(XTL_PROGRESS),"%s%s" "%s:
%lu/%lu %3d%%",
> + context?context:"", context?":
":"",
> + doing_what, done, total, percent);
You should arrange that the syslog level lookup - a linear search - is
not done on every call to ->progress, which may be called very very
requently.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Gihan Munasinghe writes ("Re: [PATCH] syslog support to
xentoollog"):> 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);
This is a bad idea because the type and semantics of new_place are not
defined.
> In stdiostream implementation this can be caste to a stream and in
> syslogger implementation this can be cast as a facility.
Yuck.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel