From: Marcio Barbosa Date: Thu, 27 Feb 2020 22:28:14 +0000 (+0000) Subject: ubik: death to SVOTE_GetSyncSite X-Git-Tag: openafs-devel-1_9_0~125 X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=8c335182115a1e16c66cde40c08ce9fd0144dccb ubik: death to SVOTE_GetSyncSite The SVOTE_GetSyncSite RPC was intended to provide the IP address of the current sync-site. Unfortunately, the RPC-L incorrectly defined ahost as an input argument instead of an output argument. As a result, the IP address in question is not returned to the callers of SVOTE_GetSyncSite. Moreover, calls to this RPC must be made through connections associated with the VOTE_SERVICE_ID. Sadly, the ubik_Call* functions call SVOTE_GetSyncSite using connections associated with the USER_SERVICE_ID. Consequently, the server getting this request returns RXGEN_OPCODE, meaning that this RPC is not implemented by the service in question. Since RPC arguments cannot be changed without causing compatibility issues between different client / server versions and the RPC in question is being called through the wrong service id, remove SVOTE_GetSyncSite and its callers. Considering that in all versions of OpenAFS calls to this RPC always return RXGEN_OPCODE, no behavior change is introduced by this commit. Also, remove the "chaseCount logic" from the ubik_Call* functions. This logic prevents the loop counter from being moved backwards indefinitely, resulting in an infinite loop. Fortunately, without the VOTE_GetSyncSite() calls this counter cannot be moved backwards more than once. Change-Id: Idd071583e8f67109e003f7a5675de02a235e5809 Reviewed-on: https://gerrit.openafs.org/14043 Reviewed-by: Marcio Brito Barbosa Reviewed-by: Andrew Deason Reviewed-by: Benjamin Kaduk Tested-by: BuildBot --- diff --git a/src/bucoord/ubik_db_if.c b/src/bucoord/ubik_db_if.c index e602310..14d4880 100644 --- a/src/bucoord/ubik_db_if.c +++ b/src/bucoord/ubik_db_if.c @@ -1002,13 +1002,10 @@ ubik_Call_SingleServer(int (*aproc) (), struct ubik_client *aclient, char *p14, char *p15, char *p16) { afs_int32 code; - afs_int32 someCode, newHost, thisHost; - afs_int32 i; + afs_int32 someCode; afs_int32 count; - int chaseCount; int pass; struct rx_connection *tc; - struct rx_peer *rxp; if ((aflags & (UF_SINGLESERVER | UF_END_SINGLESERVER)) != 0) { if (((aflags & UF_SINGLESERVER) != 0) @@ -1033,7 +1030,6 @@ ubik_Call_SingleServer(int (*aproc) (), struct ubik_client *aclient, } someCode = UNOSERVERS; - chaseCount = 0; pass = 0; count = 0; while (1) { /*w */ @@ -1063,38 +1059,12 @@ ubik_Call_SingleServer(int (*aproc) (), struct ubik_client *aclient, * requires a sync site, ubik will return UNOTSYNC, indicating the * operation won't work until you find a sync site */ - if (code == UNOTSYNC) { /*ns */ - /* means that this requires a sync site to work */ - someCode = code; /* remember an error, if this fails */ - - /* now see if we can find the sync site host */ - code = VOTE_GetSyncSite(tc, &newHost); - if (code == 0 && newHost != 0) { - newHost = htonl(newHost); /* convert back to network order */ - - /* position count at the appropriate slot in the client - * structure and retry. If we can't find in slot, we'll just - * continue through the whole list - */ - for (i = 0; i < MAXSERVERS; i++) { /*f */ - rxp = rx_PeerOf(aclient->conns[i]); - if (!(thisHost = rx_HostOf(rxp))) { - count++; /* host not found, try the next dude */ - break; - } - if (thisHost == newHost) { - /* avoid asking in a loop */ - if (chaseCount++ > 2) - break; - count = i; /* we were told to use this one */ - break; - } - } /*f */ - } else - count++; /* not directed, keep looking for a sync site */ - continue; - } /*ns */ - else if (code == UNOQUORUM) { /* this guy is still recovering */ + + /* + * Means that this requires a sync site to work or this guy is still + * recovering. + */ + if (code == UNOTSYNC || code == UNOQUORUM) { someCode = code; count++; continue; diff --git a/src/libafsauthent/afsauthent.def b/src/libafsauthent/afsauthent.def index 02c4825..7957494 100644 --- a/src/libafsauthent/afsauthent.def +++ b/src/libafsauthent/afsauthent.def @@ -111,7 +111,7 @@ EXPORTS ; ka_KeyCheckSum @110 /* duplicate */ rx_Finalize @111 rx_InitHost @112 - VOTE_GetSyncSite @113 +; VOTE_GetSyncSite @113 /* broken */ ubik_RefreshConn @114 rx_SetSecurityConfiguration @115 pioctl_utf8 @116 diff --git a/src/ubik/liboafs_ubik.la.sym b/src/ubik/liboafs_ubik.la.sym index 679bad6..4ca62db 100644 --- a/src/ubik/liboafs_ubik.la.sym +++ b/src/ubik/liboafs_ubik.la.sym @@ -2,7 +2,6 @@ EndDISK_GetFile StartDISK_GetFile VOTE_Debug VOTE_DebugOld -VOTE_GetSyncSite VOTE_SDebug VOTE_SDebugOld VOTE_XDebug diff --git a/src/ubik/ubik_int.xg b/src/ubik/ubik_int.xg index 0d6e202..78b2517 100644 --- a/src/ubik/ubik_int.xg +++ b/src/ubik/ubik_int.xg @@ -181,7 +181,12 @@ DebugOld (OUT ubik_debug_old *db) = VOTE_DEBUG_OLD; SDebugOld (IN afs_int32 which, OUT ubik_sdebug_old *db) = VOTE_SDEBUG_OLD; -GetSyncSite (IN afs_int32 *site) = VOTE_GETSYNCSITE; +/* + * The output of GetSyncSite (site) is mistakenly marked as an input argument. + * As a result, this RPC can't be used to find the current sync-site. + */ + +/* GetSyncSite (IN afs_int32 *site) = VOTE_GETSYNCSITE; */ Debug (OUT ubik_debug *db) = VOTE_DEBUG; diff --git a/src/ubik/ubikclient.c b/src/ubik/ubikclient.c index 186c620..2e6f672 100644 --- a/src/ubik/ubikclient.c +++ b/src/ubik/ubikclient.c @@ -345,62 +345,6 @@ static int synccount = 0; -/*! - * \brief Call this after getting back a #UNOTSYNC. - * - * \note Getting a #UNOTSYNC error code back does \b not guarantee - * that there is a sync site yet elected. However, if there is a sync - * site out there somewhere, and you're trying an operation that - * requires a sync site, ubik will return #UNOTSYNC, indicating the - * operation won't work until you find a sync site - */ -static int -try_GetSyncSite(struct ubik_client *aclient, afs_int32 apos) -{ - struct rx_peer *rxp; - afs_int32 code; - int i; - afs_int32 thisHost, newHost; - struct rx_connection *tc; - short origLevel; - - origLevel = aclient->initializationState; - - /* get this conn */ - tc = aclient->conns[apos]; - if (tc && rx_ConnError(tc)) { - aclient->conns[apos] = (tc = ubik_RefreshConn(tc)); - } - if (!tc) { - return -1; - } - - /* now see if we can find the sync site host */ - code = VOTE_GetSyncSite(tc, &newHost); - if (aclient->initializationState != origLevel) { - return -1; /* somebody did a ubik_ClientInit */ - } - - if (!code && newHost) { - newHost = htonl(newHost); /* convert back to network order */ - - /* - * position count at the appropriate slot in the client - * structure and retry. If we can't find in slot, we'll just - * continue through the whole list - */ - for (i = 0; i < MAXSERVERS; i++) { - rxp = rx_PeerOf(aclient->conns[i]); - thisHost = rx_HostOf(rxp); - if (!thisHost) { - return -1; - } else if (thisHost == newHost) { - return i; /* we were told to use this one */ - } - } - } - return -1; -} #define NEED_LOCK 1 #define NO_LOCK 0 @@ -497,9 +441,7 @@ ubik_Call_New(int (*aproc) (), struct ubik_client *aclient, { afs_int32 code, rcode; afs_int32 count; - afs_int32 temp; int pass; - int stepBack; short origLevel; LOCK_UBIK_CLIENT(aclient); @@ -510,7 +452,6 @@ ubik_Call_New(int (*aproc) (), struct ubik_client *aclient, /* Do two passes. First pass only checks servers known running */ for (aflags |= UPUBIKONLY, pass = 0; pass < 2; pass++, aflags &= ~UPUBIKONLY) { - stepBack = 0; count = 0; while (1) { code = @@ -525,17 +466,7 @@ ubik_Call_New(int (*aproc) (), struct ubik_client *aclient, } rcode = code; /* remember code from last good call */ - if (code == UNOTSYNC) { /* means this requires a sync site */ - if (aclient->conns[3]) { /* don't bother unless 4 or more srv */ - temp = try_GetSyncSite(aclient, count); - if (aclient->initializationState != origLevel) { - goto restart; /* somebody did a ubik_ClientInit */ - } - if ((temp >= 0) && ((temp > count) || (stepBack++ <= 2))) { - count = temp; /* generally try to make progress */ - } - } - } else if ((code >= 0) && (code != UNOQUORUM)) { + if ((code >= 0) && (code != UNOQUORUM) && (code != UNOTSYNC)) { UNLOCK_UBIK_CLIENT(aclient); return code; /* success or global error condition */ } @@ -556,8 +487,8 @@ ubik_Call(int (*aproc) (), struct ubik_client *aclient, long p5, long p6, long p7, long p8, long p9, long p10, long p11, long p12, long p13, long p14, long p15, long p16) { - afs_int32 rcode, code, newHost, thisHost, i, count; - int chaseCount, pass, needsync, inlist, j; + afs_int32 rcode, newHost, thisHost, i, count; + int pass, needsync, inlist, j; struct rx_connection *tc; struct rx_peer *rxp; short origLevel; @@ -574,7 +505,7 @@ ubik_Call(int (*aproc) (), struct ubik_client *aclient, restart: origLevel = aclient->initializationState; rcode = UNOSERVERS; - chaseCount = inlist = needsync = 0; + inlist = needsync = 0; LOCK_UCLNT_CACHE; for (j = 0; ((j < SYNCCOUNT) && calls_needsync[j]); j++) { @@ -597,24 +528,6 @@ ubik_Call(int (*aproc) (), struct ubik_client *aclient, if (aclient->syncSite) { newHost = aclient->syncSite; /* already in network order */ aclient->syncSite = 0; /* Will reset if it works */ - } else if (aclient->conns[3]) { - /* If there are fewer than four db servers in a cell, - * there's no point in making the GetSyncSite call. - * At best, it's a wash. At worst, it results in more - * RPCs than you would otherwise make. - */ - tc = aclient->conns[count]; - if (tc && rx_ConnError(tc)) { - aclient->conns[count] = tc = ubik_RefreshConn(tc); - } - if (!tc) - break; - code = VOTE_GetSyncSite(tc, &newHost); - if (aclient->initializationState != origLevel) - goto restart; /* somebody did a ubik_ClientInit */ - if (code) - newHost = 0; - newHost = htonl(newHost); /* convert to network order */ } else { newHost = 0; } @@ -629,8 +542,6 @@ ubik_Call(int (*aproc) (), struct ubik_client *aclient, if (!thisHost) break; if (thisHost == newHost) { - if (chaseCount++ > 2) - break; /* avoid loop asking */ count = i; /* this index is the sync site */ break; } @@ -693,8 +604,8 @@ afs_int32 ubik_CallRock(struct ubik_client *aclient, afs_int32 aflags, ubik_callrock_func proc, void *rock) { - afs_int32 rcode, code, newHost, thisHost, i, _ucount; - int chaseCount, pass, needsync; + afs_int32 rcode, newHost, thisHost, i, _ucount; + int pass, needsync; struct rx_connection *tc; struct rx_peer *rxp; short origLevel; @@ -706,7 +617,7 @@ ubik_CallRock(struct ubik_client *aclient, afs_int32 aflags, restart: origLevel = aclient->initializationState; rcode = UNOSERVERS; - chaseCount = needsync = 0; + needsync = 0; /* * First pass, we try all servers that are up. @@ -721,25 +632,6 @@ ubik_CallRock(struct ubik_client *aclient, afs_int32 aflags, if (aclient->syncSite) { newHost = aclient->syncSite; /* already in network order */ aclient->syncSite = 0; /* Will reset if it works */ - } else if (aclient->conns[3]) { - /* - * If there are fewer than four db servers in a cell, - * there's no point in making the GetSyncSite call. - * At best, it's a wash. At worst, it results in more - * RPCs than you would otherwise make. - */ - tc = aclient->conns[_ucount]; - if (tc && rx_ConnError(tc)) { - aclient->conns[_ucount] = tc = ubik_RefreshConn(tc); - } - if (!tc) - break; - code = VOTE_GetSyncSite(tc, &newHost); - if (aclient->initializationState != origLevel) - goto restart; /* somebody did a ubik_ClientInit */ - if (code) - newHost = 0; - newHost = htonl(newHost); /* convert to network order */ } else { newHost = 0; } @@ -755,8 +647,6 @@ ubik_CallRock(struct ubik_client *aclient, afs_int32 aflags, if (!thisHost) break; if (thisHost == newHost) { - if (chaseCount++ > 2) - break; /* avoid loop asking */ _ucount = i; /* this index is the sync site */ break; } diff --git a/src/ubik/vote.c b/src/ubik/vote.c index e7a3577..daa1dc5 100644 --- a/src/ubik/vote.c +++ b/src/ubik/vote.c @@ -536,20 +536,6 @@ SVOTE_DebugOld(struct rx_call * rxcall, /*! - * \brief Get the sync site; called by remote servers to find where they should go. - */ -afs_int32 -SVOTE_GetSyncSite(struct rx_call * rxcall, - afs_int32 * ahost) -{ - afs_int32 temp; - - temp = uvote_GetSyncSite(); - *ahost = ntohl(temp); - return 0; -} - -/*! * \brief Called once/run to init the vote module */ int