Hi,
On 1.10.2012 19:54, Pavel Březina wrote:
On 10/01/2012 10:23 AM, Jan Cholasta wrote:
> Hi,
>
> Dne 24.9.2012 15:56, Jan Cholasta napsal(a):
>> Dne 24.9.2012 15:03, Pavel Březina napsal(a):
>>> On 09/19/2012 12:09 PM, Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> this patch set changes the way the known_hosts file is updated so that
>>>> only entries for hosts that were requested recently (in the last 5
>>>> minutes) are written to the file. There is no need to keep older
>>>> entries
>>>> in the file, as host keys are needed only before the connection to the
>>>> host is established (which usually takes just a few seconds).
>>>>
>>>> The individual patches are:
>>>>
>>>> [PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
>>>
>>> Nack.
>>> Can you make new copy of attrs without the deleted attributes
>>> instead of
>>> touching the original attrs?
>>
>> Sure.
>
> It turns out I don't need this function anymore, so this patch is
> removed.
>
>>
>>>
>>>> [PATCH 2/3] SSH: Refactor sysdb code
>>>
>>> Nack.
>>>
>>> sysdb_update_ssh_host(), sysdb_store_ssh_host():
>>> Is there any advantage to allow the input attrs parameter to be NULL?
>>> Currently there is no code flow how this could happened (except sysdb
>>> test) so I would simply prohibit it and make the code simpler.
>>
>> I believe that was a copy & paste from some other sysdb function, you
>> are probably right that it can be safely removed.
>
> Done.
>
>>
>>>
>>> sysdb_update_ssh_host():
>>> What is the purpose of deleting OC and NAME attributes and then adding
>>> them again?
>>
>> ldb complains when the same value is present more than once in a single
>> attribute (such as name) and refuses to store the entry.
>
> I have taken a slightly different approach here: instead of deleting
> unwanted attributed in sysdb functions, I copy only host keys from the
> LDAP result in hostid provider (the rest of attributes is not
> interesting from SSH perspective anyway).
>
>>
>>>
>>>> [PATCH 3/3] SSH: Expire hosts in known_hosts
>>>
>>> Ack.
>
> Added a new configuration option ssh_known_hosts_timeout, which can be
> used to specify the timeout of known_hosts entries in seconds. Also
> added a sysdb upgrade routine so that this patch works for existing
> installs as well.
Nack (nitpick).
sysdb_ssh.c:125 - you have "if (alias)" inside "if (alias)". Can you
put
a comment there that would say that the value may change during
execution? Either that or you can use a boolean variable that would
indicate present of the alias in attrs. Just make sure that it is clear
from the first sight...
Sure.
Otherwise ack. Good job!
Thanks.
Updated & rebased patches attached.
Honza
--
Jan Cholasta