[pve-devel] [RFC PATCH 1/2] do not use PVE::API2 in spiceproxy.pm

[Fabian Grünbichler]

On Wed, Aug 09, 2017 at 06:17:20PM +0200, Dietmar Maurer wrote:
> Looks quite strange to me, because PVE::HTTPServer calls:
> 
>  PVE::API2->find_handler
> 
> so we should use PVE::API2 in PVE::HTTPServer?
> 
> Not sure why it works without??
> 

it does, but only in rest_handler, which AFAICT, is not called in the
spiceproxy case:

PVE::APIServer::AnyEvent has:

new which calls
accept_connections which calls
push_request_header which calls
unshift_read_header which after being done with HTTP header processing,
checks for the spiceproxy flag (set in spiceproxy.pm), and calls
verify_spice_connect_url and handle_spice_proxy_request and returns.

verify_spice_connect_url is implemented in PVE::HTTPServer, and only
uses PVE::RPCEnvironment and PVE::AccessControl

handle_spice_proxy_request is implemented in PVE::APIServer::AnyEvent,
and AFAICT does not use the API in any way.

unshift_read_header also calls both
file_upload_multipart and handle_request, which call
handle_api2_request which calls the problematic
rest_handler

but those calls in unshift_read_header happen after the spiceproxy check
mentioned above, so those paths are never taken.

so while it looks safe to remove "use PVE::API2" from spiceproxy.pm,
technically it is missing from PVE::HTTPServer.pm and the memory usage
would still be the same.

we could move PVE::API2 into the server config / $self, and let only
pveproxy.pm and pvedaemon.pm (which both have the appropriate use
statement) set the config option?

alternatively, spiceproxy.pm could use a new SpiceProxyHTTPServer, which
does not implement rest_handler and auth_handler and thus does not need
to use PVE::API2. PVE::HTTPServer could then use PVE::API2 without
affecting spiceproxy's memory usage. pveproxy, pvedaemon and
pvespiceproxy could all clean up their use statements to reflect what is
actually directly used in the respective perl file.


> > On August 9, 2017 at 2:08 PM Dominik Csapak <d.csapak at proxmox.com> wrote:
> > 
> > 
> > we do not need it there and withouth this we save ~30MB memory for
> > this daemon and its workers
> > 
> > Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> > ---
> >  PVE/Service/spiceproxy.pm | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/PVE/Service/spiceproxy.pm b/PVE/Service/spiceproxy.pm
> > index 20fd5b24..22a501b0 100755
> > --- a/PVE/Service/spiceproxy.pm
> > +++ b/PVE/Service/spiceproxy.pm
> > @@ -10,7 +10,6 @@ use warnings;
> >  use PVE::SafeSyslog;
> >  use PVE::Daemon;
> >  use PVE::API2Tools;
> > -use PVE::API2;
> >  use PVE::HTTPServer;
> >  
> >  use base qw(PVE::Daemon);
> > -- 
> > 2.11.0
> > 
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel at pve.proxmox.com
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel