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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel