Compilation with gcc 4.6.0

[Russ Allbery]

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
that you describe.

> In innd/art.c:

> 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;
> }

> The "c" variable is unused.  I believe it is here so that the parameter
> is not directly used in the function.
> Is there a recommended behaviour between:
>  - removing "c = *p"
>  - declaring "char c UNUSED;"
> ?

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;

> In lib/perl.c:


> @@ -75,12 +75,15 @@
>          if (perl_get_cv("filter_end", false) != NULL) {
>              ENTER;
>              SAVETMPS;
> +            /* call_argv automatically uses PUSHMARK(SP). */
>              perl_call_argv("filter_end", G_EVAL | G_DISCARD | G_NOARGS, argv);
> +            SPAGAIN;
>              if (SvTRUE(ERRSV)) {
>                  syslog (L_ERROR, "SERVER perl function filter_end died: %s",
>                          SvPV(ERRSV, PL_na));
>                  (void) POPs;
>              }
> +            PUTBACK;
>              FREETMPS;
>              LEAVE;
>          }

This fix is correct so far as I can tell.

> @@ -131,8 +134,9 @@
>          char *evalfile = NULL;
>          dSP;
>      
>          ENTER;
>          SAVETMPS;
> +        PUSHMARK(SP);
>  
>          /* The Perl expression which will be evaluated. */   
>          xasprintf(&evalfile, "do '%s'", startupfile);

> @@ -152,8 +159,9 @@
>              PERLreadfilter (filterfile,function) ;
>          }
>  
> +        PUTBACK;
>          FREETMPS;
>          LEAVE;
>      } else {

This part is just a mess and has needed rewriting for a while.  The real
work that requires that bracketing is happening in PERLfilter, so really
the stack manipulation should probably be moved into it.  The above
probably doesn't do any harm, but I don't think it catches all the
weirdness that we should be handling.

-- 
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.