Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2003-12-10 05:21:12 CET
Let's take a step back.
John Szakmeister noticed that entries were read from the baton after it had been closed, and you agreed that that was not a good idea. We fixed the instance that he found. My thoughts and actions proceeded like this:
+ Programmatically prevent any use of the baton after closure.
+ Fix all such uses.
I checked in unclean, inconsistent code, and you pointed that out and I reverted it. Now we should separately reconsider the design questions:
+ Do we want to fix _any_ of the existing uses after closure?
+ Do we want to programmatically prevent _some_ uses after closure?
Forget my hard-line "strictly no use after closure" policy, and assess such uses subjectively.
I think we should fix at least the case in svn_wc_process_committed where it deletes the whole directory tree and then still attempts to recurse into the child entries. Maybe we don't need to "fix" the case where just the "path" field is retrieved. The third case involves retrieving a parent access baton from the associated set; I don't know but it seems like it would be better (in an abstract, logical sense) not to do so.
I also think we should try to prevent most kinds of use after closure if we can do so cleanly and simply. (The implementation would not be of the "poisoning" kind - see below.) However, this should probably be done in a separate, later patch if at all.
Philip Martin wrote:
>>> Can you explain the constraints you are attempting to enforce?
The poisoning strategy was intended to be active in debug mode only. (I know I checked in code where the memset was done unconditionally.) In following a "poisoning" strategy I would have considered poisoning the entries pointed to by the entries hash, and all other memory directly and indirectly owned by the baton.
However, you made me realise that a "poisoning" strategy is more appropriate as a debugging technique for managers of low-level data of unknown structure. Here, where we have a known structure, an explicit marker is more appropriate. I'm now thinking of just setting the "type" field to "closed", and checking it on entry to most of the public functions with "assert (...type != ...closed)".
This is an archived mail posted to the Subversion Dev mailing list.