URL: https://github.com/SSSD/sssd/pull/909 Author: alexey-tikhonov Title: #909: sssctl/sssctl_domains.c: null dereference fixed Action: opened
PR body: """ Resolves: https://pagure.io/SSSD/sssd/issue/4102 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/909/head:pr909 git checkout pr909
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
mzidek-rh commented: """ I plan to review this today or tomorrow. """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-544868176
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
alexey-tikhonov commented: """ I have re-ran CI to see if Debian error ("sssd-ci/debian10 — Timeout reached") is persistent.
"Timeout reached" is something new. Last test in the log is "test_infopipe.py" and this test calls sssctl. OTOH, this test only executes "domain-list" (PR didn't touch this part of the code) """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-544927417
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
mzidek-rh commented: """ Output with domain with active servers:
``` # sssctl domain-status ldap.vm Online status: Online
Active servers: LDAP: master.ldap.vm
Discovered LDAP servers: - master.ldap.vm ```
Without the patch (with id_provider = files) ``` [root@master.client.vm ~]# sssctl domain-status files Online status: Online
Active servers: Segmentation fault (core dumped) ```
With the patch: ``` # sssctl domain-status files Online status: Online
Domain has no active services ```
So, it does work, but I do not like the message "Domain has no active services". It would be better to use "servers" rather then "services" because the command should list active servers. IMO output like this would be better:
``` # sssctl domain-status files Online status: Online
This domain has no active servers.
No servers discovered.
# sssctl domain-status files ```
``` # sssctl domain-status -a files This domain has no active servers.
```
``` # sssctl domain-status -r files No discovered servers. ``` """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545059200
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
alexey-tikhonov commented: """ Hi @mzidek-rh,
Thanks for hint about `-a` and `-r` options. Now I see it really makes sense to print something in both cases.
But I am not so sure about replacing "service" with "server"... "Service" is a "primary thing" here. `sssctl` first requests list of "services" and then "[active] server[s]" for each "service" obtained. In "LDAP: master.ldap.vm" and "Discovered LDAP servers:" 'LDAP' stands for "service.
It still might happen (services != NULL) but (server[s] == NULL) and I think it would be correct to distinguish this case and case when (services == NULL). For a case of (server[s]==NULL) there were messages in place already: "Active servers: not connected" and "Discovered %s servers: None so far.\n" Message I added is about case when (services==NULL). That's why it says "Domain has no active services"
But I do not have strong preference here. Please, let me know if my awkward explanation makes any difference. If you still keep the same stance then ok, I will follow with your proposal. """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545141124
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
mzidek-rh commented: """ Hi @alexey-tikhonov
See the domain-status help and -a and -r options:
``` # sssctl domain-status --help Usage: sssctl domain-status DOMAIN [OPTIONS...]
Command options: -o, --online Show online status -a, --active-server Show information about active server -r, --servers Show list of discovered servers -s, --start Start SSSD if it is not running
Help options: -?, --help Show this help message --usage Display brief usage message ```
Maybe I miss something, but it all seem to be revolving around "servers". Yes we do print it in the service: server format in the list of discovered servers and it makes sense to iterate over the services in the outer loop in the code, but I still thing the main thing are the servers. """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545502736
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
mzidek-rh commented: """ Hi @alexey-tikhonov
See the domain-status help and -a and -r options:
``` # sssctl domain-status --help Usage: sssctl domain-status DOMAIN [OPTIONS...]
Command options: -o, --online Show online status -a, --active-server Show information about active server -r, --servers Show list of discovered servers -s, --start Start SSSD if it is not running
Help options: -?, --help Show this help message --usage Display brief usage message ```
Maybe I miss something, but it all seem to be revolving around "servers". Yes we do print it in the service: server format in the list of discovered servers and it makes sense to iterate over the services in the outer loop in the code, but I still think the main thing are the servers. """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545502736
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
alexey-tikhonov commented: """ Well... I could argue that
-a, --active-server Show information about active server -r, --servers Show list of discovered servers
it is rather "... per service" is missing at the end of those ^^ lines :)
But ok, let's go the way you propose.
"""
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545527330
URL: https://github.com/SSSD/sssd/pull/909 Author: alexey-tikhonov Title: #909: sssctl/sssctl_domains.c: null dereference fixed Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/909/head:pr909 git checkout pr909
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
alexey-tikhonov commented: """ @mzidek-rh, please, see updated patch. """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545534236
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
mzidek-rh commented: """ ACK. """
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-545650306
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
pbrezina commented: """ * `master` * 75b1fe68433a5b6276fe5da38e004b863710f335 - sssctl/sssctl_domains.c: null dereference fixed
"""
See the full comment at https://github.com/SSSD/sssd/pull/909#issuecomment-548335157
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/909 Title: #909: sssctl/sssctl_domains.c: null dereference fixed
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/909 Author: alexey-tikhonov Title: #909: sssctl/sssctl_domains.c: null dereference fixed Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/909/head:pr909 git checkout pr909
sssd-devel@lists.fedorahosted.org
-
alexey-tikhonov -
mzidek-rh
-
pbrezina