headers
Julien ÉLIE | 12 Jun 2011 20:04
Favicon

Compilation with gcc 4.6.0

Hi all,

I am currently building INN with gcc 4.6.0; a few issues appear with
the new "-Wunused-but-set-parameter" and "-Wunused-but-set-variable"
warnings.

I fixed all of them in our source code (variables or parameters
really not used).

However, here are a few errors I do not not what to do with.

In authprogs/radius.c:

typedef struct _auth_req {
    unsigned char       code;
    unsigned char       id;
    unsigned short      length;
    unsigned char       vector[AUTH_VECTOR_LEN];
    unsigned char       data[NNTP_MAXLEN_COMMAND*2];
    int                 datalen;
} auth_req;

static void req_copyto (auth_req to, sending_t *from)
{
    to = from->req;
}

The function is called in the code with:
    req_copyto(req, sreq);
(Continue reading)

Permalink | Reply | 10 comments |
headers
Russ Allbery | 12 Jun 2011 20:48
Picon
Favicon
Gravatar

Re: Compilation with gcc 4.6.0

Julien ÉLIE <julien <at> trigofacile.com> writes:

> In authprogs/radius.c:

> typedef struct _auth_req {
>     unsigned char       code;
>     unsigned char       id;
>     unsigned short      length;
>     unsigned char       vector[AUTH_VECTOR_LEN];
>     unsigned char       data[NNTP_MAXLEN_COMMAND*2];
>     int                 datalen;
> } auth_req;

> static void req_copyto (auth_req to, sending_t *from)
> {
>     to = from->req;
> }

> The function is called in the code with:
>     req_copyto(req, sreq);

> gcc tells me the function does nothing ("to" is unused).  I understand
> that we should give &req as the first argument and take the *to pointer.
> However, it looks strange that we should do that.  Wasn't the radius
> authentication working?  (As I do not use one, I hadn't had the
> opportunity to test it yet.)

Oh, weird.  I wonder if we're taking advantage of compiler-specific
behavior in passing structs by value instead of by reference.  I suppose
that must be working somehow, but I'd definitely do the transformation
(Continue reading)

Permalink | Reply | 9 comments |
headers
Julien ÉLIE | 12 Jun 2011 21:46
Favicon

Re: Compilation with gcc 4.6.0

Hi Russ,

>> static bool
>> ListHas(const char **list, const char *p)
>> {
>>    const char	*q;
>>    char		c;
>>
>>    for (c = *p; (q = *list) != NULL; list++)
>>      if (strcasecmp(p, q) == 0)
>>        return true;
>>    return false;
>
> That whole function is just weirdly written.  I'd write this as:
>
>      size_t i;
>
>      for (i = 0; list[i] != NULL; i++)
>          if (strcasecmp(p, list[i]) == 0)
>              return true;
>      return false;

Your suggestion is easier to read, I agree.  Yet, it adds more 
computations (if i=100, you will have to walk to list[100] twice, and so 
on with 101, 102, etc.).  The complexity of the function is now O(n^2) 
whereas it was O(n) with list++ :-)

Anyway, the rest of the source code is not always optimized…

> This part is just a mess and has needed rewriting for a while.  The real
(Continue reading)

Permalink | Reply | 7 comments |
headers
Russ Allbery | 12 Jun 2011 23:00
Picon
Favicon
Gravatar

Re: Compilation with gcc 4.6.0

Julien ÉLIE <julien <at> trigofacile.com> writes:

>>> static bool
>>> ListHas(const char **list, const char *p)
>>> {
>>>    const char	*q;
>>>    char		c;
>>>
>>>    for (c = *p; (q = *list) != NULL; list++)
>>>      if (strcasecmp(p, q) == 0)
>>>        return true;
>>>    return false;
>>
>> That whole function is just weirdly written.  I'd write this as:
>>
>>      size_t i;
>>
>>      for (i = 0; list[i] != NULL; i++)
>>          if (strcasecmp(p, list[i]) == 0)
>>              return true;
>>      return false;

> Your suggestion is easier to read, I agree.  Yet, it adds more
> computations (if i=100, you will have to walk to list[100] twice, and so
> on with 101, 102, etc.).  The complexity of the function is now O(n^2)
> whereas it was O(n) with list++ :-)

