[sssd PR#844][opened] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mastersin]

URL: https://github.com/SSSD/sssd/pull/844
Author: mastersin
 Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails
Action: opened

PR body:
"""
Ignore AD GPO errors:
- if SecEdit/GptTmpl.inf is missing
or
- if reading sysvol_gpt_version fails and cached_gpt_version already exists
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/844/head:pr844
git checkout pr844

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[centos-ci]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

centos-ci commented:
"""
Can one of the admins verify this patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-507788021

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

jhrozek commented:
"""
ok to test
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-507789307

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mastersin]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

mastersin commented:
"""
> Can one of the admins verify this patch?

This patches tested on on complex samba stands.
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-507789359

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

jhrozek commented:
"""
Thank you for the PR @mastersin

I will assign the PR to @mzidek-rh for review, because he knows the GPO code the best of the SSSD team, but unfortunately he just begins his summer vacation today, so I guess the review will be a bit delayed..
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-507789815

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

pbrezina commented:
"""
@mzidek-rh Can you review this PR please?
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-527064059

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

pbrezina commented:
"""
```diff
+static errno_t
+gpo_cache_remove_file(const char *smb_path,
+                      const char *smb_cse_suffix)
...
+    ret = unlink(filename);
+    if (ret == -1) {
+        if (ENOENT == errno) {
+            ret = EOK;
+        } else {
+            ret = errno;
+            DEBUG(SSSDBG_CRIT_FAILURE, "failed to unlink %s\n", filename);
+            goto done;
+        }
+    }
```

I'd rather see something like:
```c
  if (ret != 0) {
    if (errno != ENOENT) {
      ret = errno;
      DEBUG(SSSDBG_CRIT_FAILURE, "failed to unlink %s [%s]: %s\n", filename, ret, sss_strerror(ret);
      goto done;
    }
  }

ret = EOK;

done:
...
```

Please, keep `if` on one line or include parentheses:
```diff
@@ -702,7 +702,7 @@ perform_smb_operations(int cached_gpt_version,
 {
...
- done:
+    if (_sysvol_gpt_version >= 0)
+        ret = EOK;
+
     smbc_free_context(smbc_ctx, 0);
     return ret;
 }
```

Otherwise it looks good code-wise. @mzidek-rh can you please review it function-wise? Thank you.
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-532597034

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mzidek-rh]

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,

doing the review now. There are some issues in this patch. I will comment here soon.
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-533020512

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mzidek-rh]

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

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mzidek-rh]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

mzidek-rh commented:
"""
Ok, I see I can not count to 5 and need to reuse number 3 twice, but I hope the message is understandable. Please ask if something is not clear.
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-533626526

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mzidek-rh]

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.
    }
}
```
- The last patch should be dismissed IMO (unless there is a reason for it to exists in which case it needs to be explained better in the commit message. like what is broken whthout it)
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-533626152

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mzidek-rh]

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 @mastersin .

Do you plan to update the patches? If you do not have time, I can update the patches myself, but I would still want you to test it in your Samba setup otherwise I will not be 100% sure if it works for you.

An important question for me is, what does not work for you without the second patch. There were problems in the patch and the patch was not doing what it advertised (because of the bad dereference), but maybe it was still enough to work around some issue. I would like to know what the issue was.

Thanks,
Michal
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-542610716

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mastersin]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

mastersin commented:
"""
Hello, @mzidek-rh.

If you could update patches, I will ready test them asap. Thank you.
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-542630052

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

alexey-tikhonov commented:
"""
> Hello, @mzidek-rh.
>
> If you could update patches, I will ready test them asap. Thank you.

What's the status here?

I guess Michal isn't going to update those patches.
And afaiu, his [question](https://github.com/SSSD/sssd/pull/844#issuecomment-542610716) wasn't answered.
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-658818233

[sssd PR#844][synchronized] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mastersin]

URL: https://github.com/SSSD/sssd/pull/844
Author: mastersin
 Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/844/head:pr844
git checkout pr844

[sssd PR#844][closed] sssd-ad and gpo_child: GPO apply fixes during reading fails

[mastersin]

URL: https://github.com/SSSD/sssd/pull/844
Author: mastersin
 Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/844/head:pr844
git checkout pr844

[sssd PR#844][comment] sssd-ad and gpo_child: GPO apply fixes during reading fails

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/844
Title: #844: sssd-ad and gpo_child: GPO apply fixes during reading fails

sumit-bose commented:
"""
Hi @mastersin,

I wonder if you can consider to revive this pull-request or open a new one with your patches. They have very helpful fixes for using SSSD with a Samba DC. I still have them archived but I would prefer if you can resend them.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/844#issuecomment-891776869