You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@corinthia.apache.org by jan i <ja...@apache.org> on 2015/08/23 12:00:53 UTC
Fwd: [1/2] incubator-corinthia git commit: Logging feature update
Hi Gabriela
Finally I got time to go in detail with your work. First thanks for making
this important work.
I have some comments to your latest commit:
---------- Forwarded message ----------
"Add dedicated general logging macros, set a symlink 'current.log', and
allow local custom log macros with dedicated names for the log file.
Remove the tar file and use snprintf instead strncat and strdup."
This all sound good, except the symlink, that is not going to work in
windows or IOS.
(#define): Add _GNU_SOURCE.
do not understand what this #define means.
(#include): Add "log_maker.h".
Add <assert.h>.
Add <error.h>.
Add <libgen.h>.
Add <stdarg.h>.
Add <stdio.h>.
Add <stdlib.h>.
Add <string.h>.
Add <time.h>.
Add <unistd.h>.
Add <errno.h>.
Add <dirent.h>.
Add <sys/types.h>.
Add <sys/stat.h>.
Add <fcntl.h>.
These are probably not all needed.
If they are not needed, they should be removed, and if they are needed then
there is a serious complexity problem.
(get_time_string): New function.
(set_log_output_function): New function. This is a stub.
I thought we agreed to make that part of log_init, I do not like it as an
extra function.
(log_init): New function. Create the logfile name with host, time
and program name. Set a symlink 'current.log' into
~/../incubator/.
the log_init function is given an output_function, it should NOT make any
file operations.
You should add a default_output_function, which you use if you get a NULL
pointer in the log_init call.
(set_log_dir): New function. Set the directory in which to write
the individual logfiles and the location where the symlonk
should be created.
that is part of the output_function. Forget symlink, the program runs in a
work-dir, and that is where the log files should go.
(close_log): New function. Close the file descriptor and inform
user about the location and name of the generated file.
I dont like this function, it adds complexity to the code, what happens if
I call close_log, and then log a message.
It is really not needed, close is done automatially when the program
closes. In the output function you should use fflush() to secure
all buffers are written to disk.
(log_msg_prefix): New function. Create the prefix containing the
name of the log level, the file, line number and function name.
I would assume that is part of the log_msg call, I do not like to see 2
different calls.
(log_msg): New function. Write the log prefix and log message to
file.
* log_maker.h:
Header file for general inclusion so log_maker.c can be used.
(#ifndef): Add LOG_MAKER_H.
Header guard. Somehow #pragma once does not work for me.
Look at our other headers, there #pragma seems to work fine.
(#if): Add _MSC_VER. Picked this up in the docs, apparently MSC
uses _sprintf instead.
this is something that must go in dfplatform.h, we do not want to have MSC
dependencies throughout the code.
(#define): Add LOG_ERR.
Add LOG_WARNING.
Add LOG_NOTICE.
Add LOG_INFO.
Add LOG_DEBUG.
These are all strings for now. I can change it back to numbers, but
currently those strings are used to print out the log message
titles.
Global variables:
(char): Add log_filename[filename_len].
that is the output_function and as such not the log code. If the output
function uses a file, code outside logger must open that file.
(int): Add log_file_fd. File descriptor for log file.
Again output_function.
(static int log_level_initialised): Guard to prevent log_init from
being called twise.
(char): Add logging_dir[filename_len].
no.
(char): Add log_symlink[filename_len].
no.
(LOG_THIS): New function. Basic macro, called by every custom macro.
Basic log macros for general usage:
(LOG_ERROR_MSG): New function.
(LOG_WARNING_MSG): New function.
(LOG_NOTICE_MSG): New function.
(LOG_INFO_MSG): New function.
(LOG_DEBUG_MSG): New function.
Prototypes for logmaker.c:
(set_log_output_function): New function.
no part of log_init
(log_init): New function.
(set_log_dir): New function.
no not used at all
(set_log_level): New function.
no part of log_init
(close_log): New function.
no
(log_msg_prefix): New function.
I think this is part of log_msg
Can we agree that on the outside logger has:
log_init(<level>, <output_function> maybe other setup parms)
log_msg(..) the function the macros call.
LOG_FOO_MSG("text", ...)
Not more!
if log_init() is called with a NULL pointer for output_function, you use
your own, that logs fixed to corinthia.log in currenct directory.
rgds
jan i.
Re: [1/2] incubator-corinthia git commit: Logging feature update
Posted by Gabriela Gibson <ga...@gmail.com>.
Thanks for the great feedback Jan :)
Allow me to beat my own drum here for a moment and proudly state that
the log message template was produced by my Logmessage Scribe from a
diff file.
This is the first time I used it in earnest and I have to say, I'm
pleased as pie with the results.
I am very very happy with the way the Scribe facilitates comprehensive
feedback, and the fact that it makes a neat list that allows a concise
review of the shape of the code, whilst listing includes, defines,
global variables and functions that were added, modified or removed.
I also have to say I find this style very useful as a 'todo list'.
I hope that others here agree with me!
It fairly much conforms to the Subversion project log message
standards, which I personally like a lot, they help me a lot to
comprehend the code I wrote.
You can find logmessage scribe here as a google web app so there is
nothing to download and maintain:
logmessage-scribe.appspot.com
To use it, you just need a git diff and paste it in and go, but I also
found a GNU diff will sort of work, with a couple of surprising bonus
bugs, but I'll sort those out this week. You will still get a
functional template that only needs minor editing to be tidy and that
makes a great framework to explain a medium/large size patch to the
community (or, to yourself)
Logmessage Scribe currently only works with C code, but has very
rudimentary Cmake support as well.
It of course still has a few minor nits (bug reports very welcome) and
there is a link to the bitbucket with the python code. It's my first
python project so, don't look too closely at the code. Let's just say
that it works, but of course could be better. Patches welcome! :))
G
On Sun, Aug 23, 2015 at 11:06 AM, jan i <ja...@apache.org> wrote:
> Just a follow up, I see
>
> log_msg(char* level, char* filename, int linenum, char *msg, ...)
>
> {
> if (level active)
> {
> snprintf(buffer, ".......", char* filename, int linenum, char *msg,
> ...)
> call output_function(buffer)
> }
> }
>
> I hope this explains a bit better how I think.
>
> rgds
> jan I.
>
>
> On 23 August 2015 at 12:00, jan i <ja...@apache.org> wrote:
>
>> Hi Gabriela
>>
>> Finally I got time to go in detail with your work. First thanks for making
>> this important work.
>>
>> I have some comments to your latest commit:
>>
>> ---------- Forwarded message ----------
>>
>> "Add dedicated general logging macros, set a symlink 'current.log', and
>> allow local custom log macros with dedicated names for the log file.
>> Remove the tar file and use snprintf instead strncat and strdup."
>>
>> This all sound good, except the symlink, that is not going to work in
>> windows or IOS.
>>
>>
>> (#define): Add _GNU_SOURCE.
>> do not understand what this #define means.
>>
>>
>> (#include): Add "log_maker.h".
>> Add <assert.h>.
>> Add <error.h>.
>> Add <libgen.h>.
>> Add <stdarg.h>.
>> Add <stdio.h>.
>> Add <stdlib.h>.
>> Add <string.h>.
>> Add <time.h>.
>> Add <unistd.h>.
>> Add <errno.h>.
>> Add <dirent.h>.
>> Add <sys/types.h>.
>> Add <sys/stat.h>.
>> Add <fcntl.h>.
>> These are probably not all needed.
>> If they are not needed, they should be removed, and if they are needed
>> then there is a serious complexity problem.
>>
>>
>> (get_time_string): New function.
>>
>> (set_log_output_function): New function. This is a stub.
>> I thought we agreed to make that part of log_init, I do not like it as an
>> extra function.
>>
>>
>> (log_init): New function. Create the logfile name with host, time
>> and program name. Set a symlink 'current.log' into
>> ~/../incubator/.
>> the log_init function is given an output_function, it should NOT make any
>> file operations.
>>
>> You should add a default_output_function, which you use if you get a NULL
>> pointer in the log_init call.
>>
>>
>> (set_log_dir): New function. Set the directory in which to write
>> the individual logfiles and the location where the symlonk
>> should be created.
>> that is part of the output_function. Forget symlink, the program runs in a
>> work-dir, and that is where the log files should go.
>>
>>
>> (close_log): New function. Close the file descriptor and inform
>> user about the location and name of the generated file.
>> I dont like this function, it adds complexity to the code, what happens if
>> I call close_log, and then log a message.
>>
>> It is really not needed, close is done automatially when the program
>> closes. In the output function you should use fflush() to secure
>> all buffers are written to disk.
>>
>> (log_msg_prefix): New function. Create the prefix containing the
>> name of the log level, the file, line number and function name.
>> I would assume that is part of the log_msg call, I do not like to see 2
>> different calls.
>>
>> (log_msg): New function. Write the log prefix and log message to
>> file.
>>
>> * log_maker.h:
>>
>> Header file for general inclusion so log_maker.c can be used.
>>
>> (#ifndef): Add LOG_MAKER_H.
>> Header guard. Somehow #pragma once does not work for me.
>> Look at our other headers, there #pragma seems to work fine.
>>
>> (#if): Add _MSC_VER. Picked this up in the docs, apparently MSC
>> uses _sprintf instead.
>>
>> this is something that must go in dfplatform.h, we do not want to have MSC
>> dependencies throughout the code.
>>
>>
>> (#define): Add LOG_ERR.
>> Add LOG_WARNING.
>> Add LOG_NOTICE.
>> Add LOG_INFO.
>> Add LOG_DEBUG.
>> These are all strings for now. I can change it back to numbers,
>> but
>> currently those strings are used to print out the log message
>> titles.
>>
>> Global variables:
>>
>> (char): Add log_filename[filename_len].
>> that is the output_function and as such not the log code. If the output
>> function uses a file, code outside logger must open that file.
>>
>> (int): Add log_file_fd. File descriptor for log file.
>> Again output_function.
>>
>> (static int log_level_initialised): Guard to prevent log_init from
>> being called twise.
>>
>> (char): Add logging_dir[filename_len].
>> no.
>>
>> (char): Add log_symlink[filename_len].
>> no.
>>
>> (LOG_THIS): New function. Basic macro, called by every custom macro.
>>
>> Basic log macros for general usage:
>>
>> (LOG_ERROR_MSG): New function.
>>
>> (LOG_WARNING_MSG): New function.
>>
>> (LOG_NOTICE_MSG): New function.
>>
>> (LOG_INFO_MSG): New function.
>>
>> (LOG_DEBUG_MSG): New function.
>>
>> Prototypes for logmaker.c:
>>
>> (set_log_output_function): New function.
>> no part of log_init
>>
>> (log_init): New function.
>>
>> (set_log_dir): New function.
>> no not used at all
>>
>> (set_log_level): New function.
>> no part of log_init
>>
>> (close_log): New function.
>> no
>>
>> (log_msg_prefix): New function.
>> I think this is part of log_msg
>>
>>
>> Can we agree that on the outside logger has:
>>
>> log_init(<level>, <output_function> maybe other setup parms)
>> log_msg(..) the function the macros call.
>>
>> LOG_FOO_MSG("text", ...)
>>
>> Not more!
>>
>> if log_init() is called with a NULL pointer for output_function, you use
>> your own, that logs fixed to corinthia.log in currenct directory.
>>
>> rgds
>> jan i.
>>
--
Visit my Coding Diary: http://gabriela-gibson.blogspot.com/
Re: [1/2] incubator-corinthia git commit: Logging feature update
Posted by jan i <ja...@apache.org>.
Just a follow up, I see
log_msg(char* level, char* filename, int linenum, char *msg, ...)
{
if (level active)
{
snprintf(buffer, ".......", char* filename, int linenum, char *msg,
...)
call output_function(buffer)
}
}
I hope this explains a bit better how I think.
rgds
jan I.
On 23 August 2015 at 12:00, jan i <ja...@apache.org> wrote:
> Hi Gabriela
>
> Finally I got time to go in detail with your work. First thanks for making
> this important work.
>
> I have some comments to your latest commit:
>
> ---------- Forwarded message ----------
>
> "Add dedicated general logging macros, set a symlink 'current.log', and
> allow local custom log macros with dedicated names for the log file.
> Remove the tar file and use snprintf instead strncat and strdup."
>
> This all sound good, except the symlink, that is not going to work in
> windows or IOS.
>
>
> (#define): Add _GNU_SOURCE.
> do not understand what this #define means.
>
>
> (#include): Add "log_maker.h".
> Add <assert.h>.
> Add <error.h>.
> Add <libgen.h>.
> Add <stdarg.h>.
> Add <stdio.h>.
> Add <stdlib.h>.
> Add <string.h>.
> Add <time.h>.
> Add <unistd.h>.
> Add <errno.h>.
> Add <dirent.h>.
> Add <sys/types.h>.
> Add <sys/stat.h>.
> Add <fcntl.h>.
> These are probably not all needed.
> If they are not needed, they should be removed, and if they are needed
> then there is a serious complexity problem.
>
>
> (get_time_string): New function.
>
> (set_log_output_function): New function. This is a stub.
> I thought we agreed to make that part of log_init, I do not like it as an
> extra function.
>
>
> (log_init): New function. Create the logfile name with host, time
> and program name. Set a symlink 'current.log' into
> ~/../incubator/.
> the log_init function is given an output_function, it should NOT make any
> file operations.
>
> You should add a default_output_function, which you use if you get a NULL
> pointer in the log_init call.
>
>
> (set_log_dir): New function. Set the directory in which to write
> the individual logfiles and the location where the symlonk
> should be created.
> that is part of the output_function. Forget symlink, the program runs in a
> work-dir, and that is where the log files should go.
>
>
> (close_log): New function. Close the file descriptor and inform
> user about the location and name of the generated file.
> I dont like this function, it adds complexity to the code, what happens if
> I call close_log, and then log a message.
>
> It is really not needed, close is done automatially when the program
> closes. In the output function you should use fflush() to secure
> all buffers are written to disk.
>
> (log_msg_prefix): New function. Create the prefix containing the
> name of the log level, the file, line number and function name.
> I would assume that is part of the log_msg call, I do not like to see 2
> different calls.
>
> (log_msg): New function. Write the log prefix and log message to
> file.
>
> * log_maker.h:
>
> Header file for general inclusion so log_maker.c can be used.
>
> (#ifndef): Add LOG_MAKER_H.
> Header guard. Somehow #pragma once does not work for me.
> Look at our other headers, there #pragma seems to work fine.
>
> (#if): Add _MSC_VER. Picked this up in the docs, apparently MSC
> uses _sprintf instead.
>
> this is something that must go in dfplatform.h, we do not want to have MSC
> dependencies throughout the code.
>
>
> (#define): Add LOG_ERR.
> Add LOG_WARNING.
> Add LOG_NOTICE.
> Add LOG_INFO.
> Add LOG_DEBUG.
> These are all strings for now. I can change it back to numbers,
> but
> currently those strings are used to print out the log message
> titles.
>
> Global variables:
>
> (char): Add log_filename[filename_len].
> that is the output_function and as such not the log code. If the output
> function uses a file, code outside logger must open that file.
>
> (int): Add log_file_fd. File descriptor for log file.
> Again output_function.
>
> (static int log_level_initialised): Guard to prevent log_init from
> being called twise.
>
> (char): Add logging_dir[filename_len].
> no.
>
> (char): Add log_symlink[filename_len].
> no.
>
> (LOG_THIS): New function. Basic macro, called by every custom macro.
>
> Basic log macros for general usage:
>
> (LOG_ERROR_MSG): New function.
>
> (LOG_WARNING_MSG): New function.
>
> (LOG_NOTICE_MSG): New function.
>
> (LOG_INFO_MSG): New function.
>
> (LOG_DEBUG_MSG): New function.
>
> Prototypes for logmaker.c:
>
> (set_log_output_function): New function.
> no part of log_init
>
> (log_init): New function.
>
> (set_log_dir): New function.
> no not used at all
>
> (set_log_level): New function.
> no part of log_init
>
> (close_log): New function.
> no
>
> (log_msg_prefix): New function.
> I think this is part of log_msg
>
>
> Can we agree that on the outside logger has:
>
> log_init(<level>, <output_function> maybe other setup parms)
> log_msg(..) the function the macros call.
>
> LOG_FOO_MSG("text", ...)
>
> Not more!
>
> if log_init() is called with a NULL pointer for output_function, you use
> your own, that logs fixed to corinthia.log in currenct directory.
>
> rgds
> jan i.
>
Re: [1/2] incubator-corinthia git commit: Logging feature update
Posted by Peter Kelly <pm...@apache.org>.
> On 27 Aug 2015, at 6:51 pm, jan i <ja...@apache.org> wrote:
>
> Hi.
>
> I think there is a general misunderstand somewhere. You are designing a log
> system, that would be good to have
> in a server side application, but we are just a little application and only
> need a simple logger.
Gabriela: I’d say a better way to think about this exercise is as a debugging system, not a logging system. As Jan suggested, a logging system is a appropriate for a server-side app, but I don’t see this code being useful for anyone other than developers. Where I’d envisage using it myself is if I’m having trouble figuring out why a conversion is failing, and I want to look at what is happening in the code. I want to see any warnings that come up (an example would be “cannot find bookmark with id foo in bookmarks.xml”) and that would help indicate what is causing problems with a conversion.
For this reason, I also don’t see a need to include the hostname or username in the log. Timing information can still be useful though, for identifying performance problems (e.g. answering why did it take 300ms to free this object?).
—
Dr Peter M. Kelly
pmkelly@apache.org
PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
(fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)
Re: [1/2] incubator-corinthia git commit: Logging feature update
Posted by jan i <ja...@apache.org>.
Hi.
I think there is a general misunderstand somewhere. You are designing a log
system, that would be good to have
in a server side application, but we are just a little application and only
need a simple logger.
Here is a couple of limitations, as I see.
The logger is limited to:
- Not have any IO, that is all handled in the user supplied function
- Not have any platform dependent calls (platform dependencies are pr.
definition in DFPlatform)
- Not call assert or any other call that stops the application.
The user supplied function will
- In windows/IOS/Android typically open a dialog box with the text
- and have a skeleton like
void foo(char *text) {
static FILE *file;
if (!file)
file = fopen("corinthia.log", "w"),
fwrite(file, text, strlen(text));
fflush(file);
}
But of course that is just my opinion. If we decide to have full fledged
log system (which I am strongly against),
there are still limitations:
- The code must work on all platforms without use of #ifdef (that would be
DFPlatform.h)
- The code must work identically (e.g. no use of symlinks, because it is
not supported on all platforms).
I do not want to demotivate you, but what we need is a couple of macros, a
simple function, that assembles a
text and calls the user supplied function.
Comments:
On 27 August 2015 at 12:35, Gabriela Gibson <ga...@gmail.com>
wrote:
> First installment (just the #defines):
>
> (#define): Add _GNU_SOURCE.
> Jan: do not understand what this #define means.
>
> It enables:
>
> STRICT_ANSI, _ISOC99_SOURCE, _POSIX_SOURCE, _POSIX_C_SOURCE,
> _XOPEN_SOURCE, _XOPEN_SOURCE_EXTENDED, _LARGEFILE_SOURCE,
> _LARGEFILE64_SOURCE, _FILE_OFFSET_BITS=N, _BSD_SOURCE, _SVID_SOURCE
>
> but I swapped it out for:
>
> #define _XOPEN_SOURCE 700
>
If your code needs this, then you have written code that are not portable,
that is no good. Must be removed
>
> since that is supposed to give better portability. Without either of
> those defines, my compiler complains a lot about implicit fucntion
> definitions. However, that said, it will still link and run.
> Thoughts?
>
If you have implicit function definitions, then it is because you have not
declared them, and that is no good
>
>
>
> Re complexity:
>
> Jan: If they are not needed, they should be removed, and if they are
> needed then there is a serious complexity problem.
>
> I double checked and added comments for what all those includes are.
> Seems I cannot reduce the number further.
>
> #include <assert.h> // assert in log_msg()
>
We do not use that, peter made a substitute that we use (see DFPlatform.h),
BUT a logger should never
call assert !
> #include <libgen.h> // 'basename'
>
You do not need that, logger has no IO, and the user supplied function uses
a simple fopen()
> #include <stdarg.h> // va_args
>
OK
> #include <stdio.h>
>
Should not be needed, but it might be the e.g. sprintf is defined here.
> #include <stdlib.h> // abort() and basename
>
never call abort()
> #include <string.h>
> #include <time.h> // time ops
> #include <unistd.h> // for unlink (symlink)
>
See DFPlatform.h unistd.h does not exist on all platforms.
> #include <errno.h> // errorno for file opening
>
hmmm....you dont need text. If the file cannot be opened, user supplied
function simple does not log if the file cannot be opened.
> #include <dirent.h> // dir operations
>
Never in a logger
> #include <sys/stat.h> // req for lstat (symlink)
>
Far to complex
> #include <fcntl.h> // needed for file ops
>
Far to complex
>
> Regards the symlink:
>
> Jan: This all sound good, except the symlink, that is not going to work in
> windows or IOS.
>
> Can we put a compiler directive here since for unix systems, having
> this service is actually very useful and saves a lot of user/dev time?
>
No way !
>
> More to come soon,
>
Would be nice to see the simple versions I sketched upfront :-)
Keep up the good work, and please do not be demotivated, you wanted to
learn so I also
see it as a learning experience for you.
rgds
jan I
>
> G
>
> On Sun, Aug 23, 2015 at 11:00 AM, jan i <ja...@apache.org> wrote:
> > Hi Gabriela
> >
> > Finally I got time to go in detail with your work. First thanks for
> making
> > this important work.
> >
> > I have some comments to your latest commit:
> >
> > ---------- Forwarded message ----------
> >
> > "Add dedicated general logging macros, set a symlink 'current.log', and
> > allow local custom log macros with dedicated names for the log file.
> > Remove the tar file and use snprintf instead strncat and strdup."
> >
> > This all sound good, except the symlink, that is not going to work in
> > windows or IOS.
> >
> >
> > (#define): Add _GNU_SOURCE.
> > do not understand what this #define means.
> >
> >
> > (#include): Add "log_maker.h".
> > Add <assert.h>.
> > Add <error.h>.
> > Add <libgen.h>.
> > Add <stdarg.h>.
> > Add <stdio.h>.
> > Add <stdlib.h>.
> > Add <string.h>.
> > Add <time.h>.
> > Add <unistd.h>.
> > Add <errno.h>.
> > Add <dirent.h>.
> > Add <sys/types.h>.
> > Add <sys/stat.h>.
> > Add <fcntl.h>.
> > These are probably not all needed.
> > If they are not needed, they should be removed, and if they are needed
> then
> > there is a serious complexity problem.
> >
> >
> > (get_time_string): New function.
> >
> > (set_log_output_function): New function. This is a stub.
> > I thought we agreed to make that part of log_init, I do not like it as an
> > extra function.
> >
> >
> > (log_init): New function. Create the logfile name with host, time
> > and program name. Set a symlink 'current.log' into
> > ~/../incubator/.
> > the log_init function is given an output_function, it should NOT make any
> > file operations.
> >
> > You should add a default_output_function, which you use if you get a NULL
> > pointer in the log_init call.
> >
> >
> > (set_log_dir): New function. Set the directory in which to write
> > the individual logfiles and the location where the symlonk
> > should be created.
> > that is part of the output_function. Forget symlink, the program runs in
> a
> > work-dir, and that is where the log files should go.
> >
> >
> > (close_log): New function. Close the file descriptor and inform
> > user about the location and name of the generated file.
> > I dont like this function, it adds complexity to the code, what happens
> if
> > I call close_log, and then log a message.
> >
> > It is really not needed, close is done automatially when the program
> > closes. In the output function you should use fflush() to secure
> > all buffers are written to disk.
> >
> > (log_msg_prefix): New function. Create the prefix containing the
> > name of the log level, the file, line number and function name.
> > I would assume that is part of the log_msg call, I do not like to see 2
> > different calls.
> >
> > (log_msg): New function. Write the log prefix and log message to
> > file.
> >
> > * log_maker.h:
> >
> > Header file for general inclusion so log_maker.c can be used.
> >
> > (#ifndef): Add LOG_MAKER_H.
> > Header guard. Somehow #pragma once does not work for me.
> > Look at our other headers, there #pragma seems to work fine.
> >
> > (#if): Add _MSC_VER. Picked this up in the docs, apparently MSC
> > uses _sprintf instead.
> >
> > this is something that must go in dfplatform.h, we do not want to have
> MSC
> > dependencies throughout the code.
> >
> >
> > (#define): Add LOG_ERR.
> > Add LOG_WARNING.
> > Add LOG_NOTICE.
> > Add LOG_INFO.
> > Add LOG_DEBUG.
> > These are all strings for now. I can change it back to numbers,
> but
> > currently those strings are used to print out the log message
> > titles.
> >
> > Global variables:
> >
> > (char): Add log_filename[filename_len].
> > that is the output_function and as such not the log code. If the output
> > function uses a file, code outside logger must open that file.
> >
> > (int): Add log_file_fd. File descriptor for log file.
> > Again output_function.
> >
> > (static int log_level_initialised): Guard to prevent log_init from
> > being called twise.
> >
> > (char): Add logging_dir[filename_len].
> > no.
> >
> > (char): Add log_symlink[filename_len].
> > no.
> >
> > (LOG_THIS): New function. Basic macro, called by every custom macro.
> >
> > Basic log macros for general usage:
> >
> > (LOG_ERROR_MSG): New function.
> >
> > (LOG_WARNING_MSG): New function.
> >
> > (LOG_NOTICE_MSG): New function.
> >
> > (LOG_INFO_MSG): New function.
> >
> > (LOG_DEBUG_MSG): New function.
> >
> > Prototypes for logmaker.c:
> >
> > (set_log_output_function): New function.
> > no part of log_init
> >
> > (log_init): New function.
> >
> > (set_log_dir): New function.
> > no not used at all
> >
> > (set_log_level): New function.
> > no part of log_init
> >
> > (close_log): New function.
> > no
> >
> > (log_msg_prefix): New function.
> > I think this is part of log_msg
> >
> >
> > Can we agree that on the outside logger has:
> >
> > log_init(<level>, <output_function> maybe other setup parms)
> > log_msg(..) the function the macros call.
> >
> > LOG_FOO_MSG("text", ...)
> >
> > Not more!
> >
> > if log_init() is called with a NULL pointer for output_function, you use
> > your own, that logs fixed to corinthia.log in currenct directory.
> >
> > rgds
> > jan i.
>
>
>
> --
> Visit my Coding Diary: http://gabriela-gibson.blogspot.com/
>
Re: [1/2] incubator-corinthia git commit: Logging feature update
Posted by Gabriela Gibson <ga...@gmail.com>.
First installment (just the #defines):
(#define): Add _GNU_SOURCE.
Jan: do not understand what this #define means.
It enables:
STRICT_ANSI, _ISOC99_SOURCE, _POSIX_SOURCE, _POSIX_C_SOURCE,
_XOPEN_SOURCE, _XOPEN_SOURCE_EXTENDED, _LARGEFILE_SOURCE,
_LARGEFILE64_SOURCE, _FILE_OFFSET_BITS=N, _BSD_SOURCE, _SVID_SOURCE
but I swapped it out for:
#define _XOPEN_SOURCE 700
since that is supposed to give better portability. Without either of
those defines, my compiler complains a lot about implicit fucntion
definitions. However, that said, it will still link and run.
Thoughts?
Re complexity:
Jan: If they are not needed, they should be removed, and if they are
needed then there is a serious complexity problem.
I double checked and added comments for what all those includes are.
Seems I cannot reduce the number further.
#include <assert.h> // assert in log_msg()
#include <libgen.h> // 'basename'
#include <stdarg.h> // va_args
#include <stdio.h>
#include <stdlib.h> // abort() and basename
#include <string.h>
#include <time.h> // time ops
#include <unistd.h> // for unlink (symlink)
#include <errno.h> // errorno for file opening
#include <dirent.h> // dir operations
#include <sys/stat.h> // req for lstat (symlink)
#include <fcntl.h> // needed for file ops
Regards the symlink:
Jan: This all sound good, except the symlink, that is not going to work in
windows or IOS.
Can we put a compiler directive here since for unix systems, having
this service is actually very useful and saves a lot of user/dev time?
More to come soon,
G
On Sun, Aug 23, 2015 at 11:00 AM, jan i <ja...@apache.org> wrote:
> Hi Gabriela
>
> Finally I got time to go in detail with your work. First thanks for making
> this important work.
>
> I have some comments to your latest commit:
>
> ---------- Forwarded message ----------
>
> "Add dedicated general logging macros, set a symlink 'current.log', and
> allow local custom log macros with dedicated names for the log file.
> Remove the tar file and use snprintf instead strncat and strdup."
>
> This all sound good, except the symlink, that is not going to work in
> windows or IOS.
>
>
> (#define): Add _GNU_SOURCE.
> do not understand what this #define means.
>
>
> (#include): Add "log_maker.h".
> Add <assert.h>.
> Add <error.h>.
> Add <libgen.h>.
> Add <stdarg.h>.
> Add <stdio.h>.
> Add <stdlib.h>.
> Add <string.h>.
> Add <time.h>.
> Add <unistd.h>.
> Add <errno.h>.
> Add <dirent.h>.
> Add <sys/types.h>.
> Add <sys/stat.h>.
> Add <fcntl.h>.
> These are probably not all needed.
> If they are not needed, they should be removed, and if they are needed then
> there is a serious complexity problem.
>
>
> (get_time_string): New function.
>
> (set_log_output_function): New function. This is a stub.
> I thought we agreed to make that part of log_init, I do not like it as an
> extra function.
>
>
> (log_init): New function. Create the logfile name with host, time
> and program name. Set a symlink 'current.log' into
> ~/../incubator/.
> the log_init function is given an output_function, it should NOT make any
> file operations.
>
> You should add a default_output_function, which you use if you get a NULL
> pointer in the log_init call.
>
>
> (set_log_dir): New function. Set the directory in which to write
> the individual logfiles and the location where the symlonk
> should be created.
> that is part of the output_function. Forget symlink, the program runs in a
> work-dir, and that is where the log files should go.
>
>
> (close_log): New function. Close the file descriptor and inform
> user about the location and name of the generated file.
> I dont like this function, it adds complexity to the code, what happens if
> I call close_log, and then log a message.
>
> It is really not needed, close is done automatially when the program
> closes. In the output function you should use fflush() to secure
> all buffers are written to disk.
>
> (log_msg_prefix): New function. Create the prefix containing the
> name of the log level, the file, line number and function name.
> I would assume that is part of the log_msg call, I do not like to see 2
> different calls.
>
> (log_msg): New function. Write the log prefix and log message to
> file.
>
> * log_maker.h:
>
> Header file for general inclusion so log_maker.c can be used.
>
> (#ifndef): Add LOG_MAKER_H.
> Header guard. Somehow #pragma once does not work for me.
> Look at our other headers, there #pragma seems to work fine.
>
> (#if): Add _MSC_VER. Picked this up in the docs, apparently MSC
> uses _sprintf instead.
>
> this is something that must go in dfplatform.h, we do not want to have MSC
> dependencies throughout the code.
>
>
> (#define): Add LOG_ERR.
> Add LOG_WARNING.
> Add LOG_NOTICE.
> Add LOG_INFO.
> Add LOG_DEBUG.
> These are all strings for now. I can change it back to numbers, but
> currently those strings are used to print out the log message
> titles.
>
> Global variables:
>
> (char): Add log_filename[filename_len].
> that is the output_function and as such not the log code. If the output
> function uses a file, code outside logger must open that file.
>
> (int): Add log_file_fd. File descriptor for log file.
> Again output_function.
>
> (static int log_level_initialised): Guard to prevent log_init from
> being called twise.
>
> (char): Add logging_dir[filename_len].
> no.
>
> (char): Add log_symlink[filename_len].
> no.
>
> (LOG_THIS): New function. Basic macro, called by every custom macro.
>
> Basic log macros for general usage:
>
> (LOG_ERROR_MSG): New function.
>
> (LOG_WARNING_MSG): New function.
>
> (LOG_NOTICE_MSG): New function.
>
> (LOG_INFO_MSG): New function.
>
> (LOG_DEBUG_MSG): New function.
>
> Prototypes for logmaker.c:
>
> (set_log_output_function): New function.
> no part of log_init
>
> (log_init): New function.
>
> (set_log_dir): New function.
> no not used at all
>
> (set_log_level): New function.
> no part of log_init
>
> (close_log): New function.
> no
>
> (log_msg_prefix): New function.
> I think this is part of log_msg
>
>
> Can we agree that on the outside logger has:
>
> log_init(<level>, <output_function> maybe other setup parms)
> log_msg(..) the function the macros call.
>
> LOG_FOO_MSG("text", ...)
>
> Not more!
>
> if log_init() is called with a NULL pointer for output_function, you use
> your own, that logs fixed to corinthia.log in currenct directory.
>
> rgds
> jan i.
--
Visit my Coding Diary: http://gabriela-gibson.blogspot.com/