Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc

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:
> Julian Foad <julianfoad@btopenworld.com> writes:
>
>>Philip Martin wrote:
>>
>>>If using the access baton is wrong, why is using memory allocated
>>>by the access baton OK? If using the memory is OK why should I not
>>>be able to use the access baton to get it?
>>
>>Using that memory was not OK in the absence of any interface
>>promises regarding it. I should have made a duplicate of that
>>string so that it (the duplicate) is definitely not associated with
>>the adm_access baton.
>
> The memory remains valid until the pool is cleared, which is perfectly
> normal in Subversion. I see no reason for that memory not to be used.
> I'm not convinced that imposing non-standard rules on the memory
> associated with access batons is a good idea.

OK.

>>> Can you explain the constraints you are attempting to enforce?
>>
>>The principle was:
>>
>> Do not use an adm_access baton after it has been closed.
>>
>>"Use" means, for example, dereference or pass to another function.
>>I think I would now include the path string and any resource that
>>was owned by the adm_access baton and not explicitly disowned by it.
>
> Are you planning to poison the entries pointed to be the entries hash?
> What about the memory pointed to by the entries themselves? I'd worry
> about the impact on performace.

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)".

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Dec 10 05:15:56 2003