Re: [PATCH 7/7] util: Make client-side polkit work even with polkit disabled

On Mon, Nov 22, 2021 at 10:07:39AM +0000, Daniel P. Berrangé wrote: > On Mon, Nov 22, 2021 at 11:03:53AM +0100, Martin Kletzander wrote: > > On Mon, Nov 22, 2021 at 09:23:01AM +0000, Daniel P. Berrangé wrote: > > > On Sun, Nov 21, 2021 at 12:10:08AM +0100, Martin Kletzander wrote: > > > > The reason for this is twofold: > > > > > > > > - the polkit build option is documented for UNIX socket access checks > > > > > > > > - there is no server-side change or dbus call done when enabling this as it only > > > > starts a polkit agent on the client-side (actually only in virsh) and does not > > > > need any requirements (starting is skipped if pkttyagent is not installed) > > > > > > > > Also move the conditional implementation to the bottom of the file so that it > > > > does not look like the whole file is build conditionally and the common > > > > functions are at the top. > > > > > > Does this still work correctly on Windows if we try by default ? > > > > > > > Any call to virPolkitAgentAvailable() should return false on Windows > > unless PKTTYAGENT (/usr/bin/pkttyagent) exists. While thinking about it > > now I will change that virFileExists() call to virFileIsExecutable() to > > catch even more possible issues. Anyway since that should return false > > on Windows (and I hope my presumptions are correct) then > > virPolkitAgentCreate() should report an error, just like it would > > without this patch if connecting to polkit-guarded libvirtd socket > > (e.g. through ssh). It should actually return a better error with this > > patch applied. > > > > And ctermid() is a POSIX function, not sure what that returns on > > windows, but it should not even get there as the first check is done > > against the existence/executability of PKTTYAGENT. > > ctermid() doesn't exist in Windows, so it shouldn't even compile if we > try to build with it ! A conditional willbe needed I expect. > Yeah, with my limited (and mostly forgotten Windows experience) I was too much reliant on our builds which all have allow_failure: true for mingw builds. Sorry for that, I'll fix that up.