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

[Thomas Lamprecht]

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.

> 
> 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.


>>> +# 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).