Re: [PATCH 3/5] gzip: refactor state tracking

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2024 11:13, Daniel P. Smith wrote:
> On 4/18/24 03:36, Jan Beulich wrote:
>> On 11.04.2024 21:24, Andrew Cooper wrote:
>>> On 11/04/2024 4:25 pm, Daniel P. Smith wrote:
>>>> diff --git a/xen/common/gzip/gunzip.c b/xen/common/gzip/gunzip.c
>>>> index 1bcb007395ba..9b4891731b8b 100644
>>>> --- a/xen/common/gzip/gunzip.c
>>>> +++ b/xen/common/gzip/gunzip.c
>>>> @@ -102,12 +109,13 @@ __init int gzip_check(char *image, unsigned long 
>>>> image_len)
>>>>   
>>>>   __init int perform_gunzip(char *output, char *image, unsigned long 
>>>> image_len)
>>>>   {
>>>> +    struct gzip_data gd;
>>>>       int rc;
>>>
>>> By the end of this series,
>>>
>>> Reading symbols from xen-syms...
>>> (gdb) p sizeof(struct gzip_data)
>>> $1 = 2120
>>>
>>> x86 has an 8k stack and this takes 1/4 of it.  Other bits of state are
>>> dynamically allocated, even in inflate.c, so I'd highly recommend doing
>>> the same for this.
>>>
>>>
>>> Also, could I nitpick the name and request:
>>>
>>> struct gzip_state *s;
>>
>> Except: Why "gzip" when it's un-zipping state?
> 
> Gzip is the name of the algo/suite for which the code is moved under, 
> and in typical fashion its structures are named after the feature they 
> belong. Still, I went and looked at the other algos. I found two that 
> have state tracking and yes, they do use the operation for the struct 
> name and not the algo/feature under which they reside. If you want this 
> yak shaved, I have no vested interest one way or another, I just need 
> the decompressor to be re-entrant.

Well. Generally speaking compressor and decompressor may need different
state to track. As we have seen with tmem, there may be reasons why a
compressor may also be needed in Xen. Hence unless it is known for sure
that either no need will ever appear for gzip, or gzip's compression
and decompression states are identical, I'd prefer the struct name to
reflect the specific purpose.

Jan