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