Mailing-List: contact dev-help@corinthia.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@corinthia.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CALfVaATqdzYf4dorXR9D74z+KHjt5SpqW==HETp_ZHhqWuNvBw@mail.gmail.com>
References: <08dde25523b549f88cbe680fb66405a8@git.apache.org>
	<CAK2iWdTZEw9j1e_5vxgAAXFU=xtE5AR9FkF0Ar2cGN5xzXxBTw@mail.gmail.com>
	<CALfVaATqdzYf4dorXR9D74z+KHjt5SpqW==HETp_ZHhqWuNvBw@mail.gmail.com>
Date: Thu, 27 Aug 2015 13:51:26 +0200
Message-ID: 
 <CAK2iWdRpDk9jgPkf6HqMocOrhCVtGoga+daOxekLsJi9FT3BoQ@mail.gmail.com>
Subject: Re: [1/2] incubator-corinthia git commit: Logging feature update
From: jan i <jani@apache.org>
To: "dev@corinthia.incubator.apache.org" <dev@corinthia.incubator.apache.org>
Content-Type: multipart/alternative; boundary=001a11c1037a4ec02f051e49964f

--001a11c1037a4ec02f051e49964f
Content-Type: text/plain; charset=UTF-8

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.gibson@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 <jani@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/
>

--001a11c1037a4ec02f051e49964f--