[SSSD] Refactor krb5_child code
While working on a different refactoring patch I came across the
krb5_child code and found it very hard to read and follow due to the
growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and
easier to follow and read. A bunch of dead stuff that really confused me
initially also got removed.
(I propose this for master only)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From a909978c5a1188a48a8838d851e29aca08809445 Mon Sep 17 00:00:00 2001
From: Simo Sorce s...@redhat.com
Date: Thu, 22 Nov 2012 12:39:38 -0500
Subject: [PATCH] Refactor krb5 child
The aim of this refactoring is to make the code readable and understandable.
This code has grown organically over time and has becomed confused and
baroque enough that understanding it's very simple flow had become very
complex for the uninitiated. Complex flows easily hide nasty bugs.
Improvements:
- Remove dead/unused data storage
- Fix and simplify talloc hierarchy, use a memory context (kr) for the
whole code and allocate kr-pd where it is filled up.
- Rename some functions to create a better name space (easier for
searching fucntions across the tree)
- Streamline setup function, by spliting out fast setup in a subroutine.
- Avoid confusing indirection in executng actual functions by not
using the krb5_req child_req member.
- Make main() flow s now simmetric, send abck data from the main function
instead of delegating a reply to every inner function that implements a
command.
Now the flow is evident from the main function:
1. read request
2. setup data
3. execute command
4. send reply back
---
src/providers/krb5/krb5_child.c | 492 +---
1 file changed, 207 insertions(+), 285 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 0532a9b0b928c4b0f7a06f1551490af951ee998a..15e636f1e7fd0ad17620f41a4fa365f1dfc96211 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -40,53 +40,16 @@
#define SSSD_KRB5_CHANGEPW_PRINCIPAL kadmin/changepw
-struct krb5_child_ctx {
-/* opts taken from kinit */
-/* in seconds */
-krb5_deltat starttime;
-krb5_deltat lifetime;
-krb5_deltat rlife;
-
-int forwardable;
-int proxiable;
-int addresses;
-
-int not_forwardable;
-int not_proxiable;
-int no_addresses;
-
-int verbose;
-
-char* principal_name;
-char* service_name;
-char* keytab_name;
-char* k5_cache_name;
-char* k4_cache_name;
-
-action_type action;
-
-char *kdcip;
-char *realm;
-char *ccache_dir;
-char *ccname_template;
-int auth_timeout;
-};
-
struct krb5_req {
krb5_context ctx;
krb5_principal princ;
char* name;
krb5_creds *creds;
krb5_get_init_creds_opt *options;
-pid_t child_pid;
-int read_from_child_fd;
-int write_to_child_fd;
-struct be_req *req;
struct pam_data *pd;
-struct krb5_child_ctx *krb5_ctx;
-errno_t (*child_req)(int fd, struct krb5_req *kr);
+char *realm;
char *ccname;
char *keytab;
bool validate;
@@ -676,14 +639,14 @@ static struct response *prepare_response_message(struct krb5_req *kr,
return resp;
}
-static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status,
-struct krb5_req *kr)
+static errno_t k5_send_data(struct krb5_req *kr, int fd,
+krb5_error_code kerr, int status)
{
struct response *resp;
size_t written;
int ret;
-resp = prepare_response_message(kr, kerr, pam_status);
+resp = prepare_response_message(kr, kerr, status);
if (resp == NULL) {
DEBUG(1, (prepare_response_message failed.\n));
return ENOMEM;
@@ -1062,7 +1025,7 @@ static int kerr_to_status(krb5_error_code kerr)
return kerr_handle_error(kerr);
}
-static errno_t changepw_child(int fd, struct krb5_req *kr)
+static errno_t changepw_child(struct krb5_req *kr, int *_status)
{
int ret;
krb5_error_code kerr = 0;
@@ -1208,15 +1171,11 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
done:
-ret = sendresponse(fd, kerr, pam_status, kr);
-if (ret != EOK) {
-DEBUG(1, (sendresponse failed.\n));
-}
-
+*_status = pam_status;
return ret;
}
-static errno_t tgt_req_child(int fd, struct krb5_req *kr)
+static errno_t tgt_req_child(struct krb5_req *kr, int *_status)
{
int ret;
krb5_error_code kerr = 0;
@@ -1283,18 +1242,13 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
done:
-ret = sendresponse(fd, kerr, pam_status, kr);
-if (ret != EOK) {
-DEBUG(1, (sendresponse failed.\n));
-}
-
+*_status = pam_status;
return ret;
}
-static errno_t kuserok_child(int fd, struct krb5_req *kr)
+static errno_t kuserok_child(struct krb5_req *kr, int *_status)
{
Re: [SSSD] Refactor krb5_child code
On Thu, 2012-11-22 at 15:19 -0500, Simo Sorce wrote:
While working on a different refactoring patch I came across the
krb5_child code and found it very hard to read and follow due to the
growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and
easier to follow and read. A bunch of dead stuff that really confused me
initially also got removed.
(I propose this for master only)
Sorry, just noticed the name of a function got out wrong (k5_send_data
instead of k5c_send_data). Amended patch attached.
Simo
--
Simo Sorce * Red Hat, Inc * New York
From 0ed9fae8ad8b4a9a296fb337c29bfd7d1f30fa4a Mon Sep 17 00:00:00 2001
From: Simo Sorce s...@redhat.com
Date: Thu, 22 Nov 2012 12:39:38 -0500
Subject: [PATCH] Refactor krb5 child
The aim of this refactoring is to make the code readable and understandable.
This code has grown organically over time and has becomed confused and
baroque enough that understanding it's very simple flow had become very
complex for the uninitiated. Complex flows easily hide nasty bugs.
Improvements:
- Remove dead/unused data storage
- Fix and simplify talloc hierarchy, use a memory context (kr) for the
whole code and allocate kr-pd where it is filled up.
- Rename some functions to create a better name space (easier for
searching fucntions across the tree)
- Streamline setup function, by spliting out fast setup in a subroutine.
- Avoid confusing indirection in executng actual functions by not
using the krb5_req child_req member.
- Make main() flow s now simmetric, send abck data from the main function
instead of delegating a reply to every inner function that implements a
command.
Now the flow is evident from the main function:
1. read request
2. setup data
3. execute command
4. send reply back
---
src/providers/krb5/krb5_child.c | 490 +---
1 file changed, 206 insertions(+), 284 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 0532a9b0b928c4b0f7a06f1551490af951ee998a..4835da0644c07ce370b959186673200d39cd1454 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -40,53 +40,16 @@
#define SSSD_KRB5_CHANGEPW_PRINCIPAL kadmin/changepw
-struct krb5_child_ctx {
-/* opts taken from kinit */
-/* in seconds */
-krb5_deltat starttime;
-krb5_deltat lifetime;
-krb5_deltat rlife;
-
-int forwardable;
-int proxiable;
-int addresses;
-
-int not_forwardable;
-int not_proxiable;
-int no_addresses;
-
-int verbose;
-
-char* principal_name;
-char* service_name;
-char* keytab_name;
-char* k5_cache_name;
-char* k4_cache_name;
-
-action_type action;
-
-char *kdcip;
-char *realm;
-char *ccache_dir;
-char *ccname_template;
-int auth_timeout;
-};
-
struct krb5_req {
krb5_context ctx;
krb5_principal princ;
char* name;
krb5_creds *creds;
krb5_get_init_creds_opt *options;
-pid_t child_pid;
-int read_from_child_fd;
-int write_to_child_fd;
-struct be_req *req;
struct pam_data *pd;
-struct krb5_child_ctx *krb5_ctx;
-errno_t (*child_req)(int fd, struct krb5_req *kr);
+char *realm;
char *ccname;
char *keytab;
bool validate;
@@ -676,14 +639,14 @@ static struct response *prepare_response_message(struct krb5_req *kr,
return resp;
}
-static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status,
-struct krb5_req *kr)
+static errno_t k5c_send_data(struct krb5_req *kr, int fd,
+ krb5_error_code kerr, int status)
{
struct response *resp;
size_t written;
int ret;
-resp = prepare_response_message(kr, kerr, pam_status);
+resp = prepare_response_message(kr, kerr, status);
if (resp == NULL) {
DEBUG(1, (prepare_response_message failed.\n));
return ENOMEM;
@@ -1062,7 +1025,7 @@ static int kerr_to_status(krb5_error_code kerr)
return kerr_handle_error(kerr);
}
-static errno_t changepw_child(int fd, struct krb5_req *kr)
+static errno_t changepw_child(struct krb5_req *kr, int *_status)
{
int ret;
krb5_error_code kerr = 0;
@@ -1208,15 +1171,11 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
done:
-ret = sendresponse(fd, kerr, pam_status, kr);
-if (ret != EOK) {
-DEBUG(1, (sendresponse failed.\n));
-}
-
+*_status = pam_status;
return ret;
}
-static errno_t tgt_req_child(int fd, struct krb5_req *kr)
+static errno_t tgt_req_child(struct krb5_req *kr, int *_status)
{
int ret;
krb5_error_code kerr = 0;
@@ -1283,18 +1242,13 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
done:
-ret = sendresponse(fd, kerr, pam_status, kr);
-if (ret != EOK) {
-DEBUG(1, (sendresponse failed.\n));
-}
-
