Using dbgtext for logging

[Dan Cohen1]

Thanks for your answer Chris.
I didn't think it was a bug. Just wondered about the reasons for the 
inconsistency.

Do you think changing these instances to be consistent with the rest of 
the code will give any value?

Thanks,
Dan Cohen
IBM - XIV, Israel
NAS Development Team



From:   "Christopher R. Hertel" <crh at samba.org>
To:     Dan Cohen1/Israel/IBM at IBMIL, 
Cc:     samba-technical at lists.samba.org
Date:   31/07/2013 16:29
Subject:        Re: Using dbgtext for logging



Dan,

Many, many years ago I overhauled the debug subsystem in Samba.  I 
provided
a more complete API, including the dbgtext() function, and I went through
the Samba source and found instances in which we were calling DEBUG macros
multiple times to compose long messages.  In those cases, I replaced the
multiple macro calls with what I called "Debug blocks", similar to what 
you
see below.  Some others also picked up the style.

The advantage of doing it that way was--and is--that there is only one 
if()
statement surrounding the block, and the programmer has the ability to do
more careful formatting of the output.  In addition, because it's a clear
block, temporary stack variables can be allocated and calculations that 
only
pertain to the block can be performed.  Those variables are dropped when 
the
block is exited.

...but it wasn't common style for most of the Team so, over time, much of
the code has been reverted to the simpler style.  In addition, the debug
subsystem has been rewritten a few times since.

So what you're seeing here is an artifact dating back to the Samba 2.2.x
days.  It's not a bug, it's just an inconsistency.

Chris -)-----

On 07/31/2013 04:00 AM, Dan Cohen1 wrote:
> Hi,
> 
> While doing some work on samba logging I've noticed that there are 
several 
> places in the samba code where logging is done directly using the 
dbgtext 
> function. For example: in source3/smbd/oplock.c:initial_break_processing 

> (line 267):
> 
> static files_struct *initial_break_processing(
>     struct smbd_server_connection *sconn, struct file_id id,
>     unsigned long file_id)
> {
>     files_struct *fsp = NULL;
> 
>     if( DEBUGLVL( 3 ) ) {
>         dbgtext( "initial_break_processing: called for %s/%u\n",
>              file_id_string_tos(&id), (int)file_id);
>         dbgtext( "Current oplocks_open (exclusive = %d, levelII = 
%d)\n",
>             sconn->oplocks.exclusive_open,
>             sconn->oplocks.level_II_open);
>     }
> 
>     /*
>      * We need to search the file open table for the
>      * entry containing this dev and inode, and ensure
>      * we have an oplock on it.
>      */
> 
>     fsp = file_find_dif(sconn, id, file_id);
> 
>     if(fsp == NULL) {
>         /* The file could have been closed in the meantime - return 
> success. */
>         if( DEBUGLVL( 3 ) ) {
>             dbgtext( "initial_break_processing: cannot find open file 
with 
> " );
>             dbgtext( "file_id %s gen_id = %lu", file_id_string_tos(&id), 

> file_id);
>             dbgtext( "allowing break to succeed.\n" );
>         }
>         return NULL;
>     }
> 
>     /* Ensure we have an oplock on the file */
> 
>     /*
>      * There is a potential race condition in that an oplock could
>      * have been broken due to another udp request, and yet there are
>      * still oplock break messages being sent in the udp message
>      * queue for this file. So return true if we don't have an oplock,
>      * as we may have just freed it.
>      */
> 
>     if(fsp->oplock_type == NO_OPLOCK) {
>         if( DEBUGLVL( 3 ) ) {
>             dbgtext( "initial_break_processing: file %s ",
>                  fsp_str_dbg(fsp));
>             dbgtext( "(file_id = %s gen_id = %lu) has no oplock.\n",
>                  file_id_string_tos(&id), fsp->fh->gen_id );
>             dbgtext( "Allowing break to succeed regardless.\n" );
>         }
>         return NULL;
>     }
> 
>     return fsp;
> }
> 
> Calling directly to dbgtext happens also in a few other files such as 
> source3/smbd/service.c, source3/smbd/posix_acl.c and more.
> This method of logging is on contrary to most of the samba code where 
> logging is done using the logging macros DEBUG, DEBUGC etc.
> 
> Is avoiding the log macros done in those files deliberately? Is it done 
> from any particular reason? Or is it just leftovers from old code or 
> something like that?
> 
> Thanks,
> Dan Cohen
> IBM - XIV, Israel
> NAS Development Team
> 

-- 
"Implementing CIFS - the Common Internet FileSystem" ISBN: 013047116X
Samba Team -- http://www.samba.org/     -)-----   Christopher R. Hertel
jCIFS Team -- http://jcifs.samba.org/   -)-----   ubiqx development, 
uninq.
ubiqx Team -- http://www.ubiqx.org/     -)-----   crh at ubiqx.mn.org
OnLineBook -- http://ubiqx.org/cifs/    -)-----   crh at ubiqx.org