On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1945,10 +1946,15 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
> static int
> remoteOpenConn(const char *uri,
> bool readonly,
> + bool preserveIdentity,
> virConnectPtr *conn)
> {
[...]
> if (!*conn)
> - return -1;
> + goto error;
> VIR_DEBUG("Opened driver %p", *conn);
> +
> + if (preserveIdentity) {
> + if (virConnectSetIdentity(*conn, params, nparams, 0) < 0)
> + goto error;
> +
> + virTypedParamsFree(params, nparams);
> + VIR_DEBUG("Forwarded current identity to secondary driver");
> + }
> +
> return 0;
> +
> + error:
> + virTypedParamsFree(params, nparams);
> + if (*conn) {
> + virConnectClose(*conn);
> + *conn = NULL;
> + }
> + return -1;
Here I would go for the tried and true
virTypedParameterPtr params = NULL;
int nparams = 0;
int ret = -1;
if (operationFailed)
goto error;
ret = 0;
cleanup:
virTypedParamsFree(params, nparams);
return ret;
error:
if (*conn) {
virConnectClose(*conn);
*conn = NULL;
}
goto cleanup;
Less repetition, and more difficult to get wrong even as other people
come in and make changes :)
[...]
> @@ -1992,6 +2025,7 @@ remoteGetInterfaceConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->interfaceURI,
> priv->readonly,
> + true,
> &priv->interfaceConn) < 0)
So when opening secondary drivers, we'll always attempt to preserve
the user identity...
[...]
> @@ -2264,14 +2304,16 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
> #endif
>
> +#ifdef VIRTPROXYD
> + preserveIdentity = true;
> +#endif /* VIRTPROXYD */
... and we'll do the same when forwarding a connection to hypervisor
daemons through virtproxyd.
Makes sense to me, I'm just not seeing the check ensuring we're
running as the same user that you hinted to in patch 43...
Is it enough to consider virtproxyd trusted for the hypervisor
drivers, and in turn the hypervisor drivers trusted for the secondary
drivers, as we seem to be doing here?
> +static int
> +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
> + remote_connect_set_identity_args *args)
As Christophe already pointed out, rerr is actually used.
> +{
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> + int rv = -1;
Usually we call this 'ret'.
> + virConnectPtr conn = remoteGetHypervisorConn(client);
> + virIdentityPtr ident = NULL;
Again as pointed out by Christophe, this can be VIR_AUTOUNREF()d.
> +++ b/src/remote/remote_protocol.x
> +struct remote_connect_set_identity_args {
> + remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>;
Pretty sure you want to use REMOTE_CONNECT_IDENTITY_PARAMS_MAX
here.
> @@ -6538,7 +6546,7 @@ enum remote_procedure {
> */
> REMOTE_PROC_NETWORK_PORT_DELETE = 410,
>
> - /**
> + /**
Unrelated indentation change, but one that's probably not deserving
of its own patch...
Anyway, while the changes overall look good, there are still a few
open questions that I hope you'll address. It also doesn't work at
all for me, at least in legacy mode on a Fedora 30 machine where I
installed the result of 'make rpm' on top of the distro packages:
$ sudo systemctl start libvirtd
Job for libvirtd.service failed because the control process exited with error code.
See "systemctl status libvirtd.service" and "journalctl -xe" for details.
$ sudo journalctl -b 0 -u libvirtd
...
Jul 30 20:06:10 kinshicho systemd[1]: Failed to start Virtualization daemon.
Jul 30 23:24:39 kinshicho systemd[1]: Starting Virtualization daemon...
Jul 30 23:24:40 kinshicho systemd[1]: libvirtd.service: Main process exited, code=exited, status=6/NOTCONFIGURED
Jul 30 23:24:40 kinshicho systemd[1]: libvirtd.service: Failed with result 'exit-code'.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