I'm not following.  list is just an array of strings.  Previously, we
traversed the array via incrementing a pointer; in the new version, we
traverse the array using an explicit offset.  But either way, the
(Continue reading)

Permalink | Reply | 6 comments |
headers
Julien ÉLIE | 12 Jun 2011 23:57
Favicon

Re: Compilation with gcc 4.6.0

Hi Russ,

>>>>     for (c = *p; (q = *list) != NULL; list++)
>>>
>>>       for (i = 0; list[i] != NULL; i++)
>>>           if (strcasecmp(p, list[i]) == 0)
> 
> I'm not following.  list is just an array of strings.  Previously, we
> traversed the array via incrementing a pointer; in the new version, we
> traverse the array using an explicit offset.

I thought that in the first version, we would only have one incrementation
(list++) and an assignment (q).  Therefore, we read the list sequentially,
traversing it.
Whereas in the new version, we would have an incrementation (i++) and
a traversal at each iteration (list[i]).

> But either way, the
> generated assembly code should amount to functionally the same thing

Yes, you're right.  I was a bit too naive when I thought that list[n]
would lead to code that parsed list[0], list[1], list[2], etc.
up to list[n].
The assembly code should directly use the address n*(size of a pointer)
after list[0].  So, OK, it is equivalent.

> I suspect there are latent stack handling bugs in PERLfilter should the
> stack ever end up getting reallocated.  There is a lot of code like:
> 
>      if (perl_get_cv("filter_before_reload", false) != NULL)    {
(Continue reading)

Permalink | Reply | 0 comments |
headers
Julien ÉLIE | 13 Jun 2011 17:15
Favicon

Re: Compilation with gcc 4.6.0

Hi Russ,

> I suspect there are latent stack handling bugs in PERLfilter should the
> stack ever end up getting reallocated.

Yep, you're totally right.
I believe it is what I experienced yesterday when innd was segfaulting. 
  The Perl stack may have been reallocated or corrupted.

A copy of the Perl stack pointer is saved at several places in the code 
but not always restored the way it should be.
In PLartfilter() in innd/perl.c, we have the following calls:

   ENTER;
   SAVETMPS;
   PUSHMARK(SP);
   rc = perl_call_sv((SV *) filter, G_EVAL|G_SCALAR|G_NOARGS);
   SPAGAIN;
   if (SvTRUE(ERRSV)) {
     (void) POPs;
     PerlFilter(false);
   }
   PUTBACK;
   FREETMPS;
   LEAVE;

And PerlFilter(), as you mentioned, also plays with the stack:

   ENTER;
   SAVETMPS;
(Continue reading)

Permalink | Reply | 0 comments |
headers
Julien ÉLIE | 13 Jun 2011 18:39
Favicon

Re: Compilation with gcc 4.6.0

Hi Russ,

> native functions that did what we're doing with perl_eval_pv() to
> load Perl code from a file.

As a side note, it is true that the current way to load our Perl 
functions is far from being optimum.
We just eval the file when reloading it.

Therefore, if I change the name "filter_art" to another thing, the 
reload is fine and it goes on working (as a matter of fact, "filter_art" 
still exists from the first eval).
However, it will of course break the next time INN is restarted.

It is not that easy to change because we have two different files:
- startup_innd.pl
- filter_innd.pl

and when we reload filter_innd.pl, we should not "undef filter_art" 
because it may have been defined in startup_innd.pl!  And if defined in 
both of these files, another problem…

The way our Python filters work is better:  the .py files are considered 
as modules.  Upon reloading them, we just reload the files as modules 
(so it is a complete refresh of all the available functions in the module).

Shouldn't we do the same thing with Perl?  It would allow to load a 
module 'Filter_innd.pm' and call 'Filter_innd::filter_art'?
On reload, we just unload 'Filter_innd' and load it again.
I still do not know what to do for startup_innd.pl; the name space would
(Continue reading)

Permalink | Reply | 3 comments |
headers
Russ Allbery | 13 Jun 2011 19:56
Picon
Favicon
Gravatar

Re: Compilation with gcc 4.6.0

Julien ÉLIE <julien <at> trigofacile.com> writes:

> Shouldn't we do the same thing with Perl?  It would allow to load a module
> Filter_innd.pm' and call 'Filter_innd::filter_art'?
> On reload, we just unload 'Filter_innd' and load it again.
> I still do not know what to do for startup_innd.pl; the name space would
> not be the same.

Yeah, this is exactly the kind of thing that it seemed like we should be
doing.  (The Perlish namespace would probably be something like
INN::Filter.)  I don't know that there's any way to do it in a way that's
backward-compatible, though.  (But I haven't thought about it hard.)

--

-- 
Russ Allbery (rra <at> stanford.edu)             <http://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <http://www.eyrie.org/~eagle/faqs/questions.html> explains why.
_______________________________________________
inn-workers mailing list
inn-workers <at> lists.isc.org
https://lists.isc.org/mailman/listinfo/inn-workers
Permalink | Reply | 2 comments |
headers
Nix | 18 Jun 2011 23:19
Picon

Re: Compilation with gcc 4.6.0

On 12 Jun 2011, Russ Allbery spake thusly:

> Julien ÉLIE <julien <at> trigofacile.com> writes:
>
>> In authprogs/radius.c:
>
>> static void req_copyto (auth_req to, sending_t *from)
>> {
>>     to = from->req;
>> }
>
>> The function is called in the code with:
>>     req_copyto(req, sreq);
>
>> gcc tells me the function does nothing ("to" is unused).  I understand
>> that we should give &req as the first argument and take the *to pointer.
[...]
> Oh, weird.  I wonder if we're taking advantage of compiler-specific
> behavior in passing structs by value instead of by reference.

Not hardly, it's been in the C Standard forever. (IIRC vendors started
adding it to C in something like 1978.)

--

-- 
NULL && (void)
_______________________________________________
inn-workers mailing list
inn-workers <at> lists.isc.org
https://lists.isc.org/mailman/listinfo/inn-workers
Permalink | Reply | 0 comments |
headers
Florian Schlichting | 22 Jun 2011 22:59
Picon
Picon

innfeed busy logging timer stats

Hi,

working on a development server, I noticed innfeed printing lots of
useless lines to news.notice:

Jun 22 00:12:05 Lucia innfeed[18100]: ME time 1000 idle 1000(1) blstats 0(1) stsfile 0(0) newart 0(0)
readart 0(0) prepart 0(0) read 0(1) write 0(0) 
Jun 22 00:12:05 Lucia innfeed[18100]: ME time 0 idle 0(1) blstats 0(1) stsfile 0(0) newart 0(0) readart
0(0) prepart 0(0) read 0(0) write 0(0) 
Jun 22 00:12:05 Lucia innfeed[18100]: ME time 0 idle 0(1) blstats 0(1) stsfile 0(0) newart 0(2) readart
0(0) prepart 0(0) read 0(1) write 0(0) 
Jun 22 00:12:05 Lucia innfeed[18100]: ME time 0 idle 0(1) blstats 0(1) stsfile 0(0) newart 0(0) readart
0(0) prepart 0(0) read 0(1) write 0(1) 
Jun 22 00:12:05 Lucia innfeed[18100]: ME time 0 idle 0(1) blstats 0(1) stsfile 0(0) newart 0(0) readart
0(0) prepart 0(0) read 0(0) write 0(1) 
Jun 22 00:12:06 Lucia innfeed[18100]: ME time 1000 idle 1000(1) blstats 0(1) stsfile 0(0) newart 0(0)
readart 0(0) prepart 0(0) read 0(1) write 0(0) 
Jun 22 00:12:06 Lucia innfeed[18100]: ME time 0 idle 0(1) blstats 0(1) stsfile 0(0) newart 0(0) readart
0(0) prepart 0(0) read 0(0) write 0(0) 

The reason seems to be that TMRnow() doesn't return the current time, as
I think innfeed assumes, but the readily usable number of milliseconds
since the last call to TMRsummary() or TMRinit(). Patch below.

Florian

---
 innfeed/endpoint.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)
(Continue reading)

Permalink | Reply | 11 comments |

Gmane