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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home