All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6]  Clean up of gzip decompressor
@ 2024-04-17 14:37 Daniel P. Smith
  2024-04-17 14:37 ` [PATCH v2 1/6] gzip: drop unused define checks Daniel P. Smith
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

An issue ran into by hyperlaunch was the need to use the gzip decompressor
multiple times. The current implementation fails when reused due to tainting of
decompressor state from a previous usage. This series seeks to colocate the
gzip unit files under a single directory similar to the other decompression
algorithms.  To enable the refactoring of the state tracking, the code is then
cleaned up in line with Xen coding style.

Changes in v2:
- patch "xen/gzip: Colocate gunzip code files" was merged
- dropped ifdef chunks that are never enabled
- addressed formatting changes missed in v1
- replaced custom memory allocator with xalloc_bytes()
- renamed gzip_data to gzip_state
- moved gzip_state to being dynamically allocated
- changed crc table to the explicit size of uint32_t 
- instead of moving huffman tracking into state, dropped altogether

Daniel P. Smith (6):
  gzip: drop unused define checks
  gzip: clean up comments and fix code alignment
  gzip: remove custom memory allocator
  gzip: refactor state tracking
  gzip: move crc state into consilidated gzip state
  gzip: drop huffman code table tracking

 xen/common/gzip/gunzip.c  |  87 ++--
 xen/common/gzip/inflate.c | 974 ++++++++++++++++++--------------------
 2 files changed, 501 insertions(+), 560 deletions(-)

-- 
2.30.2



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/6] gzip: drop unused define checks
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
@ 2024-04-17 14:37 ` Daniel P. Smith
  2024-04-17 16:27   ` Andrew Cooper
  2024-04-18  7:47   ` Jan Beulich
  2024-04-17 14:37 ` [PATCH v2 2/6] gzip: clean up comments and fix code alignment Daniel P. Smith
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

Dropping the define checks for PKZIP_BUG_WORKAROUND and NOMEMCPY define as they
never are set.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/gzip/inflate.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 58f263d9e852..e95db2de4d9b 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -661,7 +661,6 @@ static int __init inflate_codes(
                 /* do the copy */
                 do {
                     n -= (e = (e = WSIZE - ((d &= WSIZE-1) > w ? d : w)) > n ? n : e);
-#if !defined(NOMEMCPY) && !defined(DEBUG)
                     if (w - d >= e)         /* (this test assumes unsigned comparison) */
                     {
                         memcpy(slide + w, slide + d, e);
@@ -669,7 +668,6 @@ static int __init inflate_codes(
                         d += e;
                     }
                     else                      /* do it slow to avoid memcpy() overlap */
-#endif /* !NOMEMCPY */
                         do {
                             slide[w++] = slide[d++];
                             Tracevv((stderr, "%c", slide[w-1]));
@@ -845,11 +843,7 @@ static int noinline __init inflate_dynamic(void)
 
     DEBG("<dyn");
 
-#ifdef PKZIP_BUG_WORKAROUND
-    ll = malloc(sizeof(*ll) * (288+32));  /* literal/length and distance code lengths */
-#else
     ll = malloc(sizeof(*ll) * (286+30));  /* literal/length and distance code lengths */
-#endif
 
     if (ll == NULL)
         return 1;
@@ -869,11 +863,7 @@ static int noinline __init inflate_dynamic(void)
         NEEDBITS(4)
         nb = 4 + ((unsigned)b & 0xf);         /* number of bit length codes */
     DUMPBITS(4)
-#ifdef PKZIP_BUG_WORKAROUND
-        if (nl > 288 || nd > 32)
-#else
             if (nl > 286 || nd > 30)
-#endif
             {
                 ret = 1;             /* bad lengths */
                 goto out;
@@ -989,16 +979,11 @@ static int noinline __init inflate_dynamic(void)
         DEBG("dyn5d ");
         if (i == 1) {
             error("incomplete distance tree");
-#ifdef PKZIP_BUG_WORKAROUND
-            i = 0;
-        }
-#else
         huft_free(td);
     }
     huft_free(tl);
     ret = i;                   /* incomplete code set */
     goto out;
-#endif
 }
 
 DEBG("dyn6 ");
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/6] gzip: clean up comments and fix code alignment
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
  2024-04-17 14:37 ` [PATCH v2 1/6] gzip: drop unused define checks Daniel P. Smith
@ 2024-04-17 14:37 ` Daniel P. Smith
  2024-04-17 14:37 ` [PATCH v2 3/6] gzip: remove custom memory allocator Daniel P. Smith
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

This commit cleans up the comments and fixes the code alignment using Xen
coding style. This is done to make the code more legible before refactoring.

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

diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 2c6eae167d54..1bcb007395ba 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -63,7 +63,6 @@ static __init int fill_inbuf(void)
         return 0;
 }
 
-
 #include "inflate.c"
 
 static __init void flush_window(void)
@@ -137,3 +136,12 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
 
     return rc;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index e95db2de4d9b..73ccfc2bdc6c 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -1,11 +1,13 @@
 #define DEBG(x)
 #define DEBG1(x)
-/* inflate.c -- Not copyrighted 1992 by Mark Adler
-   version c10p1, 10 January 1993 */
+/*
+ * inflate.c -- Not copyrighted 1992 by Mark Adler
+ * version c10p1, 10 January 1993
+ */
 
-/* 
+/*
  * Adapted for booting Linux by Hannu Savolainen 1993
- * based on gzip-1.0.3 
+ * based on gzip-1.0.3
  *
  * Nicolas Pitre <nico@cam.org>, 1999/04/14 :
  *   Little mods for all variable to reside either into rodata or bss segments
@@ -15,92 +17,91 @@
  */
 
 /*
-   Inflate deflated (PKZIP's method 8 compressed) data.  The compression
-   method searches for as much of the current string of bytes (up to a
-   length of 258) in the previous 32 K bytes.  If it doesn't find any
-   matches (of at least length 3), it codes the next byte.  Otherwise, it
-   codes the length of the matched string and its distance backwards from
-   the current position.  There is a single Huffman code that codes both
-   single bytes (called "literals") and match lengths.  A second Huffman
-   code codes the distance information, which follows a length code.  Each
-   length or distance code actually represents a base value and a number
-   of "extra" (sometimes zero) bits to get to add to the base value.  At
-   the end of each deflated block is a special end-of-block (EOB) literal/
-   length code.  The decoding process is basically: get a literal/length
-   code; if EOB then done; if a literal, emit the decoded byte; if a
-   length then get the distance and emit the referred-to bytes from the
-   sliding window of previously emitted data.
-
-   There are (currently) three kinds of inflate blocks: stored, fixed, and
-   dynamic.  The compressor deals with some chunk of data at a time, and
-   decides which method to use on a chunk-by-chunk basis.  A chunk might
-   typically be 32 K or 64 K.  If the chunk is incompressible, then the
-   "stored" method is used.  In this case, the bytes are simply stored as
-   is, eight bits per byte, with none of the above coding.  The bytes are
-   preceded by a count, since there is no longer an EOB code.
-
-   If the data is compressible, then either the fixed or dynamic methods
-   are used.  In the dynamic method, the compressed data is preceded by
-   an encoding of the literal/length and distance Huffman codes that are
-   to be used to decode this block.  The representation is itself Huffman
-   coded, and so is preceded by a description of that code.  These code
-   descriptions take up a little space, and so for small blocks, there is
-   a predefined set of codes, called the fixed codes.  The fixed method is
-   used if the block codes up smaller that way (usually for quite small
-   chunks), otherwise the dynamic method is used.  In the latter case, the
-   codes are customized to the probabilities in the current block, and so
-   can code it much better than the pre-determined fixed codes.
- 
-   The Huffman codes themselves are decoded using a multi-level table
-   lookup, in order to maximize the speed of decoding plus the speed of
-   building the decoding tables.  See the comments below that precede the
-   lbits and dbits tuning parameters.
+ * Inflate deflated (PKZIP's method 8 compressed) data.  The compression
+ * method searches for as much of the current string of bytes (up to a
+ * length of 258) in the previous 32 K bytes.  If it doesn't find any
+ * matches (of at least length 3), it codes the next byte.  Otherwise, it
+ * codes the length of the matched string and its distance backwards from
+ * the current position.  There is a single Huffman code that codes both
+ * single bytes (called "literals") and match lengths.  A second Huffman
+ * code codes the distance information, which follows a length code.  Each
+ * length or distance code actually represents a base value and a number
+ * of "extra" (sometimes zero) bits to get to add to the base value.  At
+ * the end of each deflated block is a special end-of-block (EOB) literal/
+ * length code.  The decoding process is basically: get a literal/length
+ * code; if EOB then done; if a literal, emit the decoded byte; if a
+ * length then get the distance and emit the referred-to bytes from the
+ * sliding window of previously emitted data.
+ *
+ * There are (currently) three kinds of inflate blocks: stored, fixed, and
+ * dynamic.  The compressor deals with some chunk of data at a time, and
+ * decides which method to use on a chunk-by-chunk basis.  A chunk might
+ * typically be 32 K or 64 K.  If the chunk is incompressible, then the
+ * "stored" method is used.  In this case, the bytes are simply stored as
+ * is, eight bits per byte, with none of the above coding.  The bytes are
+ * preceded by a count, since there is no longer an EOB code.
+ *
+ * If the data is compressible, then either the fixed or dynamic methods
+ * are used.  In the dynamic method, the compressed data is preceded by
+ * an encoding of the literal/length and distance Huffman codes that are
+ * to be used to decode this block.  The representation is itself Huffman
+ * coded, and so is preceded by a description of that code.  These code
+ * descriptions take up a little space, and so for small blocks, there is
+ * a predefined set of codes, called the fixed codes.  The fixed method is
+ * used if the block codes up smaller that way (usually for quite small
+ * chunks), otherwise the dynamic method is used.  In the latter case, the
+ * codes are customized to the probabilities in the current block, and so
+ * can code it much better than the pre-determined fixed codes.
+ *
+ * The Huffman codes themselves are decoded using a multi-level table
+ * lookup, in order to maximize the speed of decoding plus the speed of
+ * building the decoding tables.  See the comments below that precede the
+ * lbits and dbits tuning parameters.
  */
 
