* [PATCH v3 0/8] Clean up of gzip decompressor
@ 2024-04-24 16:34 Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 1/8] gzip: clean up comments and fix code alignment Daniel P. Smith
` (8 more replies)
0 siblings, 9 replies; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 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 v3:
- patch "xen/gzip: Drop huffman code table tracking" was merged
- patch "xen/gzip: Remove custom memory allocator" was merged
- patch "xen/gzip: Drop unused define checks" was merged
- move of the decompressor state into struct was broken up to ease review
- expanded macros that were either only used once or obsfucated the logic
- adjusted variable types as needed to be more appropriate for their usage
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 (8):
gzip: clean up comments and fix code alignment
gzip: refactor and expand macros
gzip: refactor the gunzip window into common state
gzip: move window pointer into gunzip state
gzip: move input buffer handling into gunzip state
gzip: move output buffer into gunzip state
gzip: move bitbuffer into gunzip state
gzip: move crc state into gunzip state
xen/common/gzip/gunzip.c | 84 ++--
xen/common/gzip/inflate.c | 948 +++++++++++++++++++-------------------
2 files changed, 516 insertions(+), 516 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/8] gzip: clean up comments and fix code alignment
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-25 18:31 ` Andrew Cooper
2024-04-24 16:34 ` [PATCH v3 2/8] gzip: refactor and expand macros Daniel P. Smith
` (7 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 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 | 14 +-
xen/common/gzip/inflate.c | 787 +++++++++++++++++++-------------------
2 files changed, 406 insertions(+), 395 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 53cee9ee178a..d07c451cd875 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -54,11 +54,10 @@ static __init void error(const char *x)
static __init int fill_inbuf(void)
{
- error("ran out of input data");
- return 0;
+ error("ran out of input data");
+ return 0;
}
-
#include "inflate.c"
static __init void flush_window(void)
@@ -122,3 +121,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 a03903f73116..220d2ff4d9d9 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(unsigned *, unsigned, unsigned,
const ush *, const ush *, struct huft **, int *);
@@ -148,15 +150,17 @@ static int inflate_dynamic(void);
static int inflate_block(int *);
static int inflate(void);
-
-/* The inflate algorithm uses a sliding 32 K byte window on the uncompressed
- stream to find repeated byte strings. This is implemented here as a
- circular buffer. The index is updated simply by incrementing and then
- ANDing with 0x7fff (32K-1). */
-/* It is left to other modules to supply the 32 K area. It is assumed
- to be usable as if it were declared "uch slide[32768];" or as just
- "uch *slide;" and then malloc'ed in the latter case. The definition
- must be in unzip.h, included above. */
+/*
+ * 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,60 +229,62 @@ static const ush mask_bits[] = {
#define DUMPBITS(n) {b>>=(n);k-=(n);}
/*
- 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 */
+/*
+ * 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 */
@@ -321,7 +326,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) */
@@ -508,18 +513,17 @@ 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. */
+/*
+ * 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.
+ *
+ * @param t Table to free
+ */
+static int __init huft_free(struct huft *t)
{
register struct huft *p, *q;
-
/* Go through linked list, freeing from the malloced (t[-1]) address. */
p = t;
while (p != (struct huft *)NULL)
@@ -527,19 +531,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 */
@@ -560,77 +566,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 */
@@ -643,10 +648,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 */
@@ -667,28 +670,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 */
@@ -706,10 +707,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 */
@@ -752,7 +756,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);
@@ -766,12 +769,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;
@@ -801,32 +804,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;
@@ -850,46 +852,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;
}
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;
@@ -928,67 +930,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;
@@ -996,25 +995,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 */
@@ -1024,7 +1021,6 @@ static int __init inflate(void)
bk = 0;
bb = 0;
-
/* decompress until the last block */
do {
r = inflate_block(&e);
@@ -1043,7 +1039,6 @@ static int __init inflate(void)
/* flush out slide */
flush_output(wp);
-
/* return success */
return 0;
}
@@ -1059,12 +1054,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 */
@@ -1167,7 +1161,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) {
@@ -1196,7 +1190,7 @@ static int __init gunzip(void)
}
return -1;
}
-
+
/* Get the crc and original length */
/* crc32 (see algorithm.doc)
* uncompressed input size modulo 2^32
@@ -1205,12 +1199,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");
@@ -1226,3 +1220,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] 22+ messages in thread
* [PATCH v3 2/8] gzip: refactor and expand macros
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 1/8] gzip: clean up comments and fix code alignment Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-29 14:07 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 3/8] gzip: refactor the gunzip window into common state Daniel P. Smith
` (6 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini
This commit refactors macros into proper static functions. It in-place expands
the `flush_output` macro, allowing the clear removal of the dead code
underneath the `underrun` label.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/common/gzip/gunzip.c | 14 +++++----
xen/common/gzip/inflate.c | 61 ++++++++++++++-------------------------
2 files changed, 30 insertions(+), 45 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index d07c451cd875..b7cadadcca8b 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -25,8 +25,6 @@ typedef unsigned char uch;
typedef unsigned short ush;
typedef unsigned long ulg;
-#define get_byte() (inptr < insize ? inbuf[inptr++] : fill_inbuf())
-
/* Diagnostic functions */
#ifdef DEBUG
# define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0)
@@ -52,10 +50,14 @@ static __init void error(const char *x)
panic("%s\n", x);
}
-static __init int fill_inbuf(void)
-{
- error("ran out of input data");
- return 0;
+static __init uch get_byte(void) {
+ if ( inptr >= insize )
+ {
+ error("ran out of input data");
+ return 0; /* should never reach */
+ }
+
+ return inbuf[inptr++];
}
#include "inflate.c"
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 220d2ff4d9d9..02a395aeb86a 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -119,6 +119,18 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
#endif /* !__XEN__ */
+/*
+ * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
+ * stream to find repeated byte strings. This is implemented here as a
+ * circular buffer. The index is updated simply by incrementing and then
+ * ANDing with 0x7fff (32K-1).
+ *
+ * It is left to other modules to supply the 32 K area. It is assumed
+ * to be usable as if it were declared "uch slide[32768];" or as just
+ * "uch *slide;" and then malloc'ed in the latter case. The definition
+ * must be in unzip.h, included above.
+ */
+#define wp outcnt
#define slide window
/*
@@ -150,21 +162,6 @@ static int inflate_dynamic(void);
static int inflate_block(int *);
static int inflate(void);
-/*
- * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
- * stream to find repeated byte strings. This is implemented here as a
- * circular buffer. The index is updated simply by incrementing and then
- * ANDing with 0x7fff (32K-1).
- *
- * It is left to other modules to supply the 32 K area. It is assumed
- * to be usable as if it were declared "uch slide[32768];" or as just
- * "uch *slide;" and then malloc'ed in the latter case. The definition
- * must be in unzip.h, included above.
- */
-/* unsigned wp; current position in slide */
-#define wp outcnt
-#define flush_output(w) (wp=(w),flush_window())
-
/* Tables for deflate from PKZIP's appnote.txt. */
static const unsigned border[] = { /* Order of the bit length code lengths */
16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15};
@@ -224,7 +221,7 @@ static const ush mask_bits[] = {
0x01ff, 0x03ff, 0x07ff, 0x0fff, 0x1fff, 0x3fff, 0x7fff, 0xffff
};
-#define NEXTBYTE() ({ int v = get_byte(); if (v < 0) goto underrun; (uch)v; })
+#define NEXTBYTE() (get_byte()) /* get_byte will panic on failure */
#define NEEDBITS(n) {while(k<(n)){b|=((ulg)NEXTBYTE())<<k;k+=8;}}
#define DUMPBITS(n) {b>>=(n);k-=(n);}
@@ -582,7 +579,8 @@ static int __init inflate_codes(
Tracevv((stderr, "%c", slide[w-1]));
if (w == WSIZE)
{
- flush_output(w);
+ wp = w;
+ flush_window();
w = 0;
}
}
@@ -629,7 +627,8 @@ static int __init inflate_codes(
} while (--e);
if (w == WSIZE)
{
- flush_output(w);
+ wp = w;
+ flush_window();
w = 0;
}
} while (n);
@@ -643,9 +642,6 @@ static int __init inflate_codes(
/* done */
return 0;
-
- underrun:
- return 4; /* Input underrun */
}
/* "decompress" an inflated type 0 (stored) block. */
@@ -685,7 +681,8 @@ static int __init inflate_stored(void)
slide[w++] = (uch)b;
if (w == WSIZE)
{
- flush_output(w);
+ wp = w;
+ flush_window();
w = 0;
}
DUMPBITS(8);
@@ -698,9 +695,6 @@ static int __init inflate_stored(void)
DEBG(">");
return 0;
-
- underrun:
- return 4; /* Input underrun */
}
@@ -956,10 +950,6 @@ static int noinline __init inflate_dynamic(void)
out:
free(ll);
return ret;
-
- underrun:
- ret = 4; /* Input underrun */
- goto out;
}
/*
@@ -1005,9 +995,6 @@ static int __init inflate_block(int *e)
/* bad block type */
return 2;
-
- underrun:
- return 4; /* Input underrun */
}
/* decompress an inflated entry */
@@ -1037,7 +1024,7 @@ static int __init inflate(void)
}
/* flush out slide */
- flush_output(wp);
+ flush_window();
/* return success */
return 0;
@@ -1148,8 +1135,8 @@ static int __init gunzip(void)
NEXTBYTE();
NEXTBYTE();
- (void)NEXTBYTE(); /* Ignore extra flags for the moment */
- (void)NEXTBYTE(); /* Ignore OS type for the moment */
+ NEXTBYTE(); /* Ignore extra flags for the moment */
+ NEXTBYTE(); /* Ignore OS type for the moment */
if ((flags & EXTRA_FIELD) != 0) {
unsigned len = (unsigned)NEXTBYTE();
@@ -1215,10 +1202,6 @@ static int __init gunzip(void)
return -1;
}
return 0;
-
- underrun: /* NEXTBYTE() goto's here if needed */
- error("out of input data");
- return -1;
}
/*
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/8] gzip: refactor the gunzip window into common state
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 1/8] gzip: clean up comments and fix code alignment Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 2/8] gzip: refactor and expand macros Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-24 21:34 ` Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 4/8] gzip: move window pointer into gunzip state Daniel P. Smith
` (5 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini
Begin moving core state, in this case the gunzip window, into struct
gunzip_state to allow a per decompression instance. In doing so, drop the
define aliasing of window to slide.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/common/gzip/gunzip.c | 21 ++++++++----
xen/common/gzip/inflate.c | 68 +++++++++++++++++++--------------------
2 files changed, 48 insertions(+), 41 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index b7cadadcca8b..e47f10ae19ad 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -4,10 +4,12 @@
#include <xen/lib.h>
#include <xen/mm.h>
-static unsigned char *__initdata window;
-
#define WSIZE 0x80000000U
+struct gunzip_state {
+ unsigned char *window;
+};
+
static unsigned char *__initdata inbuf;
static unsigned int __initdata insize;
@@ -43,7 +45,7 @@ typedef unsigned long ulg;
#endif
static long __initdata bytes_out;
-static void flush_window(void);
+static void flush_window(struct gunzip_state *s);
static __init void error(const char *x)
{
@@ -62,7 +64,7 @@ static __init uch get_byte(void) {
#include "inflate.c"
-static __init void flush_window(void)
+static __init void flush_window(struct gunzip_state *s)
{
/*
* The window is equal to the output buffer therefore only need to
@@ -72,7 +74,7 @@ static __init void flush_window(void)
unsigned int n;
unsigned char *in, ch;
- in = window;
+ in = s->window;
for ( n = 0; n < outcnt; n++ )
{
ch = *in++;
@@ -99,12 +101,17 @@ __init int gzip_check(char *image, unsigned long image_len)
__init int perform_gunzip(char *output, char *image, unsigned long image_len)
{
+ struct gunzip_state *s;
int rc;
if ( !gzip_check(image, image_len) )
return 1;
- window = (unsigned char *)output;
+ s = (struct gunzip_state *)malloc(sizeof(struct gunzip_state));
+ if ( !s )
+ return -ENOMEM;
+
+ s->window = (unsigned char *)output;
inbuf = (unsigned char *)image;
insize = image_len;
inptr = 0;
@@ -112,7 +119,7 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
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 02a395aeb86a..5fa5c039c6d1 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -126,12 +126,11 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
* 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
+ * to be usable as if it were declared "uch window[32768];" or as just
+ * "uch *window;" and then malloc'ed in the latter case. The definition
* must be in unzip.h, included above.
*/
#define wp outcnt
-#define slide window
/*
* Huffman code lookup table entry--this entry is four bytes for machines
@@ -155,12 +154,13 @@ struct huft {
static int huft_build(unsigned *, unsigned, unsigned,
const ush *, const ush *, struct huft **, int *);
static int huft_free(struct huft *);
-static int inflate_codes(struct huft *, struct huft *, int, int);
-static int inflate_stored(void);
-static int inflate_fixed(void);
-static int inflate_dynamic(void);
-static int inflate_block(int *);
-static int inflate(void);
+static int inflate_codes(
+ struct gunzip_state *, struct huft *, struct huft *, int, int);
+static int inflate_stored(struct gunzip_state *s);
+static int inflate_fixed(struct gunzip_state *s);
+static int inflate_dynamic(struct gunzip_state *s);
+static int inflate_block(struct gunzip_state *, int *);
+static int inflate(struct gunzip_state *s);
/* Tables for deflate from PKZIP's appnote.txt. */
static const unsigned border[] = { /* Order of the bit length code lengths */
@@ -542,7 +542,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 gunzip_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 */
@@ -575,12 +575,12 @@ static int __init inflate_codes(
DUMPBITS(t->b);
if (e == 16) /* then it's a literal */
{
- slide[w++] = (uch)t->v.n;
- Tracevv((stderr, "%c", slide[w-1]));
+ s->window[w++] = (uch)t->v.n;
+ Tracevv((stderr, "%c", s->window[w-1]));
if (w == WSIZE)
{
wp = w;
- flush_window();
+ flush_window(s);
w = 0;
}
}
@@ -616,19 +616,19 @@ static int __init inflate_codes(
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);
+ memcpy(s->window + w, s->window + 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]));
+ s->window[w++] = s->window[d++];
+ Tracevv((stderr, "%c", s->window[w-1]));
} while (--e);
if (w == WSIZE)
{
wp = w;
- flush_window();
+ flush_window(s);
w = 0;
}
} while (n);
@@ -645,7 +645,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 gunzip_state *s)
{
unsigned n; /* number of bytes in block */
unsigned w; /* current window position */
@@ -678,11 +678,11 @@ static int __init inflate_stored(void)
while (n--)
{
NEEDBITS(8);
- slide[w++] = (uch)b;
+ s->window[w++] = (uch)b;
if (w == WSIZE)
{
wp = w;
- flush_window();
+ flush_window(s);
w = 0;
}
DUMPBITS(8);
@@ -707,7 +707,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 gunzip_state *s)
{
int i; /* temporary variable */
struct huft *tl; /* literal/length code table */
@@ -751,7 +751,7 @@ 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;
}
@@ -768,7 +768,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 gunzip_state *s)
{
int i; /* temporary variables */
unsigned j;
@@ -934,7 +934,7 @@ 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;
}
@@ -957,7 +957,7 @@ static int noinline __init inflate_dynamic(void)
*
* @param e Last block flag
*/
-static int __init inflate_block(int *e)
+static int __init inflate_block(struct gunzip_state *s, int *e)
{
unsigned t; /* block type */
register ulg b; /* bit buffer */
@@ -985,11 +985,11 @@ static int __init inflate_block(int *e)
/* 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(">");
@@ -998,7 +998,7 @@ static int __init inflate_block(int *e)
}
/* decompress an inflated entry */
-static int __init inflate(void)
+static int __init inflate(struct gunzip_state *s)
{
int e; /* last block flag */
int r; /* result code */
@@ -1010,7 +1010,7 @@ static int __init inflate(void)
/* decompress until the last block */
do {
- r = inflate_block(&e);
+ r = inflate_block(s, &e);
if (r)
return r;
} while (!e);
@@ -1023,8 +1023,8 @@ static int __init inflate(void)
inptr--;
}
- /* flush out slide */
- flush_window();
+ /* flush out window */
+ flush_window(s);
/* return success */
return 0;
@@ -1092,7 +1092,7 @@ static void __init makecrc(void)
/*
* Do the uncompression!
*/
-static int __init gunzip(void)
+static int __init gunzip(struct gunzip_state *s)
{
uch flags;
unsigned char magic[2]; /* magic header */
@@ -1156,7 +1156,7 @@ static int __init gunzip(void)
}
/* Decompress */
- if ((res = inflate())) {
+ if ((res = inflate(s))) {
switch (res) {
case 0:
break;
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 4/8] gzip: move window pointer into gunzip state
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
` (2 preceding siblings ...)
2024-04-24 16:34 ` [PATCH v3 3/8] gzip: refactor the gunzip window into common state Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-29 14:08 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 5/8] gzip: move input buffer handling " Daniel P. Smith
` (4 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini
Move the window pointer, outcnt/wp, into struct gunzip_data. It was erroneously
labeled as outcnt and then define aliased to wp, this eliminates the aliasing
and only refers to as wp, the window pointer.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/common/gzip/gunzip.c | 11 +++++------
xen/common/gzip/inflate.c | 17 ++++++++---------
2 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index e47f10ae19ad..11bfe45d8222 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -8,6 +8,8 @@
struct gunzip_state {
unsigned char *window;
+ /* window pointer: */
+ unsigned int wp;
};
static unsigned char *__initdata inbuf;
@@ -16,9 +18,6 @@ static unsigned int __initdata insize;
/* Index of next byte to be processed in inbuf: */
static unsigned int __initdata inptr;
-/* Bytes in output buffer: */
-static unsigned int __initdata outcnt;
-
#define malloc(a) xmalloc_bytes(a)
#define free(a) xfree(a)
#define memzero(s, n) memset((s), 0, (n))
@@ -75,15 +74,15 @@ static __init void flush_window(struct gunzip_state *s)
unsigned char *in, ch;
in = s->window;
- for ( n = 0; n < outcnt; n++ )
+ for ( n = 0; n < s->wp; n++ )
{
ch = *in++;
c = crc_32_tab[((int)c ^ ch) & 0xff] ^ (c >> 8);
}
crc = c;
- bytes_out += (unsigned long)outcnt;
- outcnt = 0;
+ bytes_out += (unsigned long)s->wp;
+ s->wp = 0;
}
__init int gzip_check(char *image, unsigned long image_len)
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 5fa5c039c6d1..78b2f20a97ba 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -130,7 +130,6 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
* "uch *window;" and then malloc'ed in the latter case. The definition
* must be in unzip.h, included above.
*/
-#define wp outcnt
/*
* Huffman code lookup table entry--this entry is four bytes for machines
@@ -556,7 +555,7 @@ static int __init inflate_codes(
/* make local copies of globals */
b = bb; /* initialize bit buffer */
k = bk;
- w = wp; /* initialize window position */
+ w = s->wp; /* initialize window position */
/* inflate the coded data */
ml = mask_bits[bl]; /* precompute masks for speed */
@@ -579,7 +578,7 @@ static int __init inflate_codes(
Tracevv((stderr, "%c", s->window[w-1]));
if (w == WSIZE)
{
- wp = w;
+ s->wp = w;
flush_window(s);
w = 0;
}
@@ -627,7 +626,7 @@ static int __init inflate_codes(
} while (--e);
if (w == WSIZE)
{
- wp = w;
+ s->wp = w;
flush_window(s);
w = 0;
}
@@ -636,7 +635,7 @@ static int __init inflate_codes(
}
/* restore the globals from the locals */
- wp = w; /* restore global window pointer */
+ s->wp = w; /* restore global window pointer */
bb = b; /* restore global bit buffer */
bk = k;
@@ -657,7 +656,7 @@ static int __init inflate_stored(struct gunzip_state *s)
/* make local copies of globals */
b = bb; /* initialize bit buffer */
k = bk;
- w = wp; /* initialize window position */
+ w = s->wp; /* initialize window position */
/* go to byte boundary */
@@ -681,7 +680,7 @@ static int __init inflate_stored(struct gunzip_state *s)
s->window[w++] = (uch)b;
if (w == WSIZE)
{
- wp = w;
+ s->wp = w;
flush_window(s);
w = 0;
}
@@ -689,7 +688,7 @@ static int __init inflate_stored(struct gunzip_state *s)
}
/* restore the globals from the locals */
- wp = w; /* restore global window pointer */
+ s->wp = w; /* restore global window pointer */
bb = b; /* restore global bit buffer */
bk = k;
@@ -1004,7 +1003,7 @@ static int __init inflate(struct gunzip_state *s)
int r; /* result code */
/* initialize window, bit buffer */
- wp = 0;
+ s->wp = 0;
bk = 0;
bb = 0;
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 5/8] gzip: move input buffer handling into gunzip state
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
` (3 preceding siblings ...)
2024-04-24 16:34 ` [PATCH v3 4/8] gzip: move window pointer into gunzip state Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-29 15:04 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 6/8] gzip: move output buffer " Daniel P. Smith
` (3 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini
Move the input buffer handling, buffer pointer(inbuf), size(insize), and
index(inptr), into gunzip state. Adjust functions and macros that consumed the
input buffer to accept a struct gunzip_state reference.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/common/gzip/gunzip.c | 23 +++++-----
xen/common/gzip/inflate.c | 92 +++++++++++++++++++--------------------
2 files changed, 57 insertions(+), 58 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 11bfe45d8222..3fb9589b069e 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -10,13 +10,12 @@ struct gunzip_state {
unsigned char *window;
/* window pointer: */
unsigned int wp;
-};
-
-static unsigned char *__initdata inbuf;
-static unsigned int __initdata insize;
-/* Index of next byte to be processed in inbuf: */
-static unsigned int __initdata inptr;
+ unsigned char *inbuf;
+ size_t insize;
+ /* Index of next byte to be processed in inbuf: */
+ unsigned int inptr;
+};
#define malloc(a) xmalloc_bytes(a)
#define free(a) xfree(a)
@@ -51,14 +50,14 @@ static __init void error(const char *x)
panic("%s\n", x);
}
-static __init uch get_byte(void) {
- if ( inptr >= insize )
+static __init uch get_byte(struct gunzip_state *s) {
+ if ( s->inptr >= s->insize )
{
error("ran out of input data");
return 0; /* should never reach */
}
- return inbuf[inptr++];
+ return s->inbuf[s->inptr++];
}
#include "inflate.c"
@@ -111,9 +110,9 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
return -ENOMEM;
s->window = (unsigned char *)output;
- inbuf = (unsigned char *)image;
- insize = image_len;
- inptr = 0;
+ s->inbuf = (unsigned char *)image;
+ s->insize = image_len;
+ s->inptr = 0;
bytes_out = 0;
makecrc();
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index 78b2f20a97ba..f1a3b04cef8f 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -220,8 +220,8 @@ static const ush mask_bits[] = {
0x01ff, 0x03ff, 0x07ff, 0x0fff, 0x1fff, 0x3fff, 0x7fff, 0xffff
};
-#define NEXTBYTE() (get_byte()) /* get_byte will panic on failure */
-#define NEEDBITS(n) {while(k<(n)){b|=((ulg)NEXTBYTE())<<k;k+=8;}}
+#define NEXTBYTE(s) (get_byte(s)) /* get_byte will panic on failure */
+#define NEEDBITS(s,n) {while(k<(n)){b|=((ulg)NEXTBYTE(s))<<k;k+=8;}}
#define DUMPBITS(n) {b>>=(n);k-=(n);}
/*
@@ -562,14 +562,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 */
@@ -590,22 +590,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));
@@ -665,10 +665,10 @@ static int __init inflate_stored(struct gunzip_state *s)
/* 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);
@@ -676,7 +676,7 @@ static int __init inflate_stored(struct gunzip_state *s)
/* read and output the compressed data */
while (n--)
{
- NEEDBITS(8);
+ NEEDBITS(s, 8);
s->window[w++] = (uch)b;
if (w == WSIZE)
{
@@ -798,13 +798,13 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
k = 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)
@@ -818,7 +818,7 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
/* 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);
}
@@ -845,7 +845,7 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
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;
@@ -853,7 +853,7 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
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) {
@@ -865,7 +865,7 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
}
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) {
@@ -878,7 +878,7 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
}
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) {
@@ -969,12 +969,12 @@ static int __init inflate_block(struct gunzip_state *s, int *e)
k = 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);
@@ -1019,7 +1019,7 @@ static int __init inflate(struct gunzip_state *s)
*/
while (bk >= 8) {
bk -= 8;
- inptr--;
+ s->inptr--;
}
/* flush out window */
@@ -1100,9 +1100,9 @@ static int __init gunzip(struct gunzip_state *s)
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 */
@@ -1116,7 +1116,7 @@ static int __init gunzip(struct gunzip_state *s)
return -1;
}
- flags = (uch)get_byte();
+ flags = (uch)get_byte(s);
if ((flags & ENCRYPTED) != 0) {
error("Input is encrypted");
return -1;
@@ -1129,29 +1129,29 @@ static int __init gunzip(struct gunzip_state *s)
error("Input has invalid flags");
return -1;
}
- NEXTBYTE(); /* Get timestamp */
- NEXTBYTE();
- NEXTBYTE();
- NEXTBYTE();
+ NEXTBYTE(s); /* Get timestamp */
+ NEXTBYTE(s);
+ NEXTBYTE(s);
+ NEXTBYTE(s);
- NEXTBYTE(); /* Ignore extra flags for the moment */
- NEXTBYTE(); /* Ignore OS type for the moment */
+ NEXTBYTE(s); /* Ignore extra flags for the moment */
+ 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 */
if ((flags & ORIG_NAME) != 0) {
/* Discard the old name */
- while (NEXTBYTE() != 0) /* null */ ;
+ while (NEXTBYTE(s) != 0) /* null */ ;
}
/* Discard file comment if any */
if ((flags & COMMENT) != 0) {
- while (NEXTBYTE() != 0) /* null */ ;
+ while (NEXTBYTE(s) != 0) /* null */ ;
}
/* Decompress */
@@ -1181,15 +1181,15 @@ static int __init gunzip(struct gunzip_state *s)
/* crc32 (see algorithm.doc)
* uncompressed input size modulo 2^32
*/
- orig_crc = (ulg) NEXTBYTE();
- 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;
+ orig_crc = (ulg) NEXTBYTE(s);
+ orig_crc |= (ulg) NEXTBYTE(s) << 8;
+ orig_crc |= (ulg) NEXTBYTE(s) << 16;
+ orig_crc |= (ulg) NEXTBYTE(s) << 24;
+
+ orig_len = (ulg) NEXTBYTE(s);
+ orig_len |= (ulg) NEXTBYTE(s) << 8;
+ orig_len |= (ulg) NEXTBYTE(s) << 16;
+ orig_len |= (ulg) NEXTBYTE(s) << 24;
/* Validate decompression */
if (orig_crc != CRC_VALUE) {
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 6/8] gzip: move output buffer into gunzip state
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
` (4 preceding siblings ...)
2024-04-24 16:34 ` [PATCH v3 5/8] gzip: move input buffer handling " Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-29 15:07 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 7/8] gzip: move bitbuffer " Daniel P. Smith
` (2 subsequent siblings)
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 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 | 7 ++++---
xen/common/gzip/inflate.c | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 3fb9589b069e..95d924d36726 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -15,6 +15,8 @@ struct gunzip_state {
size_t insize;
/* Index of next byte to be processed in inbuf: */
unsigned int inptr;
+
+ unsigned long bytes_out;
};
#define malloc(a) xmalloc_bytes(a)
@@ -42,7 +44,6 @@ typedef unsigned long ulg;
# define Tracecv(c, x)
#endif
-static long __initdata bytes_out;
static void flush_window(struct gunzip_state *s);
static __init void error(const char *x)
@@ -80,7 +81,7 @@ static __init void flush_window(struct gunzip_state *s)
}
crc = c;
- bytes_out += (unsigned long)s->wp;
+ s->bytes_out += (unsigned long)s->wp;
s->wp = 0;
}
@@ -113,7 +114,7 @@ __init int perform_gunzip(char *output, char *image, unsigned long image_len)
s->inbuf = (unsigned char *)image;
s->insize = image_len;
s->inptr = 0;
- bytes_out = 0;
+ s->bytes_out = 0;
makecrc();
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index f1a3b04cef8f..bec8801df487 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -1196,7 +1196,7 @@ static int __init gunzip(struct gunzip_state *s)
error("crc error");
return -1;
}
- if (orig_len != bytes_out) {
+ if (orig_len != s->bytes_out) {
error("length error");
return -1;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
` (5 preceding siblings ...)
2024-04-24 16:34 ` [PATCH v3 6/8] gzip: move output buffer " Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-25 19:23 ` Andrew Cooper
2024-04-24 16:34 ` [PATCH v3 8/8] gzip: move crc state " Daniel P. Smith
2024-04-24 16:48 ` [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
8 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 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 | 3 +++
xen/common/gzip/inflate.c | 43 ++++++++++++++++++---------------------
2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 95d924d36726..0043ff8ac886 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -17,6 +17,9 @@ struct gunzip_state {
unsigned int inptr;
unsigned long bytes_out;
+
+ unsigned long bb; /* bit buffer */
+ unsigned int bk; /* bits in bit buffer */
};
#define malloc(a) xmalloc_bytes(a)
diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
index bec8801df487..8da14880cfbe 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -211,9 +211,6 @@ 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,
@@ -553,8 +550,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 = s->wp; /* initialize window position */
/* inflate the coded data */
@@ -636,8 +633,8 @@ static int __init inflate_codes(
/* restore the globals from the locals */
s->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;
@@ -654,8 +651,8 @@ static int __init inflate_stored(struct gunzip_state *s)
DEBG("<stor");
/* make local copies of globals */
- b = bb; /* initialize bit buffer */
- k = bk;
+ b = s->bb; /* initialize bit buffer */
+ k = s->bk;
w = s->wp; /* initialize window position */
@@ -689,8 +686,8 @@ static int __init inflate_stored(struct gunzip_state *s)
/* restore the globals from the locals */
s->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;
@@ -794,8 +791,8 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
return 1;
/* make local bit buffer */
- b = bb;
- k = bk;
+ b = s->bb;
+ k = s->bk;
/* read in table lengths */
NEEDBITS(s, 5);
@@ -899,8 +896,8 @@ static int noinline __init inflate_dynamic(struct gunzip_state *s)
DEBG("dyn5 ");
/* restore the global bit buffer */
- bb = b;
- bk = k;
+ s->bb = b;
+ s->bk = k;
DEBG("dyn5a ");
@@ -965,8 +962,8 @@ static int __init inflate_block(struct gunzip_state *s, int *e)
DEBG("<blk");
/* make local bit buffer */
- b = bb;
- k = bk;
+ b = s->bb;
+ k = s->bk;
/* read in last block bit */
NEEDBITS(s, 1);
@@ -979,8 +976,8 @@ static int __init inflate_block(struct gunzip_state *s, int *e)
DUMPBITS(2);
/* restore the global bit buffer */
- bb = b;
- bk = k;
+ s->bb = b;
+ s->bk = k;
/* inflate that block type */
if (t == 2)
@@ -1004,8 +1001,8 @@ static int __init inflate(struct gunzip_state *s)
/* initialize window, bit buffer */
s->wp = 0;
- bk = 0;
- bb = 0;
+ s->bk = 0;
+ s->bb = 0;
/* decompress until the last block */
do {
@@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
/* 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;
+ while (s->bk >= 8) {
+ s->bk -= 8;
s->inptr--;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 8/8] gzip: move crc state into gunzip state
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
` (6 preceding siblings ...)
2024-04-24 16:34 ` [PATCH v3 7/8] gzip: move bitbuffer " Daniel P. Smith
@ 2024-04-24 16:34 ` Daniel P. Smith
2024-04-25 19:19 ` Andrew Cooper
2024-04-29 15:17 ` Jan Beulich
2024-04-24 16:48 ` [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
8 siblings, 2 replies; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:34 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Daniel P. Smith, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini
Move the crc and its state into struct gunzip_state. In the process, expand the
only use of CRC_VALUE as it is hides what is being compared.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
xen/common/gzip/gunzip.c | 11 +++++++----
xen/common/gzip/inflate.c | 14 +++++---------
2 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
index 0043ff8ac886..26d2a4aa9d36 100644
--- a/xen/common/gzip/gunzip.c
+++ b/xen/common/gzip/gunzip.c
@@ -20,6 +20,9 @@ struct gunzip_state {
unsigned long bb; /* bit buffer */
unsigned int bk; /* bits in bit buffer */
+
+ uint32_t crc_32_tab[256];
+ uint32_t crc;
};
#define malloc(a) xmalloc_bytes(a)
@@ -72,7 +75,7 @@ static __init void flush_window(struct gunzip_state *s)
* The window is equal to the output buffer therefore only need to
* compute the crc.
*/
- unsigned long c = crc;
+ unsigned int c = s->crc;
unsigned int n;
unsigned char *in, ch;
@@ -80,9 +83,9 @@ static __init void flush_window(struct gunzip_state *s)
for ( n = 0; n < s->wp; 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->wp;
s->wp = 0;
@@ -119,7 +122,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 8da14880cfbe..dc940e59d853 100644
--- a/xen/common/gzip/inflate.c
+++ b/xen/common/gzip/inflate.c
@@ -1032,16 +1032,12 @@ static int __init inflate(struct gunzip_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)
-
/*
* 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 gunzip_state *s)
{
/* Not copyrighted 1990 Mark Adler */
@@ -1058,7 +1054,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++)
{
@@ -1069,11 +1065,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)~0u; /* shift register contents */
}
/* gzip flag byte */
@@ -1189,7 +1185,7 @@ static int __init gunzip(struct gunzip_state *s)
orig_len |= (ulg) NEXTBYTE(s) << 24;
/* Validate decompression */
- if (orig_crc != CRC_VALUE) {
+ if (orig_crc != (s->crc ^ ~0u)) {
error("crc error");
return -1;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/8] Clean up of gzip decompressor
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
` (7 preceding siblings ...)
2024-04-24 16:34 ` [PATCH v3 8/8] gzip: move crc state " Daniel P. Smith
@ 2024-04-24 16:48 ` Daniel P. Smith
8 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 16:48 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini
On 4/24/24 12:34, 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.
>
Forgot to add this comment to the cover letter.
Concern was raised about taking updates from original source to the
code. In this case the original source is from the Linux kernel, which
can be found in lib/inflate.c, and added to Xen in 2009. The last time
there was a logic change to that code in the Linux kernel was in 2008,
before it was added to Xen. Since then, it has only been updated for
changes made to included headers. If fact, as far as I can see, while
the file is still in place, nothing uses it. For zlib decompression,
three is a new lib/zlib_deflate code base that is used. The scope of
this work is not to completely fix/replace zlib decompression for Xen,
but to stabilize it to allow hyperlaunch to decompress more than one
zlib compressed kernel.
V/r,
Daniel P. Smith
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/8] gzip: refactor the gunzip window into common state
2024-04-24 16:34 ` [PATCH v3 3/8] gzip: refactor the gunzip window into common state Daniel P. Smith
@ 2024-04-24 21:34 ` Daniel P. Smith
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Smith @ 2024-04-24 21:34 UTC (permalink / raw)
To: xen-devel
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini
On 4/24/24 12:34, Daniel P. Smith wrote:
> Begin moving core state, in this case the gunzip window, into struct
> gunzip_state to allow a per decompression instance. In doing so, drop the
> define aliasing of window to slide.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> xen/common/gzip/gunzip.c | 21 ++++++++----
> xen/common/gzip/inflate.c | 68 +++++++++++++++++++--------------------
> 2 files changed, 48 insertions(+), 41 deletions(-)
>
> diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
> index b7cadadcca8b..e47f10ae19ad 100644
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -4,10 +4,12 @@
> #include <xen/lib.h>
> #include <xen/mm.h>
>
> -static unsigned char *__initdata window;
> -
> #define WSIZE 0x80000000U
>
> +struct gunzip_state {
> + unsigned char *window;
> +};
> +
> static unsigned char *__initdata inbuf;
> static unsigned int __initdata insize;
>
> @@ -43,7 +45,7 @@ typedef unsigned long ulg;
> #endif
>
> static long __initdata bytes_out;
> -static void flush_window(void);
> +static void flush_window(struct gunzip_state *s);
>
> static __init void error(const char *x)
> {
> @@ -62,7 +64,7 @@ static __init uch get_byte(void) {
>
> #include "inflate.c"
>
> -static __init void flush_window(void)
> +static __init void flush_window(struct gunzip_state *s)
> {
> /*
> * The window is equal to the output buffer therefore only need to
> @@ -72,7 +74,7 @@ static __init void flush_window(void)
> unsigned int n;
> unsigned char *in, ch;
>
> - in = window;
> + in = s->window;
> for ( n = 0; n < outcnt; n++ )
> {
> ch = *in++;
> @@ -99,12 +101,17 @@ __init int gzip_check(char *image, unsigned long image_len)
>
> __init int perform_gunzip(char *output, char *image, unsigned long image_len)
> {
> + struct gunzip_state *s;
> int rc;
>
> if ( !gzip_check(image, image_len) )
> return 1;
>
> - window = (unsigned char *)output;
> + s = (struct gunzip_state *)malloc(sizeof(struct gunzip_state));
Looks like I inadvertently dropped the corresponding free when breaking
up the monolithic patch.
v/r,
dps
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/8] gzip: clean up comments and fix code alignment
2024-04-24 16:34 ` [PATCH v3 1/8] gzip: clean up comments and fix code alignment Daniel P. Smith
@ 2024-04-25 18:31 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-04-25 18:31 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini
On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> 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>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 8/8] gzip: move crc state into gunzip state
2024-04-24 16:34 ` [PATCH v3 8/8] gzip: move crc state " Daniel P. Smith
@ 2024-04-25 19:19 ` Andrew Cooper
2024-04-29 15:17 ` Jan Beulich
1 sibling, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-04-25 19:19 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini
On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> Move the crc and its state into struct gunzip_state. In the process, expand the
> only use of CRC_VALUE as it is hides what is being compared.
"All variables here should be uint32_t rather than unsigned long, which
halves the storage space required."
> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index 8da14880cfbe..dc940e59d853 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -1069,11 +1065,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)~0u; /* shift register contents */
The (ulg) wasn't ever necessary, and can be dropped as part of this cleanup.
Can fix on commit.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
2024-04-24 16:34 ` [PATCH v3 7/8] gzip: move bitbuffer " Daniel P. Smith
@ 2024-04-25 19:23 ` Andrew Cooper
2024-04-26 5:55 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-04-25 19:23 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: Jason Andryuk, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini
On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> diff --git a/xen/common/gzip/inflate.c b/xen/common/gzip/inflate.c
> index bec8801df487..8da14880cfbe 100644
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
> /* 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;
> + while (s->bk >= 8) {
> + s->bk -= 8;
> s->inptr--;
> }
Isn't it just me, but isn't this just:
s->inptr -= (s->bk >> 3);
s->bk &= 7;
?
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
2024-04-25 19:23 ` Andrew Cooper
@ 2024-04-26 5:55 ` Jan Beulich
2024-04-26 5:57 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-04-26 5:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: Jason Andryuk, George Dunlap, Julien Grall, Stefano Stabellini,
Daniel P. Smith, xen-devel
On 25.04.2024 21:23, Andrew Cooper wrote:
> On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
>> --- a/xen/common/gzip/inflate.c
>> +++ b/xen/common/gzip/inflate.c
>> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
>> /* 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;
>> + while (s->bk >= 8) {
>> + s->bk -= 8;
>> s->inptr--;
>> }
>
> Isn't it just me, but isn't this just:
>
> s->inptr -= (s->bk >> 3);
> s->bk &= 7;
>
> ?
I'd say yes, if only there wasn't the comment talking of just a single byte,
and even there supposedly some of the bits.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
2024-04-26 5:55 ` Jan Beulich
@ 2024-04-26 5:57 ` Jan Beulich
2024-04-26 12:36 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-04-26 5:57 UTC (permalink / raw)
To: Daniel P. Smith
Cc: Jason Andryuk, George Dunlap, Julien Grall, Stefano Stabellini,
xen-devel, Andrew Cooper
On 26.04.2024 07:55, Jan Beulich wrote:
> On 25.04.2024 21:23, Andrew Cooper wrote:
>> On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
>>> --- a/xen/common/gzip/inflate.c
>>> +++ b/xen/common/gzip/inflate.c
>>> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
>>> /* 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;
>>> + while (s->bk >= 8) {
>>> + s->bk -= 8;
>>> s->inptr--;
>>> }
>>
>> Isn't it just me, but isn't this just:
>>
>> s->inptr -= (s->bk >> 3);
>> s->bk &= 7;
>>
>> ?
>
> I'd say yes, if only there wasn't the comment talking of just a single byte,
> and even there supposedly some of the bits.
Talking of the comment: Considering what patch 1 supposedly does, how come
that isn't Xen-style (yet)?
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
2024-04-26 5:57 ` Jan Beulich
@ 2024-04-26 12:36 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-04-26 12:36 UTC (permalink / raw)
To: Jan Beulich, Daniel P. Smith
Cc: Jason Andryuk, George Dunlap, Julien Grall, Stefano Stabellini,
xen-devel
On 26/04/2024 6:57 am, Jan Beulich wrote:
> On 26.04.2024 07:55, Jan Beulich wrote:
>> On 25.04.2024 21:23, Andrew Cooper wrote:
>>> On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
>>>> --- a/xen/common/gzip/inflate.c
>>>> +++ b/xen/common/gzip/inflate.c
>>>> @@ -1017,8 +1014,8 @@ static int __init inflate(struct gunzip_state *s)
>>>> /* 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;
>>>> + while (s->bk >= 8) {
>>>> + s->bk -= 8;
>>>> s->inptr--;
>>>> }
>>> Isn't it just me, but isn't this just:
>>>
>>> s->inptr -= (s->bk >> 3);
>>> s->bk &= 7;
>>>
>>> ?
>> I'd say yes, if only there wasn't the comment talking of just a single byte,
>> and even there supposedly some of the bits.
The comments in this file leave a lot to be desired...
I'm reasonably confident they were not adjusted when a piece of
userspace code was imported into Linux.
This cannot ever have been just a byte's worth of bits, seeing as the
bit count is from 8 or more. Right now it's an unsigned long's worth of
bits.
> Talking of the comment: Considering what patch 1 supposedly does, how come
> that isn't Xen-style (yet)?
I had been collecting some minor fixes like this to fold into patch 1
when I committed the whole series.
I'll see if I can fold them elsewhere.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/8] gzip: refactor and expand macros
2024-04-24 16:34 ` [PATCH v3 2/8] gzip: refactor and expand macros Daniel P. Smith
@ 2024-04-29 14:07 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-04-29 14:07 UTC (permalink / raw)
To: Daniel P. Smith
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, xen-devel
On 24.04.2024 18:34, Daniel P. Smith wrote:
> This commit refactors macros into proper static functions. It in-place expands
> the `flush_output` macro, allowing the clear removal of the dead code
> underneath the `underrun` label.
But it's NEXTBYTE() which uses the label, not flush_output(). I'm actually
unconvinced of its expanding / removal.
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -25,8 +25,6 @@ typedef unsigned char uch;
> typedef unsigned short ush;
> typedef unsigned long ulg;
>
> -#define get_byte() (inptr < insize ? inbuf[inptr++] : fill_inbuf())
> -
> /* Diagnostic functions */
> #ifdef DEBUG
> # define Assert(cond, msg) do { if (!(cond)) error(msg); } while (0)
> @@ -52,10 +50,14 @@ static __init void error(const char *x)
> panic("%s\n", x);
> }
>
> -static __init int fill_inbuf(void)
> -{
> - error("ran out of input data");
> - return 0;
I'm not convinced of the removal of this as a separate function. It only
calling error() right now could change going forward, so I'd at least
expect a little bit of a justification.
> +static __init uch get_byte(void) {
Nit: Brace goes on its own line. Also for (effectively) new code it would
be nice if __init (and alike) would be placed "canonically", i.e. between
type and identifier.
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -119,6 +119,18 @@ static char rcsid[] = "#Id: inflate.c,v 0.14 1993/06/10 13:27:04 jloup Exp #";
>
> #endif /* !__XEN__ */
>
> +/*
> + * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
> + * stream to find repeated byte strings. This is implemented here as a
> + * circular buffer. The index is updated simply by incrementing and then
> + * ANDing with 0x7fff (32K-1).
> + *
> + * It is left to other modules to supply the 32 K area. It is assumed
> + * to be usable as if it were declared "uch slide[32768];" or as just
> + * "uch *slide;" and then malloc'ed in the latter case. The definition
> + * must be in unzip.h, included above.
Nit: s/definition/declaration/ ?
> + */
> +#define wp outcnt
> #define slide window
>
> /*
> @@ -150,21 +162,6 @@ static int inflate_dynamic(void);
> static int inflate_block(int *);
> static int inflate(void);
>
> -/*
> - * The inflate algorithm uses a sliding 32 K byte window on the uncompressed
> - * stream to find repeated byte strings. This is implemented here as a
> - * circular buffer. The index is updated simply by incrementing and then
> - * ANDing with 0x7fff (32K-1).
> - *
> - * It is left to other modules to supply the 32 K area. It is assumed
> - * to be usable as if it were declared "uch slide[32768];" or as just
> - * "uch *slide;" and then malloc'ed in the latter case. The definition
> - * must be in unzip.h, included above.
Oh, an earlier comment just moves up. Is there really a need for this
extra churn?
> @@ -224,7 +221,7 @@ static const ush mask_bits[] = {
> 0x01ff, 0x03ff, 0x07ff, 0x0fff, 0x1fff, 0x3fff, 0x7fff, 0xffff
> };
>
> -#define NEXTBYTE() ({ int v = get_byte(); if (v < 0) goto underrun; (uch)v; })
> +#define NEXTBYTE() (get_byte()) /* get_byte will panic on failure */
Nit: No need for the outer parentheses.
> @@ -1148,8 +1135,8 @@ static int __init gunzip(void)
> NEXTBYTE();
> NEXTBYTE();
>
> - (void)NEXTBYTE(); /* Ignore extra flags for the moment */
> - (void)NEXTBYTE(); /* Ignore OS type for the moment */
> + NEXTBYTE(); /* Ignore extra flags for the moment */
> + NEXTBYTE(); /* Ignore OS type for the moment */
In Misra discussions there were indications that such casts may need (re-)
introducing. Perhaps better leave this alone, the more when it's not
really fitting the patch's purpose?
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 4/8] gzip: move window pointer into gunzip state
2024-04-24 16:34 ` [PATCH v3 4/8] gzip: move window pointer into gunzip state Daniel P. Smith
@ 2024-04-29 14:08 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-04-29 14:08 UTC (permalink / raw)
To: Daniel P. Smith
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, xen-devel
On 24.04.2024 18:34, Daniel P. Smith wrote:
> Move the window pointer, outcnt/wp, into struct gunzip_data. It was erroneously
> labeled as outcnt and then define aliased to wp, this eliminates the aliasing
> and only refers to as wp, the window pointer.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 5/8] gzip: move input buffer handling into gunzip state
2024-04-24 16:34 ` [PATCH v3 5/8] gzip: move input buffer handling " Daniel P. Smith
@ 2024-04-29 15:04 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-04-29 15:04 UTC (permalink / raw)
To: Daniel P. Smith
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, xen-devel
On 24.04.2024 18:34, Daniel P. Smith wrote:
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -10,13 +10,12 @@ struct gunzip_state {
> unsigned char *window;
> /* window pointer: */
> unsigned int wp;
> -};
> -
> -static unsigned char *__initdata inbuf;
> -static unsigned int __initdata insize;
>
> -/* Index of next byte to be processed in inbuf: */
> -static unsigned int __initdata inptr;
> + unsigned char *inbuf;
> + size_t insize;
> + /* Index of next byte to be processed in inbuf: */
> + unsigned int inptr;
I'm puzzled by the (suddenly) different types, seeing that ...
> +};
>
> #define malloc(a) xmalloc_bytes(a)
> #define free(a) xfree(a)
> @@ -51,14 +50,14 @@ static __init void error(const char *x)
> panic("%s\n", x);
> }
>
> -static __init uch get_byte(void) {
> - if ( inptr >= insize )
> +static __init uch get_byte(struct gunzip_state *s) {
> + if ( s->inptr >= s->insize )
... both are compared with one another.
> @@ -1129,29 +1129,29 @@ static int __init gunzip(struct gunzip_state *s)
> error("Input has invalid flags");
> return -1;
> }
> - NEXTBYTE(); /* Get timestamp */
> - NEXTBYTE();
> - NEXTBYTE();
> - NEXTBYTE();
> + NEXTBYTE(s); /* Get timestamp */
> + NEXTBYTE(s);
> + NEXTBYTE(s);
> + NEXTBYTE(s);
>
> - NEXTBYTE(); /* Ignore extra flags for the moment */
> - NEXTBYTE(); /* Ignore OS type for the moment */
> + NEXTBYTE(s); /* Ignore extra flags for the moment */
> + 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);
Would you mind moving the body of this while() to its own line, as you
touch this anyway?
> }
>
> /* Get original file name if it was truncated */
> if ((flags & ORIG_NAME) != 0) {
> /* Discard the old name */
> - while (NEXTBYTE() != 0) /* null */ ;
> + while (NEXTBYTE(s) != 0) /* null */ ;
> }
>
> /* Discard file comment if any */
> if ((flags & COMMENT) != 0) {
> - while (NEXTBYTE() != 0) /* null */ ;
> + while (NEXTBYTE(s) != 0) /* null */ ;
> }
For these two doing the same may help, too.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 6/8] gzip: move output buffer into gunzip state
2024-04-24 16:34 ` [PATCH v3 6/8] gzip: move output buffer " Daniel P. Smith
@ 2024-04-29 15:07 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-04-29 15:07 UTC (permalink / raw)
To: Daniel P. Smith
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, xen-devel
On 24.04.2024 18:34, Daniel P. Smith wrote:
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -15,6 +15,8 @@ struct gunzip_state {
> size_t insize;
> /* Index of next byte to be processed in inbuf: */
> unsigned int inptr;
> +
> + unsigned long bytes_out;
> };
The conversion to unsigned long from ...
> @@ -42,7 +44,6 @@ typedef unsigned long ulg;
> # define Tracecv(c, x)
> #endif
>
> -static long __initdata bytes_out;
... this originally wants justifying in the (then no longer empty) description.
It's not a lot that needs saying, but such a type change cannot go entirely
silently.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 8/8] gzip: move crc state into gunzip state
2024-04-24 16:34 ` [PATCH v3 8/8] gzip: move crc state " Daniel P. Smith
2024-04-25 19:19 ` Andrew Cooper
@ 2024-04-29 15:17 ` Jan Beulich
1 sibling, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-04-29 15:17 UTC (permalink / raw)
To: Daniel P. Smith
Cc: Jason Andryuk, Andrew Cooper, George Dunlap, Julien Grall,
Stefano Stabellini, xen-devel
On 24.04.2024 18:34, Daniel P. Smith wrote:
> Move the crc and its state into struct gunzip_state. In the process, expand the
> only use of CRC_VALUE as it is hides what is being compared.
Andrew mentioned uint32_t only for the description, but I think you want
(maybe even need) to go further and actually use that type also, e.g.
> --- a/xen/common/gzip/gunzip.c
> +++ b/xen/common/gzip/gunzip.c
> @@ -20,6 +20,9 @@ struct gunzip_state {
>
> unsigned long bb; /* bit buffer */
> unsigned int bk; /* bits in bit buffer */
> +
> + uint32_t crc_32_tab[256];
> + uint32_t crc;
> };
... not just here, but also ...
> @@ -72,7 +75,7 @@ static __init void flush_window(struct gunzip_state *s)
> * The window is equal to the output buffer therefore only need to
> * compute the crc.
> */
> - unsigned long c = crc;
> + unsigned int c = s->crc;
... here.
> --- a/xen/common/gzip/inflate.c
> +++ b/xen/common/gzip/inflate.c
> @@ -1032,16 +1032,12 @@ static int __init inflate(struct gunzip_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)
Note how this is _not_ same as ~0u, also ...
> @@ -1069,11 +1065,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)~0u; /* shift register contents */
... applicable here then: sizeof(int) >= 4, not ==.
As Andrew indicates, the cast previously wasn't needed here. Keeping it is
at best misleading, when s->crc isn't of that type anymore.
Finally please stick to upper-case number suffixes; see all the related Misra
changes, which (iirc) put in place only upper-case ones.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-04-29 15:18 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 16:34 [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 1/8] gzip: clean up comments and fix code alignment Daniel P. Smith
2024-04-25 18:31 ` Andrew Cooper
2024-04-24 16:34 ` [PATCH v3 2/8] gzip: refactor and expand macros Daniel P. Smith
2024-04-29 14:07 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 3/8] gzip: refactor the gunzip window into common state Daniel P. Smith
2024-04-24 21:34 ` Daniel P. Smith
2024-04-24 16:34 ` [PATCH v3 4/8] gzip: move window pointer into gunzip state Daniel P. Smith
2024-04-29 14:08 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 5/8] gzip: move input buffer handling " Daniel P. Smith
2024-04-29 15:04 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 6/8] gzip: move output buffer " Daniel P. Smith
2024-04-29 15:07 ` Jan Beulich
2024-04-24 16:34 ` [PATCH v3 7/8] gzip: move bitbuffer " Daniel P. Smith
2024-04-25 19:23 ` Andrew Cooper
2024-04-26 5:55 ` Jan Beulich
2024-04-26 5:57 ` Jan Beulich
2024-04-26 12:36 ` Andrew Cooper
2024-04-24 16:34 ` [PATCH v3 8/8] gzip: move crc state " Daniel P. Smith
2024-04-25 19:19 ` Andrew Cooper
2024-04-29 15:17 ` Jan Beulich
2024-04-24 16:48 ` [PATCH v3 0/8] Clean up of gzip decompressor Daniel P. Smith
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).