[PATCHES v4] Logging to multiple debug backend (was Re: [PATCHES v3] Logging to multiple debug backend)

[Amitay Isaacs]

On Sat, Mar 21, 2015 at 9:54 AM, Christof Schmitt <cs at samba.org> wrote:

> On Fri, Mar 20, 2015 at 07:57:03AM -0700, Christof Schmitt wrote:
> > On Fri, Mar 20, 2015 at 12:39:48PM +1100, Amitay Isaacs wrote:
> > >    On Fri, Mar 20, 2015 at 9:11 AM, Jeremy Allison <[1]jra at samba.org>
> wrote:
> > >      > > Wow. I *really* hate the:
> > >      > >
> > >      > > #define FORMAT_BUFR_SIZE 1024
> > >      > > #define FORMAT_BUFR_MAX (FORMAT_BUFR_SIZE - 1)
> > >      > >
> > >      > >
> > >      > > +       char msg_no_nl[FORMAT_BUFR_SIZE];
> > >      > > ...
> > >      > > +       len = MIN(strlen(msg), FORMAT_BUFR_MAX);
> > >      > >
> > >      > > idiom where a #define length is used as an array
> > >      > > specifier, and a *different* #define is used
> > >      > > as a length limiter.... But that's a patch
> > >      > > for another day :-).
> > >      > >
> > >      > > Other than that - LGTM. Pushed, thanks !
>
> I added a patch at the beginning of the updated patch series to remove
> FORMAT_BUFR_MAX and only use FORMAT_BUFR_SIZE.
>
> > >    Here are the fixes to get standalone ctdb build working.  You can
> squash
> > >    them with the patches that introduce systemd and lttng backends.
> >
> > Thank you.
> >
> > >    However, why are the options for systemd and lttng added to
> top-level
> > >    wscript?  Since the systemd and lttng features are only required for
> > >    logging, those options could be added in lib/util/wscript instead of
> > >    top-level wscript.  In that case, there won't be a need for special
> fix
> > >    for standalone ctdb.
> >
> > Good point. I was probably thinking more in terms of the old configure
> > script than the new waf framework. Let me try to move the checks.
>
> Ok, i learned something new about the waf framework. I initially added
> the build checks to the top-level script, since that one has the systemd
> checks. Since systemd is only used in lib/util, those checks can be
> moved to lib/util, and then it also makes sense to add the new checks in
> that directory.
>

Much better, now that systemd/lttng checks are in lib/util.


>
> Here is the updated patch series for review again. I hope that we are
> getting closer :-)
>

Two minor fixes.  You can squash them with the appropriate patches.

1. Move the #ifdef outside the conditional.  If the condition is false,
there is no code to execute.
2. Since you moved the removal of FORMAT_BUFR_MAX as the first patch,
replace FORMAT_BUFR_MAX with FORMAT_BUFR_SIZE-1

Because of the 1st fix, some of the subsequent patches are slightly
different.

Reviewed-by: me.

Amitay.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches
Type: application/octet-stream
Size: 72176 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150323/8bd89a5e/attachment.obj>