Demand-Attach: Remove dangerous trailing else

Change 9aa09f5e634db8a8b2dedf3a749f2a90ef206e2f introduced a
dangerous trailing else into the VScheduleSalvage_r function:

 #ifdef A
    if (foo) {
      ...
    } else
 #endif
 #ifdef B
    if (bar) {
      ...
    }
 #endif
    something_else()

In a situation where we have A && !B, then something_else() ends up
only being run when foo is false. Given that something_else() is
VOL_LOCK, this will not end well.

In the real world, we hit this problen when we build the volume
package with SALVSYNC_BUILD_CLIENT and !FSYNC_BUILD_CLIENT - in
other words, whilst building the fileserver.

Change-Id: I97e07a6e730df8ac480d295b4cf30b0695ace511
Reviewed-on: http://gerrit.openafs.org/1832
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

diff --git a/src/vol/volume.c b/src/vol/volume.c
index 177c230f44cbb2ba836e761ce0ac35e4eb180987..e19487829ffa864154b8dcec8b4672dd1b540854 100644 (file)
--- a/src/vol/volume.c
+++ b/src/vol/volume.c
@@ -4947,6 +4947,49 @@ VUpdateSalvagePriority_r(Volume * vp)
 
 
 #if defined(SALVSYNC_BUILD_CLIENT) || defined(FSSYNC_BUILD_CLIENT)
+
+/* A couple of little helper functions. These return true if we tried to
+ * use this mechanism to schedule a salvage, false if we haven't tried.
+ * If we did try a salvage then the results are contained in code.
+ */
+
+static inline int
+try_SALVSYNC(Volume *vp, char *partName, int *code) {
+#ifdef SALVSYNC_BUILD_CLIENT
+    if (VCanUseSALVSYNC()) {
+       /* can't use V_id() since there's no guarantee
+        * we have the disk data header at this point */
+       *code = SALVSYNC_SalvageVolume(vp->hashid,
+                                      partName,
+                                      SALVSYNC_SALVAGE,
+                                      vp->salvage.reason,
+                                      vp->salvage.prio,
+                                      NULL);
+       return 1;
+    }
+#endif
+    return 0;
+}
+
+static_inline int
+try_FSSYNC(Volume *vp, char *partName, int *code) {
+#ifdef FSSYNC_BUILD_CLIENT
+    if (VCanUseFSSYNC()) {
+       /*
+        * If we aren't the fileserver, tell the fileserver the volume
+        * needs to be salvaged. We could directly tell the
+        * salvageserver, but the fileserver keeps track of some stats
+        * related to salvages, and handles some other salvage-related
+        * complications for us.
+         */
+        *code = FSYNC_VolOp(vp->hashid, partName,
+                            FSYNC_VOL_FORCE_ERROR, FSYNC_SALVAGE, NULL);
+       return 1;
+    }
+#endif /* FSSYNC_BUILD_CLIENT */
+    return 0;
+}
+
 /**
  * schedule a salvage with the salvage server or fileserver.
  *
@@ -5017,31 +5060,8 @@ VScheduleSalvage_r(Volume * vp)
        state_save = VChangeState_r(vp, VOL_STATE_SALVSYNC_REQ);
        VOL_UNLOCK;
 
-#ifdef SALVSYNC_BUILD_CLIENT
-       if (VCanUseSALVSYNC()) {
-           /* can't use V_id() since there's no guarantee
-            * we have the disk data header at this point */
-           code = SALVSYNC_SalvageVolume(vp->hashid,
-                                         partName,
-                                         SALVSYNC_SALVAGE,
-                                         vp->salvage.reason,
-                                         vp->salvage.prio,
-                                         NULL);
-       } else
-#endif /* SALVSYNC_BUILD_CLIENT */
-#ifdef FSSYNC_BUILD_CLIENT
-        if (VCanUseFSSYNC()) {
-            /*
-             * If we aren't the fileserver, tell the fileserver the volume
-             * needs to be salvaged. We could directly tell the
-             * salvageserver, but the fileserver keeps track of some stats
-             * related to salvages, and handles some other salvage-related
-             * complications for us.
-             */
-            code = FSYNC_VolOp(vp->hashid, partName,
-                               FSYNC_VOL_FORCE_ERROR, FSYNC_SALVAGE, NULL);
-        }
-#endif /* FSSYNC_BUILD_CLIENT */
+       assert(try_SALVSYNC(vp, partName, &code) ||
+              try_FSSYNC(vp, partName, &code));
 
        VOL_LOCK;
        VChangeState_r(vp, state_save);