Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
> Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
>>
>> On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
>>> On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
>>>> On 18.7.2014 06:41, William wrote:
>>>>>>> Ignore that last patch, I messed up and didn't include a
.h file.
>>>>>>> Here
>>>>>>> is the fixed patch.
>>>>>> This new patch seems to be based on some older version of the
>>>>>> patch, it
>>>>>> uses wrong option name, sysdb_update_ssh_host still has the
confdb
>>>>>> argument, etc.
>>>>>>
>>>>> Fixed, and fixed the documentation into the xml file, removed the
>>>>> references to the .pot file.
>>>> I don't see any of the requested fixes in the patch.
>>>>
>>>> See attachment for fixed patch. I also did a few additional tweaks to
>>>> make
>>>> the patch work when SSSD is updated from a previous version and when
>>>> building without SSH bits.
>>> Awesome thank you, I guess some of us need to review the patch now
>>> you're also an author.
>> While Jan was so kind to give me some hints on how to test patch
>> properly, we found out a little bug. So Jan is going to prepare an
>> updated patch.
>>
>> I'll do the review then.
>
> I can't reproduce the issue, maybe something else was wrong in your
> setup?
>
> For the record, I installed IPA in a VM and then ran these commands on
> it:
>
> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
> [no errors]
> $ cat /var/lib/sss/pubconf/known_hosts
> [not empty]
> # systemctl stop ipa
> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
> [no errors]
> $ cat /var/lib/sss/pubconf/known_hosts
> [not empty]
> # sss_cache -H
> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
> [no errors]
> $ cat /var/lib/sss/pubconf/known_hosts
> [empty]
> # systemctl start ipa
> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
> [no errors]
> $ cat /var/lib/sss/pubconf/known_hosts
> [not empty]
>
> Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step,
> I have it non-empty as expected.
>
> (sorry for the delay, I was on leave)
>
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least
separate the man page changes to separate patch - so we can ask some
native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages,
could you document them?
Please see some minor comments inline:
Fixed everything, except:
> + ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs,
mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it
(other sysdb_set_*_attr functions do this as well).
Updated patch attached.
--
Jan Cholasta