-
 /*
-   Notes beyond the 1.93a appnote.txt:
-
-   1. Distance pointers never point before the beginning of the output
-      stream.
-   2. Distance pointers can point back across blocks, up to 32k away.
-   3. There is an implied maximum of 7 bits for the bit length table and
-      15 bits for the actual data.
-   4. If only one code exists, then it is encoded using one bit.  (Zero
-      would be more efficient, but perhaps a little confusing.)  If two
-      codes exist, they are coded using one bit each (0 and 1).
-   5. There is no way of sending zero distance codes--a dummy must be
-      sent if there are none.  (History: a pre 2.0 version of PKZIP would
-      store blocks with no distance codes, but this was discovered to be
-      too harsh a criterion.)  Valid only for 1.93a.  2.04c does allow
-      zero distance codes, which is sent as one code of zero bits in
-      length.
-   6. There are up to 286 literal/length codes.  Code 256 represents the
-      end-of-block.  Note however that the static length tree defines
-      288 codes just to fill out the Huffman codes.  Codes 286 and 287
-      cannot be used though, since there is no length base or extra bits
-      defined for them.  Similarly, there are up to 30 distance codes.
-      However, static trees define 32 codes (all 5 bits) to fill out the
-      Huffman codes, but the last two had better not show up in the data.
-   7. Unzip can check dynamic Huffman blocks for complete code sets.
-      The exception is that a single code would not be complete (see #4).
-   8. The five bits following the block type is really the number of
-      literal codes sent minus 257.
-   9. Length codes 8,16,16 are interpreted as 13 length codes of 8 bits
-      (1+6+6).  Therefore, to output three times the length, you output
-      three codes (1+1+1), whereas to output four times the same length,
-      you only need two codes (1+3).  Hmm.
-  10. In the tree reconstruction algorithm, Code = Code + Increment
-      only if BitLength(i) is not zero.  (Pretty obvious.)
-  11. Correction: 4 Bits: # of Bit Length codes - 4     (4 - 19)
-  12. Note: length code 284 can represent 227-258, but length code 285
-      really is 258.  The last length deserves its own, short code
-      since it gets used a lot in very redundant files.  The length
-      258 is special since 258 - 3 (the min match length) is 255.
-  13. The literal/length and distance code bit lengths are read as a
-      single stream of lengths.  It is possible (and advantageous) for
-      a repeat code (16, 17, or 18) to go across the boundary between
-      the two sets of lengths.
+ * Notes beyond the 1.93a appnote.txt:
+ *
+ *  1. Distance pointers never point before the beginning of the output
+ *     stream.
+ *  2. Distance pointers can point back across blocks, up to 32k away.
+ *  3. There is an implied maximum of 7 bits for the bit length table and
+ *     15 bits for the actual data.
+ *  4. If only one code exists, then it is encoded using one bit.  (Zero
+ *     would be more efficient, but perhaps a little confusing.)  If two
+ *     codes exist, they are coded using one bit each (0 and 1).
+ *  5. There is no way of sending zero distance codes--a dummy must be
+ *     sent if there are none.  (History: a pre 2.0 version of PKZIP would
+ *     store blocks with no distance codes, but this was discovered to be
+ *     too harsh a criterion.)  Valid only for 1.93a.  2.04c does allow
+ *     zero distance codes, which is sent as one code of zero bits in
+ *     length.
+ *  6. There are up to 286 literal/length codes.  Code 256 represents the
+ *     end-of-block.  Note however that the static length tree defines
+ *     288 codes just to fill out the Huffman codes.  Codes 286 and 287
+ *     cannot be used though, since there is no length base or extra bits
+ *     defined for them.  Similarly, there are up to 30 distance codes.
+ *     However, static trees define 32 codes (all 5 bits) to fill out the
+ *     Huffman codes, but the last two had better not show up in the data.
+ *  7. Unzip can check dynamic Huffman blocks for complete code sets.
+ *     The exception is that a single code would not be complete (see #4).
+ *  8. The five bits following the block type is really the number of
+ *     literal codes sent minus 257.
+ *  9. Length codes 8,16,16 are interpreted as 13 length codes of 8 bits
+ *     (1+6+6).  Therefore, to output three times the length, you output
+ *     three codes (1+1+1), whereas to output four times the same length,
+ *     you only need two codes (1+3).  Hmm.
+ * 10. In the tree reconstruction algorithm, Code = Code + Increment
+ *     only if BitLength(i) is not zero.  (Pretty obvious.)
+ * 11. Correction: 4 Bits: # of Bit Length codes - 4     (4 - 19)
+ * 12. Note: length code 284 can represent 227-258, but length code 285
+ *     really is 258.  The last length deserves its own, short code
+ *     since it gets used a lot in very redundant files.  The length
+ *     258 is special since 258 - 3 (the min match length) is 255.
+ * 13. The literal/length and distance code bit lengths are read as a
+ *     single stream of lengths.  It is possible (and advantageous) for
+ *     a repeat code (16, 17, or 18) to go across the boundary between
+ *     the two sets of lengths.
  */
 
 #ifdef RCSID
@@ -120,13 +121,15 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
 
 #define slide window
 
-/* Huffman code lookup table entry--this entry is four bytes for machines
-   that have 16-bit pointers (e.g. PC's in the small or medium model).
-   Valid extra bits are 0..13.  e == 15 is EOB (end of block), e == 16
-   means that v is a literal, 16 < e < 32 means that v is a pointer to
-   the next table, which codes e - 16 bits, and lastly e == 99 indicates
-   an unused code.  If a code with e == 99 is looked up, this implies an
-   error in the data. */
+/*
+ * Huffman code lookup table entry--this entry is four bytes for machines
+ * that have 16-bit pointers (e.g. PC's in the small or medium model).
+ * Valid extra bits are 0..13.  e == 15 is EOB (end of block), e == 16
+ * means that v is a literal, 16 < e < 32 means that v is a pointer to
+ * the next table, which codes e - 16 bits, and lastly e == 99 indicates
+ * an unused code.  If a code with e == 99 is looked up, this implies an
+ * error in the data.
+ */
 struct huft {
     uch e;                /* number of extra bits or operation */
     uch b;                /* number of bits in this code or subcode */
@@ -136,7 +139,6 @@ struct huft {
     } v;
 };
 
-
 /* Function prototypes */
 static int huft_build OF((unsigned *, unsigned, unsigned,
                           const ush *, const ush *, struct huft **, int *));
