You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2003/12/08 22:39:56 UTC
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
julianfoad@tigris.org writes:
> Author: julianfoad
> Date: Mon Dec 8 14:51:27 2003
> New Revision: 7961
>
> Modified:
> trunk/subversion/libsvn_wc/adm_ops.c
> trunk/subversion/libsvn_wc/lock.c
> trunk/subversion/libsvn_wc/log.c
> Log:
> Avoid using an adm_access baton after it has been closed.
>
> * subversion/libsvn_wc/log.c (svn_wc__run_log):
> * subversion/libsvn_wc/adm_ops.c (svn_wc_process_committed,
> svn_wc_remove_from_revision_control):
> Do not use adm_access after it has been closed.
>
> * subversion/libsvn_wc/lock.c
> (do_close): Invalidate the baton so that mis-use may be caught.
Yuck! I don't like this change.
Using memset to "invalidate" the access baton is a hack, and it
doesn't even work. If we really want to do this, it would be better
to use some sort of positive check rather than relying on memory
poisoning (e.g. closing a write lock sets the lock type to "closed",
perhaps this could be extended to read locks.)
> Modified: trunk/subversion/libsvn_wc/adm_ops.c
> ==============================================================================
> --- trunk/subversion/libsvn_wc/adm_ops.c (original)
> +++ trunk/subversion/libsvn_wc/adm_ops.c Mon Dec 8 14:51:27 2003
> @@ -393,8 +393,9 @@
>
> /* Run the log file we just created. */
> SVN_ERR (svn_wc__run_log (adm_access, NULL, pool));
> -
> - if (recurse)
> +
> + /* Recurse if required, unless we have just removed the entire directory. */
> + if (recurse && svn_wc_adm_locked (adm_access))
This call of svn_wc_adm_locked is using an access baton after it has
been closed, the very thing you are trying to prevent! The memset
doesn't catch it, since the zero it produces looks like an unlocked
access baton.
> {
> apr_hash_t *entries;
> apr_hash_index_t *hi;
> @@ -1721,6 +1722,7 @@
> apr_pool_t *subpool = svn_pool_create (pool);
> apr_hash_index_t *hi;
> svn_wc_entry_t incomplete_entry;
> + const char *adm_access_path = svn_wc_adm_access_path (adm_access);
This is storing a pointer to memory allocated by the access baton...
>
> /* ### sanity check: check 2 places for DELETED flag? */
>
> @@ -1861,8 +1863,7 @@
> *non*-recursive dir_remove should work. If it doesn't,
> no big deal. Just assume there are unversioned items in
> there and set "left_something" */
> - err = svn_io_dir_remove_nonrecursive
> - (svn_wc_adm_access_path (adm_access), subpool);
> + err = svn_io_dir_remove_nonrecursive (adm_access_path, subpool);
...and here that memory is accessed after the baton is closed.
Now the memory associated with the access baton remains allocated
until the pool is cleared. 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? Can
you explain the constraints you are attempting to enforce?
Access batons with write locks can not be used to modify the entries
file after they have been closed. Prior to this commit, the validity
of memory associated with an access baton followed the usual rules for
pools. I'm not convinced that adding different memory validity rules
for access baton memory is an improvement, so I think this commit
should be reverted.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by John Szakmeister <jo...@szakmeister.net>.
On Thursday 11 December 2003 11:45, Philip Martin wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > It appears that in copy-test 9, we end up calling adm_retrieve() on
> > a write baton that's been closed though.
>
> When? During the commit? One effect of close is to delete the baton
> from the hash, so retrieve is unlikely to find the baton.
Yeah, during the commit. I did some more investigation and it turns out it's
svn_wc__run_log() going back and retrieving the access baton for the parent.
So, all is well... which you probably knew anyways. :-)
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
John Szakmeister <jo...@szakmeister.net> writes:
> It appears that in copy-test 9, we end up calling adm_retrieve() on
> a write baton that's been closed though.
When? During the commit? One effect of close is to delete the baton
from the hash, so retrieve is unlikely to find the baton.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by John Szakmeister <jo...@szakmeister.net>.
On Wednesday 10 December 2003 12:29, Philip Martin wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > Again, whether or not it was allocated in the pool or not isn't the issue
> > here. It attempting to reuse a resource that you said you were finished
> > with.
>
> You are trying to impose API rules that are not in the present code.
> Originally, libsvn_wc was written without access batons or caching and
> it's speed was a significant problem. By the time the batons and the
> caching were introduced there was a lot of complicated wc code, and
> the batons were introduced without attempting to rewrite the code. The
> resulting API is more or less accidental.
Understood.
> Try thinking of svn_wc_adm_close as "unlock" rather than as "destroy".
> It's the pool lifetime that determines memory validity, the close
> function simply removes locks and log files.
This was actually my concern. That is, if we are using these batons after
we've closed them (removing the locks), and then try to use them again to
re-acquire a lock on a child directory/file. If we're protected against
this, then I'll stop worrying. Otherwise, it looks like we have potential to
lose atomicity in certain cases. I've actually been trying to track one such
case, but have not been able to successfully determine whether it's really a
problem or not. It appears that in copy-test 9, we end up calling
adm_retrieve() on a write baton that's been closed though.
Would there be a problem if I document the fact that it's safe to do read-only
operations on the baton, such as svn_wc_adm_access_path()? My confusion
arised from the fact that the name of the function, svn_wc_adm_close(),
implies that I shouldn't use the baton after this is called.
I'm sorry for spamming the list for what seems/is a meaningless issue. I've
been bitten in a nasty way before from subtlies like this, and I don't care
to see anyone suffer through the headaches of it.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
John Szakmeister <jo...@szakmeister.net> writes:
> Again, whether or not it was allocated in the pool or not isn't the issue
> here. It attempting to reuse a resource that you said you were finished
> with.
You are trying to impose API rules that are not in the present code.
Originally, libsvn_wc was written without access batons or caching and
it's speed was a significant problem. By the time the batons and the
caching were introduced there was a lot of complicated wc code, and
the batons were introduced without attempting to rewrite the code. The
resulting API is more or less accidental.
Try thinking of svn_wc_adm_close as "unlock" rather than as "destroy".
It's the pool lifetime that determines memory validity, the close
function simply removes locks and log files.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by John Szakmeister <jo...@szakmeister.net>.
On Tuesday 09 December 2003 13:09, Philip Martin wrote:
> Julian Foad <ju...@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.
It is seems normal for operations in which you expect that to happen. The
problem is that while the API says that you can call svn_wc_adm_close()
again, it does not say that you are allowed to use it in any other
operations. If we're going to do that, then we should explicitly state that
it is allowed. Otherwise, we should avoid doing so.
> >> 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 reason for trying to enforce that principle (both by trying to
> > fix some current violations and by trying to add automatic
> > detection) is, of course, that use of any resource after it is
> > closed is hard to detect in other ways and is often a source of
> > subtle bugs.
>
> Tools like valgrind, and purify will detect attempts to use the memory
> after a pool has been cleared. I suspect that attempting to write
> your own memory detection stuff is going to be less effective than
> those existing tools. Further, any additional problems that your own
> system detects will be problems introduced by your new rules.
You are right, tools like valgrind will help to point out problems where we
actually try to use freed memory. However, it doesn't necessarily find
violations in our API, which is what I think this is. The point here is not
to catch a memory problem per se, but a problem in the way the API is being
used.
> At present access batons are bound by pool lifetime, just like most
> other things in Subversion. Access batons with write locks are a bit
> special, because they must prevent attempts to modify the entries file
> once the lock file has been removed. The current code already detects
> and prevents such modification. Access batons without write locks are
> just memory structures, there is not even any guarantee that they
> represent the working copy (an access baton with a write lock could
> have modified it). As far as I am concerned there is no need to
> explicitly close read-only batons, simply abondoning them to pool
> cleanup is sufficient.
Again, whether or not it was allocated in the pool or not isn't the issue
here. It attempting to reuse a resource that you said you were finished
with. It may be just a memory structure now, but that doesn't mean that it
always will be, and letting these sorts of mistakes slide only leads to
problems down the road.
I realize this sort of thing may be nit-picky, but I've been in this situation
before and it's a nightmare to go back and fix when many other clients have
come to depend on a 'feature' that wasn't promised.
I'll admit that the first attempt at this was a bit of hack, and some of that
is my fault as well. We should find a better way to do this, but I don't
feel we should let it slide. We should either promise this behavior, or fix
it and find a way to help detect these subtle errors.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE
Posted by Sander Striker <st...@apache.org>.
On Wed, 2003-12-10 at 16:53, Julian Foad wrote:
> Greg Stein wrote:
> >
> > We're trying to stabilize and release 1.0. This kind of work appears to be *very* contrary to that.
>
> Of course I am aware that we are trying to stabilise for release 1.0. The problem is knowing where to
> draw the line between "stabilisation" and "destabilisation". This change was not a critical bug fix
> but nor was it, in my opinion, significantly destabilising.
>
> I hear your point that we should only be doing critical fixes, but this is not the message I have got
> from the mailing list so far. I was led to believe that we are going to branch for stabilisation,
> and that non-critical work can continue on the trunk. I would welcome further clarification.
Since the estimated time to 1.0 is short, I thought we decided to
stabilize on trunk and branch _after_ 1.0. However, if this period is
longer than a few weeks, I would certainly advocate branching as to
not stifle development.
> > Think about it: you had to REVERT the change. What does that say? When was
> [...]
>
> I apologise for checking in poor code, especially at this stage. However, I did post it for pre-commit
> review, and specifically said that I wasn't sure about the "memset" part (which, as far as I am aware,
> is the only bad part). I received only positive feedback. Now, perhaps I didn't leave enough time for
> other reviewers, and anyway I don't blame review, or lack of it, for my mistakes. I was too hasty, and
> I'm sorry for it.
I think Greg snapped at you a bit hard. We know your intentions are
good and the tree is in a healthy state. Let's move on (while slowly,
as to not destabilize the tree ;).
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Stein wrote:
>
> We're trying to stabilize and release 1.0. This kind of work appears to be *very* contrary to that.
Of course I am aware that we are trying to stabilise for release 1.0. The problem is knowing where to draw the line between "stabilisation" and "destabilisation". This change was not a critical bug fix but nor was it, in my opinion, significantly destabilising.
I hear your point that we should only be doing critical fixes, but this is not the message I have got from the mailing list so far. I was led to believe that we are going to branch for stabilisation, and that non-critical work can continue on the trunk. I would welcome further clarification.
> Think about it: you had to REVERT the change. What does that say? When was
[...]
I apologise for checking in poor code, especially at this stage. However, I did post it for pre-commit review, and specifically said that I wasn't sure about the "memset" part (which, as far as I am aware, is the only bad part). I received only positive feedback. Now, perhaps I didn't leave enough time for other reviewers, and anyway I don't blame review, or lack of it, for my mistakes. I was too hasty, and I'm sorry for it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961
- trunk/subversion/libsvn_wc)
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, December 11, 2003 8:55 AM -0600 kfogel@collab.net wrote:
> as the recent wc access baton thing, or Tobias's SSL auth work, should
> only be done if they bring a large benefit, and even then need three
> +1's from full committers to go in?
+1.
I think if you get 3 full committers to explicitly agree that a commit should
go in, it should be fine and shouldn't affect stability.
This is a rather big organizational change, but one I think would go a long
way to achieving the stability goal. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961
- trunk/subversion/libsvn_wc)
Posted by Sander Striker <st...@apache.org>.
On Thu, 2003-12-11 at 18:47, Justin Erenkrantz wrote:
> --On Thursday, December 11, 2003 10:42 AM -0600 kfogel@collab.net wrote:
>
> > Sander Striker <st...@apache.org> writes:
> >> If that is the deal, branch today and backport on 3 +1s.
> >
> > 0.35 issues aren't done.
>
> To be honest, why couldn't those be backported too if we branched today? It
> could be used as a demonstration of the policy for those who haven't been
> introduced to RTC on a branch. And, trunk remains open. -- justin
Exactly my thought.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961
- trunk/subversion/libsvn_wc)
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, December 11, 2003 10:42 AM -0600 kfogel@collab.net wrote:
> Sander Striker <st...@apache.org> writes:
>> If that is the deal, branch today and backport on 3 +1s.
>
> 0.35 issues aren't done.
To be honest, why couldn't those be backported too if we branched today? It
could be used as a demonstration of the policy for those who haven't been
introduced to RTC on a branch. And, trunk remains open. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by kf...@collab.net.
Sander Striker <st...@apache.org> writes:
> If that is the deal, branch today and backport on 3 +1s.
0.35 issues aren't done.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961
- trunk/subversion/libsvn_wc)
Posted by Sander Striker <st...@apache.org>.
On Thu, 2003-12-11 at 15:55, kfogel@collab.net wrote:
> I hate straddling the middle ground, because it seems so wishy-washy,
> but I think Greg Stein's got a point, just not as strong a point as he
> would have it.
>
> We don't need to completely shut down all non-0.35 commits on trunk.
> But we do need to get somewhat stricter than we have been.
>
> Can we agree that until we branch, it's fine to commit riskless things
> like error message fixes, comment fixes, and null checks (where the
> thing *definitely* shouldn't be null)... but that larger changes such
> as the recent wc access baton thing, or Tobias's SSL auth work, should
> only be done if they bring a large benefit, and even then need three
> +1's from full committers to go in?
If that is the deal, branch today and backport on 3 +1s.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by "Jostein Chr. Andersen" <jo...@josander.net>.
On Thursday 11 December 2003 18.15, Ben Collins-Sussman wrote:
> But I'm also sad at the same time. My prediction is that #1093 will
> become the single biggest FAQ/disappointment among the Hordes that try
> svn for the first time after we release 1.0. I really wish we had
> paid more attention to this issue a lot earlier (regret, regret). It
> goes into my "embarrassing shortcomings" bucket -- where
> "non-restartable checkouts" used to live. Oh well.
We can. It's so many bright brains here and all that power can do this,
especially since a lot of stuff and thoughts will have to wait until the
trunk opens for full traffic again.
My 2 øre (0.02 Kroner ;-)
Jostein
--
http://www.josander.net/kontakt/ ||
http://www.josander.net/en/contact/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 -
trunk/subversion/libsvn_wc)
Posted by Ben Collins-Sussman <su...@collab.net>.
On Thu, 2003-12-11 at 11:04, Philip Martin wrote:
> It might be acceptable to release 1.0
> with the current diff, but if we may get complaints.
I agree that cmpilato's decision to table #1093 for post-1.0 is "sane",
especially given our super-conservative 1.0 release mood.
But I'm also sad at the same time. My prediction is that #1093 will
become the single biggest FAQ/disappointment among the Hordes that try
svn for the first time after we release 1.0. I really wish we had paid
more attention to this issue a lot earlier (regret, regret). It goes
into my "embarrassing shortcomings" bucket -- where "non-restartable
checkouts" used to live. Oh well.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 -
trunk/subversion/libsvn_wc)
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
>> Regarding Mike Pilato's proposed fix for issue #1093: that would be a
>> prime candidate for the three +1's. Suggest that we use the issue
>> tracker for recording votes (and if a proposed patch doesn't have an
>> issue, then it needs to get one).
>
> Mike realized there were more interface complexities here, and has
> dropped the idea of getting this into 1.0 -- high sanity, IMHO. He'll
> attach his patch to the issue and unassign it from himself.
After the last diff interface discussion, when we agreed the --old
--new stuff, I implemented the current interface. I didn't change the
underlying diff implementation, and so I think the current interface
promises more than it delivers. It might be acceptable to release 1.0
with the current diff, but if we may get complaints.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
kfogel@collab.net wrote:
>>1.0-stabilization branch. I don't think Tobias's commit should be
>>reverted, and obviously Julian was acting in good faith. Just from
>>this point on, let's please be more conservative.
>
> Since Tobias *didn't* commit, please ignore that comment.
Hehe. I was just checking my logs to be sure. :-)
And a +1 on your +3 requirement before commit proposal.
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by kf...@collab.net.
Crap. Seems my brain is melting from trying to do too many things at
once today. Need to concentrate on #1601 and not do email for a
while! :-)
However, to clear up any confusion I may have caused:
kfogel@collab.net writes:
> 1.0-stabilization branch. I don't think Tobias's commit should be
> reverted, and obviously Julian was acting in good faith. Just from
> this point on, let's please be more conservative.
Since Tobias *didn't* commit, please ignore that comment.
> We definitely need to lose the attitude of "Gotta cram this new UI
> into the trunk before the 1.0 branch, so it's Right for 1.0." There
> will still have to be UI improvements after 1.0 no matter what, we
> need to accept that.
Please apply this reasoning to the commit Tobias didn't make but was
considering :-).
> Regarding Mike Pilato's proposed fix for issue #1093: that would be a
> prime candidate for the three +1's. Suggest that we use the issue
> tracker for recording votes (and if a proposed patch doesn't have an
> issue, then it needs to get one).
Mike realized there were more interface complexities here, and has
dropped the idea of getting this into 1.0 -- high sanity, IMHO. He'll
attach his patch to the issue and unassign it from himself.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by kf...@collab.net.
I hate straddling the middle ground, because it seems so wishy-washy,
but I think Greg Stein's got a point, just not as strong a point as he
would have it.
We don't need to completely shut down all non-0.35 commits on trunk.
But we do need to get somewhat stricter than we have been.
Can we agree that until we branch, it's fine to commit riskless things
like error message fixes, comment fixes, and null checks (where the
thing *definitely* shouldn't be null)... but that larger changes such
as the recent wc access baton thing, or Tobias's SSL auth work, should
only be done if they bring a large benefit, and even then need three
+1's from full committers to go in?
Like Greg, I'm not complaining about a specific commit, just trying to
foster a general attitude. We should be treating the trunk carefully
at this point, even though we're not officially branched, because
after all anything that goes into trunk now is going to be in the
1.0-stabilization branch. I don't think Tobias's commit should be
reverted, and obviously Julian was acting in good faith. Just from
this point on, let's please be more conservative.
We definitely need to lose the attitude of "Gotta cram this new UI
into the trunk before the 1.0 branch, so it's Right for 1.0." There
will still have to be UI improvements after 1.0 no matter what, we
need to accept that.
So that's where I sit, somewhere between Greg and Greg :-).
Regarding Mike Pilato's proposed fix for issue #1093: that would be a
prime candidate for the three +1's. Suggest that we use the issue
tracker for recording votes (and if a proposed patch doesn't have an
issue, then it needs to get one).
-Karl
Greg Stein <gs...@lyra.org> writes:
> Part of the problem here is that it is *very* hard to say "don't commit".
> That implies a certain amount of "I have more say in the project, and I
> can tell you what to do." Even supposing that that right is conferred on
> the "core team" (as you put it) by the community, I would posit that they
> would be quite uncomfortable exercising that right.
>
> Instead, I believe it is more about individual developers making the
> decision themselves. Over the past few years, and I believe moving
> forward, this has worked very well. We all share a very strong notion of
> what 1.0 is all about, and have been able to focus on that goal. ("make
> SVN's feature set on par with CVS, and grab low hanging fruit to make it
> better") I think there are very different ideas on how to approach the
> end game, though. We know that 1.0 is "almost there" but don't quite agree
> on how to get that last 1% :-)
>
> I intended no slight towards you or Julian. I'm concerned about the
> pattern rather than the specifics.
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Dec 10, 2003 at 05:16:35PM +0100, Erik Huelsmann wrote:
>...
> If you (all) want me or Julian to stop
> committing, that's fine. Just tell me so.
Part of the problem here is that it is *very* hard to say "don't commit".
That implies a certain amount of "I have more say in the project, and I
can tell you what to do." Even supposing that that right is conferred on
the "core team" (as you put it) by the community, I would posit that they
would be quite uncomfortable exercising that right.
Instead, I believe it is more about individual developers making the
decision themselves. Over the past few years, and I believe moving
forward, this has worked very well. We all share a very strong notion of
what 1.0 is all about, and have been able to focus on that goal. ("make
SVN's feature set on par with CVS, and grab low hanging fruit to make it
better") I think there are very different ideas on how to approach the
end game, though. We know that 1.0 is "almost there" but don't quite agree
on how to get that last 1% :-)
I intended no slight towards you or Julian. I'm concerned about the
pattern rather than the specifics.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by kf...@collab.net.
"Erik Huelsmann" <e....@gmx.net> writes:
> Ok. Then we will have an explicit moment starting at which commits are not
> fine anymore: the moment of branching?
The moment of branching is when we start getting very picky about
commits. "The moment of branching" means the moment we copy tags/0.35
to branches/1.0-stabilization, on Dec. 17th. From that point on,
we'll scrutinize commits to the branch heavily. Of course, that
doesn't mean we should go hog wild on the trunk :-). Let's try not to
have the two lines diverge too much before 1.0 is released.
Even today, everyone is already conscious that very destabilizing
commits to trunk are a bad idea, even though we haven't officially
branched. (The commit in question was appropriate, IMHO. I've no
complaints about the decision to commit, even though it turned out to
need reversion later).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by Julian Foad <ju...@btopenworld.com>.
Ben Collins-Sussman wrote:
>
> I just wanted to say how happy I am with all the commits coming from
> Julian and Erik, in general. You guys have become the supreme "code
> scrubbers", cleaning up stuff left and right. It's really great!
Thanks, everyone, for comments like this and for keeping up such a high standard of coding which makes it a pleasure to work with you all.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by John Szakmeister <jo...@szakmeister.net>.
On Wednesday 10 December 2003 12:15, Ben Collins-Sussman wrote:
> On Wed, 2003-12-10 at 10:16, Erik Huelsmann wrote:
> > Exactly my understanding of the situation. No, there was no *need* to
> > have it in Subversion 1.0. On the other hand do I feel that we can save a
> > lot of time spent on support and make a generally good impression if the
> > program look good. That is what I'm trying to go for.
>
> I just wanted to say how happy I am with all the commits coming from
> Julian and Erik, in general. You guys have become the supreme "code
> scrubbers", cleaning up stuff left and right. It's really great!
I agree. I wish I could've contributed half as much as Julian.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 -
trunk/subversion/libsvn_wc)
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> I just wanted to say how happy I am with all the commits coming from
> Julian and Erik, in general. You guys have become the supreme "code
> scrubbers", cleaning up stuff left and right. It's really great!
I agree. I objected to this particular change, but I have no problem
with the cleanups in general.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961
- trunk/subversion/libsvn_wc)
Posted by Ben Collins-Sussman <su...@collab.net>.
On Wed, 2003-12-10 at 10:16, Erik Huelsmann wrote:
> Exactly my understanding of the situation. No, there was no *need* to have
> it in Subversion 1.0. On the other hand do I feel that we can save a lot of
> time spent on support and make a generally good impression if the program look
> good. That is what I'm trying to go for.
I just wanted to say how happy I am with all the commits coming from
Julian and Erik, in general. You guys have become the supreme "code
scrubbers", cleaning up stuff left and right. It's really great!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by Erik Huelsmann <e....@gmx.net>.
Feeling spoken to, I just have to react to this.
> We agreed to branch, because the stabilization period will be
> short, but I think we're going to do that just after 0.35, which is
> beta. Right now we're still in alpha.
This is my understanding of the situation too.
> > We can clean and fix and clean and fix and clean and fix for the next
> six
> > damn years. How about we stop and get 1.0 out the door instead?
This is all so true, but I believe I'm not interfering with core development
and neither am I taking lots of time of the core development team. I don't
think that my understanding of different functions in the big picture is big
enough to tacle any big issues. I can, however, contribute to the codebase by
trying to make error messages consistent and find leaking temporary files.
That way, the core development team can - in my opinion - concentrate on the
more important tasks at hand... If you (all) want me or Julian to stop
committing, that's fine. Just tell me so. Submitting patches for review prior to
committing is what we both have been doing for many of the contributed cleanups.
> > If you're
> > about to type 'svn commit' ask yourself: do we really need this? do our
> > users need this for 1.0? is this an improvement so fantastic that it
> > *must* go into the codebase? Does that APR -> svn_io change have to
> > happen? Do we have to fix that error message? Does the comment *really*
> > need to change? Most likely: no. Not at all.
>
> We're still in alpha, and none of the changes you're talking about have
> been destablizing. (Even Julian's didn't break anything, as far as I
> know, although it certainly made the code unclean.)
Exactly my understanding of the situation. No, there was no *need* to have
it in Subversion 1.0. On the other hand do I feel that we can save a lot of
time spent on support and make a generally good impression if the program look
good. That is what I'm trying to go for.
As far as the changes for either the error messages or the svn_io_*
interface not going in: these were explicitly granted by Karl. Once again: Just tell
me stop. No need to get excited about and I won't take it personal. You - and
I - want to get 1.0 and a good product out the door. It seems to me we have
a common goal :-)
> > We're shooting for 1.0. Treat the trunk appropriately.
>
> We're not really at that point yet. There are certainly a lot of
> changes which wouldn't be appropriate for late alpha, but cleanups are
> still fine.
Ok. Then we will have an explicit moment starting at which commits are not
fine anymore: the moment of branching?
Just wanting to not-be-in-the-way and help,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by "Jostein Chr. Andersen" <jo...@josander.net>.
On Thursday 11 December 2003 08.33, Greg Stein wrote:
> > > We're shooting for 1.0. Treat the trunk appropriately.
> >
> > We're not really at that point yet. There are certainly a lot of
> > changes which wouldn't be appropriate for late alpha, but cleanups
> > are still fine.
>
> I disagree. :-) We don't have to agree, and I'm just one voice, but I
> seriously think it *is* time to just stop committing unless it can be
> shown that the change is *very* necessary.
I think Greg is right here. It's many good reasons to "slow" down (and
think) for a short period. Two things that comes right to my mind is:
*Subversion _is_ damned good already. Let's make sure that our child
can walk, run and dance like a cat and and stay as a rock. It's more
that enough to do when the trunk "opens" again.
*We don't really know 100% what the end users and vendors (the svn
guis, IDEs and so on) really needs/likes/hates before after the
1.0 release. It will probably be some other priorities when
Subversion has met the real life.
Jostein
--
http://www.josander.net/kontakt/ ||
http://www.josander.net/en/contact/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by kf...@collab.net.
Greg Hudson <gh...@MIT.EDU> writes:
> I apologize for contributing to confusion here, but... no, we didn't. I
> only wanted to stabilize on the trunk because I thought it would take
> six months (and even then, I wanted to branch at the end of that
> period). We agreed to branch, because the stabilization period will be
> short, but I think we're going to do that just after 0.35, which is
> beta. Right now we're still in alpha.
Yah, what he said.
We haven't branched for 1.0-stabilization yet. After 0.35 is released
on Dec 17th, the story will be different... :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Dec 10, 2003 at 10:53:56AM -0500, Greg Hudson wrote:
> On Wed, 2003-12-10 at 04:10, Greg Stein wrote:
> > Think about it: you had to REVERT the change. What does that say? When was
> > the last time that happend? Maybe during development it happened here and
> > there, but it wasn't common. And now we're in the END GAME, and a commit
> > went in which was Not Right and had to be reverted.
>
> It has happened from time to time. It is rarely foreseen by the person
> making the commit.
Certainly. I would doubt that anybody commits with the intent to revert :-)
> > Look: everybody agreed to staying on the trunk for the 1.0 release.
>
> I apologize for contributing to confusion here, but... no, we didn't. I
Gah... okee dokee. I missed the resolution here. Regardless of where it
happens, the "branch point" is still an arbitrary point in time. The trunk
today has *very* little difference from the branch next week. A commit
today isn't "magically safer" because it occurred now rather than
post-branch. If we chose to branch yesterday or today, then would any
given commit suddenly morph into "oh, well that is unsafe and shouldn't
happen". A commit that is made will introduce an absolute amount of
potential for breakage. At this point in the lifetime of SVN, those
amounts should be as minimal as possible.
> only wanted to stabilize on the trunk because I thought it would take
> six months (and even then, I wanted to branch at the end of that
> period). We agreed to branch, because the stabilization period will be
> short, but I think we're going to do that just after 0.35, which is
> beta. Right now we're still in alpha.
Um. "we're still in alpha [so we're still free to commit]" doesn't really
work here. Our notion of alpha is awfully strict, and the simple fact is
that "beta" (for svn) is "we don't think we're going to change the code
any more", more popularly known as "1.0 release candidate".
Within those guides, the notion that an alpha can suddenly make certain
types of commits somehow "safe" isn't very supportable.
>...
> We're still in alpha,
And I think that is an arbitrary label, and our usage doesn't really
confer the notion of "so these changes are okay".
> and none of the changes you're talking about have
> been destablizing. (Even Julian's didn't break anything, as far as I
> know, although it certainly made the code unclean.)
Agreed. We're talking about potentials here. Each commit has a potential.
We caught this one. How about the recent change to how SSL errors are
tracked. Some basic data structures changed. Was that one safe? Sure, I
bet it was (given my cursory review of it). What about the next change?
I'm not trying to call out a problem with Julian's work or even that one
commit which needed to be reverted. To me, it seems that the notion of
"let's wrap this up and call it 1.0" just hasn't sunk in.
If all commits stop, and attention is turned towards the commits which
*have* to go in, then we'll get good eyeballs on those commits. But while
a dozen commits are occurring every day, it doesn't help the focus.
> > We're shooting for 1.0. Treat the trunk appropriately.
>
> We're not really at that point yet. There are certainly a lot of
> changes which wouldn't be appropriate for late alpha, but cleanups are
> still fine.
I disagree. :-) We don't have to agree, and I'm just one voice, but I
seriously think it *is* time to just stop committing unless it can be
shown that the change is *very* necessary.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: stabilize means STABILIZE (was: svn commit: rev 7961 -
trunk/subversion/libsvn_wc)
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2003-12-10 at 04:10, Greg Stein wrote:
> Think about it: you had to REVERT the change. What does that say? When was
> the last time that happend? Maybe during development it happened here and
> there, but it wasn't common. And now we're in the END GAME, and a commit
> went in which was Not Right and had to be reverted.
It has happened from time to time. It is rarely foreseen by the person
making the commit.
> Look: everybody agreed to staying on the trunk for the 1.0 release.
I apologize for contributing to confusion here, but... no, we didn't. I
only wanted to stabilize on the trunk because I thought it would take
six months (and even then, I wanted to branch at the end of that
period). We agreed to branch, because the stabilization period will be
short, but I think we're going to do that just after 0.35, which is
beta. Right now we're still in alpha.
> We can clean and fix and clean and fix and clean and fix for the next six
> damn years. How about we stop and get 1.0 out the door instead? If you're
> about to type 'svn commit' ask yourself: do we really need this? do our
> users need this for 1.0? is this an improvement so fantastic that it
> *must* go into the codebase? Does that APR -> svn_io change have to
> happen? Do we have to fix that error message? Does the comment *really*
> need to change? Most likely: no. Not at all.
We're still in alpha, and none of the changes you're talking about have
been destablizing. (Even Julian's didn't break anything, as far as I
know, although it certainly made the code unclean.)
> We're shooting for 1.0. Treat the trunk appropriately.
We're not really at that point yet. There are certainly a lot of
changes which wouldn't be appropriate for late alpha, but cleanups are
still fine.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
stabilize means STABILIZE (was: svn commit: rev 7961 - trunk/subversion/libsvn_wc)
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Dec 10, 2003 at 04:21:12AM +0000, Julian Foad wrote:
>...
> 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?
We're trying to stabilize and release 1.0. This kind of work appears to be
*very* contrary to that.
Think about it: you had to REVERT the change. What does that say? When was
the last time that happend? Maybe during development it happened here and
there, but it wasn't common. And now we're in the END GAME, and a commit
went in which was Not Right and had to be reverted.
Come on. Please stop this kind of thing. It isn't going to help us reach a
1.0 release with this happening.
Look: everybody agreed to staying on the trunk for the 1.0 release. That
means treating it with extreme frickin' care. If commits like this happen
even one more time, then we are going to HAVE to branch.
We can clean and fix and clean and fix and clean and fix for the next six
damn years. How about we stop and get 1.0 out the door instead? If you're
about to type 'svn commit' ask yourself: do we really need this? do our
users need this for 1.0? is this an improvement so fantastic that it
*must* go into the codebase? Does that APR -> svn_io change have to
happen? Do we have to fix that error message? Does the comment *really*
need to change? Most likely: no. Not at all.
Is there a memory corruption? Scalability problem? Crasher? Security
violation? Then yah. But how about posting a patch first? Maybe some
discussion? Maybe there are alternatives?
We're shooting for 1.0. Treat the trunk appropriately.
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
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 <ju...@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
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@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.
>> 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 reason for trying to enforce that principle (both by trying to
> fix some current violations and by trying to add automatic
> detection) is, of course, that use of any resource after it is
> closed is hard to detect in other ways and is often a source of
> subtle bugs.
Tools like valgrind, and purify will detect attempts to use the memory
after a pool has been cleared. I suspect that attempting to write
your own memory detection stuff is going to be less effective than
those existing tools. Further, any additional problems that your own
system detects will be problems introduced by your new rules.
At present access batons are bound by pool lifetime, just like most
other things in Subversion. Access batons with write locks are a bit
special, because they must prevent attempts to modify the entries file
once the lock file has been removed. The current code already detects
and prevents such modification. Access batons without write locks are
just memory structures, there is not even any guarantee that they
represent the working copy (an access baton with a write lock could
have modified it). As far as I am concerned there is no need to
explicitly close read-only batons, simply abondoning them to pool
cleanup is sufficient.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> julianfoad@tigris.org writes:
>
>>Author: julianfoad
>>Date: Mon Dec 8 14:51:27 2003
>>New Revision: 7961
>>
>>Modified:
>> trunk/subversion/libsvn_wc/adm_ops.c
>> trunk/subversion/libsvn_wc/lock.c
>> trunk/subversion/libsvn_wc/log.c
>>Log:
>>Avoid using an adm_access baton after it has been closed.
>>
>>* subversion/libsvn_wc/log.c (svn_wc__run_log):
>>* subversion/libsvn_wc/adm_ops.c (svn_wc_process_committed,
>> svn_wc_remove_from_revision_control):
>> Do not use adm_access after it has been closed.
>>
>>* subversion/libsvn_wc/lock.c
>> (do_close): Invalidate the baton so that mis-use may be caught.
>
> Yuck! I don't like this change.
>
> Using memset to "invalidate" the access baton is a hack,
Agreed it's a hack. It is a valid technique when used in the "free" or "close" path of an object manager that does not know the structure of the objects that it manages but in this case we're at a higher level where we do know the structure. I should have done something better, or just used that for experimentation but not committed it.
> and it doesn't even work.
In what way does it not work? In that it doesn't catch _all_ attempts to access it? I know that; the idea was to catch _most_ attempts to use it, or even _some_ attempts.
> If we really want to do this, it would be better
> to use some sort of positive check rather than relying on memory
> poisoning (e.g. closing a write lock sets the lock type to "closed",
> perhaps this could be extended to read locks.)
OK. I'll revert this part right away and then think about a better solution (if we want one at all) for later.
>>Modified: trunk/subversion/libsvn_wc/adm_ops.c
>>==============================================================================
>>--- trunk/subversion/libsvn_wc/adm_ops.c (original)
>>+++ trunk/subversion/libsvn_wc/adm_ops.c Mon Dec 8 14:51:27 2003
>>@@ -393,8 +393,9 @@
>>
>> /* Run the log file we just created. */
>> SVN_ERR (svn_wc__run_log (adm_access, NULL, pool));
>>-
>>- if (recurse)
>>+
>>+ /* Recurse if required, unless we have just removed the entire directory. */
>>+ if (recurse && svn_wc_adm_locked (adm_access))
>
> This call of svn_wc_adm_locked is using an access baton after it has
> been closed, the very thing you are trying to prevent! The memset
> doesn't catch it, since the zero it produces looks like an unlocked
> access baton.
Oh, darn. That was stupid of me. Half of my brain analysed the way that the "memset" was affecting it, but I didn't notice the conceptual contradiction. Thanks for catching that ... and the stuff below.
>>@@ -1721,6 +1722,7 @@
>> apr_pool_t *subpool = svn_pool_create (pool);
>> apr_hash_index_t *hi;
>> svn_wc_entry_t incomplete_entry;
>>+ const char *adm_access_path = svn_wc_adm_access_path (adm_access);
>
> This is storing a pointer to memory allocated by the access baton...
>>@@ -1861,8 +1863,7 @@
>> *non*-recursive dir_remove should work. If it doesn't,
>> no big deal. Just assume there are unversioned items in
>> there and set "left_something" */
>>- err = svn_io_dir_remove_nonrecursive
>>- (svn_wc_adm_access_path (adm_access), subpool);
>>+ err = svn_io_dir_remove_nonrecursive (adm_access_path, subpool);
>
> ...and here that memory is accessed after the baton is closed.
>
> Now the memory associated with the access baton remains allocated
> until the pool is cleared. 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.
> 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.
The reason for trying to enforce that principle (both by trying to fix some current violations and by trying to add automatic detection) is, of course, that use of any resource after it is closed is hard to detect in other ways and is often a source of subtle bugs.
> Access batons with write locks can not be used to modify the entries
> file after they have been closed. Prior to this commit, the validity
> of memory associated with an access baton followed the usual rules for
> pools. I'm not convinced that adding different memory validity rules
> for access baton memory is an improvement, so I think this commit
> should be reverted.
OK. You have convinced me that this change was pretty poor and should be reverted. I will try to create a better version that addresses the concerns, as I believe the principle is sound. (Of course you may dispute the principle, too, if you wish.)
Reverted in r7966.
Apologies for the badness.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org