Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config

[Dominik Csapak <d.csapak@proxmox.com>]

On 6/6/23 12:45, Thomas Lamprecht wrote:
> Am 06/06/2023 um 11:41 schrieb Dominik Csapak:
>>>>    +my $untaint_recursive;
>>>
>>> I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
>>> inside the body alive forever, don't we need a weaken?
>>>
>>> E.g., like we had to do in PVE::Status::Graphite's assemble.
>>
>> mhmm isn't that because there we use variables from outside the
>> function? here we only use the parameters themselves
> 
> I'm not 100% sure about the details, but since then, seeing something like
> this pattern triggers my cycle instincts, I'd like to have that checked out
> closely.
> 

sure

>>
>> anyway the solution there is to set the sub to undef after use, but
>> we can do that here only if we move the sub into the regular
>> function
>>
>> i can also make it a proper sub if that's better?
>>
> 
> 
> 
>> how can i test for these things properly?
> 
> easiest: pass large hashes and loop, then see if memory goes up. For metrics
> back then you could see the RSS of pbvestatd grow by ~ the metric data size
> every update.
> 
> 
> What I also used back then, IIRC, was the Devel::Cycle module, it should give
> you a more specific answer if there's a cycle, but naturally has no idea what
> the practical implications are.

i wrote a small program where i copied the function to and did basically:

---8<---
use Storable qw(dclone);

my $normalize;
$normalize = sub {...};

my $data = /* create large hash here, with nested data */;

while(1) {
     my $newdata = dclone($data);
     $newdata = $normalize->($newdata);
}
--->8---


executed it and monitored the rss usage, while letting it run for multiple minutes
the memory usage increased after the initial creation of the hash, and the first
dclone, but not after.

is that a sufficient test?

> 
>>>> +# convert arrays to strings where we expect a '-list' format and convert scalar
>>>> +# values to arrays when we expect an array (because of www-form-urlencoded)
>>>> +#
>>>> +# only on the top level, since www-form-urlencoded cannot be nested anyway
>>>> +#
>>>> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
>>>> +# behaviour deprecated with 9.x, and remove it with 10.x
>>>> +my $convert_params = sub { my ($param, $schema) = @_;
>>>
>>> please keep the method paramethers on it's own line.
>>
>> oops, one shift+j to many without noticing^^
>>
>>>
>>> Also, maybe go for a more telling names, as convert_params could mean everytrhing
>>> and nothing ^^
>>>
>>
>> sure, any suggestions? 😉
> 
> sure, lets start with what this actually does in more explicit steps:
> 
> 1) normalize_form_data
> still a bit general but we now know that it handles form data and normalizes it to a
> single representation
> 
> 
> 2) normalize_param_list_to_array
> A bit more telling about what happens.
> 
> 
> 3) convert_legacy_list_format_to_array
> very telling, but as this is a internal helper, and thus not used elsewhere, that
> wouldn't hurt, having "legacy" in it underlines that we want to drop it sometimes.
> 
> Personally, I'd favor 3) or 2).

mhmm both are ok for me, but it also does the reverse
(convert an array to a legacy list so to speak)

maybe i'd use something like 'normalize_legacy_param_formats' ?
it's a bit more general than 2,3 but still has legacy in its name...