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

Thomas Lamprecht t.lamprecht at proxmox.com
Tue Jun 6 12:45:57 CEST 2023

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

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

More information about the pve-devel mailing list