@@ -148,15 +150,17 @@ static int inflate_dynamic OF((void));
 static int inflate_block OF((int *));
 static int inflate OF((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. */
+/*
+ * 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())
@@ -180,36 +184,35 @@ static const ush cpdext[] = {         /* Extra bits for distance codes */
     7, 7, 8, 8, 9, 9, 10, 10, 11, 11,
     12, 12, 13, 13};
 
-
-
-/* Macros for inflate() bit peeking and grabbing.
-   The usage is:
-   
-        NEEDBITS(j)
-        x = b & mask_bits[j];
-        DUMPBITS(j)
-
-   where NEEDBITS makes sure that b has at least j bits in it, and
-   DUMPBITS removes the bits from b.  The macros use the variable k
-   for the number of bits in b.  Normally, b and k are register
-   variables for speed, and are initialized at the beginning of a
-   routine that uses these macros from a global bit buffer and count.
-
-   If we assume that EOB will be the longest code, then we will never
-   ask for bits with NEEDBITS that are beyond the end of the stream.
-   So, NEEDBITS should not read any more bytes than are needed to
-   meet the request.  Then no bytes need to be "returned" to the buffer
-   at the end of the last block.
-
-   However, this assumption is not true for fixed blocks--the EOB code
-   is 7 bits, but the other literal/length codes can be 8 or 9 bits.
-   (The EOB code is shorter than other codes because fixed blocks are
-   generally short.  So, while a block always has an EOB, many other
-   literal/length codes have a significantly lower probability of
-   showing up at all.)  However, by making the first table have a
-   lookup of seven bits, the EOB code will be found in that first
-   lookup, and so will not require that too many bits be pulled from
-   the stream.
+/*
+ * Macros for inflate() bit peeking and grabbing.
+ * The usage is:
+ *
+ *      NEEDBITS(j)
+ *      x = b & mask_bits[j];
+ *      DUMPBITS(j)
+ *
+ * where NEEDBITS makes sure that b has at least j bits in it, and
+ * DUMPBITS removes the bits from b.  The macros use the variable k
+ * for the number of bits in b.  Normally, b and k are register
+ * variables for speed, and are initialized at the beginning of a
+ * routine that uses these macros from a global bit buffer and count.
+ *
+ * If we assume that EOB will be the longest code, then we will never
+ * ask for bits with NEEDBITS that are beyond the end of the stream.
+ * So, NEEDBITS should not read any more bytes than are needed to
+ * meet the request.  Then no bytes need to be "returned" to the buffer
+ * at the end of the last block.
+ *
+ * However, this assumption is not true for fixed blocks--the EOB code
+ * is 7 bits, but the other literal/length codes can be 8 or 9 bits.
+ * (The EOB code is shorter than other codes because fixed blocks are
+ * generally short.  So, while a block always has an EOB, many other
+ * literal/length codes have a significantly lower probability of
+ * showing up at all.)  However, by making the first table have a
+ * lookup of seven bits, the EOB code will be found in that first
+ * lookup, and so will not require that too many bits be pulled from
+ * the stream.
  */
 
 static ulg __initdata bb;                /* bit buffer */
@@ -226,7 +229,8 @@ static const ush mask_bits[] = {
 #define DUMPBITS(n) {b>>=(n);k-=(n);}
 
 #ifndef NO_INFLATE_MALLOC
-/* A trivial malloc implementation, adapted from
+/*
+ * A trivial malloc implementation, adapted from
  *  malloc by Hannu Savolainen 1993 and Matthias Urlichs 1994
  */
 
@@ -272,64 +276,64 @@ static void __init free(void *where)
 #endif
 
 /*
-   Huffman code decoding is performed using a multi-level table lookup.
-   The fastest way to decode is to simply build a lookup table whose
-   size is determined by the longest code.  However, the time it takes
-   to build this table can also be a factor if the data being decoded
-   is not very long.  The most common codes are necessarily the
-   shortest codes, so those codes dominate the decoding time, and hence
-   the speed.  The idea is you can have a shorter table that decodes the
-   shorter, more probable codes, and then point to subsidiary tables for
-   the longer codes.  The time it costs to decode the longer codes is
-   then traded against the time it takes to make longer tables.
-
-   This results of this trade are in the variables lbits and dbits
-   below.  lbits is the number of bits the first level table for literal/
-   length codes can decode in one step, and dbits is the same thing for
-   the distance codes.  Subsequent tables are also less than or equal to
-   those sizes.  These values may be adjusted either when all of the
-   codes are shorter than that, in which case the longest code length in
-   bits is used, or when the shortest code is *longer* than the requested
-   table size, in which case the length of the shortest code in bits is
-   used.
-
-   There are two different values for the two tables, since they code a
-   different number of possibilities each.  The literal/length table
-   codes 286 possible values, or in a flat code, a little over eight
-   bits.  The distance table codes 30 possible values, or a little less
-   than five bits, flat.  The optimum values for speed end up being
-   about one bit more than those, so lbits is 8+1 and dbits is 5+1.
-   The optimum values may differ though from machine to machine, and
-   possibly even between compilers.  Your mileage may vary.
+ * Huffman code decoding is performed using a multi-level table lookup.
+ * The fastest way to decode is to simply build a lookup table whose
+ * size is determined by the longest code.  However, the time it takes
+ * to build this table can also be a factor if the data being decoded
+ * is not very long.  The most common codes are necessarily the
+ * shortest codes, so those codes dominate the decoding time, and hence
+ * the speed.  The idea is you can have a shorter table that decodes the
+ * shorter, more probable codes, and then point to subsidiary tables for
+ * the longer codes.  The time it costs to decode the longer codes is
+ * then traded against the time it takes to make longer tables.
+ *
+ * This results of this trade are in the variables lbits and dbits
+ * below.  lbits is the number of bits the first level table for literal/
+ * length codes can decode in one step, and dbits is the same thing for
+ * the distance codes.  Subsequent tables are also less than or equal to
+ * those sizes.  These values may be adjusted either when all of the
+ * codes are shorter than that, in which case the longest code length in
+ * bits is used, or when the shortest code is *longer* than the requested
+ * table size, in which case the length of the shortest code in bits is
+ * used.
+ *
+ * There are two different values for the two tables, since they code a
+ * different number of possibilities each.  The literal/length table
+ * codes 286 possible values, or in a flat code, a little over eight
+ * bits.  The distance table codes 30 possible values, or a little less
+ * than five bits, flat.  The optimum values for speed end up being
+ * about one bit more than those, so lbits is 8+1 and dbits is 5+1.
+ * The optimum values may differ though from machine to machine, and
+ * possibly even between compilers.  Your mileage may vary.
  */
 
-
 static const int lbits = 9;          /* bits in base literal/length lookup table */
 static const int dbits = 6;          /* bits in base distance lookup table */
 
-
 /* If BMAX needs to be larger than 16, then h and x[] should be ulg. */
 #define BMAX 16         /* maximum bit length of any code (16 for explode) */
 #define N_MAX 288       /* maximum number of codes in any set */
 
-
 static unsigned __initdata hufts;      /* track memory usage */
 
-
+/*
+ * Given a list of code lengths and a maximum table size, make a set of
+ * tables to decode that set of codes.  Return zero on success, one if
+ * the given code set is incomplete (the tables are still built in this
+ * case), two if the input is invalid (all zero length codes or an
+ * oversubscribed set of lengths), and three if not enough memory.
+ *
+ * @param b Code lengths in bits (all assumed <= BMAX)
+ * @param n Number of codes (assumed <= N_MAX)
+ * @param s Number of simple-valued codes (0..s-1)
+ * @param d List of base values for non-simple codes
+ * @param e List of extra bits for non-simple codes
+ * @param t Result: starting table
+ * @param m Maximum lookup bits, returns actual
+ */
 static int __init huft_build(
-    unsigned *b,            /* code lengths in bits (all assumed <= BMAX) */
-    unsigned n,             /* number of codes (assumed <= N_MAX) */
-    unsigned s,             /* number of simple-valued codes (0..s-1) */
-    const ush *d,           /* list of base values for non-simple codes */
-    const ush *e,           /* list of extra bits for non-simple codes */
-    struct huft **t,        /* result: starting table */
-    int *m                  /* maximum lookup bits, returns actual */
-    )
-/* Given a list of code lengths and a maximum table size, make a set of
-   tables to decode that set of codes.  Return zero on success, one if
-   the given code set is incomplete (the tables are still built in this
-   case), two if the input is invalid (all zero length codes or an
-   oversubscribed set of lengths), and three if not enough memory. */
+    unsigned *b, unsigned n, unsigned s, const ush *d, const ush *e,
+    struct huft **t, int *m)
 {
     unsigned a;                   /* counter for codes of length k */
     unsigned f;                   /* i repeats in table every f entries */
@@ -371,7 +375,7 @@ static int __init huft_build(
     memzero(stk->c, sizeof(stk->c));
     p = b;  i = n;
     do {
-        Tracecv(*p, (stderr, (n-i >= ' ' && n-i <= '~' ? "%c %d\n" : "0x%x %d\n"), 
+        Tracecv(*p, (stderr, (n-i >= ' ' && n-i <= '~' ? "%c %d\n" : "0x%x %d\n"),
                      n-i, *p));
         c[*p]++;                    /* assume all entries <= BMAX */
         p++;                      /* Can't combine with above line (Solaris bug) */
@@ -559,14 +563,13 @@ static int __init huft_build(
     return ret;
 }
 
-
-
-static int __init huft_free(
-    struct huft *t         /* table to free */
-    )
 /* Free the malloc'ed tables built by huft_build(), which makes a linked
-   list of the tables it made, with the links in a dummy first entry of
-   each table. */
+ * list of the tables it made, with the links in a dummy first entry of
+ * each table.
+ *
+ * @param t Table to free
+ */
+static int __init huft_free(struct huft *t)
 {
     register struct huft *p, *q;
 
@@ -578,19 +581,21 @@ static int __init huft_free(
         q = (--p)->v.t;
         free((char*)p);
         p = q;
-    } 
+    }
     return 0;
 }
 
-
+/*
+ * inflate (decompress) the codes in a deflated (compressed) block.
+ * Return an error code or zero if it all goes ok.
+ *
+ * @param huft tl Literal/length decoder tables
+ * @param huft td Distance decoder tables
+ * @param bl  Number of bits decoded by tl[]
+ * @param bd  Number of bits decoded by td[]
+ */
 static int __init inflate_codes(
-    struct huft *tl,    /* literal/length decoder tables */
-    struct huft *td,    /* distance decoder tables */
-    int bl,             /* number of bits decoded by tl[] */
-    int bd              /* number of bits decoded by td[] */
-    )
-/* inflate (decompress) the codes in a deflated (compressed) block.
-   Return an error code or zero if it all goes ok. */
+    struct huft *tl, struct huft *td, int bl, int bd)
 {
     register unsigned e;  /* table entry flag/number of extra bits */
     unsigned n, d;        /* length and index for copy */
@@ -611,77 +616,76 @@ static int __init inflate_codes(
     md = mask_bits[bd];
     for (;;)                      /* do until end of block */
     {
-        NEEDBITS((unsigned)bl)
-            if ((e = (t = tl + ((unsigned)b & ml))->e) > 16)
+        NEEDBITS((unsigned)bl);
+        if ((e = (t = tl + ((unsigned)b & ml))->e) > 16)
+            do {
+            if (e == 99)
+                return 1;
+            DUMPBITS(t->b);
+            e -= 16;
+            NEEDBITS(e);
+            } while ((e = (t = t->v.t + ((unsigned)b & mask_bits[e]))->e) > 16);
+        DUMPBITS(t->b);
+        if (e == 16)                /* then it's a literal */
+        {
+            slide[w++] = (uch)t->v.n;
+            Tracevv((stderr, "%c", slide[w-1]));
+            if (w == WSIZE)
+            {
+                flush_output(w);
+                w = 0;
+            }
+        }
+        else                        /* it's an EOB or a length */
+        {
+            /* exit if end of block */
+            if (e == 15)
+                break;
+
+            /* get length of block to copy */
+            NEEDBITS(e);
+            n = t->v.n + ((unsigned)b & mask_bits[e]);
+            DUMPBITS(e);
+
+            /* decode distance of block to copy */
+            NEEDBITS((unsigned)bd);
+            if ((e = (t = td + ((unsigned)b & md))->e) > 16)
                 do {
                     if (e == 99)
                         return 1;
-                    DUMPBITS(t->b)
-                        e -= 16;
-                    NEEDBITS(e)
-                        } while ((e = (t = t->v.t + ((unsigned)b & mask_bits[e]))->e) > 16);
-        DUMPBITS(t->b)
-            if (e == 16)                /* then it's a literal */
-            {
-                slide[w++] = (uch)t->v.n;
-                Tracevv((stderr, "%c", slide[w-1]));
+                    DUMPBITS(t->b);
+                    e -= 16;
+                    NEEDBITS(e);
+                } while ((e = (t = t->v.t + ((unsigned)b & mask_bits[e]))->e) > 16);
+            DUMPBITS(t->b);
+            NEEDBITS(e);
+            d = w - t->v.n - ((unsigned)b & mask_bits[e]);
+            DUMPBITS(e);
+            Tracevv((stderr,"\\[%d,%d]", w-d, n));
+
+            /* do the copy */
+            do {
+                n -= (e = (e = WSIZE - ((d &= WSIZE-1) > w ? d : w)) > n ? n : e);
+                if (w - d >= e)         /* (this test assumes unsigned comparison) */
+                {
+                    memcpy(slide + w, slide + d, e);
+                    w += e;
+                    d += e;
+                }
+                else                      /* do it slow to avoid memcpy() overlap */
+                    do {
+                        slide[w++] = slide[d++];
+                        Tracevv((stderr, "%c", slide[w-1]));
+                    } while (--e);
                 if (w == WSIZE)
                 {
                     flush_output(w);
                     w = 0;
                 }
-            }
-            else                        /* it's an EOB or a length */
-            {
-                /* exit if end of block */
-                if (e == 15)
-                    break;
-
-                /* get length of block to copy */
-                NEEDBITS(e)
-                    n = t->v.n + ((unsigned)b & mask_bits[e]);
-                DUMPBITS(e);
-
-                /* decode distance of block to copy */
-                NEEDBITS((unsigned)bd)
-                    if ((e = (t = td + ((unsigned)b & md))->e) > 16)
-                        do {
-                            if (e == 99)
-                                return 1;
-                            DUMPBITS(t->b)
-                                e -= 16;
-                            NEEDBITS(e)
-                                } while ((e = (t = t->v.t + ((unsigned)b & mask_bits[e]))->e) > 16);
-                DUMPBITS(t->b)
-                    NEEDBITS(e)
-                    d = w - t->v.n - ((unsigned)b & mask_bits[e]);
-                DUMPBITS(e)
-                    Tracevv((stderr,"\\[%d,%d]", w-d, n));
-
-                /* do the copy */
-                do {
-                    n -= (e = (e = WSIZE - ((d &= WSIZE-1) > w ? d : w)) > n ? n : e);
-                    if (w - d >= e)         /* (this test assumes unsigned comparison) */
-                    {
-                        memcpy(slide + w, slide + d, e);
-                        w += e;
-                        d += e;
-                    }
-                    else                      /* do it slow to avoid memcpy() overlap */
-                        do {
-                            slide[w++] = slide[d++];
-                            Tracevv((stderr, "%c", slide[w-1]));
-                        } while (--e);
-                    if (w == WSIZE)
-                    {
-                        flush_output(w);
-                        w = 0;
-                    }
-                } while (n);
-            }
+            } while (n);
+        }
     }
 
-
     /* restore the globals from the locals */
     wp = w;                       /* restore global window pointer */
     bb = b;                       /* restore global bit buffer */
@@ -694,10 +698,8 @@ static int __init inflate_codes(
     return 4;   /* Input underrun */
 }
 
-
-
-static int __init inflate_stored(void)
 /* "decompress" an inflated type 0 (stored) block. */
+static int __init inflate_stored(void)
 {
     unsigned n;           /* number of bytes in block */
     unsigned w;           /* current window position */
@@ -718,28 +720,26 @@ static int __init inflate_stored(void)
 
 
     /* get the length and its complement */
-    NEEDBITS(16)
-        n = ((unsigned)b & 0xffff);
-    DUMPBITS(16)
-        NEEDBITS(16)
-        if (n != (unsigned)((~b) & 0xffff))
-            return 1;                   /* error in compressed data */
-    DUMPBITS(16)
-
-
-        /* read and output the compressed data */
-        while (n--)
+    NEEDBITS(16);
+    n = ((unsigned)b & 0xffff);
+    DUMPBITS(16);
+    NEEDBITS(16);
+    if (n != (unsigned)((~b) & 0xffff))
+        return 1;                   /* error in compressed data */
+    DUMPBITS(16);
+
+    /* read and output the compressed data */
+    while (n--)
+    {
+        NEEDBITS(8);
+        slide[w++] = (uch)b;
+        if (w == WSIZE)
         {
-            NEEDBITS(8)
-                slide[w++] = (uch)b;
-            if (w == WSIZE)
-            {
-                flush_output(w);
-                w = 0;
-            }
-            DUMPBITS(8)
-                }
-
+            flush_output(w);
+            w = 0;
+        }
+        DUMPBITS(8);
+    }
 
     /* restore the globals from the locals */
     wp = w;                       /* restore global window pointer */
@@ -757,10 +757,13 @@ static int __init inflate_stored(void)
 /*
  * We use `noinline' here to prevent gcc-3.5 from using too much stack space
  */
+
+/*
+ * decompress an inflated type 1 (fixed Huffman codes) block.  We should
+ * either replace this with a custom decoder, or at least precompute the
+ * Huffman tables.
+ */
 static int noinline __init inflate_fixed(void)
-/* decompress an inflated type 1 (fixed Huffman codes) block.  We should
-   either replace this with a custom decoder, or at least precompute the
-   Huffman tables. */
 {
     int i;                /* temporary variable */
     struct huft *tl;      /* literal/length code table */
@@ -803,7 +806,6 @@ static int noinline __init inflate_fixed(void)
         return i;
     }
 
-
     /* decompress until an end-of-block code */
     if (inflate_codes(tl, td, bl, bd)) {
         free(l);
@@ -817,12 +819,12 @@ static int noinline __init inflate_fixed(void)
     return 0;
 }
 
-
 /*
  * We use `noinline' here to prevent gcc-3.5 from using too much stack space
  */
-static int noinline __init inflate_dynamic(void)
+
 /* decompress an inflated type 2 (dynamic Huffman codes) block. */
+static int noinline __init inflate_dynamic(void)
 {
     int i;                /* temporary variables */
     unsigned j;
@@ -852,32 +854,31 @@ static int noinline __init inflate_dynamic(void)
     b = bb;
     k = bk;
 
-
     /* read in table lengths */
-    NEEDBITS(5)
-        nl = 257 + ((unsigned)b & 0x1f);      /* number of literal/length codes */
-    DUMPBITS(5)
-        NEEDBITS(5)
-        nd = 1 + ((unsigned)b & 0x1f);        /* number of distance codes */
-    DUMPBITS(5)
-        NEEDBITS(4)
-        nb = 4 + ((unsigned)b & 0xf);         /* number of bit length codes */
-    DUMPBITS(4)
-            if (nl > 286 || nd > 30)
-            {
-                ret = 1;             /* bad lengths */
-                goto out;
-            }
+    NEEDBITS(5);
+    nl = 257 + ((unsigned)b & 0x1f);      /* number of literal/length codes */
+    DUMPBITS(5);
+    NEEDBITS(5);
+    nd = 1 + ((unsigned)b & 0x1f);        /* number of distance codes */
+    DUMPBITS(5);
+    NEEDBITS(4);
+    nb = 4 + ((unsigned)b & 0xf);         /* number of bit length codes */
+    DUMPBITS(4);
+    if (nl > 286 || nd > 30)
+    {
+        ret = 1;             /* bad lengths */
+        goto out;
+    }
 
     DEBG("dyn1 ");
 
     /* read in bit-length-code lengths */
     for (j = 0; j < nb; j++)
     {
-        NEEDBITS(3)
-            ll[border[j]] = (unsigned)b & 7;
-        DUMPBITS(3)
-            }
+        NEEDBITS(3);
+        ll[border[j]] = (unsigned)b & 7;
+        DUMPBITS(3);
+    }
     for (; j < 19; j++)
         ll[border[j]] = 0;
 
@@ -901,46 +902,46 @@ static int noinline __init inflate_dynamic(void)
     i = l = 0;
     while ((unsigned)i < n)
     {
-        NEEDBITS((unsigned)bl)
-            j = (td = tl + ((unsigned)b & m))->b;
-        DUMPBITS(j)
-            j = td->v.n;
+        NEEDBITS((unsigned)bl);
+        j = (td = tl + ((unsigned)b & m))->b;
+        DUMPBITS(j);
+        j = td->v.n;
         if (j < 16)                 /* length of code in bits (0..15) */
             ll[i++] = l = j;          /* save last length in l */
         else if (j == 16)           /* repeat last length 3 to 6 times */
         {
-            NEEDBITS(2)
-                j = 3 + ((unsigned)b & 3);
-            DUMPBITS(2)
-                if ((unsigned)i + j > n) {
-                    ret = 1;
-                    goto out;
-                }
+            NEEDBITS(2);
+            j = 3 + ((unsigned)b & 3);
+            DUMPBITS(2);
+            if ((unsigned)i + j > n) {
+                ret = 1;
+                goto out;
+            }
             while (j--)
-                ll[i++] = l;
+            ll[i++] = l;
         }
         else if (j == 17)           /* 3 to 10 zero length codes */
         {
-            NEEDBITS(3)
-                j = 3 + ((unsigned)b & 7);
-            DUMPBITS(3)
-                if ((unsigned)i + j > n) {
-                    ret = 1;
-                    goto out;
-                }
+            NEEDBITS(3);
+            j = 3 + ((unsigned)b & 7);
+            DUMPBITS(3);
+            if ((unsigned)i + j > n) {
+                ret = 1;
+                goto out;
+            }
             while (j--)
                 ll[i++] = 0;
             l = 0;
         }
         else                        /* j == 18: 11 to 138 zero length codes */
         {
-            NEEDBITS(7)
-                j = 11 + ((unsigned)b & 0x7f);
-            DUMPBITS(7)
-                if ((unsigned)i + j > n) {
-                    ret = 1;
-                    goto out;
-                }
+            NEEDBITS(7);
+            j = 11 + ((unsigned)b & 0x7f);
+            DUMPBITS(7);
+            if ((unsigned)i + j > n) {
+                ret = 1;
+                goto out;
+            }
             while (j--)
                 ll[i++] = 0;
             l = 0;
@@ -979,67 +980,64 @@ static int noinline __init inflate_dynamic(void)
         DEBG("dyn5d ");
         if (i == 1) {
             error("incomplete distance tree");
-        huft_free(td);
+            huft_free(td);
+        }
+        huft_free(tl);
+        ret = i;                   /* incomplete code set */
+        goto out;
     }
-    huft_free(tl);
-    ret = i;                   /* incomplete code set */
-    goto out;
-}
 
-DEBG("dyn6 ");
+    DEBG("dyn6 ");
 
-  /* decompress until an end-of-block code */
-if (inflate_codes(tl, td, bl, bd)) {
-    ret = 1;
-    goto out;
-}
+      /* decompress until an end-of-block code */
+    if (inflate_codes(tl, td, bl, bd)) {
+        ret = 1;
+        goto out;
+    }
 
-DEBG("dyn7 ");
+    DEBG("dyn7 ");
 
-  /* free the decoding tables, return */
-huft_free(tl);
-huft_free(td);
+      /* free the decoding tables, return */
+    huft_free(tl);
+    huft_free(td);
 
-DEBG(">");
-ret = 0;
-out:
-free(ll);
-return ret;
+    DEBG(">");
+    ret = 0;
+ out:
+    free(ll);
+    return ret;
 
-underrun:
-ret = 4;   /* Input underrun */
-goto out;
+ underrun:
+    ret = 4;   /* Input underrun */
+    goto out;
 }
 
-
-
-static int __init inflate_block(
-int *e                  /* last block flag */
-)
-/* decompress an inflated block */
+/*
+ * decompress an inflated block
+ *
+ * @param e Last block flag
+ */
+static int __init inflate_block(int *e)
 {
-unsigned t;           /* block type */
-register ulg b;       /* bit buffer */
-register unsigned k;  /* number of bits in bit buffer */
-
-DEBG("<blk");
+    unsigned t;           /* block type */
+    register ulg b;       /* bit buffer */
+    register unsigned k;  /* number of bits in bit buffer */
 
-/* make local bit buffer */
-b = bb;
-k = bk;
+    DEBG("<blk");
 
+    /* make local bit buffer */
+    b = bb;
+    k = bk;
 
-/* read in last block bit */
-NEEDBITS(1)
+    /* read in last block bit */
+    NEEDBITS(1);
     *e = (int)b & 1;
-    DUMPBITS(1)
-
+    DUMPBITS(1);
 
     /* read in block type */
-    NEEDBITS(2)
+    NEEDBITS(2);
     t = (unsigned)b & 3;
-    DUMPBITS(2)
-
+    DUMPBITS(2);
 
     /* restore the global bit buffer */
     bb = b;
@@ -1047,25 +1045,23 @@ NEEDBITS(1)
 
     /* inflate that block type */
     if (t == 2)
-    return inflate_dynamic();
+        return inflate_dynamic();
     if (t == 0)
-    return inflate_stored();
+        return inflate_stored();
     if (t == 1)
-    return inflate_fixed();
+        return inflate_fixed();
 
     DEBG(">");
 
     /* bad block type */
     return 2;
 
-    underrun:
+ underrun:
     return 4;   /* Input underrun */
 }
 
-
-
-static int __init inflate(void)
 /* decompress an inflated entry */
+static int __init inflate(void)
 {
     int e;                /* last block flag */
     int r;                /* result code */
@@ -1076,7 +1072,6 @@ static int __init inflate(void)
     bk = 0;
     bb = 0;
 
-
     /* decompress until the last block */
     h = 0;
     do {
@@ -1102,7 +1097,6 @@ static int __init inflate(void)
     /* flush out slide */
     flush_output(wp);
 
-
     /* return success */
 #ifdef DEBUG
     fprintf(stderr, "<%u> ", h);
@@ -1121,12 +1115,11 @@ static ulg __initdata crc;  /* initialized in makecrc() so it'll reside in bss *
 #define CRC_VALUE (crc ^ 0xffffffffUL)
 
 /*
- * Code to compute the CRC-32 table. Borrowed from 
+ * Code to compute the CRC-32 table. Borrowed from
  * gzip-1.0.3/makecrc.c.
  */
 
-static void __init
-makecrc(void)
+static void __init makecrc(void)
 {
 /* Not copyrighted 1990 Mark Adler */
 
@@ -1229,7 +1222,7 @@ static int __init gunzip(void)
     if ((flags & ORIG_NAME) != 0) {
         /* Discard the old name */
         while (NEXTBYTE() != 0) /* null */ ;
-    } 
+    }
 
     /* Discard file comment if any */
     if ((flags & COMMENT) != 0) {
@@ -1258,7 +1251,7 @@ static int __init gunzip(void)
         }
         return -1;
     }
-     
+
     /* Get the crc and original length */
     /* crc32  (see algorithm.doc)
      * uncompressed input size modulo 2^32
@@ -1267,12 +1260,12 @@ static int __init gunzip(void)
     orig_crc |= (ulg) NEXTBYTE() << 8;
     orig_crc |= (ulg) NEXTBYTE() << 16;
     orig_crc |= (ulg) NEXTBYTE() << 24;
-    
+
     orig_len = (ulg) NEXTBYTE();
     orig_len |= (ulg) NEXTBYTE() << 8;
     orig_len |= (ulg) NEXTBYTE() << 16;
     orig_len |= (ulg) NEXTBYTE() << 24;
-    
+
     /* Validate decompression */
     if (orig_crc != CRC_VALUE) {
         error("crc error");
@@ -1288,3 +1281,12 @@ static int __init gunzip(void)
     error("out of input data");
     return -1;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/6] gzip: remove custom memory allocator
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
  2024-04-17 14:37 ` [PATCH v2 1/6] gzip: drop unused define checks Daniel P. Smith
  2024-04-17 14:37 ` [PATCH v2 2/6] gzip: clean up comments and fix code alignment Daniel P. Smith
@ 2024-04-17 14:37 ` Daniel P. Smith
  2024-04-17 16:52   ` Andrew Cooper
  2024-04-17 14:37 ` [PATCH v2 4/6] gzip: refactor state tracking Daniel P. Smith
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

All the other decompression routines use xmalloc_bytes(), thus there is no
reason for gzip to be handling its own allocation of memory. In fact, there is
a bug somewhere in the allocator as decompression started to break when adding
additional allocations. Instead of troubleshooting the allocator, replace it
with xmalloc_bytes().

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

diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 1bcb007395ba..1b448d6e3655 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -4,12 +4,7 @@
 #include <xen/lib.h>
 #include <xen/mm.h>
 
-#define HEAPORDER 3
-
 static unsigned char *__initdata window;
-#define memptr long
-static memptr __initdata free_mem_ptr;
-static memptr __initdata free_mem_end_ptr;
 
 #define WSIZE           0x80000000U
 
@@ -24,6 +19,8 @@ static unsigned int __initdata outcnt;
 
 #define OF(args)        args
 
+#define malloc(a)       xmalloc_bytes(a)
+#define free(a)         xfree(a)
 #define memzero(s, n)   memset((s), 0, (n))
 
 typedef unsigned char   uch;
@@ -108,14 +105,6 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
         return 1;
 
     window = (unsigned char *)output;
-
-    free_mem_ptr = (unsigned long)alloc_xenheap_pages(HEAPORDER, 0);
-    if ( !free_mem_ptr )
-        return -ENOMEM;
-
-    free_mem_end_ptr = free_mem_ptr + (PAGE_SIZE << HEAPORDER);
-    init_allocator();
-
     inbuf = (unsigned char *)image;
     insize = image_len;
     inptr = 0;
@@ -132,8 +121,6 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
         rc = 0;
     }
 
-    free_xenheap_pages((void *)free_mem_ptr, HEAPORDER);
-
     return rc;
 }
 
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 73ccfc2bdc6c..512d9bf0ee2e 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -228,53 +228,6 @@ static const ush mask_bits[] = {
 #define NEEDBITS(n) {while(k<(n)){b|=((ulg)NEXTBYTE())<<k;k+=8;}}
 #define DUMPBITS(n) {b>>=(n);k-=(n);}
 
-#ifndef NO_INFLATE_MALLOC
-/*
- * A trivial malloc implementation, adapted from
- *  malloc by Hannu Savolainen 1993 and Matthias Urlichs 1994
- */
-
-static unsigned long __initdata malloc_ptr;
-static int __initdata malloc_count;
-
-static void __init init_allocator(void)
-{
-    malloc_ptr = free_mem_ptr;
-    malloc_count = 0;
-}
-
-static void *__init malloc(int size)
-{
-    void *p;
-
-    if (size < 0)
-        error("Malloc error");
-    if (!malloc_ptr)
-        malloc_ptr = free_mem_ptr;
-
-    malloc_ptr = (malloc_ptr + 3) & ~3;     /* Align */
-
-    p = (void *)malloc_ptr;
-    malloc_ptr += size;
-
-    if (free_mem_end_ptr && malloc_ptr >= free_mem_end_ptr)
-        error("Out of memory");
-
-    malloc_count++;
-    return p;
-}
-
-static void __init free(void *where)
-{
-    malloc_count--;
-    if (!malloc_count)
-        malloc_ptr = free_mem_ptr;
-}
-#else
-#define malloc(a) kmalloc(a, GFP_KERNEL)
-#define free(a) kfree(a)
-#endif
-
 /*
  * Huffman code decoding is performed using a multi-level table lookup.
  * The fastest way to decode is to simply build a lookup table whose
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 4/6] gzip: refactor state tracking
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
                   ` (2 preceding siblings ...)
  2024-04-17 14:37 ` [PATCH v2 3/6] gzip: remove custom memory allocator Daniel P. Smith
@ 2024-04-17 14:37 ` Daniel P. Smith
  2024-04-17 18:45   ` Andrew Cooper
  2024-04-17 14:37 ` [PATCH v2 5/6] gzip: move crc state into consilidated gzip state Daniel P. Smith
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

Move the core state into struct gzip_data to allow a per decompression
instance.

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

diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 1b448d6e3655..8178a05a0190 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -4,18 +4,25 @@
 #include <xen/lib.h>
 #include <xen/mm.h>
 
-static unsigned char *__initdata window;
-
 #define WSIZE           0x80000000U
 
-static unsigned char *__initdata inbuf;
-static unsigned int __initdata insize;
+struct gzip_state {
+    unsigned char *window;
+
+    unsigned char *inbuf;
+    unsigned int insize;
+
+    /* Index of next byte to be processed in inbuf: */
+    unsigned int inptr;
 
-/* Index of next byte to be processed in inbuf: */
-static unsigned int __initdata inptr;
+    /* Bytes in output buffer: */
+    unsigned int outcnt;
 
-/* Bytes in output buffer: */
-static unsigned int __initdata outcnt;
+    long bytes_out;
+
+    unsigned long bb;      /* bit buffer */
+    unsigned bk;           /* bits in bit buffer */
+};
 
 #define OF(args)        args
 
@@ -27,7 +34,7 @@ typedef unsigned char   uch;
 typedef unsigned short  ush;
 typedef unsigned long   ulg;
 
-#define get_byte()      (inptr < insize ? inbuf[inptr++] : fill_inbuf())
+#define get_byte()     (s->inptr < s->insize ? s->inbuf[s->inptr++] : fill_inbuf())
 
 /* Diagnostic functions */
 #ifdef DEBUG
@@ -46,8 +53,7 @@ typedef unsigned long   ulg;
 #  define Tracecv(c, x)
 #endif
 
-static long __initdata bytes_out;
-static void flush_window(void);
+static void flush_window(struct gzip_state *s);
 
 static __init void error(const char *x)
 {
@@ -62,7 +68,7 @@ static __init int fill_inbuf(void)
 
 #include "inflate.c"
 
-static __init void flush_window(void)
+static __init void flush_window(struct gzip_state *s)
 {
     /*
      * The window is equal to the output buffer therefore only need to
@@ -72,16 +78,16 @@ static __init void flush_window(void)
     unsigned int n;
     unsigned char *in, ch;
 
-    in = window;
-    for ( n = 0; n < outcnt; n++ )
+    in = s->window;
+    for ( n = 0; n < s->outcnt; n++ )
     {
         ch = *in++;
         c = crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
     }
     crc = c;
 
-    bytes_out += (unsigned long)outcnt;
-    outcnt = 0;
+    s->bytes_out += (unsigned long)s->outcnt;
+    s->outcnt = 0;
 }
 
 __init int gzip_check(char *image, unsigned long image_len)
@@ -99,20 +105,25 @@ __init int gzip_check(char *image, unsigned long image_len)
 
 __init int perform_gunzip(char *output, char *image, unsigned long image_len)
 {
+    struct gzip_state *s;
     int rc;
 
     if ( !gzip_check(image, image_len) )
         return 1;
 
-    window = (unsigned char *)output;
-    inbuf = (unsigned char *)image;
-    insize = image_len;
-    inptr = 0;
-    bytes_out = 0;
+    s = (struct gzip_state *)malloc(sizeof(struct gzip_state));
+    if ( !s )
+        return -ENOMEM;
+
+    s->window = (unsigned char *)output;
+    s->inbuf = (unsigned char *)image;
+    s->insize = image_len;
+    s->inptr = 0;
+    s->bytes_out = 0;
 
     makecrc();
 
-    if ( gunzip() < 0 )
+    if ( gunzip(s) < 0 )
     {
         rc = -EINVAL;
     }
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 512d9bf0ee2e..5735bbcf7eb4 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -119,7 +119,7 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
 
 #endif /* !__XEN__ */
 
-#define slide window
+#define slide s->window
 
 /*
  * Huffman code lookup table entry--this entry is four bytes for machines
@@ -143,12 +143,12 @@ struct huft {
 static int huft_build OF((unsigned *, unsigned, unsigned,
                           const ush *, const ush *, struct huft **, int *));
 static int huft_free OF((struct huft *));
-static int inflate_codes OF((struct huft *, struct huft *, int, int));
-static int inflate_stored OF((void));
-static int inflate_fixed OF((void));
-static int inflate_dynamic OF((void));
-static int inflate_block OF((int *));
-static int inflate OF((void));
+static int inflate_codes OF((struct gzip_state *, struct huft *, struct huft *, int, int));
+static int inflate_stored OF((struct gzip_state *));
+static int inflate_fixed OF((struct gzip_state *));
+static int inflate_dynamic OF((struct gzip_state *));
+static int inflate_block OF((struct gzip_state *, int *));
+static int inflate OF((struct gzip_state *));
 
 /*
  * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
@@ -162,8 +162,8 @@ static int inflate OF((void));
  * must be in unzip.h, included above.
  */
 /* unsigned wp;             current position in slide */
-#define wp outcnt
-#define flush_output(w) (wp=(w),flush_window())
+#define wp s->outcnt
+#define flush_output(s, w) (wp=(w),flush_window(s))
 
 /* Tables for deflate from PKZIP's appnote.txt. */
 static const unsigned border[] = {    /* Order of the bit length code lengths */
@@ -215,17 +215,14 @@ static const ush cpdext[] = {         /* Extra bits for distance codes */
  * the stream.
  */
 
-static ulg __initdata bb;                /* bit buffer */
-static unsigned __initdata bk;           /* bits in bit buffer */
-
 static const ush mask_bits[] = {
     0x0000,
     0x0001, 0x0003, 0x0007, 0x000f, 0x001f, 0x003f, 0x007f, 0x00ff,
     0x01ff, 0x03ff, 0x07ff, 0x0fff, 0x1fff, 0x3fff, 0x7fff, 0xffff
 };
 
-#define NEXTBYTE()  ({ int v = get_byte(); if (v < 0) goto underrun; (uch)v; })
-#define NEEDBITS(n) {while(k<(n)){b|=((ulg)NEXTBYTE())<<k;k+=8;}}
+#define NEXTBYTE(s)  ({ int v = get_byte(); if (v < 0) goto underrun; (uch)v; })
+#define NEEDBITS(s, n) {while(k<(n)){b|=((ulg)NEXTBYTE(s))<<k;k+=8;}}
 #define DUMPBITS(n) {b>>=(n);k-=(n);}
 
 /*
@@ -548,7 +545,7 @@ static int __init huft_free(struct huft *t)
  * @param bd  Number of bits decoded by td[]
  */
 static int __init inflate_codes(
-    struct huft *tl, struct huft *td, int bl, int bd)
+    struct gzip_state *s, struct huft *tl, struct huft *td, int bl, int bd)
 {
     register unsigned e;  /* table entry flag/number of extra bits */
     unsigned n, d;        /* length and index for copy */
@@ -560,8 +557,8 @@ static int __init inflate_codes(
 
 
     /* make local copies of globals */
-    b = bb;                       /* initialize bit buffer */
-    k = bk;
+    b = s->bb;                   /* initialize bit buffer */
+    k = s->bk;
     w = wp;                       /* initialize window position */
 
     /* inflate the coded data */
@@ -569,14 +566,14 @@ static int __init inflate_codes(
     md = mask_bits[bd];
     for (;;)                      /* do until end of block */
     {
-        NEEDBITS((unsigned)bl);
+        NEEDBITS(s, (unsigned)bl);
         if ((e = (t = tl + ((unsigned)b & ml))->e) > 16)
             do {
             if (e == 99)
                 return 1;
             DUMPBITS(t->b);
             e -= 16;
-            NEEDBITS(e);
+            NEEDBITS(s, e);
             } while ((e = (t = t->v.t + ((unsigned)b & mask_bits[e]))->e) > 16);
         DUMPBITS(t->b);
         if (e == 16)                /* then it's a literal */
@@ -585,7 +582,7 @@ static int __init inflate_codes(
             Tracevv((stderr, "%c", slide[w-1]));
             if (w == WSIZE)
             {
-                flush_output(w);
+                flush_output(s, w);
                 w = 0;
             }
         }
@@ -596,22 +593,22 @@ static int __init inflate_codes(
                 break;
 
             /* get length of block to copy */
-            NEEDBITS(e);
+            NEEDBITS(s, e);
             n = t->v.n + ((unsigned)b & mask_bits[e]);
             DUMPBITS(e);
 
             /* decode distance of block to copy */
-            NEEDBITS((unsigned)bd);
+            NEEDBITS(s, (unsigned)bd);
             if ((e = (t = td + ((unsigned)b & md))->e) > 16)
                 do {
                     if (e == 99)
                         return 1;
                     DUMPBITS(t->b);
                     e -= 16;
-                    NEEDBITS(e);
+                    NEEDBITS(s, e);
                 } while ((e = (t = t->v.t + ((unsigned)b & mask_bits[e]))->e) > 16);
             DUMPBITS(t->b);
-            NEEDBITS(e);
+            NEEDBITS(s, e);
             d = w - t->v.n - ((unsigned)b & mask_bits[e]);
             DUMPBITS(e);
             Tracevv((stderr,"\\[%d,%d]", w-d, n));
@@ -632,7 +629,7 @@ static int __init inflate_codes(
                     } while (--e);
                 if (w == WSIZE)
                 {
-                    flush_output(w);
+                    flush_output(s, w);
                     w = 0;
                 }
             } while (n);
@@ -641,8 +638,8 @@ static int __init inflate_codes(
 
     /* restore the globals from the locals */
     wp = w;                       /* restore global window pointer */
-    bb = b;                       /* restore global bit buffer */
-    bk = k;
+    s->bb = b;                   /* restore global bit buffer */
+    s->bk = k;
 
     /* done */
     return 0;
@@ -652,7 +649,7 @@ static int __init inflate_codes(
 }
 
 /* "decompress" an inflated type 0 (stored) block. */
-static int __init inflate_stored(void)
+static int __init inflate_stored(struct gzip_state *s)
 {
     unsigned n;           /* number of bytes in block */
     unsigned w;           /* current window position */
@@ -662,8 +659,8 @@ static int __init inflate_stored(void)
     DEBG("<stor");
 
     /* make local copies of globals */
-    b = bb;                       /* initialize bit buffer */
-    k = bk;
+    b = s->bb;                   /* initialize bit buffer */
+    k = s->bk;
     w = wp;                       /* initialize window position */
 
 
@@ -673,10 +670,10 @@ static int __init inflate_stored(void)
 
 
     /* get the length and its complement */
-    NEEDBITS(16);
+    NEEDBITS(s, 16);
     n = ((unsigned)b & 0xffff);
     DUMPBITS(16);
-    NEEDBITS(16);
+    NEEDBITS(s, 16);
     if (n != (unsigned)((~b) & 0xffff))
         return 1;                   /* error in compressed data */
     DUMPBITS(16);
@@ -684,11 +681,11 @@ static int __init inflate_stored(void)
     /* read and output the compressed data */
     while (n--)
     {
-        NEEDBITS(8);
+        NEEDBITS(s, 8);
         slide[w++] = (uch)b;
         if (w == WSIZE)
         {
-            flush_output(w);
+            flush_output(s, w);
             w = 0;
         }
         DUMPBITS(8);
@@ -696,8 +693,8 @@ static int __init inflate_stored(void)
 
     /* restore the globals from the locals */
     wp = w;                       /* restore global window pointer */
-    bb = b;                       /* restore global bit buffer */
-    bk = k;
+    s->bb = b;                   /* restore global bit buffer */
+    s->bk = k;
 
     DEBG(">");
     return 0;
@@ -716,7 +713,7 @@ static int __init inflate_stored(void)
  * either replace this with a custom decoder, or at least precompute the
  * Huffman tables.
  */
-static int noinline __init inflate_fixed(void)
+static int noinline __init inflate_fixed(struct gzip_state *s)
 {
     int i;                /* temporary variable */
     struct huft *tl;      /* literal/length code table */
@@ -760,7 +757,8 @@ static int noinline __init inflate_fixed(void)
     }
 
     /* decompress until an end-of-block code */
-    if (inflate_codes(tl, td, bl, bd)) {
+    if ( inflate_codes(s, tl, td, bl, bd) )
+    {
         free(l);
         return 1;
     }
@@ -777,7 +775,7 @@ static int noinline __init inflate_fixed(void)
  */
 
 /* decompress an inflated type 2 (dynamic Huffman codes) block. */
-static int noinline __init inflate_dynamic(void)
+static int noinline __init inflate_dynamic(struct gzip_state *s)
 {
     int i;                /* temporary variables */
     unsigned j;
@@ -804,17 +802,17 @@ static int noinline __init inflate_dynamic(void)
         return 1;
 
     /* make local bit buffer */
-    b = bb;
-    k = bk;
+    b = s->bb;
+    k = s->bk;
 
     /* read in table lengths */
-    NEEDBITS(5);
+    NEEDBITS(s, 5);
     nl = 257 + ((unsigned)b & 0x1f);      /* number of literal/length codes */
     DUMPBITS(5);
-    NEEDBITS(5);
+    NEEDBITS(s, 5);
     nd = 1 + ((unsigned)b & 0x1f);        /* number of distance codes */
     DUMPBITS(5);
-    NEEDBITS(4);
+    NEEDBITS(s, 4);
     nb = 4 + ((unsigned)b & 0xf);         /* number of bit length codes */
     DUMPBITS(4);
     if (nl > 286 || nd > 30)
@@ -828,7 +826,7 @@ static int noinline __init inflate_dynamic(void)
     /* read in bit-length-code lengths */
     for (j = 0; j < nb; j++)
     {
-        NEEDBITS(3);
+        NEEDBITS(s, 3);
         ll[border[j]] = (unsigned)b & 7;
         DUMPBITS(3);
     }
@@ -855,7 +853,7 @@ static int noinline __init inflate_dynamic(void)
     i = l = 0;
     while ((unsigned)i < n)
     {
-        NEEDBITS((unsigned)bl);
+        NEEDBITS(s, (unsigned)bl);
         j = (td = tl + ((unsigned)b & m))->b;
         DUMPBITS(j);
         j = td->v.n;
@@ -863,7 +861,7 @@ static int noinline __init inflate_dynamic(void)
             ll[i++] = l = j;          /* save last length in l */
         else if (j == 16)           /* repeat last length 3 to 6 times */
         {
-            NEEDBITS(2);
+            NEEDBITS(s, 2);
             j = 3 + ((unsigned)b & 3);
             DUMPBITS(2);
             if ((unsigned)i + j > n) {
@@ -875,7 +873,7 @@ static int noinline __init inflate_dynamic(void)
         }
         else if (j == 17)           /* 3 to 10 zero length codes */
         {
-            NEEDBITS(3);
+            NEEDBITS(s, 3);
             j = 3 + ((unsigned)b & 7);
             DUMPBITS(3);
             if ((unsigned)i + j > n) {
@@ -888,7 +886,7 @@ static int noinline __init inflate_dynamic(void)
         }
         else                        /* j == 18: 11 to 138 zero length codes */
         {
-            NEEDBITS(7);
+            NEEDBITS(s, 7);
             j = 11 + ((unsigned)b & 0x7f);
             DUMPBITS(7);
             if ((unsigned)i + j > n) {
@@ -909,8 +907,8 @@ static int noinline __init inflate_dynamic(void)
     DEBG("dyn5 ");
 
     /* restore the global bit buffer */
-    bb = b;
-    bk = k;
+    s->bb = b;
+    s->bk = k;
 
     DEBG("dyn5a ");
 
@@ -943,7 +941,8 @@ static int noinline __init inflate_dynamic(void)
     DEBG("dyn6 ");
 
       /* decompress until an end-of-block code */
-    if (inflate_codes(tl, td, bl, bd)) {
+    if ( inflate_codes(s, tl, td, bl, bd) )
+    {
         ret = 1;
         goto out;
     }
@@ -968,9 +967,10 @@ static int noinline __init inflate_dynamic(void)
 /*
  * decompress an inflated block
  *
+ * @param s Gzip decompression state
  * @param e Last block flag
  */
-static int __init inflate_block(int *e)
+static int __init inflate_block(struct gzip_state *s, int *e)
 {
     unsigned t;           /* block type */
     register ulg b;       /* bit buffer */
@@ -979,30 +979,30 @@ static int __init inflate_block(int *e)
     DEBG("<blk");
 
     /* make local bit buffer */
-    b = bb;
-    k = bk;
+    b = s->bb;
+    k = s->bk;
 
     /* read in last block bit */
-    NEEDBITS(1);
+    NEEDBITS(s, 1);
     *e = (int)b & 1;
     DUMPBITS(1);
 
     /* read in block type */
-    NEEDBITS(2);
+    NEEDBITS(s, 2);
     t = (unsigned)b & 3;
     DUMPBITS(2);
 
     /* restore the global bit buffer */
-    bb = b;
-    bk = k;
+    s->bb = b;
+    s->bk = k;
 
     /* inflate that block type */
     if (t == 2)
-        return inflate_dynamic();
+        return inflate_dynamic(s);
     if (t == 0)
-        return inflate_stored();
+        return inflate_stored(s);
     if (t == 1)
-        return inflate_fixed();
+        return inflate_fixed(s);
 
     DEBG(">");
 
@@ -1014,7 +1014,7 @@ static int __init inflate_block(int *e)
 }
 
 /* decompress an inflated entry */
-static int __init inflate(void)
+static int __init inflate(struct gzip_state *s)
 {
     int e;                /* last block flag */
     int r;                /* result code */
@@ -1022,8 +1022,8 @@ static int __init inflate(void)
 
     /* initialize window, bit buffer */
     wp = 0;
-    bk = 0;
-    bb = 0;
+    s->bk = 0;
+    s->bb = 0;
 
     /* decompress until the last block */
     h = 0;
@@ -1032,7 +1032,7 @@ static int __init inflate(void)
 #ifdef ARCH_HAS_DECOMP_WDOG
         arch_decomp_wdog();
 #endif
-        r = inflate_block(&e);
+        r = inflate_block(s, &e);
         if (r)
             return r;
         if (hufts > h)
@@ -1042,13 +1042,13 @@ static int __init inflate(void)
     /* Undo too much lookahead. The next read will be byte aligned so we
      * can discard unused bits in the last meaningful byte.
      */
-    while (bk >= 8) {
-        bk -= 8;
-        inptr--;
+    while (s->bk >= 8) {
+        s->bk -= 8;
+        s->inptr--;
     }
 
     /* flush out slide */
-    flush_output(wp);
+    flush_output(s, wp);
 
     /* return success */
 #ifdef DEBUG
@@ -1119,7 +1119,7 @@ static void __init makecrc(void)
 /*
  * Do the uncompression!
  */
-static int __init gunzip(void)
+static int __init gunzip(struct gzip_state *s)
 {
     uch flags;
     unsigned char magic[2]; /* magic header */
@@ -1128,9 +1128,9 @@ static int __init gunzip(void)
     ulg orig_len = 0;       /* original uncompressed length */
     int res;
 
-    magic[0] = NEXTBYTE();
-    magic[1] = NEXTBYTE();
-    method   = NEXTBYTE();
+    magic[0] = NEXTBYTE(s);
+    magic[1] = NEXTBYTE(s);
+    method   = NEXTBYTE(s);
 
     if (magic[0] != 037 ||                            /* octal-ok */
         ((magic[1] != 0213) && (magic[1] != 0236))) { /* octal-ok */
@@ -1157,18 +1157,18 @@ static int __init gunzip(void)
         error("Input has invalid flags");
         return -1;
     }
-    NEXTBYTE(); /* Get timestamp */
-    NEXTBYTE();
-    NEXTBYTE();
-    NEXTBYTE();
+    NEXTBYTE(s); /* Get timestamp */
+    NEXTBYTE(s);
+    NEXTBYTE(s);
+    NEXTBYTE(s);
 
-    (void)NEXTBYTE();  /* Ignore extra flags for the moment */
-    (void)NEXTBYTE();  /* Ignore OS type for the moment */
+    (void)NEXTBYTE(s);  /* Ignore extra flags for the moment */
+    (void)NEXTBYTE(s);  /* Ignore OS type for the moment */
 
     if ((flags & EXTRA_FIELD) != 0) {
-        unsigned len = (unsigned)NEXTBYTE();
-        len |= ((unsigned)NEXTBYTE())<<8;
-        while (len--) (void)NEXTBYTE();
+        unsigned len = (unsigned)NEXTBYTE(s);
+        len |= ((unsigned)NEXTBYTE(s))<<8;
+        while (len--) (void)NEXTBYTE(s);
     }
 
     /* Get original file name if it was truncated */
@@ -1179,11 +1179,11 @@ static int __init gunzip(void)
 
     /* Discard file comment if any */
     if ((flags & COMMENT) != 0) {
-        while (NEXTBYTE() != 0) /* null */ ;
+        while (NEXTBYTE(s) != 0) /* null */ ;
     }
 
     /* Decompress */
-    if ((res = inflate())) {
+    if ((res = inflate(s))) {
         switch (res) {
         case 0:
             break;
@@ -1224,13 +1224,13 @@ static int __init gunzip(void)
         error("crc error");
         return -1;
     }
-    if (orig_len != bytes_out) {
+    if (orig_len != s->bytes_out) {
         error("length error");
         return -1;
     }
     return 0;
 
- underrun:   /* NEXTBYTE() goto's here if needed */
+ underrun:   /* NEXTBYTE(s) goto's here if needed */
     error("out of input data");
     return -1;
 }
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 5/6] gzip: move crc state into consilidated gzip state
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
                   ` (3 preceding siblings ...)
  2024-04-17 14:37 ` [PATCH v2 4/6] gzip: refactor state tracking Daniel P. Smith
@ 2024-04-17 14:37 ` Daniel P. Smith
  2024-04-17 17:00   ` Andrew Cooper
  2024-04-17 14:37 ` [PATCH v2 6/6] gzip: drop huffman code table tracking Daniel P. Smith
  2024-04-18  7:43 ` [PATCH v2 0/6] Clean up of gzip decompressor Jan Beulich
  6 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

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

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
@@ -22,6 +22,9 @@ struct gzip_state {
 
     unsigned long bb;      /* bit buffer */
     unsigned bk;           /* bits in bit buffer */
+
+    uint32_t crc_32_tab[256];
+    uint32_t crc;
 };
 
 #define OF(args)        args
@@ -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;
     unsigned int n;
     unsigned char *in, ch;
 
@@ -82,9 +85,9 @@ static __init void flush_window(struct gzip_state *s)
     for ( n = 0; n < s->outcnt; n++ )
     {
         ch = *in++;
-        c = crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
+        c = s->crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
     }
-    crc = c;
+    s->crc = c;
 
     s->bytes_out += (unsigned long)s->outcnt;
     s->outcnt = 0;
@@ -121,7 +124,7 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
     s->inptr = 0;
     s->bytes_out = 0;
 
-    makecrc();
+    makecrc(s);
 
     if ( gunzip(s) < 0 )
     {
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)
 
 /*
  * 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 */
 }
 
 /* gzip flag byte */
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 6/6] gzip: drop huffman code table tracking
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
                   ` (4 preceding siblings ...)
  2024-04-17 14:37 ` [PATCH v2 5/6] gzip: move crc state into consilidated gzip state Daniel P. Smith
@ 2024-04-17 14:37 ` Daniel P. Smith
  2024-04-17 16:31   ` Andrew Cooper
  2024-04-18  7:43 ` [PATCH v2 0/6] Clean up of gzip decompressor Jan Beulich
  6 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Smith @ 2024-04-17 14:37 UTC (permalink / raw)
  To: xen-devel
  Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini

The "tracking" bits does not appear to be used, so dropping from the code.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/common/gzip/inflate.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index c18ce20210b0..15bc187c2bbe 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -264,8 +264,6 @@ static const int dbits = 6;          /* bits in base distance lookup table */
 #define BMAX 16         /* maximum bit length of any code (16 for explode) */
 #define N_MAX 288       /* maximum number of codes in any set */
 
-static unsigned __initdata hufts;      /* track memory usage */
-
 /*
  * Given a list of code lengths and a maximum table size, make a set of
  * tables to decode that set of codes.  Return zero on success, one if
@@ -445,7 +443,6 @@ static int __init huft_build(
                     goto out;
                 }
                 DEBG1("4 ");
-                hufts += z + 1;         /* track memory usage */
                 *t = q + 1;             /* link to list for huft_free() */
                 *(t = &(q->v.t)) = (struct huft *)NULL;
                 u[h] = ++q;             /* table starts after link */
@@ -1028,15 +1025,12 @@ static int __init inflate(struct gzip_state *s)
     /* decompress until the last block */
     h = 0;
     do {
-        hufts = 0;
 #ifdef ARCH_HAS_DECOMP_WDOG
         arch_decomp_wdog();
 #endif
         r = inflate_block(s, &e);
         if (r)
             return r;
-        if (hufts > h)
-            h = hufts;
     } while (!e);
 
     /* Undo too much lookahead. The next read will be byte aligned so we
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/6] gzip: drop unused define checks
  2024-04-17 14:37 ` [PATCH v2 1/6] gzip: drop unused define checks Daniel P. Smith
@ 2024-04-17 16:27   ` Andrew Cooper
  2024-04-18  7:47   ` Jan Beulich
  1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2024-04-17 16:27 UTC (permalink / raw)
  To: Daniel P. Smith, xen-devel
  Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
	Stefano Stabellini

On 17/04/2024 3:37 pm, Daniel P. Smith wrote:
> Dropping the define checks for PKZIP_BUG_WORKAROUND and NOMEMCPY define as they
> never are set.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

It looks like ARCH_HAS_DECOMP_WDOG is another one that can go.

There's only a single instance, in inflate(), which I'm happy to fold on
commit.

~Andrew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 6/6] gzip: drop huffman code table tracking
  2024-04-17 14:37 ` [PATCH v2 6/6] gzip: drop huffman code table tracking Daniel P. Smith
@ 2024-04-17 16:31   ` Andrew Cooper
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2024-04-17 16:31 UTC (permalink / raw)
  To: Daniel P. Smith, xen-devel
  Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
	Stefano Stabellini

On 17/04/2024 3:37 pm, Daniel P. Smith wrote:
> The "tracking" bits does not appear to be used, so dropping from the code.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/common/gzip/inflate.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index c18ce20210b0..15bc187c2bbe 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -264,8 +264,6 @@ static const int dbits = 6;          /* bits in base distance lookup table */
>  #define BMAX 16         /* maximum bit length of any code (16 for explode) */
>  #define N_MAX 288       /* maximum number of codes in any set */
>  
> -static unsigned __initdata hufts;      /* track memory usage */
> -
>  /*
>   * Given a list of code lengths and a maximum table size, make a set of
>   * tables to decode that set of codes.  Return zero on success, one if
> @@ -445,7 +443,6 @@ static int __init huft_build(
>                      goto out;
>                  }
>                  DEBG1("4 ");
> -                hufts += z + 1;         /* track memory usage */
>                  *t = q + 1;             /* link to list for huft_free() */
>                  *(t = &(q->v.t)) = (struct huft *)NULL;
>                  u[h] = ++q;             /* table starts after link */
> @@ -1028,15 +1025,12 @@ static int __init inflate(struct gzip_state *s)
>      /* decompress until the last block */
>      h = 0;
>      do {
> -        hufts = 0;
>  #ifdef ARCH_HAS_DECOMP_WDOG
>          arch_decomp_wdog();
>  #endif
>          r = inflate_block(s, &e);
>          if (r)
>              return r;
> -        if (hufts > h)
> -            h = hufts;
>      } while (!e);

With 'hufts' removed, the local variable 'h' is now dead too.  It gets
read inside an #ifdef DEBUG, but as it's rendering a unqualified number
to stderr, it can also be deleted.

Specifically, I recommend this additional delta:

diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 15bc187c2bbe..13015bb45f4a 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -1015,7 +1015,6 @@ static int __init inflate(struct gzip_state *s)
 {
     int e;                /* last block flag */
     int r;                /* result code */
-    unsigned h;           /* maximum struct huft's malloc'ed */
 
     /* initialize window, bit buffer */
     wp = 0;
@@ -1023,7 +1022,6 @@ static int __init inflate(struct gzip_state *s)
     s->bb = 0;
 
     /* decompress until the last block */
-    h = 0;
     do {
 #ifdef ARCH_HAS_DECOMP_WDOG
         arch_decomp_wdog();
@@ -1045,9 +1043,6 @@ static int __init inflate(struct gzip_state *s)
     flush_output(s, wp);
 
     /* return success */
-#ifdef DEBUG
-    fprintf(stderr, "<%u> ", h);
-#endif /* DEBUG */
     return 0;
 }
 

which I'm happy to fold on commit.

~Andrew


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/6] gzip: remove custom memory allocator
  2024-04-17 14:37 ` [PATCH v2 3/6] gzip: remove custom memory allocator Daniel P. Smith
@ 2024-04-17 16:52   ` Andrew Cooper
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2024-04-17 16:52 UTC (permalink / raw)
  To: Daniel P. Smith, xen-devel
  Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
	Stefano Stabellini

On 17/04/2024 3:37 pm, Daniel P. Smith wrote:
> All the other decompression routines use xmalloc_bytes(), thus there is no
> reason for gzip to be handling its own allocation of memory. In fact, there is
> a bug somewhere in the allocator as decompression started to break when adding
> additional allocations. Instead of troubleshooting the allocator, replace it
> with xmalloc_bytes().
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/common/gzip/gunzip.c  | 17 ++------------
>  xen/common/gzip/inflate.c | 47 ---------------------------------------
>  2 files changed, 2 insertions(+), 62 deletions(-)

Good riddance.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 5/6] gzip: move crc state into consilidated gzip state
  2024-04-17 14:37 ` [PATCH v2 5/6] gzip: move crc state into consilidated gzip state Daniel P. Smith
@ 2024-04-17 17:00   ` Andrew Cooper
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2024-04-17 17:00 UTC (permalink / raw)
  To: Daniel P. Smith, xen-devel
  Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
	Stefano Stabellini

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

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/6] gzip: refactor state tracking
  2024-04-17 14:37 ` [PATCH v2 4/6] gzip: refactor state tracking Daniel P. Smith
@ 2024-04-17 18:45   ` Andrew Cooper
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2024-04-17 18:45 UTC (permalink / raw)
  To: Daniel P. Smith, xen-devel
  Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
	Stefano Stabellini

On 17/04/2024 3:37 pm, Daniel P. Smith wrote:
> diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
> index 1b448d6e3655..8178a05a0190 100644
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -4,18 +4,25 @@
>  #include <xen/lib.h>
>  #include <xen/mm.h>
>  
> -static unsigned char *__initdata window;
> -
>  #define WSIZE           0x80000000U
>  
> -static unsigned char *__initdata inbuf;
> -static unsigned int __initdata insize;
> +struct gzip_state {
> +    unsigned char *window;
> +
> +    unsigned char *inbuf;
> +    unsigned int insize;
> +
> +    /* Index of next byte to be processed in inbuf: */
> +    unsigned int inptr;

perform_gunzip() passes in an unsigned long size, which means it's
truncated in this state.

Please at least make these size_t.  Life is too short to deal with the
fallout of this going wrong.

>  
> -/* Index of next byte to be processed in inbuf: */
> -static unsigned int __initdata inptr;
> +    /* Bytes in output buffer: */
> +    unsigned int outcnt;

See later, but I think the comment for outcnt is wrong.

>  
> -/* Bytes in output buffer: */
> -static unsigned int __initdata outcnt;
> +    long bytes_out;

See later, but I don't think this can be signed.  It's only used to
cross-check the header-reported length, which is done as an unsigned long.

> +
> +    unsigned long bb;      /* bit buffer */
> +    unsigned bk;           /* bits in bit buffer */

As this is effectively new code, "unsigned int" please.

> @@ -27,7 +34,7 @@ typedef unsigned char   uch;
>  typedef unsigned short  ush;
>  typedef unsigned long   ulg;
>  
> -#define get_byte()      (inptr < insize ? inbuf[inptr++] : fill_inbuf())
> +#define get_byte()     (s->inptr < s->insize ? s->inbuf[s->inptr++] : fill_inbuf())

This is a bit weird:
> $ git grep -w get_byte common/gzip
> common/gzip/gunzip.c:40:#define get_byte()     (s->inptr < s->insize ?
> s->inbuf[s->inptr++] : fill_inbuf())
> common/gzip/inflate.c:224:#define NEXTBYTE(s)  ({ int v = get_byte();
> if (v < 0) goto underrun; (uch)v; })
> common/gzip/inflate.c:1131:    flags  = (uch)get_byte();

NEXTBYTE() gets an s parameter, but doesn't pass it to get_byte() and
instead relies on state always having the name 's' in scope.

I'd suggest turning get_byte() into a proper static function.  It will
be more readable that throwing extra ()'s around s in the existing macro.

Except...  fill_inbuf() is a fatal error anyway, so that can be folded
together to simplify the result.


> @@ -72,16 +78,16 @@ static __init void flush_window(void)
>      unsigned int n;
>      unsigned char *in, ch;
>  
> -    in = window;
> -    for ( n = 0; n < outcnt; n++ )
> +    in = s->window;
> +    for ( n = 0; n < s->outcnt; n++ )
>      {
>          ch = *in++;
>          c = crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
>      }
>      crc = c;
>  
> -    bytes_out += (unsigned long)outcnt;
> -    outcnt = 0;
> +    s->bytes_out += (unsigned long)s->outcnt;

I can't figure out why this was written this way, even originally.

AFAICT, outcnt doesn't even match it's comment.

/* Bytes in output buffer: */

It looks like it's the number of bytes in window.  This also matches the
"wp" define which I guess is "window position".

Anyway, irrespective of naming, the cast is useless so lets drop it
while modifying the line.

> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index 512d9bf0ee2e..5735bbcf7eb4 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -119,7 +119,7 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
>  
>  #endif /* !__XEN__ */
>  
> -#define slide window
> +#define slide s->window

Given only 8 uses, I'd expand this in place and drop the macro.

But, there's an entire comment block discussing "slide" which I think is
wrong for Xen's implementation.  That wants to go too, I think.

>  
>  /*
>   * Huffman code lookup table entry--this entry is four bytes for machines
> @@ -143,12 +143,12 @@ struct huft {
>  static int huft_build OF((unsigned *, unsigned, unsigned,
>                            const ush *, const ush *, struct huft **, int *));
>  static int huft_free OF((struct huft *));
> -static int inflate_codes OF((struct huft *, struct huft *, int, int));
> -static int inflate_stored OF((void));
> -static int inflate_fixed OF((void));
> -static int inflate_dynamic OF((void));
> -static int inflate_block OF((int *));
> -static int inflate OF((void));
> +static int inflate_codes OF((struct gzip_state *, struct huft *, struct huft *, int, int));
> +static int inflate_stored OF((struct gzip_state *));
> +static int inflate_fixed OF((struct gzip_state *));
> +static int inflate_dynamic OF((struct gzip_state *));
> +static int inflate_block OF((struct gzip_state *, int *));
> +static int inflate OF((struct gzip_state *));

These are the only users of OF, and it turns out it's a no-op macro. 
This code would be far-less WTF-worthy if it was expanded as part of
patch 1.

>  
>  /*
>   * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
> @@ -162,8 +162,8 @@ static int inflate OF((void));
>   * must be in unzip.h, included above.
>   */
>  /* unsigned wp;             current position in slide */
> -#define wp outcnt
> -#define flush_output(w) (wp=(w),flush_window())
> +#define wp s->outcnt
> +#define flush_output(s, w) (wp=(w),flush_window(s))

The combination of these two isn't safe, I don't think, especially as
one caller passes wp into flush_output().

There are very few users of wp, so expand it in line, and turn
flush_output() into a static function.  In particular, it will make ...

> @@ -560,8 +557,8 @@ static int __init inflate_codes(
>  
>  
>      /* make local copies of globals */
> -    b = bb;                       /* initialize bit buffer */
> -    k = bk;
> +    b = s->bb;                   /* initialize bit buffer */
> +    k = s->bk;
>      w = wp;                       /* initialize window position */

... this look less weird.  I'd suggest dropping the remark about "globals".


> @@ -1157,18 +1157,18 @@ static int __init gunzip(void)
>          error("Input has invalid flags");
>          return -1;
>      }
> -    NEXTBYTE(); /* Get timestamp */
> -    NEXTBYTE();
> -    NEXTBYTE();
> -    NEXTBYTE();
> +    NEXTBYTE(s); /* Get timestamp */
> +    NEXTBYTE(s);
> +    NEXTBYTE(s);
> +    NEXTBYTE(s);
>  
> -    (void)NEXTBYTE();  /* Ignore extra flags for the moment */
> -    (void)NEXTBYTE();  /* Ignore OS type for the moment */
> +    (void)NEXTBYTE(s);  /* Ignore extra flags for the moment */
> +    (void)NEXTBYTE(s);  /* Ignore OS type for the moment */

I'd drop these (void) casts. They're not different to the timestamp above.

> @@ -1224,13 +1224,13 @@ static int __init gunzip(void)
>          error("crc error");
>          return -1;
>      }
> -    if (orig_len != bytes_out) {
> +    if (orig_len != s->bytes_out) {
>          error("length error");
>          return -1;
>      }
>      return 0;
>  
> - underrun:   /* NEXTBYTE() goto's here if needed */
> + underrun:   /* NEXTBYTE(s) goto's here if needed */
>      error("out of input data");
>      return -1;

Just for completeness, this is a dead path because fill_inbuf() is
implemented as panic().

Fixing this wants putting on a todo list somewhere.

~Andrew


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 0/6] Clean up of gzip decompressor
  2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
                   ` (5 preceding siblings ...)
  2024-04-17 14:37 ` [PATCH v2 6/6] gzip: drop huffman code table tracking Daniel P. Smith
@ 2024-04-18  7:43 ` Jan Beulich
  6 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2024-04-18  7:43 UTC (permalink / raw)
  To: Daniel P. Smith
  Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, xen-devel

On 17.04.2024 16:37, Daniel P. Smith wrote:
> An issue ran into by hyperlaunch was the need to use the gzip decompressor
> multiple times. The current implementation fails when reused due to tainting of
> decompressor state from a previous usage. This series seeks to colocate the
> gzip unit files under a single directory similar to the other decompression
> algorithms.  To enable the refactoring of the state tracking, the code is then
> cleaned up in line with Xen coding style.

I don't mind the content and style adjustments, but what I'm missing here and
in the patch descriptions is weighing of doing so versus ease of importing
changes to the original source.

Jan



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/6] gzip: drop unused define checks
  2024-04-17 14:37 ` [PATCH v2 1/6] gzip: drop unused define checks Daniel P. Smith
  2024-04-17 16:27   ` Andrew Cooper
@ 2024-04-18  7:47   ` Jan Beulich
  1 sibling, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2024-04-18  7:47 UTC (permalink / raw)
  To: Daniel P. Smith
  Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, xen-devel

On 17.04.2024 16:37, Daniel P. Smith wrote:
> Dropping the define checks for PKZIP_BUG_WORKAROUND and NOMEMCPY define as they
> never are set.

You don't mention or otherwise touch DEBUG uses, yet ...

> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -661,7 +661,6 @@ static int __init inflate_codes(
>                  /* do the copy */
>                  do {
>                      n -= (e = (e = WSIZE - ((d &= WSIZE-1) > w ? d : w)) > n ? n : e);
> -#if !defined(NOMEMCPY) && !defined(DEBUG)

... you also don't replace this by "#ifndef DEBUG", but outright delete
the line.

Jan


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-04-18  7:47 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 14:37 [PATCH v2 0/6] Clean up of gzip decompressor Daniel P. Smith
2024-04-17 14:37 ` [PATCH v2 1/6] gzip: drop unused define checks Daniel P. Smith
2024-04-17 16:27   ` Andrew Cooper
2024-04-18  7:47   ` Jan Beulich
2024-04-17 14:37 ` [PATCH v2 2/6] gzip: clean up comments and fix code alignment Daniel P. Smith
2024-04-17 14:37 ` [PATCH v2 3/6] gzip: remove custom memory allocator Daniel P. Smith
2024-04-17 16:52   ` Andrew Cooper
2024-04-17 14:37 ` [PATCH v2 4/6] gzip: refactor state tracking Daniel P. Smith
2024-04-17 18:45   ` Andrew Cooper
2024-04-17 14:37 ` [PATCH v2 5/6] gzip: move crc state into consilidated gzip state Daniel P. Smith
2024-04-17 17:00   ` Andrew Cooper
2024-04-17 14:37 ` [PATCH v2 6/6] gzip: drop huffman code table tracking Daniel P. Smith
2024-04-17 16:31   ` Andrew Cooper
2024-04-18  7:43 ` [PATCH v2 0/6] Clean up of gzip decompressor Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.