From 44da0b32ee192017a080e26037e4f4b18e90717c Mon Sep 17 00:00:00 2001 From: Simon Wilkinson Date: Sun, 25 Apr 2010 20:14:02 +0100 Subject: [PATCH] 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 Tested-by: Derrick Brashear --- src/vol/volume.c | 70 +++++++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 25 deletions(-) diff --git a/src/vol/volume.c b/src/vol/volume.c index 177c230f4..e19487829 100644 --- 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); -- 2.39.5