On Tue, Aug 27, 2013 at 11:18:23AM +0200, Jakub Hrozek wrote:
On Mon, Aug 26, 2013 at 04:34:14PM +0200, Sumit Bose wrote:
> On Thu, Aug 22, 2013 at 11:50:27AM +0200, Jakub Hrozek wrote:
> > On Mon, Aug 19, 2013 at 08:14:03AM -0400, Simo Sorce wrote:
> > > On Mon, 2013-08-19 at 11:33 +0200, Sumit Bose wrote:
> > > > On Sun, Aug 18, 2013 at 10:26:46PM +0200, Jakub Hrozek wrote:
> > > > > There seems to be a logic bug in sysdb_master_domain_add_info().
We
> > > > > first update the domain data with sysdb search results and then
update
> > > > > sysdb. I think it should be the other way around.
> > > >
> > > > iirc the idea was to read the sysdb entry first to make sure we have
the
> > > > latest data from sysdb. Because of the asynchronous nature of the
> > > > request it might be possible that another request already updated
the
> > > > sysdb entry. To avoid unneeded writes we try to compare the new data
> > > > with the latest data we know. This is led by the assumption that
> > > > reading from sysdb is a lot cheaper then writing to it.
> > >
> > > How often does this operation happen ?
> > > Simo.
> >
> > Never until now because it didn't work :-) But if there's a cheap way
to
> > optimize, then why not. I think the code should just make it clear
> > what's happening.
> >
> > Attached is a new patch that explains why we call the domain update also
> > before the sysdb update and adds another domain update after the sysdb
> > update to make sure the sss_domain_info structure has the latest data.
>
>
> After a bit more thinking I think the first version of the patch is the
> better one. The main reason is that a change in the value will only
> happen once for an IPA server and never for AD. When running
> ipa-adtrust-install the IPA domain will get its SID and flat name and a
> running SSSD will detect the change and update his structures. Since it
> is a rare event I think it's acceptable to let parallel running task to
> update the domain entry twice because in the general case the call to
> sysdb_master_domain_update() is not needed because the server data and
> the data in the struct will be the same.
OK. I don't really mind either way but attached is the first version
again.
>
> Additionally both callers set the realm to NULL, i.e. it is not updated
> at all because it is derived from the configuration for the master
> domains. Would you mind to just remove this parameter?
Yes, that's in a separate patch so that the changes can be seen
logically.
ACK
bye,
Sumit