Re: [SSSD] [PATCH] Let the PAM client send its PID
On Sat, Sep 12, 2009 at 09:02:34PM -0400, Simo Sorce wrote: On Sat, 2009-09-12 at 10:11 +0200, Sumit Bose wrote: On Fri, Sep 11, 2009 at 05:46:24PM -0400, Simo Sorce wrote: On Fri, 2009-09-11 at 17:10 +0200, Sumit Bose wrote: Most of items are not mandatory at the protocol level. If e.g. the remote host is not known to the client it is not sent to the server and the server complains if he really needs it, e.g. the user name. I haven't put a check like 'if cli_pid==0 do not send to the server' because as getpid(2) says These functions are always successful.. On the server side cli_pid is 0 if the client does not send a PID item. I think the way it currently works is the way your are expecting it to work. Will the unpacking function work is the client doesn't send the pid at all (ie it is an older client ?). Yes, it will work. This was one of the main ideas why I have changed the original protocol some time ago. Every item has an identifier. So it is always clear for the unpacking function what the next item will be. If one item is missing, it is just left empty (NULL,0) on the server side. Oh I know the items are recognized by our code, but I am not sure that dbus_message_get_args() is as forgiving. Or does it just stop getting args when it sees DBUS_TYPE_INVALID even if there are more in the actual message ? Internally the cli_pid is always send, even if it is 0. Nevertheless, selfNACK I have forgotten to put the size of the cli_pid into the protocol. This might look a bit redundant, but if you have an older server and a newer client the server does not know about the cli_pid item and how large it might be. With the size it can jump to the next item on the wire and continue working with items it knows about. bye, Sumit ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] python bindings for managing users in local domains
On Thu, 2009-09-10 at 23:36 +0200, Jakub Hrozek wrote:
Patch 1:
- mostly good but mem hierarchy between ops_ctx and tools_ctx needs
to
be reversed
^^^ this is still not addressed, once you do 0001 can be acked.
- also some cases where codyng style is not followed (missing space
between 'if' and '('
Should be done. Note that the patch is different than the original one
b/c of the removal of shadow-utils fallback.
The rest looks ok.
Patch 2: NACK
- do not create tools/common/ keep all in tools/ until we have a
lot's
of files to get out of the way
done
- main NACK point is that tevent_req async coding style has not been
followed in the sync wrappers, details explained on IRC
OK, I pretty much rewrote the whole thing to be tevent_req-stylish
with
_send _recv etc. I agree that it's way more readable now.
Much better, we are close, but not there yet.
0. It is absolutely paramount that you always have:
state structure
_Send()
step1_done()
step2_done()
step3_done()
...
_recv()
In this order, if you have to jump around to find out any step the code
becomes very soon unreadable and it is extremely easy to make mistakes.
This code is already somewhat harder than usual to debug (need to set a
lof of break points to follow the flow) so it is imperative that you can
read it as a single set of consecutive functions.
The importance of this is immediately evident whan you have to A) review
some else's code (like I am doing now), B) you have to modify come code.
Especially important is to share only utility functions but never
functions that create tevent_req requests.
1. You are trying to use one common state structure for all calls.
Please don't do that, and especially don't add more function pointer
(member_dn_fn is really a no go).
When you share the same state structure a typo later in the code where
you set (for example) a wrong callback (say a copypaste error where you
call the xx_users_recv() instead of the xx_groups_recv() will give back
no error. If you use different state functions though, you can use
talloc_get_type() to check for the type, and NULL will be returned if
the type is wrong resulting in a segfault the first time you test it.
Also remember that tevent_req_create() does not zero out the memory (on
purpose it's not a bug), so anything you do not initialize explicitly in
the _send() function is random.
2. Really don't like add_to_groups() for a few reasons.
- it is creating a subreq so it *should* be a _send() function and
*must* return the subreq and must not set the callback itself.
- this function and the following should probably be turned into a
_send(),_done(),_recv() subrequest.
- it uses member_dn_fn() please don't do that.
3. In tevent_req() _done() functions try to never do something like:
return foo_function(req);
- You should either call tevent_req_error() or tevent_req_done(), or
call a subrequests: subreq = foo_send() and set a callback and then just
return;
- a return foo_function(req) is a spy that something is not right (there
are exceptions, but they must be carefully thought out.
I know some of the functions (like user_mod_cont()) seem to save a bit
of duplication and makes the code slightly smaller, but this is an
optimization that kills readability and doesn't really save much.
Readability is much more important when it comes to tevent_req stuff.
- also do the transaction as a separate operation and simply pass in
the
handle to the sync ones, so that multiple sync operations can be
linked
into a single transaction.
Transactions are now done outside the sync module, that is, either
in
the tools or in the python bindings module.
good.
Patch 3:
- looks sane but I'd like a second look from one of ours python
resident
experts
CC-ed John.
I see John replied to that part, I'll let you address the concerns he
raised for the 3rd patch.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] Make offline status backend global
With this patch all the backend providers can share the same offline
status. This means composite backends like what AD or IPA will be that
use a mix of ldap and krb can share the offline status for both
protocols.
A further extension might allow to have per protocol online/offline
status, that will be done eventually when we integrate also the DNS
discovery options.
Tested with ldap_id+ldap_auth and ldap_id+krb5_auth
Simo.
--
Simo Sorce * Red Hat, Inc * New York
From 0cdf03e956838ae727760f8c22255958199f8e89 Mon Sep 17 00:00:00 2001
From: Simo Sorce sso...@redhat.com
Date: Sat, 12 Sep 2009 00:05:55 -0400
Subject: [PATCH] Make the offline status backend-global
Add helpers functions to query/set the offline status per backend.
Now all providers share the same offline status.
---
server/providers/data_provider_be.c | 117 +-
server/providers/dp_backend.h | 11 +++-
server/providers/krb5/krb5_auth.c | 12 +++-
server/providers/ldap/ldap_auth.c | 22 +--
server/providers/ldap/ldap_id.c | 55 ++--
server/providers/proxy.c| 76 +++
6 files changed, 79 insertions(+), 214 deletions(-)
diff --git a/server/providers/data_provider_be.c b/server/providers/data_provider_be.c
index 55fc278..2e83ab6 100644
--- a/server/providers/data_provider_be.c
+++ b/server/providers/data_provider_be.c
@@ -133,66 +133,39 @@ static int be_file_request(struct be_ctx *ctx,
return EOK;
}
-static void online_chk_callback(struct be_req *req, int status,
-const char *errstr)
-{
-struct be_online_req *oreq;
-DBusMessage *reply;
-DBusConnection *dbus_conn;
-dbus_bool_t dbret;
-dbus_uint16_t online;
-dbus_uint16_t err_maj = 0;
-dbus_uint32_t err_min = 0;
-const char *err_msg = Success;
-
-oreq = talloc_get_type(req-req_data, struct be_online_req);
-if (status != EOK) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = status;
-err_msg = errstr;
-}
-
-online = oreq-online;
-reply = (DBusMessage *)req-pvt;
+bool be_is_offline(struct be_ctx *ctx)
+{
+time_t now = time(NULL);
-dbret = dbus_message_append_args(reply,
- DBUS_TYPE_UINT16, online,
- DBUS_TYPE_UINT16, err_maj,
- DBUS_TYPE_UINT32, err_min,
- DBUS_TYPE_STRING, err_msg,
- DBUS_TYPE_INVALID);
-if (!dbret) {
-DEBUG(1, (Failed to generate dbus reply\n));
-return;
+/* check if we are past the offline blackout timeout */
+/* FIXME: get offline_timeout from configuration */
+if (ctx-offstat.went_offline + 60 now) {
+ctx-offstat.offline = false;
}
-dbus_conn = sbus_get_connection(req-be_ctx-dp_conn);
-dbus_connection_send(dbus_conn, reply, NULL);
-dbus_message_unref(reply);
+return ctx-offstat.offline;
+}
-DEBUG(4, (Request processed. Returned %d,%d,%s\n,
- err_maj, err_min, err_msg));
+void be_mark_offline(struct be_ctx *ctx)
+{
+DEBUG(8, (Going offline!\n));
-/* finally free the request */
-talloc_free(req);
+ctx-offstat.went_offline = time(NULL);
+ctx-offstat.offline = true;
}
-
static int be_check_online(DBusMessage *message, struct sbus_connection *conn)
{
-struct be_online_req *req;
-struct be_req *be_req;
struct be_ctx *ctx;
DBusMessage *reply;
+DBusConnection *dbus_conn;
dbus_bool_t dbret;
void *user_data;
-int ret;
dbus_uint16_t online;
-dbus_uint16_t err_maj;
-dbus_uint32_t err_min;
-const char *err_msg;
+dbus_uint16_t err_maj = 0;
+dbus_uint32_t err_min = 0;
+static const char *err_msg = Success;
user_data = sbus_conn_get_private_data(conn);
if (!user_data) return EINVAL;
@@ -202,45 +175,10 @@ static int be_check_online(DBusMessage *message, struct sbus_connection *conn)
reply = dbus_message_new_method_return(message);
if (!reply) return ENOMEM;
-/* process request */
-be_req = talloc(ctx, struct be_req);
-if (!be_req) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = ENOMEM;
-err_msg = Out of memory;
-goto done;
-}
-be_req-be_ctx = ctx;
-be_req-fn = online_chk_callback;
-be_req-pvt = reply;
-
-req = talloc(be_req, struct be_online_req);
-if (!req) {
+if (be_is_offline(ctx)) {
online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = ENOMEM;
-err_msg = Out of memory;
-goto done;
-}
-req-online = 0;
-
-be_req-req_data = req;
-
-ret = be_file_request(ctx, ctx-bet_info[BET_ID].bet_ops-check_online, be_req);
-if (ret != EOK) {
-online = MOD_OFFLINE;
-err_maj = DP_ERR_FATAL;
-err_min = ret;
-
