[PATCH] RFC debug detect winbindd version mismatches
Noel Power
nopower at suse.com
Thu Jan 30 02:36:55 MST 2014
Hi,
On 27/01/14 19:03, Andrew Bartlett wrote:
> On Mon, 2014-01-27 at 15:36 +0100, Volker Lendecke wrote:
>> On Mon, Jan 27, 2014 at 02:25:09PM +0000, Noel Power wrote:
>>>> I'm not opposed to this feature. Question: Is there a way to
>>>> do this without a global variable?
>>> everything is possible :-)
[...]
>>> Not sure about consensus, I'm just uttering my personal
>>> opinion here. I could not even block the global variable,
>>> but I would just like to not see that go in this way.
> I tend to agree here. The concept is quite useful, but we need to
> improve the implementation. I don't see why this can't be passed back
> as an extra pointer argument for example, of the server version if
> obtained. That layer isn't in an ABI, it can be reworked,
you you mean passing back the version mismatch info via some out
parameter? if so then this is what I thought about trying to do
previously the code really doesn't lend itself easily to shoe-horning
that in, after some attempts I was not happy with the results, imo it
makes things even uglier
>
>
> Either way, declaring the prototype in another C file strictly against
> our coding standards:
> +int winbindd_get_mismatched_version(void);
wasn't aware of this restriction, I just followed what was already there
where I added to the existing function prototypes already declared, I
attach a patch that removes those old instances
> and
> static/global variables almost always bite us in the end.
While that statement might be generally correct I don't see adding a new
'static/global' to store error info that is synced with and relates to
another global (the winbind_fd) that actually underpins the api
implementation as being *that* worse or evil. However I attach another
patch that avoids the global as an alternative
> But the other issue I see is that this will only work if the structure
> size hasn't changed. If that changes, we are back to square one - or
> worse, a timeout waiting for the last few bytes in the request/reply.
> To really do this properly,
yes if you are talking about some version mismatch that would prevent
the initial interface request from succeeding then indeed we are
screwed, but..... that is outside the scope of this change which only is
relevant for better reporting of 'detected' version mismatches
anyway, I appreciate the reviews and responses, please see latest patches
Noel
-------------- next part --------------
From f92ecb25439a606f2bf4621bc7b84ea6690acc86 Mon Sep 17 00:00:00 2001
From: Noel Power <noel.power at suse.com>
Date: Wed, 29 Jan 2014 16:41:07 +0000
Subject: [PATCH 1/3] remove prototype declarations of functions defined in
wb_common.c
Signed-off-by: Noel Power <noel.power at suse.com>
---
nsswitch/libwbclient/wbclient.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/nsswitch/libwbclient/wbclient.c b/nsswitch/libwbclient/wbclient.c
index 19bb3e9..28e8f3e 100644
--- a/nsswitch/libwbclient/wbclient.c
+++ b/nsswitch/libwbclient/wbclient.c
@@ -24,15 +24,7 @@
#include "replace.h"
#include "libwbclient.h"
-
-/* From wb_common.c */
-
-NSS_STATUS winbindd_request_response(int req_type,
- struct winbindd_request *request,
- struct winbindd_response *response);
-NSS_STATUS winbindd_priv_request_response(int req_type,
- struct winbindd_request *request,
- struct winbindd_response *response);
+#include "../winbind_client.h"
/*
result == NSS_STATUS_UNAVAIL: winbind not around
--
1.8.1.4
-------------- next part --------------
From 4d25772cf336c8e2af4b063f59c8d3fbb48ffa34 Mon Sep 17 00:00:00 2001
From: Noel Power <noel.power at suse.com>
Date: Wed, 29 Jan 2014 17:34:20 +0000
Subject: [PATCH 2/3] Add new wbcError type to detect version mismatch
Also splits out a new function in wb_common.c to allow the version to be
queried when socket to winbindd is opened. Same function is used in wbclient.c
to decide to return WBC_ERR_VERSION_MISMATCH or WBC_ERR_WINBIND_NOT_AVAILABLE
Signed-off-by: Noel Power <noel.power at suse.com>
---
nsswitch/libwbclient/wbclient.c | 14 +++++++++++++-
nsswitch/libwbclient/wbclient.h | 3 ++-
nsswitch/wb_common.c | 28 ++++++++++++++++++++++------
nsswitch/winbind_client.h | 4 ++++
4 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/nsswitch/libwbclient/wbclient.c b/nsswitch/libwbclient/wbclient.c
index 28e8f3e..eca086e 100644
--- a/nsswitch/libwbclient/wbclient.c
+++ b/nsswitch/libwbclient/wbclient.c
@@ -59,8 +59,18 @@ static wbcErr wbcRequestResponseInt(
wbc_status = WBC_ERR_SUCCESS;
break;
case NSS_STATUS_UNAVAIL:
- wbc_status = WBC_ERR_WINBIND_NOT_AVAILABLE;
+ {
+ struct winbindd_request ver_request;
+ struct winbindd_response ver_response;
+ ZERO_STRUCT(ver_request);
+ ZERO_STRUCT(ver_response);
+ if ((winbind_open_and_version_check_socket(&ver_request,&ver_response) == NSS_STATUS_SUCCESS) && (ver_response.data.interface_version != WINBIND_INTERFACE_VERSION)) {
+ wbc_status = WBC_ERR_VERSION_MISMATCH;
+ } else {
+ wbc_status = WBC_ERR_WINBIND_NOT_AVAILABLE;
+ }
break;
+ }
case NSS_STATUS_NOTFOUND:
wbc_status = WBC_ERR_DOMAIN_NOT_FOUND;
break;
@@ -134,6 +144,8 @@ const char *wbcErrorString(wbcErr error)
return "WBC_ERR_AUTH_ERROR";
case WBC_ERR_PWD_CHANGE_FAILED:
return "WBC_ERR_PWD_CHANGE_FAILED";
+ case WBC_ERR_VERSION_MISMATCH:
+ return "WBC_ERR_VERSION_MISMATCH";
}
return "unknown wbcErr value";
diff --git a/nsswitch/libwbclient/wbclient.h b/nsswitch/libwbclient/wbclient.h
index dc3e822..ab34a74 100644
--- a/nsswitch/libwbclient/wbclient.h
+++ b/nsswitch/libwbclient/wbclient.h
@@ -46,7 +46,8 @@ enum _wbcErrType {
WBC_ERR_AUTH_ERROR, /**< Authentication failed **/
WBC_ERR_UNKNOWN_USER, /**< User account cannot be found */
WBC_ERR_UNKNOWN_GROUP, /**< Group account cannot be found */
- WBC_ERR_PWD_CHANGE_FAILED /**< Password Change has failed */
+ WBC_ERR_PWD_CHANGE_FAILED, /**< Password Change has failed */
+ WBC_ERR_VERSION_MISMATCH /**< Interface version mismatch */
};
typedef enum _wbcErrType wbcErr;
diff --git a/nsswitch/wb_common.c b/nsswitch/wb_common.c
index 5fde8d0..646affc 100644
--- a/nsswitch/wb_common.c
+++ b/nsswitch/wb_common.c
@@ -300,6 +300,27 @@ static const char *winbindd_socket_dir(void)
return WINBINDD_SOCKET_DIR;
}
+NSS_STATUS winbind_open_and_version_check_socket(
+ struct winbindd_request *request,
+ struct winbindd_response *response)
+{
+ if (winbindd_fd == -1 ) {
+ if ((winbindd_fd = winbind_named_pipe_sock(
+ winbindd_socket_dir()))== -1) {
+ return NSS_STATUS_UNAVAIL;
+ }
+ }
+
+ if ((winbindd_request_response(WINBINDD_INTERFACE_VERSION,request, response) != NSS_STATUS_SUCCESS)) {
+ winbind_close_sock();
+ return NSS_STATUS_UNAVAIL;
+ }
+ if (response->data.interface_version != WINBIND_INTERFACE_VERSION) {
+ winbind_close_sock();
+ }
+ return NSS_STATUS_SUCCESS;
+}
+
/* Connect to winbindd socket */
static int winbind_open_pipe_sock(int recursing, int need_priv)
@@ -328,17 +349,12 @@ static int winbind_open_pipe_sock(int recursing, int need_priv)
return -1;
}
- if ((winbindd_fd = winbind_named_pipe_sock(winbindd_socket_dir())) == -1) {
- return -1;
- }
-
is_privileged = 0;
/* version-check the socket */
request.wb_flags = WBFLAG_RECURSE;
- if ((winbindd_request_response(WINBINDD_INTERFACE_VERSION, &request, &response) != NSS_STATUS_SUCCESS) || (response.data.interface_version != WINBIND_INTERFACE_VERSION)) {
- winbind_close_sock();
+ if ((winbind_open_and_version_check_socket(&request, &response) != NSS_STATUS_SUCCESS) || (response.data.interface_version != WINBIND_INTERFACE_VERSION)) {
return -1;
}
diff --git a/nsswitch/winbind_client.h b/nsswitch/winbind_client.h
index 905a189..1bf5e61 100644
--- a/nsswitch/winbind_client.h
+++ b/nsswitch/winbind_client.h
@@ -38,6 +38,10 @@ NSS_STATUS winbindd_request_response(int req_type,
NSS_STATUS winbindd_priv_request_response(int req_type,
struct winbindd_request *request,
struct winbindd_response *response);
+NSS_STATUS winbind_open_and_version_check_socket(
+ struct winbindd_request *request,
+ struct winbindd_response *response);
+
#define winbind_env_set() \
(strcmp(getenv(WINBINDD_DONT_ENV)?getenv(WINBINDD_DONT_ENV):"0","1") == 0)
--
1.8.1.4
More information about the samba-technical
mailing list