On Wed, 30 Jul 2014, Nathaniel W Filardo wrote:
Hey all,
As part of the cleanup in http://gerrit.openafs.org/#change,11331 I stumbled
upon a few things that probably should go here for review (thanks to Ben for
suggesting).
I see Jeff has already commented on the gerrit change requesting separate
fixes for these two issues. That seems like a good plan to me.
src/volser/vos.c:2155 says "if (entry.serverFlags[j] != ITSROVOL)", but that
field is a bitmask, so I think that that should be "if
(!(entry.serverFlags[j] & ITSROVOL))" (but also renamed to VLSF_ROVOL,
naturally) so that DeleteVolume will not skip RO volumes with other VLSF
bits asserted (e.g., VLSF_DONTUSE or VLSF_NEWREPSITE). Does that seem
right?
This is in the branch that is looking up the entry in the VLDB to fill out
partition and/or server informatin for the delete operation. That said, I
still can't think of a reason why we would want to ignore entries with
other flags set, so it does look like changing "!=" to "&" is the right
thing to do.
src/volser/vsprocs.c:6217 says "entry.serverFlags[idx] = ITSBACKVOL;".
ITSBACKVOL and VLSF_BACKVOL are almost never otherwise used in the codebase,
and in particular the rest of the codebase seems to assume that the backup
volume has VLSF_RWVOL asserted, not VLSF_BACKVOL. I believe that
CheckVolume is here trying to recreate a backup volume record that will get
detected as such, and so should read "entry.serverFlags[idx] = VLSF_RWVOL;".
Does that, too, seem right?
The revert of dcafea769a1cb70c7b1f8763ae4f7b7744b2a436 that already hit
gerrit looks plausible to me. I guess I should go comment on it.
If these behavioral changes seem right, I'll move them out to their own
commits, rather than leaving them in 11331.
Please do, thanks.
-Ben
_______________________________________________
OpenAFS-devel mailing list
OpenAFS-devel@openafs.org
https://lists.openafs.org/mailman/listinfo/openafs-devel
