I'm sorry, I didn't find enough continuous time over the weekend to do a full
review of big patches..
On Sun, Feb 26, 2012 at 08:39:24PM -0500, Stephen Gallagher wrote:
> On Sun, 2012-02-26 at 21:12 +0100, Jan Cholasta wrote:
> > Hi,
> >
> > I have updated and rebased the patches on top of current master.
> >
> > On 24.2.2012 20:35, Stephen Gallagher wrote:
> > > On Fri, 2012-02-24 at 19:10 +0100, Jan Cholasta wrote:
> > >> Hi,
> > >>
> > >> this patchset contains these patches:
> > >>
> > >> [PATCH 1/3] SSH: Save SSH host name aliases
> > >>
> > >> This is needed in order to properly generate the known_hosts file when
> > >> user uses ssh with non-FQDN host name.
> > >>
> > >
> > > I fixed up a few minor issues with this patch. The check for
> > > in_transaction should not have been conditional on ret (in the current
> > > code it was fine, but defensively it only needs to check for
> > > in_transaction).
> > >
> > > It's against our coding policy to mix pointers to structs and plain
> > > structs in the same variable declaration, so I split
> > > + struct addrinfo ai_hint;
> > > + struct addrinfo *ai = NULL;
> > > for clarity.
> > >
> > > You forgot to check for memory allocation error in
> > > sss_ssh_cmd_get_host_pubkeys() when creating the new primary name. I
> > > added it.
> > >
> > > And I moved the SSS_SSH_KNOWN_HOSTS_PATH and
> > > SSS_SSH_KNOWN_HOSTS_TEMP_TMPL define to patch 0003 where it belongs.
> >
> > Thanks.
> >
> > >
> > > Functionally, this is fine, so ack with my changes (attached).
I'm not sure about the getaddrinfo() call, that looks like a blocking
call to me? Also when the strdup following the getaddrinfo fails,
you leak ai.
> >
> > I have done this additional little change in ipa_hostid.c:
> >
> > @@ -227,8 +231,7 @@ hosts_get_connect_done(struct tevent_req *subreq)
> > subreq = ipa_host_info_send(state, state->ev, state->sysdb,
> > sdap_id_op_handle(state->op),
> > state->ctx->sdap_id_ctx->opts,
> > state->name,
> > - state->ctx->ipa_opts->host_map,
> > - state->ctx->ipa_opts->hostgroup_map,
> > + state->ctx->ipa_opts->host_map, NULL,
> > state->ctx->host_search_bases);
> > if (!subreq) {
> > tevent_req_error(req, ENOMEM);
> >
> > >
> > >>
> > >> [PATCH 2/3] SSH: Move responder and client common code to util
> > >>
> > >> Don't manage user-specific known_hosts file in sss_ssh_knownhostsproxy.
> > >>
> > >> Continue connecting to SSH server even when SSSD is not running in
> > >> sss_ssh_knownhostsproxy and don't drop the connection when the
> > >> sss_ssh_knownhostsproxy process receives a signal.
> > >>
> > >> https://fedorahosted.org/sssd/ticket/1179
> > >> https://fedorahosted.org/sssd/ticket/1184
> > >>
> > >
> > > I'm not comfortable enough in this code to review it. I'd prefer to have
> > > Jakub's sign-off here.
> > >
> > > It would also be really nice if you could split this into multiple
> > > logical patches, because it's really overwhelming.
> >
> > Done, I have split this patch into 3 patches:
> >
> > [PATCH 2/5] SSH: Refactor responder and client common code
>
sss_ssh_format_pubkey(): the first "case" in the switch is missing a
break.
> > [PATCH 3/5] UTIL: Add function for atomic I/O
Ack, although I would prefer an enum instead of the bool as the last
parameter.
> > [PATCH 4/5] SSH: Continue connecting to SSH server even when SSSD is not
> > running in sss_ssh_knownhostsproxy
> >
Ack
> > Additionally, don't drop the connection when the sss_ssh_knownhostsproxy
> > process receives a signal.
> >
> > https://fedorahosted.org/sssd/ticket/1179
> > https://fedorahosted.org/sssd/ticket/1184
> >
> > >
> > >>
> > >> [PATCH 3/3] SSH: Manage global known_hosts file in the responder
> > >>
> > >> https://fedorahosted.org/sssd/ticket/1193
> > >>
> > >> The known_hosts file is stored in /var/lib/sss/pubconf/known_hosts.
> > >
> > >
> > > Nack
> > > You need to wrap the write() in a loop to make sure it writes
> > > completely. It also needs to loop on EINTR.
> >
> > Fixed.
> >
> > I have also moved the "don't manage user-specific known_hosts" bit of
> > the previous patch to this patch.
> >
> > Honza
> >
>
>
>
> Patch 0001: Ack
>
> Patch 0002: Ack
>
> Patch 0003: Full ack, and please open a ticket to have us convert all of
> our read and write loops to use this function.
>
> Patch 0004: Ack
>
> Patch 0005: Ack, with one caveat. Please submit a patch to correct the
> manpage as to the proper option to set for GlobalKnownHosts. I want this
> as a separate patch because we're in string freeze and we may decide to
> only carry this change in master (and release-note the doc bug). In that
> patch, please also add rsc/sss_client/ssh/* to po/POTFILES.in, since you
> added translatable error message strings.
>
Path 0005: Ack
>
> Pushed all five patches to master. Please provide a rebased set of
> patches for sssd-1-8. This batch must currently be applied atop Jan's
> ipa_host refactoring patch (which is master-only).
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
