ubik: death to SVOTE_GetSyncSite 43/14043/10
authorMarcio Barbosa <mbarbosa@sinenomine.net>
Thu, 27 Feb 2020 22:28:14 +0000 (22:28 +0000)
committerBenjamin Kaduk <kaduk@mit.edu>
Tue, 24 Mar 2020 05:18:50 +0000 (01:18 -0400)
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 <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: BuildBot <buildbot@rampaginggeek.com>

src/bucoord/ubik_db_if.c
src/libafsauthent/afsauthent.def
src/ubik/liboafs_ubik.la.sym
src/ubik/ubik_int.xg
src/ubik/ubikclient.c
src/ubik/vote.c

index e602310..14d4880 100644 (file)
@@ -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;
index 02c4825..7957494 100644 (file)
@@ -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
index 679bad6..4ca62db 100644 (file)
@@ -2,7 +2,6 @@ EndDISK_GetFile
 StartDISK_GetFile
 VOTE_Debug
 VOTE_DebugOld
-VOTE_GetSyncSite
 VOTE_SDebug
 VOTE_SDebugOld
 VOTE_XDebug
index 0d6e202..78b2517 100644 (file)
@@ -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;
 
index 186c620..2e6f672 100644 (file)
@@ -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;
                        }
index e7a3577..daa1dc5 100644 (file)
@@ -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