Re: [PATCH v2] Saving agent-owned secrets for newly created connections

[Dan Williams <dcbw redhat com>]

On Thu, 2011-11-10 at 13:09 +0100, Jirka Klimes wrote:
> On Wednesday 09 of November 2011 19:12:34 Dan Williams wrote:
> > On Wed, 2011-11-09 at 17:13 +0100, Jirka Klimes wrote:
> > > Hello,
> > > 
> > > attached is a patch for sending secrets to agents when a new connection
> > > is created (AddConnection, AddAndActivateConnection).
> > > 
> > > At present, when a new connection is created, the secrets are not sent to
> > > agents and thus not saved. One has to edit the connection again. It is
> > > easily reproducible by adding a VPN connection. When it is opened again
> > > in the editor, the secrets are not there.
> > 
> > Hmm, could we do the save from pk_add_cb() and make the patch smaller?
> > That way nm_settings_connection_save_agent_secrets() could be made
> > private to nm-settings.c.  And we wouldn't have to pass the caller_uid
> > to the nm_settings_add_connection() callback, which saves some churn.
> > 
> 
> You are right. Done. 
> I wanted to save some (copy-paste) code, but due to passing caller_uid 
> didn't save much. I also didn't realize that having the stuff in pk_add_cb() 
> allows more contained code and not to pollute nm-manager.c.

Looks good, thanks!

Dan

> > Ordering might also be interesting here; since this could trigger a
> > SaveSecrets before the agent has been able to process the new
> > connection, meaning that the agent may not actually have grabbed the
> > connection yet (since that's another dbus call) when SaveSecrets comes
> > in.  I don't think that's a problem in practice for the nm-applet agent
> > but it might be for others.  Not sure if there's anything we can do
> > about it though since the ordering would be correct.
> > 
> > One thing we should probably do (later though) is not call SaveSecrets
> > at all if there aren't any secrets left to send after filtering for
> > AGENT_OWNED secrets.  Shouldn't have any real effect, but would be more
> > "correct".
> > 
> > Dan