Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
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
Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
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
Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
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
Re: [PATCH v3 7/8] gzip: move bitbuffer into gunzip state
On 24/04/2024 5:34 pm, Daniel P. Smith wrote:
> Signed-off-by: Daniel P. Smith
Acked-by: Andrew Cooper
> 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
