Re: [PATCH v2 5/6] gzip: move crc state into consilidated gzip state

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/04/2024 3:37 pm, Daniel P. Smith wrote:
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

The change in type is fine, but does need discussing.  Furthermore, ...

> diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
> index 8178a05a0190..bef324d3d166 100644
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -74,7 +77,7 @@ static __init void flush_window(struct gzip_state *s)
>       * The window is equal to the output buffer therefore only need to
>       * compute the crc.
>       */
> -    unsigned long c = crc;
> +    unsigned long c = s->crc;

... this wants to be unsigned int I think.

> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index 5735bbcf7eb4..c18ce20210b0 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -1063,16 +1063,14 @@ static int __init inflate(struct gzip_state *s)
>   *
>   **********************************************************************/
>  
> -static ulg __initdata crc_32_tab[256];
> -static ulg __initdata crc;  /* initialized in makecrc() so it'll reside in 
> bss */
> -#define CRC_VALUE (crc ^ 0xffffffffUL)
> +#define CRC_VALUE (s->crc ^ 0xffffffffUL)

$ git grep CRC_VALUE
common/gzip/inflate.c:1052:#define CRC_VALUE (s->crc ^ 0xffffffffUL)
common/gzip/inflate.c:1207:    if (orig_crc != CRC_VALUE) {

I'd expand this in it's single user, but like ...

>  
>  /*
>   * Code to compute the CRC-32 table. Borrowed from
>   * gzip-1.0.3/makecrc.c.
>   */
>  
> -static void __init makecrc(void)
> +static void __init makecrc(struct gzip_state *s)
>  {
>  /* Not copyrighted 1990 Mark Adler */
>  
> @@ -1089,7 +1087,7 @@ static void __init makecrc(void)
>      for (i = 0; i < sizeof(p)/sizeof(int); i++)
>          e |= 1L << (31 - p[i]);
>  
> -    crc_32_tab[0] = 0;
> +    s->crc_32_tab[0] = 0;
>  
>      for (i = 1; i < 256; i++)
>      {
> @@ -1100,11 +1098,11 @@ static void __init makecrc(void)
>              if (k & 1)
>                  c ^= e;
>          }
> -        crc_32_tab[i] = c;
> +        s->crc_32_tab[i] = c;
>      }
>  
>      /* this is initialized here so this code could reside in ROM */
> -    crc = (ulg)0xffffffffUL; /* shift register contents */
> +    s->crc = (ulg)0xffffffffUL; /* shift register contents */

... this, the constant should become -1u or ~0u because of the type change.

I'm not sure what to make of the ROM comment, but I suspect it means the
XOR can be dropped with a bit of care too.

~Andrew