URL:
https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails
mzidek-rh commented:
"""
Hello,
1. so, first about the patch: ad: gpo: ignore GPO if SecEdit/GptTmpl.inf is missing
The part that deals with the case sensitivity and the smbc file close would be better to
put in a separate patch.
2. The whole unlinking of the cached file is IMO wrong. If Samba creates GPOs that do not
have them, then there should be nothing to remove anyway and if the file is cached and
SSSD can not fetch the newer version of it (note that we alredy know there is newer
version because we have read the INI file and the version there is bigger then the cached
one) we should IMO write a loud debug message about it or even put message into a syslog
(in the case the file does not exist a SSSDBG_TRACE_FUNC message would be nice as well
IMO).
I tested modified version of the patches that do not remove the cached file and it worked
for me. It is even better from debugging point of view to not remove the cached file
because it can contain important information.
3. The second patch is a bit baffling for me. Could we get a bit more context as to why it
is needed? The commit message only describes the change in code made, but not the
purpose.
```
- int sysvol_gpt_version;
+ int sysvol_gpt_version = cached_gpt_version;
```
Why is it initialized with cached_gpt_version? The value will be overwritten anyway in the
```ret = ad_gpo_parse_ini_file(smb_path, &sysvol_gpt_version);``` If the point was to
make the sysvol_gpt_version have a sane value even if the ad_gpo_parse_ini_file fails,
then it needs to be dealt with after the function is called and return value checked, but
I am not sure if that was the reason.
```
- done:
+ if (_sysvol_gpt_version >= 0)
^^^^^^^^^ This is a pointer, not the actual value. I think you missed
'*' before the _sysvol_gpt_version
+ ret = EOK;
+
```
3. In addition to the coding style issues that Pavel mentioned I spotted Yoda conditioning
:)
(in the first patch)
```
+ if (optional && ENOENT == ret) {
^^^^ should be 'ret == ENOENT'
```
4. Summary:
- the case sensitivity and smbc_getFunctionClose(smbc_ctx)(smbc_ctx, file); are fine, but
better have them in separated patch to not distract from the main issue
Other then that we should just:
```
if (GptTmpl file is missing on the server) {
if (we have cached version) {
write a syslog warning or at least a MINOR_FAILURE debug message
} else {
write not so loud debug message and skip the GPO.
}
}
```
"""
See the full comment at
https://github.com/SSSD/sssd/pull/844#issuecomment-533626152