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

[Wolfgang Bumiller]

On Thu, Aug 10, 2017 at 09:34:00AM +0200, Fabian Grünbichler wrote:
> 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.

In any case, `use` statements belong into the files which actually use
them, which in this case means the patch is valid. Further fixups can
follow later on.

Applied.