Mailing List Archive

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 syslog

I'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

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

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