On 12/04/2024 1:51 pm, Daniel P. Smith wrote:
> On 4/12/24 08:18, Andrew Cooper wrote:
>> On 12/04/2024 12:41 pm, Daniel P. Smith wrote:
>>> On 4/12/24 07:34, Daniel P. Smith wrote:
>>>> On 4/11/24 15: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.
>>>>
>>>> I take it you are mainly talking about crc_32_tab? Yes, I can switch
>>>> that to being dynamically allocated.
>>>
>>> Never mind, reading your comment on patch4 made me realize you wanted
>>> the instance of struct dynamically allocated. Though the answer is
>>> still, yes, we can dynamically allocate it.
>>
>> I wrote this before realising that crc_32_tag could be shrunk.
>>
>> If it's only1k on the stack, then that's a whole lot less bad, and is
>> perhaps ok. I guess it depends on the stack size of the other
>> architectures.
>>
>> Still - I expect dynamically allocating would be a safer course of
>> action. Internal blocks are dynamically allocated already, so this is
>> "just" one more.
>
> Another course of action that could be considered is making a unit
> file global instance of the struct, and then memset() it to zero
> instead of allocating and freeing from heap. The global instance
> should be able to be made init_data and dropped after init was complete.
>
> I am good with either way, just let me know which would be preferred
> and I will adjust appropriately.
Other things inside gzunip() are dynamically allocated. I'd keep this
consistent with the others.
~Andrew
