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...
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
