[SSSD] Re: [PATCH] Make responder connectin code more generic
On Tue, 2016-01-12 at 14:04 +0100, Jakub Hrozek wrote:
> On Mon, Jan 11, 2016 at 01:39:33PM -0500, Simo Sorce wrote:
> > The following 2 patches change the connection setup code to be more
> > flexible.
> >
> > They are the groundwork to add a new secrets[1] responder that uses a
> > REST API over a unix socket and therefore requires a different protocol
> > handler.
> >
> > The first patch move some protocol and state data into opaque pointers
> > so that responders can arbitrarily set them.
> > Ticket: https://fedorahosted.org/sssd/ticket/2918
> >
> > The second patch adds the ability to use socket activation for
> > responders. Although this is not currently wired in for use in
> > currently.
> > It is related to: https://fedorahosted.org/sssd/ticket/2243
> >
> > These patches are being worked on on my secsrv branch here:
> > https://fedorapeople.org/cgit/simo/public_git/sssd.git/log/?h=secsrv
> >
> >
> > [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SecretsService
> >
> > --
> > Simo Sorce * Red Hat, Inc * New York
>
> > From e82a6c4fc5bd0c6dc264961c5b0fa4f97372e984 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce
> > Date: Fri, 8 Jan 2016 17:51:06 -0500
> > Subject: [PATCH] Responders: Make the client context more generic
> >
> > This is useufl to allow reusing the responder code with other protocols.
> > Store protocol data and responder state data behind opaque pointers and
> > use tallog_get_type to check they are of the right type.
> >
> > This also allows to store per responder state_ctx so that, for example,
> > the autofs responder does not have to carry useless variables used only
> > by the nss responder.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2918
>
> There are some new Coverity warnings with this code:
> Error: NEGATIVE_RETURNS (CWE-394):
> sssd-1.13.90/src/responder/common/responder_common.c:756: var_tested_neg:
> Assigning: "rctx->lfd" = a negative value.
> sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns:
> "rctx->lfd" is passed to a parameter that cannot be negative.
> sssd-1.13.90/src/responder/common/responder_common.c:745:5:
> neg_sink_parm_call: Passing "rctx->lfd" to "close", which cannot accept a
> negative number.
> # 788| #endif
> # 789|
> # 790|-> ret = set_unix_socket(rctx, conn_setup);
> # 791| if (ret != EOK) {
> # 792| DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> sockets\n");
>
> Error: NEGATIVE_RETURNS (CWE-394):
> sssd-1.13.90/src/responder/common/responder_common.c:757: var_tested_neg:
> Assigning: "rctx->priv_lfd" = a negative value.
> sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns:
> "rctx->priv_lfd" is passed to a parameter that cannot be negative.
> sssd-1.13.90/src/responder/common/responder_common.c:746:5:
> neg_sink_parm_call: Passing "rctx->priv_lfd" to "close", which cannot accept
> a negative number.
> # 788| #endif
> # 789|
> # 790|-> ret = set_unix_socket(rctx, conn_setup);
> # 791| if (ret != EOK) {
> # 792| DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
> sockets\n");
>
> But in general it looks good to me. I need to run more tests, though..
I'll check these out.
> > From 796ad20628994e671b9af903ed396eb8103d18c4 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce
> > Date: Thu, 7 Jan 2016 10:17:38 -0500
> > Subject: [PATCH 2/2] Responders: Add support for socket activation
> >
> > Add helper that uses systemd socket activation if available to accept a
> > pre-listining socket at startup.
> >
> > Related:
> > https://fedorahosted.org/sssd/ticket/2913
>
> [...]
>
> > +ret = sd_listen_fds(1);
> > +if (ret < 0) {
> > +DEBUG(SSSDBG_MINOR_FAILURE,
> > + "Unexpected error probing for active sockets. "
> > + "Will proceed with no sockets. [Error %d (%s)]\n",
> > + -ret, sss_strerror(-ret));
> > +} else if (ret > numfds) {
> > +DEBUG(SSSDBG_FATAL_FAILURE,
> > + "Too many activated sockets have been found, "
> > + "expected %d, found %d\n", numfds, ret);
> > +ret = E2BIG;
> > +goto done;
> > +}
> > +
> > +if (ret == numfds) {
>
> Is this exact comparison correct? If we had a deamon that has both public
> and private sockets and the client only wrote to the public socket,
> wouldn't sd_listen_fds() only return 1?
It is correct, what sd_listen_fds() tells you is how many socket systemd
has created for you and is passing to you upon activation. It tells
nothing about clients.
After you epoll the socket you can gain any knowledge on whether there
are clients waiting.
Simo.
> > +rctx->lfd = SD_LISTEN_FDS_START;
> > +ret = sss_fd_nonblocking(rctx->lfd);
> > +if (ret != EOK) goto done;
> > +if (numfds == 2) {
> > +rctx->priv_lfd = SD_LISTEN_FDS_START + 1;
> > +ret = sss_fd_nonblocking(rctx->priv_lfd);
> > +if (r
[SSSD] Re: [PATCH] Make responder connectin code more generic
On Mon, Jan 11, 2016 at 01:39:33PM -0500, Simo Sorce wrote:
> The following 2 patches change the connection setup code to be more
> flexible.
>
> They are the groundwork to add a new secrets[1] responder that uses a
> REST API over a unix socket and therefore requires a different protocol
> handler.
>
> The first patch move some protocol and state data into opaque pointers
> so that responders can arbitrarily set them.
> Ticket: https://fedorahosted.org/sssd/ticket/2918
>
> The second patch adds the ability to use socket activation for
> responders. Although this is not currently wired in for use in
> currently.
> It is related to: https://fedorahosted.org/sssd/ticket/2243
>
> These patches are being worked on on my secsrv branch here:
> https://fedorapeople.org/cgit/simo/public_git/sssd.git/log/?h=secsrv
>
>
> [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SecretsService
>
> --
> Simo Sorce * Red Hat, Inc * New York
> From e82a6c4fc5bd0c6dc264961c5b0fa4f97372e984 Mon Sep 17 00:00:00 2001
> From: Simo Sorce
> Date: Fri, 8 Jan 2016 17:51:06 -0500
> Subject: [PATCH] Responders: Make the client context more generic
>
> This is useufl to allow reusing the responder code with other protocols.
> Store protocol data and responder state data behind opaque pointers and
> use tallog_get_type to check they are of the right type.
>
> This also allows to store per responder state_ctx so that, for example,
> the autofs responder does not have to carry useless variables used only
> by the nss responder.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2918
There are some new Coverity warnings with this code:
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.90/src/responder/common/responder_common.c:756: var_tested_neg:
Assigning: "rctx->lfd" = a negative value.
sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns:
"rctx->lfd" is passed to a parameter that cannot be negative.
sssd-1.13.90/src/responder/common/responder_common.c:745:5: neg_sink_parm_call:
Passing "rctx->lfd" to "close", which cannot accept a negative number.
# 788| #endif
# 789|
# 790|-> ret = set_unix_socket(rctx, conn_setup);
# 791| if (ret != EOK) {
# 792| DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
sockets\n");
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.90/src/responder/common/responder_common.c:757: var_tested_neg:
Assigning: "rctx->priv_lfd" = a negative value.
sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns:
"rctx->priv_lfd" is passed to a parameter that cannot be negative.
sssd-1.13.90/src/responder/common/responder_common.c:746:5: neg_sink_parm_call:
Passing "rctx->priv_lfd" to "close", which cannot accept a negative number.
# 788| #endif
# 789|
# 790|-> ret = set_unix_socket(rctx, conn_setup);
# 791| if (ret != EOK) {
# 792| DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing
sockets\n");
But in general it looks good to me. I need to run more tests, though..
> From 796ad20628994e671b9af903ed396eb8103d18c4 Mon Sep 17 00:00:00 2001
> From: Simo Sorce
> Date: Thu, 7 Jan 2016 10:17:38 -0500
> Subject: [PATCH 2/2] Responders: Add support for socket activation
>
> Add helper that uses systemd socket activation if available to accept a
> pre-listining socket at startup.
>
> Related:
> https://fedorahosted.org/sssd/ticket/2913
[...]
> +ret = sd_listen_fds(1);
> +if (ret < 0) {
> +DEBUG(SSSDBG_MINOR_FAILURE,
> + "Unexpected error probing for active sockets. "
> + "Will proceed with no sockets. [Error %d (%s)]\n",
> + -ret, sss_strerror(-ret));
> +} else if (ret > numfds) {
> +DEBUG(SSSDBG_FATAL_FAILURE,
> + "Too many activated sockets have been found, "
> + "expected %d, found %d\n", numfds, ret);
> +ret = E2BIG;
> +goto done;
> +}
> +
> +if (ret == numfds) {
Is this exact comparison correct? If we had a deamon that has both public
and private sockets and the client only wrote to the public socket,
wouldn't sd_listen_fds() only return 1?
> +rctx->lfd = SD_LISTEN_FDS_START;
> +ret = sss_fd_nonblocking(rctx->lfd);
> +if (ret != EOK) goto done;
> +if (numfds == 2) {
> +rctx->priv_lfd = SD_LISTEN_FDS_START + 1;
> +ret = sss_fd_nonblocking(rctx->priv_lfd);
> +if (ret != EOK) goto done;
> +}
> +}
> +#endif
> +
> +ret = set_unix_socket(rctx, conn_setup);
> +if (ret != EOK) {
> +DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing sockets\n");
> +goto done;
> +}
> +
> +done:
> +return ret;
> +}
> +
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
