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.
>From b303e25a4a891aebb61e04e97b40dd61c7191f01 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Sat, 17 Aug 2013 09:12:03 +0200 Subject: [PATCH] DB: Update sss_domain_info with new updated data The code didn't reload the sss_domain_info structure in memory after changing the domain data on disk, which caused the SSSD to keep using stale data. Also adds a comment that explains why we call the update twice. --- src/db/sysdb_subdomains.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index 58df470182ea6ecbb5b48e288ab6a3d3bcec18ba..e6eab46f320f04d50097dd5d1bfc3141e6b29be9 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -264,6 +264,11 @@ errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, return ENOMEM; } + /* First, update the domain object to make sure we're using the same + * data as the data on disk. Then we might find out that just the + * structure in memory was outdated and won't need the write + * to cache at all + */ ret = sysdb_master_domain_update(domain); if (ret != EOK) { goto done; @@ -350,6 +355,14 @@ errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, goto done; } + /* Because the contents of domain on disk changed, we need to refresh + * the structure as well + */ + ret = sysdb_master_domain_update(domain); + if (ret != EOK) { + goto done; + } + ret = EOK; done: -- 1.8.3.1
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
