[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: logging from PAM modules



Solar Designer wrote:
[]
> >   char defbuf[256];  <== some not very large but still enouth
> >                          for most cases size
> >   char *buf = defbuf;
> >   int size = sizeof(defbuf);
> >   while (snprintf(buf, size, ...) cant fit in size) {
> 
> The "can't fit" is ">= size" with non-broken snprintf()'s.  I don't
> think we should support broken ones at all, simply use a reasonable
> size by default and, if that isn't enough, a truncated log entry is
> the price paid for using a broken libc. ;-)

I didn't mean broken snprintf/etc, maybe I just improperly
written that condition.  I mean "while(size isn't enouth for
formatted string of full length)".

> >     size *= 2;
> >     buf = (buf == defbuf) ? malloc(size) : realloc(buf, size);
> >   }
> 
> Yes, this is what I meant.  I don't think you need that one buffer on
> the stack, -- it only adds complexity.  You also need a free().
it adds minimal complexity and removes unnecessary for 99.9% cases
malloc/free pair.
if (buf != defbuf) free(buf);
But that's at least "non-essential" thing.  ;)

[pam_log vs pam_log & pam_vlog]
> 
> What does this buy you?  I think pam_log() can be simple enough.

Sometimes it is very nice to have two-args log(format, va_list) --
this can simplify code in (some) places gracefully.
One example if some sort of "macro" like this (in module):

  static inline void _pam_debug(int flags, pamh, char *fmt, ...) {
    if (flags & F_DEBUG) {
      va_list ap; va_start(ap, fmt);
      pam_log(pamh, LOG_DEBUG, fmt, ap);
      va_end(ap);
    }
  }

This hides tons of really unnecessary code like the test (flags & F_DEBUG)
that looks just ugly in places where you do real work.
To implement that _pam_debug() as a macro, you need gcc-style
"macro varargs", and no other way exists for that trivial task.

 (#define _pam_debug(flags,pamh,arg...) \
   ((void)((flags)&F_DEBUG?pam_log(pamh,LOG_DEBUG,arg):0))
 )

pam_log+pam_vlog _is_ as simple as single pam_log, it just split
onto two parts, that's all.

[]





[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index] []