On 08/29/2014 10:41 AM, Jan Cholasta wrote:
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.
Thanks,
+ <varlistentry condition="with_ssh">
+ <term>
+ <option>-h</option>,<option>--ssh-host</option>
+ <replaceable>autofs-map</replaceable>
^^^^^
copy&paste mistake?
+ </term>
+ <listitem>
+ <para>
+ Invalidate SSH public keys of a specific host.
+ </para>
+ </listitem>
+ </varlistentry>
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel