3:15 a.m.
URL: https://github.com/SSSD/sssd/pull/106
Author: jhrozek
Title: #106: Add a new "files" provider
Action: opened
PR body:
"""
This patch set implements a new provider that mirrors the contents of passwd and groups
files. The intent is to make these users and groups available through the SSSD memory
cache to improve performance and make it possible to read extended attributes via the sssd
D-Bus interface.
As the next step, we will implement a writable D-Bus interface to make it possible to also
manage local users with an API and supersede the AccountService API.
At the moment, the domain must be enabled explicitly. When this branch is merged, another
commit would also enable the files domain for all installations by default.
In order to make the resolution precise, the files domain is disabled once an inotify
notification arrives During testing, I realized there might be a delay between changing
the UNIX files by replacing them and *receiving* the inotify notification. Therefore the
tests add a sleep as well. That's not nice and I would be glad if the reviewer can
spot how to speed up the inotify notification receiving.
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/106/head:pr106
git checkout pr106
9:02 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
One suggestion came from simo about using FGETPWENT(3) instead of opening the NSS module
directly.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-265761268
6:51 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
I will try to do some review in this week.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-266423740
8:24 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
On Mon, Dec 12, 2016 at 04:51:31AM -0800, Pavel Březina wrote:
I will try to do some review in this week.
Thank you, but please note that I will try to implement Simo's
suggestion to use the fgetpwent family of functions. This would allow to
simplify the code a bit, at least in the sense that the test module
wouldn't have to be built at all.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-266443179
5:13 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
I though it will be a small change, but if it is going to affect the code more widely,
I'll wait.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-266711979
3:47 a.m.
New subject: [sssd PR#106][+Changes requested] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: +Changes requested
3:47 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
I just set the Changes Requested label so that it's clear to reviewers new patch set
is coming up..
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-267285088
2:18 p.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
lslebodn commented:
"""
I hope you noticed that this PR need to be rebased (due to refactoring of nss responder)
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-268347295
2:54 p.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
I hope you noticed the earlier comment that says "I just set the Changes Requested
label so that it's clear to reviewers new patch set is coming up.."
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-268356995
9:03 a.m.
New subject: [sssd PR#106][synchronized] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Author: jhrozek
Title: #106: Add a new "files" provider
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/106/head:pr106
git checkout pr106
9:43 a.m.
New subject: [sssd PR#106][synchronized] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Author: jhrozek
Title: #106: Add a new "files" provider
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/106/head:pr106
git checkout pr106
9:44 a.m.
New subject: [sssd PR#106][-Changes requested] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: -Changes requested
9:44 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
OK, ready for review, @pbrezina
There is still a commit that generates the default configuration. We already have a PR
#108 that reverts this functionality -- I guess it would be best to provide a
configure-time option or even a runtime option that would disable the files domain.
Otherwise, the files domain would be always enabled and always first.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-269988218
8:35 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
lslebodn commented:
"""
I had some inline comments to the python tests. And do not forget to run `pep8
src/tests/intg/*.py` after changes.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-270127000
9:26 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
lslebodn commented:
"""
I slowly started to fix python3 issues in integration tests. There are mostly issues with
bytes/unicode.
Here is a patch which fixes issues in your patches
https://paste.fedoraproject.org/519378/45686414/
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-270139369
5:17 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
So far it looks good and I have only very few comments. I didn't read the code
thoroughly yet.
*NSS: Skip disabled domains during requests*
Rename to cache_req: please, because the change is there and not in nss responder.
*RESPONDER: A sbus interface to reset negatively cached users*
*DP: Add internal DP interface to enable and disable domains*
I will look how much work would it be to implement signals. I believe most of the work has
been done already so if we can finish it rather quickly, we should do it right.
*CONFDB: Make pwfield configurable per-domain*
Can you move 'nss_get_pwfield' into nss_util.c? It doesn't really have
anything common with protocol.
*CONFDB: The files domain defaults to "x" as pwfield*
Are we also able to authenticate with pam_sss without pam_unix?
*FILES: Add the files provider*
You say that a domain is disabled during enumeration and we fall back to nss files. Do you
expect the update to take a really long time? Wouldn't it be better to jus wait until
the enumeration is done?
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271260823
9:36 a.m.
New subject: [sssd PR#106][+Changes requested] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: +Changes requested
10 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
lslebodn commented:
"""
On (09/01/17 07:44), Jakub Hrozek wrote:
jhrozek commented on this pull request.
> + contents = self._read_contents()
+ kindex = self._get_named_line(key, contents)
+ contents.pop(kindex)
+ self._write_contents(contents)
+
+ contents = self._read_contents()
+
+
+class PasswdOps(FilesOps):
+ """
+ A naive implementation of user operations
+ """
+ def __init__(self, file_name):
+ super(PasswdOps, self).__init__(file_name)
+
+ def _pwd2line(self, name, uid, gid, passwd, gecos, dir, shell):
I changed the dir name in pwd2line, but not in useradd or userdel. The reason I kept them
the same is that the user dictionaries expect `dir` as well and using the user
dictionaries can be nice as:
```
passwd_ops_setup.useradd(**user1)
ent.assert_passwd_by_name("user1", user1)
```
That will work if you change the constant user1 (dir -> dictionary)
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271324043
10:51 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
Yes, but as I said in the comment (in the part you quoted out), assert_passwd_by_name
won't work, because it expects dir and uses dir in its control directory:
```
Traceback (most recent call last):
File "/home/remote/jhrozek/devel/sssd/src/tests/intg/test_files_ops.py", line
55, in test_userdel
ent.assert_passwd_by_name("user1", USER1)
File "/home/remote/jhrozek/devel/sssd/src/tests/intg/ent.py", line 216, in
assert_passwd_by_name
d = _diff(ent, pattern)
File "/home/remote/jhrozek/devel/sssd/src/tests/intg/ent.py", line 111, in
_diff
d = _diff(ent[key], value, item_map)
KeyError: 'directory'
```
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271338295
10:52 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
so the solution would be to convert ent.py to not use dir internally either and I'm
fine doing that if you think it would help
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271338530
2:18 p.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
On Mon, Jan 09, 2017 at 03:17:24AM -0800, Pavel Březina wrote:
So far it looks good and I have only very few comments. I didn't
read the code thoroughly yet.
*NSS: Skip disabled domains during requests*
Rename to cache_req: please, because the change is there and not in nss responder.
Done.
*RESPONDER: A sbus interface to reset negatively cached users*
*DP: Add internal DP interface to enable and disable domains*
I will look how much work would it be to implement signals. I believe most of the work
has been done already so if we can finish it rather quickly, we should do it right.
Thank you
*CONFDB: Make pwfield configurable per-domain*
Can you move 'nss_get_pwfield' into nss_util.c? It doesn't really have
anything common with protocol.
That's what I tried to do initially, but nss_util.c doesn't have access
to struct nss_ctx. I wasn't sure if it makes sense to include
nss_private.h into nss_util.c. I'm fine both ways, but the current
version of the patch tried to not include more headers than we already
do.
*CONFDB: The files domain defaults to "x" as pwfield*
Are we also able to authenticate with pam_sss without pam_unix?
There is no auth_provider=files, but it should be possible to use
auth_provider=proxy configured with pam_unix.
*FILES: Add the files provider*
You say that a domain is disabled during enumeration and we fall back to nss files. Do
you expect the update to take a really long time? Wouldn't it be better to jus wait
until the enumeration is done?
I was thinking about this for some time and it seemed safer to me to
fall back. But just when I was thinking about this again today, I
realized that at least the InfoPipe interface has nowhere to fall back
to, so the behaviour must either differ on the cache_req level between
the nss responder and the ifp responder or we should wait until the
domain updates in both cases.
Maybe we could even do something in-between, but I really wonder if it
is an optimization or over-engineering:
- when a domain is disabled, attach a request and wait
- when a timeout passes, return a 'not found' error
- the responder would be able to configure the timeout (not the
user, this is really too low level)
- the nss responder would select something quite small (half a
second?) just to make sure we don't delay lookups too much and the
ifp responder would select several second
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271395720
2:18 p.m.
New subject: [sssd PR#106][synchronized] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Author: jhrozek
Title: #106: Add a new "files" provider
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/106/head:pr106
git checkout pr106
2:19 p.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
All the python-related comments should be fixed and pep8 shouldn't report any more
errors.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271395952
6:58 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
**UTIL: Add a generic inotify module**
```c
if ((debug_level & SSSDBG_TRACE_LIBS) == false) {
return;
}
^ use DEBUG_IS_SET(SSSDBG_TRACE_LIBS)
```
**D-Bus Interfaces:**
I would like to see the interfaces like this. It reads better to me as a command.
```xml
<interface name="org.freedesktop.sssd.Responder.Domain">
<annotation value="iface_responder_domain"
name="org.freedesktop.DBus.GLib.CSymbol"/>
<method name="EnableDomain">
<arg name="name" type="s" direction="in"
/>
</method>
<method name="DisableDomain">
<arg name="name" type="s" direction="in"
/>
</method>
</interface>
<interface name="org.freedesktop.sssd.Responder.NegativeCache">
<annotation value="iface_responder_ncache"
name="org.freedesktop.DBus.GLib.CSymbol"/>
<method name="ResetUsers" />
<method name="ResetGroups" />
</interface>
```
**DP: Add internal DP interface to enable and disable domains**
**DP: Add internal interface to reset negative cache from DP**
You can use `sbus_create_message()`
**FILES: Add the files provider**
```c
static struct dp_reply_std
files_account_info_handler(struct dp_id_data *data,
struct sss_domain_info *domain)
```
Change it to `errno_t files_check_account_input(struct dp_id_data *data)` or move it into
`files_account_info_handler_send()` please. It is not necessary to move creation of reply
further into the code.
I didn't know what `sf_` prefix stands for (sssd files), I believe keeping `files_`
prefix as in the names of sources files and structures and other functions will read
better.
`delete_all_users/groups()` -> you can use `sysdb_delete_recursive()`
**CONFDB: The files provider always enumerates**
We do not have to support enumeration just because we do enumeration internally. We can
make it default, yes. But do not force it.
**CONFDB: Make pwfield configurable per-domain**
`nss_get_pwfield()` move to `nss_util.c`. You can include nss_private.h there for sure.
> You say that a domain is disabled during enumeration and we fall
back to nss files. Do you expect the update to take a really long time? Wouldn't it be
better to jus wait until the enumeration is done?
I was thinking about this for some time and it seemed safer to me to
fall back. But just when I was thinking about this again today, I
realized that at least the InfoPipe interface has nowhere to fall back
to, so the behaviour must either differ on the cache_req level between
the nss responder and the ifp responder or we should wait until the
domain updates in both cases.
Do not special case. Currently users are waiting several seconds to get remote identities
so I think an occasional little delay with local users is acceptable (but we need to block
client until enumeration is finished so we always return accurate information). Keep in
mind that:
* the change will be still pretty fast on most systems since sssd users do not usually
have large `passwd` and `groups`
* unless it is a kickstart the change in files will be triggered by user input and there
will be a delay before the user will provide another command
If there will be pushback we can try to optimize it later but I would start simple.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-271569020
6:05 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
@lslebodn Asked me to address my comments in separate patches. I wrote patches for most of
the above expect changing the `sf_` prefix and removing logic that disables domain during
enumeration. This is something I want to agree upon with @jhrozek first.
I do not know if there is a way to add my patches to this pull request besides pushing to
Jakub's branch (which I can't, of course), so you can look at them here:
https://github.com/pbrezina/sssd/commits/files
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-274057277
2:01 p.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
On Fri, Jan 20, 2017 at 04:05:47AM -0800, Pavel Březina wrote:
@lslebodn Asked me to address my comments in separate patches. I
wrote patches for most of the above expect changing the `sf_` prefix and removing logic
that disables domain during enumeration. This is something I want to agree upon with
@jhrozek first.
Thank you very much, I will try to merge the commits in the next couple
of days or so.
btw I was thinking about the disabled domains as well. I wonder if the
best way to go is to let the cache_req queue the requests for the
disabled domain and then after a timeout, just return 'not found'.
The files provider serves two purposes -- the NSS responder's part is more or
less just about the fast cache. There we obviously care about performance,
because /it's the only added benefit of exposing files users through
NSS/. But the potentially more interesting part is that by exposing the
files users through IFP, we can call all the SSSD D-Bus methods, which
might be interesting for cases like smart cart logins through
pam_sss.so
So with that in mind, maybe it would make sense to set the queue
interval for the NSS responder to zero, which would return immediatelly
when the domain was disabled and some kind of short timeout (a couple of
seconds) otherwise. Then we would retain the fast-failing-to-nss-files
for the NSS provider, but the only thing that would differ would be the
default timeout value between the two responders.
I do not know if there is a way to add my patches to this pull request besides pushing to
Jakub's branch (which I can't, of course), so you can look at them here:
https://github.com/pbrezina/sssd/commits/files
I /think/ you could just try pushing the patches to /my/ branch. The PR
has a box that says 'Allow edits from maintainers' ticked.
But I'm fine with this approach as well, it allows me to check the
patches before squashing them.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-274165999
4:49 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
On Mon, Jan 23, 2017 at 03:20:47AM -0800, Pavel Březina wrote:
On 01/20/2017 09:01 PM, Jakub Hrozek wrote:
> On Fri, Jan 20, 2017 at 04:05:47AM -0800, Pavel Březina wrote:
>> @lslebodn Asked me to address my comments in separate patches. I wrote
> patches for most of the above expect changing the `sf_` prefix and
> removing logic that disables domain during enumeration. This is
> something I want to agree upon with @jhrozek first.
>
> Thank you very much, I will try to merge the commits in the next couple
> of days or so.
>
> btw I was thinking about the disabled domains as well. I wonder if the
> best way to go is to let the cache_req queue the requests for the
> disabled domain and then after a timeout, just return 'not found'.
I want to avoid implementing any sort of queue inside cache_req. At this
moment we do not contact data provider for files. Would it be big
performance lost if we do it the other way around, i.e. always contact
data provider?
The idea would be:
1) Contact data provider.
2) DP would no if there is any ongoing update
yes) Wait until the update is finished and return (queue is already
implemented)
no) Return immediately.
In 2) We can check if the caller is nss and if yes we can immediately
return with a specific return code to indicate that passwd should be
asked directly.
I need to measure how much time there is spent in the IPC. Maybe it
would be doable as repeated requests would be answered from the memory
cache anyway.
This would be the easiest and cleanest approach in my opinion.
If the performance lost will be significant, we can change it to call
data provider only on the update process.
OK, in this case I would prefer not to use 'disabled' as the state that
the provider sets but add a new state, maybe 'dirty' or 'inconsistent'
and amend cache_req to send a DP update when there are requests against a
domain in this state. I still think we need some short timeout (couple
of seconds at max) to protect against issues in the DP update that would
block any NSS request, though.
If we see later that there are issues with update, we can either improve
the performance of the back end update or just fall through.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-274770015
3:47 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
Yes. Inconsistent sounds good.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-275063633
10:37 a.m.
New subject: [sssd PR#106][synchronized] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Author: jhrozek
Title: #106: Add a new "files" provider
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/106/head:pr106
git checkout pr106
4:49 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
With some help from Pavel, I fixed a memory issue we had in the sbus code. I think the
patches are now ready for review. One thing I will submit separately is automatic
enablement of the files provider -- I think it's OK to submit separately as in the
meantime anyone interested can define the provider on their own and I still need to think
it if would be better to drop a files provider snippet downstream or unconditionally
enable the files domain in the code.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279353106
4:49 a.m.
New subject: [sssd PR#106][-Changes requested] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: -Changes requested
5:40 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
CI: http://sssd-ci.duckdns.org/logs/job/62/54/summary.html
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279363621
6:52 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
Hi, those changes looks good. I have just one comment -- what happens if both groups and
passwd are being updated? I see that both will trigger files_account_info_finished(), this
looks like a dangerous race condition.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279382232
9:01 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
I hope this wouldn't be an issue because the inotify code doesn't call the
callbacks immediately but with a tevent timer (by default immediate tevent action) which
IIRC waits until the next 'tick' of the event loop and therefore serializes the
callbacks. In the first version, I even had a single `updating` variable instead of the
two for passwd and group. But I agree having two variables but having them both finish a
single request makes little sense.
So maybe it would be more defensive to add something like:
```
if (id_ctx->updating_passwd == true || id_ctx->updating_groups== true) {
/* Another concurrent callback is still updating the db, don't finish the request
yet */
return;
}
```
to files_account_info_finished. Do you think that would help?
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279415804
4:50 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
I hope this wouldn't be an issue because the inotify code
doesn't call the callbacks immediately but with a tevent timer (by default immediate
tevent action) which IIRC waits until the next 'tick' of the event loop and
therefore serializes the callbacks.
Hmm, we actually don't need the request at all since the update is synchronous:
```
inotify:
run callback
pwupdate:
update := true
update()
update := false
finish request(ctx->request)
```
Therefore all `D-Bus` messages that comes during cache update are queued until the update
is finished and only then are messages read from the bus and processed. Thus having
ctx->request non null should not be posssible.
However, there should be two update states and two requests -- one for users (DP_USER) and
one for groups (DP_GROUP) to stay completely clean and safe.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279674481
8:03 a.m.
New subject: [sssd PR#106][+Changes requested] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: +Changes requested
3:55 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
Hopefully these patches implement a safer method of waiting for the update. I tested the
code by modifying the update to start a tevent timer event and then, before the event
fired, I issued several look ups.
I also changed some of the DP related patches so that they compile on their own.
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279966260
3:55 a.m.
New subject: [sssd PR#106][-Changes requested] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: -Changes requested
4:26 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
CI: http://sssd-ci.duckdns.org/logs/job/62/60/summary.html
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279973347
4:39 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
pbrezina commented:
"""
Ack, thank you. Good job :-)
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-279976416
7:56 a.m.
New subject: [sssd PR#106][closed] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Author: jhrozek
Title: #106: Add a new "files" provider
Action: closed
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/106/head:pr106
git checkout pr106
7:56 a.m.
New subject: [sssd PR#106][comment] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
jhrozek commented:
"""
Thank you for the careful review. I pushed the whole lot into master:
0e7047c1533e5e424b28959488e8ffa91613abd9
89e53f7139b134360fda9e43d47ebfb89fcaac92
da95ec568a941c85982e30611398efb86bc884ab
ee6c7e8b589497119ec1ee40e99611f362111600
f9f1310ba1b87223f8d4d935b30b8238e5c00022
8bdb8c0970dc9acb5b0a54dab0bae306ca964944
3728db53ac32da51fcaae96b132e8e56ebbaebfa
8578fba1500d43ad9632784462c255bf8bb360fe
1921d739ff7b028baa591272cc8969e330c8f872
4e17c050dac8f2c6e2d278c4c4a27001c8d7d164
26577ac05dc33c201c15164bcf19f45393beeca5
ece2ac6889da2b58f5d4027ec0a1d97992056f66
c778c36c5170c2b9f1cf7a6e3b0811124534df03
a60e6ec802cd2858dc85eabd442cff16fb23618f
c71e0a6710418991d759a329b8dcb77c7ad3e16e
90a103d6050b266fd8fc8fd0636be32de5885dec
8cfb42e1985550e99585d311f68087d414932806
50c740cbc2bb27cbe488fa8587e2901b8b85cf87
26866484a985adbc7edf2e79a1e95b3bb6b8624c
2c61b6eee24d90b11f3d2cab7b9cd8690df29f34
5007103e82f34e64a0ff3b278797b9fa42ba1dda
af28fa659f7ffcd12ecf8bda64e79cf5dd225651
b3ee4be9e1794fa823696d70d4958f3b0269939c
205a0b9e9234327730fa808be95b2e1db7ffee95
c109f063b4469818fd335b8b509f0458e7b33b0a
2d1a59f6c2cf3cf4667cf2d01b2d780db916db42
f2047f6c5b56d6759bd8e6d504f572a593476c65
c3a225d4d735d3a01883125592dda7a030a64e00
99a32e4f5164e174d5a3ffa5a1fe622075a8fe45
"""
See the full comment at https://github.com/SSSD/sssd/pull/106#issuecomment-280016765
7:56 a.m.
New subject: [sssd PR#106][+Pushed] Add a new "files" provider
URL: https://github.com/SSSD/sssd/pull/106
Title: #106: Add a new "files" provider
Label: +Pushed
2629
days inactive
2698
days old
sssd-devel@lists.fedorahosted.org
42 comments
3 participants
participants (3)
-
fidencio -
jhrozek
-
pbrezina