corinthia-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gabriela Gibson <>
Subject Re: [1/2] incubator-corinthia git commit: Logging feature update
Date Wed, 26 Aug 2015 20:26:42 GMT
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:

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! :))


On Sun, Aug 23, 2015 at 11:06 AM, jan i <> 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 <> 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:

View raw message