Re: Typecast problems in SKFP driver

[Loïc Grenié]

2008/7/17 Mikael Pettersson <mikpe@xxxxxxxx>:
> Loic Grenie writes:
>  >     I've just compiled the 2.6.26-git5 kernel and the SKFP driver
>  >     compilation produces complaints about typecasts. These complaints
>  >     disappear with the following patch.
>
> NAK, this needs to be done by someone with better understanding of C.

     or, better, by someone with a better understanding of the driver.

>  >
>  >       Thanks,
>  >
>  >           Loïc Grenié
>  > diff --git a/drivers/net/skfp/ess.c b/drivers/net/skfp/ess.c
>  > index 889f987..d3a8afc 100644
>  > --- a/drivers/net/skfp/ess.c
>  > +++ b/drivers/net/skfp/ess.c
>  > @@ -510,7 +510,7 @@ static void ess_send_response(struct s_smc *smc, struct smt_header *sm,
>  >      chg->path.para.p_type = SMT_P320B ;
>  >      chg->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
>  >      chg->path.mib_index = SBAPATHINDEX ;
>  > -    chg->path.path_pad = (u_short)NULL ;
>  > +    chg->path.path_pad = (u_short)(long)NULL ;
>  >      chg->path.path_index = PRIMARY_RING ;
>
> Putting NULL (a pointer type) in an u_short field is utterly
> and completely broken. My guess is that the code really wants:
>
>        chg->path.path_pad = 0;

    Indeed. I do not even understand why it was done that way in the
  first place.

>  >      /* set P320F */
>  > @@ -606,7 +606,7 @@ static void ess_send_alc_req(struct s_smc *smc)
>  >      req->path.para.p_type = SMT_P320B ;
>  >      req->path.para.p_len = sizeof(struct smt_p_320b) - PARA_LEN ;
>  >      req->path.mib_index = SBAPATHINDEX ;
>  > -    req->path.path_pad = (u_short)NULL ;
>  > +    req->path.path_pad = (u_short)(long)NULL ;
>  >      req->path.path_index = PRIMARY_RING ;
>
> ditto
>
>  >
>  >      /* set P0017 */
>  > @@ -636,7 +636,7 @@ static void ess_send_alc_req(struct s_smc *smc)
>  >      /* set P19 */
>  >      req->a_addr.para.p_type = SMT_P0019 ;
>  >      req->a_addr.para.p_len = sizeof(struct smt_p_0019) - PARA_LEN ;
>  > -    req->a_addr.sba_pad = (u_short)NULL ;
>  > +    req->a_addr.sba_pad = (u_short)(long)NULL ;
>  >      req->a_addr.alloc_addr = null_addr ;
>
> ditto
>
>  >
>  >      /* set P1A */
>  > diff --git a/drivers/net/skfp/pmf.c b/drivers/net/skfp/pmf.c
>  > index ea85de9..5798be4 100644
>  > --- a/drivers/net/skfp/pmf.c
>  > +++ b/drivers/net/skfp/pmf.c
>  > @@ -44,17 +44,17 @@ static SMbuf *smt_build_pmf_response(struct s_smc *smc, struct smt_header *req,
>  >                                   int set, int local);
>  >  static int port_to_mib(struct s_smc *smc, int p);
>  >
>  > -#define MOFFSS(e)   ((int)&(((struct fddi_mib *)0)->e))
>  > -#define MOFFSA(e)   ((int) (((struct fddi_mib *)0)->e))
>  > +#define MOFFSS(e)   ((long)&(((struct fddi_mib *)0)->e))
>  > +#define MOFFSA(e)   ((long) (((struct fddi_mib *)0)->e))
>
> NAK, use offsetof().
>
> How is the MOFFSA() used? As written it makes no sense at all
> as evaluating it as an r-value will oops.

     I think it is used when e is an array (unchecked).

>  >
>  > -#define MOFFMS(e)   ((int)&(((struct fddi_mib_m *)0)->e))
>  > -#define MOFFMA(e)   ((int) (((struct fddi_mib_m *)0)->e))
>  > +#define MOFFMS(e)   ((long)&(((struct fddi_mib_m *)0)->e))
>  > +#define MOFFMA(e)   ((long) (((struct fddi_mib_m *)0)->e))
>
> ditto
>
>  >
>  > -#define MOFFAS(e)   ((int)&(((struct fddi_mib_a *)0)->e))
>  > -#define MOFFAA(e)   ((int) (((struct fddi_mib_a *)0)->e))
>  > +#define MOFFAS(e)   ((long)&(((struct fddi_mib_a *)0)->e))
>  > +#define MOFFAA(e)   ((long) (((struct fddi_mib_a *)0)->e))
>
> ditto
>
>  >
>  > -#define MOFFPS(e)   ((int)&(((struct fddi_mib_p *)0)->e))
>  > -#define MOFFPA(e)   ((int) (((struct fddi_mib_p *)0)->e))
>  > +#define MOFFPS(e)   ((long)&(((struct fddi_mib_p *)0)->e))
>  > +#define MOFFPA(e)   ((long) (((struct fddi_mib_p *)0)->e))
>
> ditto
>
> To be fair, the original code is crap, but your changes don't
> make it any better.
>

     I agree... Maybe someone with stronger guts than mine will have
  read about it and will correct it for some next release.

      Thanks once again,

          Loïc
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/