On Tue, Jul 30, 2019 at 11:32:22AM +0200, Christophe de Dinechin wrote:
>
> Daniel P. Berrangé writes:
>
> > When opening a connection to a second driver inside the daemon, we must
> > ensure the identity of the current user is passed across. This allows
> > the second daemon to perform access control checks against the real end
> > users, instead of against the libvirt daemon that's proxying across the
> > API calls.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > src/libvirt_remote.syms | 1 +
> > src/remote/remote_daemon_dispatch.c | 110 +++++++++++++++++++++++++---
> > src/remote/remote_driver.c | 1 +
> > src/remote/remote_protocol.x | 18 ++++-
> > src/remote_protocol-structs | 8 ++
> > src/rpc/virnetserverclient.c | 12 +++
> > src/rpc/virnetserverclient.h | 2 +
> > 7 files changed, 140 insertions(+), 12 deletions(-)
> >
> > +static int
> > +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
> > + virNetServerClientPtr client,
> > + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> > + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
>
> Why ATTRIBUTE_UNUSED? Seems used in the cleanup?
copy+paste mistake
> > + remote_connect_set_identity_args *args)
> > +{
> > + virTypedParameterPtr params = NULL;
> > + int nparams = 0;
> > + int rv = -1;
> > + virConnectPtr conn = remoteGetHypervisorConn(client);
> > + virIdentityPtr ident = NULL;
>
> (Trying to learn about coding style and conventions)
> Why is this not autounref here? Is there a convention that if you
> have explicit cleanup, you don't autounref?
autounref is our preferred modern style. I'm just not in the habit
well enough, especially when copying existnig code.
>
> > + if (!conn)
> > + goto cleanup;
> > +
> > + VIR_DEBUG("Received forwarded identity");
> > + if (virTypedParamsDeserialize((virTypedParameterRemotePtr) args->params.params_val,
> > + args->params.params_len,
> > + REMOTE_CONNECT_IDENTITY_PARAMS_MAX,
> > + ¶ms,
> > + &nparams) < 0)
> > + goto cleanup;
>
> Would it be useful to change the value rv over these cases,
> and if rv < 0, add a VIR_DEBUG with its value? Or is there
> sufficient debugging info from the individual calls already?
By convention in libvirt the return values are usually only ever
-1 or 0. We have only a few places with return '-errno' in the
code. So we don't need to report the return value most of the
time.
>
> > +
> > + VIR_TYPED_PARAMS_DEBUG(params, nparams);
> > +
> > + if (virConnectSetIdentityEnsureACL(conn) < 0)
> > + goto cleanup;
> > +
> > + if (!(ident = virIdentityNew()))
> > + goto cleanup;
> > +
> > + if (virIdentitySetParameters(ident, params, nparams) < 0)
> > + goto cleanup;
> > +
> > + virNetServerClientSetIdentity(client, ident);
> > +
> > + rv = 0;
> > +
> > + cleanup:
> > + virTypedParamsFree(params, nparams);
> > + virObjectUnref(ident);
> > + if (rv < 0)
> > + virNetMessageSaveError(rerr);
> > + return rv;
> > +}
> > +
> > +
> > +
> > static int
> > remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED,
> > virNetServerClientPtr client,
> > diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> > index a42b4a9671..05229f00c5 100644
> > --- a/src/remote_protocol-structs
> > +++ b/src/remote_protocol-structs
> > @@ -3105,6 +3105,13 @@ struct remote_domain_checkpoint_delete_args {
> > remote_nonnull_domain_checkpoint checkpoint;
> > u_int flags;
> > };
> > +struct remote_connect_set_identity_args {
> > + struct {
> > + u_int params_len;
> > + remote_typed_param * params_val;
>
> Indent by 8 spaces and try to align variables in the same file?
> Nothing good could come out of it ;-)
This particular file content has to match the auto-generated
output from the 'pdwtags' command. It is basically a sanity
check to catch people who mistakenly change something in the
remote protocol which would break ABI.
>
> > + } params;
> > + u_int flags;
> > +};
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
