ulogd's SQLITE3 "buffer" option

ulogd's SQLITE3 "buffer" option

[Alex Xu]

I was attempting to configure sqlite3 output in ulogd and could not
determine the function of the "buffer" configuration option.

$ grep buffer output/sqlite3/ulogd_output_sqlite3.c
        int buffer_size;
        int buffer_curr;
                        .key = "buffer",
#define buffer_ce(pi)   (pi)->config_kset->ces[2].u.value
                priv->buffer_curr++;
        /* initialize our buffer size and counter */
        priv->buffer_size = buffer_ce(pi);
        priv->buffer_curr = 0;
$ grep -rF buffer_curr .
output/sqlite3/ulogd_output_SQLITE3.c:  int buffer_curr;
output/sqlite3/ulogd_output_SQLITE3.c:          priv->buffer_curr++;
output/sqlite3/ulogd_output_SQLITE3.c:  priv->buffer_curr = 0;

Now, it is quite likely that I am missing something, but it would
appear that buffer_curr is never actually *read*, only *written*,
and it's a basic fact of programming that a variable can have no effect
if it is never read from. [citation needed]

It would appear that the original code in ulogd-1.x employed a 'buffer'
which was actually a counter for when to tell sqlite to sync to disk.
When the code was rewritten by "Holger" and committed by Pablo Neira
Ayuso (8f7bb61), the syncing part was removed but the buffer variable
was simply renamed.

Nobody noticed because one fsync every few seconds is tiny.

Therefore, I think we should remove the "buffer" option entirely.

Thoughts? I will send a patch if there are no objections.

Please CC me on replies.

Re: ulogd's SQLITE3 "buffer" option

[Pablo Neira Ayuso]

On Tue, Dec 29, 2015 at 06:38:31PM -0500, Alex Xu wrote:
> I was attempting to configure sqlite3 output in ulogd and could not
> determine the function of the "buffer" configuration option.
> 
> $ grep buffer output/sqlite3/ulogd_output_sqlite3.c
>         int buffer_size;
>         int buffer_curr;
>                         .key = "buffer",
> #define buffer_ce(pi)   (pi)->config_kset->ces[2].u.value
>                 priv->buffer_curr++;
>         /* initialize our buffer size and counter */
>         priv->buffer_size = buffer_ce(pi);
>         priv->buffer_curr = 0;
> $ grep -rF buffer_curr .
> output/sqlite3/ulogd_output_SQLITE3.c:  int buffer_curr;
> output/sqlite3/ulogd_output_SQLITE3.c:          priv->buffer_curr++;
> output/sqlite3/ulogd_output_SQLITE3.c:  priv->buffer_curr = 0;
> 
> Now, it is quite likely that I am missing something, but it would
> appear that buffer_curr is never actually *read*, only *written*,
> and it's a basic fact of programming that a variable can have no effect
> if it is never read from. [citation needed]
> 
> It would appear that the original code in ulogd-1.x employed a 'buffer'
> which was actually a counter for when to tell sqlite to sync to disk.
> When the code was rewritten by "Holger" and committed by Pablo Neira
> Ayuso (8f7bb61), the syncing part was removed but the buffer variable
> was simply renamed.
> 
> Nobody noticed because one fsync every few seconds is tiny.
> 
> Therefore, I think we should remove the "buffer" option entirely.
> 
> Thoughts? I will send a patch if there are no objections.

This may be well a leftover that was introduced by when that large
rewrite took, please submit a patch that we can review.

I'm also Cc'ing Eric Leblond, he maintains ulogd2, just in case he
finds some cycles to confirm this issue.

Thanks for reporting.

Re: ulogd's SQLITE3 "buffer" option

[Eric Leblond]

Hello,

On Wed, 2015-12-30 at 01:19 +0100, Pablo Neira Ayuso wrote:
> On Tue, Dec 29, 2015 at 06:38:31PM -0500, Alex Xu wrote:
> > I was attempting to configure sqlite3 output in ulogd and could not
> > determine the function of the "buffer" configuration option.
> > 
> > $ grep buffer output/sqlite3/ulogd_output_sqlite3.c
> >         int buffer_size;
> >         int buffer_curr;
> >                         .key = "buffer",
> > #define buffer_ce(pi)   (pi)->config_kset->ces[2].u.value
> >                 priv->buffer_curr++;
> >         /* initialize our buffer size and counter */
> >         priv->buffer_size = buffer_ce(pi);
> >         priv->buffer_curr = 0;
> > $ grep -rF buffer_curr .
> > output/sqlite3/ulogd_output_SQLITE3.c:  int buffer_curr;
> > output/sqlite3/ulogd_output_SQLITE3.c:          priv-
> > >buffer_curr++;
> > output/sqlite3/ulogd_output_SQLITE3.c:  priv->buffer_curr = 0;
> > 
> > Now, it is quite likely that I am missing something, but it would
> > appear that buffer_curr is never actually *read*, only *written*,
> > and it's a basic fact of programming that a variable can have no
> > effect
> > if it is never read from. [citation needed]
> > 
> > It would appear that the original code in ulogd-1.x employed a
> > 'buffer'
> > which was actually a counter for when to tell sqlite to sync to
> > disk.
> > When the code was rewritten by "Holger" and committed by Pablo
> > Neira
> > Ayuso (8f7bb61), the syncing part was removed but the buffer
> > variable
> > was simply renamed.
> > 
> > Nobody noticed because one fsync every few seconds is tiny.
> > 
> > Therefore, I think we should remove the "buffer" option entirely.
> > 
> > Thoughts? I will send a patch if there are no objections.
> 
> This may be well a leftover that was introduced by when that large
> rewrite took, please submit a patch that we can review.

Agree on diagnostic, it seems unused in current version of the code.
Waiting for the patch.

Happy new year to all,
-- 
Eric Leblond <eric@regit.org>


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html