Dne 2.9.2014 v 16:37 Lukas Slebodnik napsal(a):

On (02/09/14 16:21), Pavel Reichl wrote:

On 09/01/2014 02:27 PM, Pavel Reichl wrote:

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?

Fixed.

+                </term>
+                <listitem>
+                    <para>
+                        Invalidate SSH public keys of a specific host.
+                    </para>
+                </listitem>
+            </varlistentry>

Could you also add ticket number (#2358) to the commit message? Thanks!

I wasn't aware there is a ticket for this.

Honza,
we have a template in git repo ".git-commit-template"

For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc

OK, thanks.

LS

Updated patch attached.

_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel