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 <gabriela.gib...@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 <j...@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/ >