On 24.04.2024 18:34, Daniel P. Smith wrote:
> This commit refactors macros into proper static functions. It in-place expands
> the `flush_output` macro, allowing the clear removal of the dead code
> underneath the `underrun` label.
But it's NEXTBYTE() which uses the label, not flush_output(). I'm actually
unconvinced of its expanding / removal.
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -25,8 +25,6 @@ typedef unsigned char uch;
> typedef unsigned short ush;
> typedef unsigned long ulg;
>
> -#define get_byte() (inptr < insize ? inbuf[inptr++] : fill_inbuf())
> -
> /* Diagnostic functions */
> #ifdef DEBUG
> # define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0)
> @@ -52,10 +50,14 @@ static __init void error(const char *x)
> panic("%s\n", x);
> }
>
> -static __init int fill_inbuf(void)
> -{
> - error("ran out of input data");
> - return 0;
I'm not convinced of the removal of this as a separate function. It only
calling error() right now could change going forward, so I'd at least
expect a little bit of a justification.
> +static __init uch get_byte(void) {
Nit: Brace goes on its own line. Also for (effectively) new code it would
be nice if __init (and alike) would be placed "canonically", i.e. between
type and identifier.
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -119,6 +119,18 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10
> 13:27:04 jloup Exp #";
>
> #endif /* !__XEN__ */
>
> +/*
> + * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
> + * stream to find repeated byte strings. This is implemented here as a
> + * circular buffer. The index is updated simply by incrementing and then
> + * ANDing with 0x7fff (32K-1).
> + *
> + * It is left to other modules to supply the 32 K area. It is assumed
> + * to be usable as if it were declared "uch slide[32768];" or as just
> + * "uch *slide;" and then malloc'ed in the latter case. The definition
> + * must be in unzip.h, included above.
Nit: s/definition/declaration/ ?
> + */
> +#define wp outcnt
> #define slide window
>
> /*
> @@ -150,21 +162,6 @@ static int inflate_dynamic(void);
> static int inflate_block(int *);
> static int inflate(void);
>
> -/*
> - * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
> - * stream to find repeated byte strings. This is implemented here as a
> - * circular buffer. The index is updated simply by incrementing and then
> - * ANDing with 0x7fff (32K-1).
> - *
> - * It is left to other modules to supply the 32 K area. It is assumed
> - * to be usable as if it were declared "uch slide[32768];" or as just
> - * "uch *slide;" and then malloc'ed in the latter case. The definition
> - * must be in unzip.h, included above.
Oh, an earlier comment just moves up. Is there really a need for this
extra churn?
> @@ -224,7 +221,7 @@ static const ush mask_bits[] = {
> 0x01ff, 0x03ff, 0x07ff, 0x0fff, 0x1fff, 0x3fff, 0x7fff, 0xffff
> };
>
> -#define NEXTBYTE() ({ int v = get_byte(); if (v < 0) goto underrun; (uch)v;
> })
> +#define NEXTBYTE() (get_byte()) /* get_byte will panic on failure */
Nit: No need for the outer parentheses.
> @@ -1148,8 +1135,8 @@ static int __init gunzip(void)
> NEXTBYTE();
> NEXTBYTE();
>
> - (void)NEXTBYTE(); /* Ignore extra flags for the moment */
> - (void)NEXTBYTE(); /* Ignore OS type for the moment */
> + NEXTBYTE(); /* Ignore extra flags for the moment */
> + NEXTBYTE(); /* Ignore OS type for the moment */
In Misra discussions there were indications that such casts may need (re-)
introducing. Perhaps better leave this alone, the more when it's not
really fitting the patch's purpose?
Jan
