From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3DBE49DEF4 for ; Tue, 6 Jun 2023 13:19:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1C1B933E1F for ; Tue, 6 Jun 2023 13:19:24 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 6 Jun 2023 13:19:23 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 33CDD48BFC; Tue, 6 Jun 2023 13:19:23 +0200 (CEST) Message-ID: <461caa2a-e5ac-a2c9-bbd9-f0ec38e976cf@proxmox.com> Date: Tue, 6 Jun 2023 13:19:22 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20230606083914.1400960-1-d.csapak@proxmox.com> <20230606083914.1400960-2-d.csapak@proxmox.com> <7f0da808-115b-6f31-2cf2-3bd3f0e7e27b@proxmox.com> <3f88f726-9d89-3026-2a2c-4b3e9dbda7db@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <3f88f726-9d89-3026-2a2c-4b3e9dbda7db@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.015 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Jun 2023 11:19:24 -0000 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...