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 <[email protected]>
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 <[email protected]> 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/
>
