[pve-devel] [RFC common] fix misleading warning caused by Devel::Cycle

[Stoiko Ivanov]

On Fri, 22 Nov 2019 19:05:44 +0100
Thomas Lamprecht <t.lamprecht at proxmox.com> wrote:

> On 11/22/19 7:03 PM, Thomas Lamprecht wrote:
> > On 11/22/19 6:21 PM, Stoiko Ivanov wrote:  
> >> When validating the parameters (or return values) of an API call against the
> >> schema we run the parameter hash through Devel::Cycle::find_cycle.
> >>
> >> find_cycle has no handler for GLOB types (e.g. filehandles).
> >> Recently we introduced passing a filehandle as return attribute in
> >> PMG::API2::Quarantine::download
> >>
> >> When find_cycle encounters such an object it emits a warning:
> >> `Unhandled type: GLOB at /usr/share/perl5/Devel/Cycle.pm line 109`
> >> which ends up in the journal of pmgproxy (in that example).
> >>
> >> Since the line might mislead someone to think that this is a more serious bug
> >> we silence it, by overriding the warnhandler ($SIG{'__WARN__'}, [0,1]) for the
> >> time when find_cycle runs, to ignore messages starting with
> >> 'Unhandled type: GLOB at'. The particular location of and in in Devel::Cycle
> >> might change and is left out of the match.
> >>
> >> [0] man perlvar
> >> [1] perldoc -f warn
> >>
> >> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> >> ---
> >>
> >> sending as RFC, since the comment above the patch already indicate that we
> >> could do away with the cycle-search on every call  
> > 
> > I'd really rather to that, this is a build check, and thus we
> > do not want to do it on every validation - besides those errors
> > you ran into, it's also a bit costly..
> > 
> > I'd check for an environment variable, e.g. for "DEB_BUILD_ARCH",
> > which we always have set when building. As some may want to have
> > validation also in other cases we could also check a opt-in one,
> > for example:
> > 
> > return if $ENV{DEB_BUILD_ARCH} && !$ENV{PROXMOX_FORCE_SCHEMA_VALIDATION};  
> 
> argh, missed the ! from the $ENV{DEB_BUILD_ARCH}, could be written as
> 
> # only check when building a package or told to do so
> return if !($ENV{DEB_BUILD_ARCH} || $ENV{PROXMOX_FORCE_SCHEMA_VALIDATION});

Sounds sensible to me - will send a v2 with your proposal - Thanks!