[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ * master: 4a311702045b065a97a0c0fc0ccc7a1fc84b38cf 85517b57685809ff96818bbd3e3b4678ac74b461 85a93ca67ae020607006cd035170c9360fb0a450 684a13e8de1526257ca2e40b6bf2e05585d4eaca 4b37ee7d37000351

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ ACK, CI: http://vm-058-233.XXX/logs/job/74/33/summary.html """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-325391191

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration olivergs commented: """ Tested the latest build for the patches from @fidencio COPR repo and it worked as expected. """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-325337915 __

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration olivergs commented: """ Tested the latest build for the patches from @fidencio COPR repo and it worked as expected. On Thu, Aug 17, 2017 at 4:23 PM, fidencio wrote: > Patch set updated. > > This is the patch

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration pbrezina commented: """ On 08/22/2017 01:54 PM, fidencio wrote: > @pbrezina : patchset has been updated. > Thanks for your suggestions! Ack. """ See the full comment at https://gi

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @pbrezina: patchset has been updated. Thanks for your suggestions! """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-324003707 ___

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration pbrezina commented: """ Typo in "related": ```xml + +Default: id_provider is used if it +is set and can perform session reltted tasks

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ On Mon, Aug 21, 2017 at 12:39 PM, Pavel Březina wrote: > I think it is better to set the method only when needed. Try this diff > instead of your patch: > > --- a/src/providers/data_provi

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration pbrezina commented: """ I think it is better to set the method only when needed. Try this diff instead of your patch: ```c --- a/src/providers/data_provider/dp_target_auth.c +++ b/src/providers/data_provider/dp_

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @pbrezina: So, here's the patch that solves the issue: ``` From ae60eae181c7a3214d76b3ff00d9e431f060bbc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Fri, 18 Au

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @jhrozek: @pbrezina told me (on #sssd) to test with `ipa_enable_deskprofile = false`. Here's the result: ``` (Fri Aug 18 10:19:45 2017) [sssd[be[ipa.example]]] [dp_pam_handler] (0x0100):

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ On Fri, Aug 18, 2017 at 09:26:14AM +, Pavel Březina wrote: > I just briefly read the code, haven't tried it. > > ```c > @@ -126,10 +126,9 @@ static void choose_target(struct data_provid

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration pbrezina commented: """ I just briefly read the code, haven't tried it. ```c @@ -126,10 +126,9 @@ static void choose_target(struct data_provider *provider, name = "PAM Chpass 2nd"; break

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @jhrozek, I've updated the patch set including **DESKPROFILE: Add ipa_deskprofile_request_interval**, which is a fix for https://pagure.io/SSSD/sssd/issue/3482 """ See the full comment

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @jhrozek, I've updated the patch set including **DESKPROFILE: Add ipa_deskprofile_request_interval**, which fix for https://pagure.io/SSSD/sssd/issue/3482 """ See the full comment at h

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ Thank you, code-wise ACK. I'm not going to add the accepted label so that the patches don't get pushed by accident until we hear from the desktop group about results of their testing. Than

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ Patch set updated. This is the patch that I've squashed: ``` From 97f189d3a4fcbbfe453cc6ba8eab8710ca31c4ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Thu, 17

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ Patch set updated. This is the patch that I've squashed: ``` From 97f189d3a4fcbbfe453cc6ba8eab8710ca31c4ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= Date: Thu, 17

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ On Thu, Aug 17, 2017 at 04:25:07AM -0700, Jakub Hrozek wrote: > Thank you for the updates. I think only the `permuts` dereference should be > changed to happen after the check. > > In the

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ Thank you for the updates. I think only the `permuts` dereference should be changed to happen after the check. In the meantime, I've started CI, Coverity and downstream HBAC tests. """ Se

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ I have updated the patch set and, hopefully, all your comments have been addressed. For a few comments I ended up taking the path I believe is the right one. Let me know whether you need

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ btw the downstream regression test passed (job ID 2005455) """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322566071

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ Since most of the comments were minor issues with little impact on functionality, I went ahead and scheduled regression runs. """ See the full comment at https://github.com/SSSD/sssd/pull

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ Well, I'm struggling to see how would `sss_create_dir` return -1. All control paths either return a directly assigned return value (ENOMEM) or errno. """ See the full comment at https://

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ Hmm. That's actually a bad idea and I'm struggling to see a better solution for this. Suggestions are welcome. """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecom

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @jhrozek, about the coverity warnings, I guess the safest path to take is just use int instead of errno_t in that function where the warning happend (and, of course, change the whole chai

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ retest this please """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322233302 ___ sssd-devel mailing list -- sssd-

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ I think to fix the warnings, you just need to `errno=ret` before printing the DEBUG message """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-322233270 __

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ There are still some Coverity warnings: ``` Error: NEGATIVE_RETURNS (CWE-394): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:232: negative_return_fn: Function "sss_create_dir("

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @jhrozek, a new version of the patchset has been pushed and hopefully all the comments have been addressed. A few more patches were introduced in this new patchset. """ See the full comm

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ I think the error condition is not really as much of a code issue as a user-friendliness issue. Admins of older IPA servers would suddendly see an odd message saying that something failed.

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ @jhrozek, about the issue when connecting to old server that has no support for the desktop profiles ... In that case we'd always get a reply could as 0. Would be okay, for you, to just

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ sorry, I hit comment too soon. I think we want to handle that more gracefully and not throw an error. By the way, I'm still not finished with the review of the last patch (the previous on

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ With an old server that has no support for the desktop profiles, I'm getting: ``` (Tue Aug 1 10:19:17 2017) [sssd[be[ipa.test]]] [ipa_deskprofile_get_config_done] (0x0040): Unexpected numb

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ In general, to get rid of the TOCTOU issues, the key is using the `*at` and `*fd` family of functions - you might open the top level directory to get a fd and then instead of `stat` call `

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration jhrozek commented: """ There are some issues Coverity found: ``` Error: TOCTOU (CWE-367): sssd-1.15.4/src/providers/ipa/ipa_deskprofile_rules_util.c:157: fs_check_call: Calling function "stat" to perform check on

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ I've updated the patch set and the changes are: - as @jhrozek agreed on the approach proposed by FleetCommander guys, the 2 "fixup" patches got squashed into "DESKPROFILE: Introduce the ne

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ Here [0] you can find some step-by-step on how to test the functionality of those patches. It's important to mention that @olivergs has been doing some functional tests on those patches,

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ CI: http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/7173/ Failures do not seem to be related to this patch set. """ See the full comment at https://github.com/SSSD/sssd/pull/241#i

[SSSD] [sssd PR#241][comment] FleetCommander Integration

URL: https://github.com/SSSD/sssd/pull/241 Title: #241: FleetCommander Integration fidencio commented: """ I've updated the patch set accordingly to https://pagure.io/SSSD/sssd/issue/2995#comment-447913 """ See the full comment at https://github.com/SSSD/sssd/pull/241#issuecomment-314018566 _