| Message ID | 20240424163422.23276-8-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up of gzip decompressor | expand |
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
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
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
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
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--; }
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(-)