diff mbox series

[v3,2/8] gzip: refactor and expand macros

Message ID 20240424163422.23276-3-dpsmith@apertussolutions.com (mailing list archive)
State New, archived
Headers show
Series Clean up of gzip decompressor | expand

Commit Message

Daniel P. Smith April 24, 2024, 4:34 p.m. UTC
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.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/gzip/gunzip.c  | 14 +++++----
 xen/common/gzip/inflate.c | 61 ++++++++++++++-------------------------
 2 files changed, 30 insertions(+), 45 deletions(-)

Comments

Jan Beulich April 29, 2024, 2:07 p.m. UTC | #1
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
diff mbox series

Patch

diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index d07c451cd875..b7cadadcca8b 100644
--- 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;
+static __init uch get_byte(void) {
+    if ( inptr >= insize )
+    {
+        error("ran out of input data");
+        return 0; /* should never reach */
+    }
+
+    return inbuf[inptr++];
 }
 
 #include "inflate.c"
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 220d2ff4d9d9..02a395aeb86a 100644
--- 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.
+ */
+#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.
- */
-/* unsigned wp;             current position in slide */
-#define wp outcnt
-#define flush_output(w) (wp=(w),flush_window())
-
 /* Tables for deflate from PKZIP's appnote.txt. */
 static const unsigned border[] = {    /* Order of the bit length code lengths */
     16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15};
@@ -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 */
 #define NEEDBITS(n) {while(k<(n)){b|=((ulg)NEXTBYTE())<<k;k+=8;}}
 #define DUMPBITS(n) {b>>=(n);k-=(n);}
 
@@ -582,7 +579,8 @@  static int __init inflate_codes(
             Tracevv((stderr, "%c", slide[w-1]));
             if (w == WSIZE)
             {
-                flush_output(w);
+                wp = w;
+                flush_window();
                 w = 0;
             }
         }
@@ -629,7 +627,8 @@  static int __init inflate_codes(
                     } while (--e);
                 if (w == WSIZE)
                 {
-                    flush_output(w);
+                    wp = w;
+                    flush_window();
                     w = 0;
                 }
             } while (n);
@@ -643,9 +642,6 @@  static int __init inflate_codes(
 
     /* done */
     return 0;
-
- underrun:
-    return 4;   /* Input underrun */
 }
 
 /* "decompress" an inflated type 0 (stored) block. */
@@ -685,7 +681,8 @@  static int __init inflate_stored(void)
         slide[w++] = (uch)b;
         if (w == WSIZE)
         {
-            flush_output(w);
+            wp = w;
+            flush_window();
             w = 0;
         }
         DUMPBITS(8);
@@ -698,9 +695,6 @@  static int __init inflate_stored(void)
 
     DEBG(">");
     return 0;
-
- underrun:
-    return 4;   /* Input underrun */
 }
 
 
@@ -956,10 +950,6 @@  static int noinline __init inflate_dynamic(void)
  out:
     free(ll);
     return ret;
-
- underrun:
-    ret = 4;   /* Input underrun */
-    goto out;
 }
 
 /*
@@ -1005,9 +995,6 @@  static int __init inflate_block(int *e)
 
     /* bad block type */
     return 2;
-
- underrun:
-    return 4;   /* Input underrun */
 }
 
 /* decompress an inflated entry */
@@ -1037,7 +1024,7 @@  static int __init inflate(void)
     }
 
     /* flush out slide */
-    flush_output(wp);
+    flush_window();
 
     /* return success */
     return 0;
@@ -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 */
 
     if ((flags & EXTRA_FIELD) != 0) {
         unsigned len = (unsigned)NEXTBYTE();
@@ -1215,10 +1202,6 @@  static int __init gunzip(void)
         return -1;
     }
     return 0;
-
- underrun:   /* NEXTBYTE() goto's here if needed */
-    error("out of input data");
-    return -1;
 }
 
 /*