Re: Probable bug in file run-command.c function clear_child_for_cleanup
Hi guys, Sorry for the delayed reply - what passes for my real life intruded somewhat. I'll get on to it today, but please be aware this will be my first-ever patch for ANY project, so am likely to foul up the process. I am reading the How To Submit Patches document even now Cheers, David On 10/09/12 21:12, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Mon, Sep 10, 2012 at 01:00:35PM -0700, Junio C Hamano wrote: And to conclude my bikeshedding for the day: Shouldn't last ideally be called something like prev instead? It's the previously visited element, not the last element in the list. It is the last element visited (just as last week is not the end of the world), but yes, it is ambiguous, and prev is not. Either is fine by me. OK, so who's gonna do the honors? I was hoping to give David a chance to submit his first-ever patch to git. OK. David, is it OK for us to expect a patch from you sometime not in distant future (it is an old bug we survived for a long time and nothing ultra-urgent)? -- David Gould, Personal Trainer Register of Kettlebell Professionals INWA Nordic Walking Instructor Optimise Fitness Ltd -- fit for life 01264 720709 www.optimisefitness.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] clear_child_for_cleanup must correctly manage children_to_clean
Iterate through children_to_clean using 'next' fields but with an
extra level of indirection. This allows us to update the chain when
we remove a child and saves us managing several variables around
the loop mechanism.
---
run-command.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index f9922b9..c42d63b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)
static void clear_child_for_cleanup(pid_t pid)
{
- struct child_to_clean **last, *p;
+ struct child_to_clean **pp;
- last = children_to_clean;
- for (p = children_to_clean; p; p = p-next) {
- if (p-pid == pid) {
- *last = p-next;
- free(p);
+ for (pp = children_to_clean; *pp; pp = (*pp)-next) {
+ if ((*pp)-pid == pid) {
+ struct child_to_clean *clean_me = *pp;
+ *pp = clean_me-next;
+ free(clean_me);
return;
}
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] fix broken list iteration in clear_child_for_cleanup
We iterate through the list of children to cleanup, but do not keep
our last pointer up to date. As a result, we may end up cutting off
part of the list instead of removing a single element.
Iterate through children_to_clean using 'next' fields but with an
extra level of indirection. This allows us to update the chain when
we remove a child and saves us managing several variables around
the loop mechanism.
Signed-off-by: David Gould da...@optimisefitness.com
Acked-by: Jeff King p...@peff.net
---
PATCHv2 updates PATCH only in the commit message: Peff suggested
both a helpful subject and a more-informative introduction.
run-command.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index f9922b9..c42d63b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -53,13 +53,13 @@ static void mark_child_for_cleanup(pid_t pid)
static void clear_child_for_cleanup(pid_t pid)
{
- struct child_to_clean **last, *p;
+ struct child_to_clean **pp;
- last = children_to_clean;
- for (p = children_to_clean; p; p = p-next) {
- if (p-pid == pid) {
- *last = p-next;
- free(p);
+ for (pp = children_to_clean; *pp; pp = (*pp)-next) {
+ if ((*pp)-pid == pid) {
+ struct child_to_clean *clean_me = *pp;
+ *pp = clean_me-next;
+ free(clean_me);
return;
}
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Probable bug in file run-command.c function clear_child_for_cleanup
Hi,
This is probably the wrong way to do this, and I'm sorry if I end up
wasting someone's time. That said, here goes
While idly browsing through the git source (as you do on a sunny Sunday
afternoon), I spotted the following code (that appears to be wrong) in
the file https://github.com/git/git/blob/master/run-command.c It's the
same in branches maint, next and pu. The branch todo gives me a 404.
(line 53 is here)
static void clear_child_for_cleanup(pid_t pid)
{
struct child_to_clean **last, *p;
last = children_to_clean;
for (p = children_to_clean; p; p = p-next) {
if (p-pid == pid) {
*last = p-next;
free(p);
return;
}
}
}
(line 67 is here)
It appears that last is intended to point to the next field that's being
updated, but fails to follow the p pointer along the chain. The result
is that children_to_clean will end up pointing to the entry after the
deleted one, and all the entries before it will be lost. It'll only be
fine in the case that the pid is that of the first entry in the chain.
You want something like:
for (... {
if (... {
...
}
last = p-next;
}
or (probably clearer, but I haven't read your coding style guide, if
there is one, and some people don't like this approach)
for (p = children_to_clean; p; last = p-next, p = p-next) {
...
Cheers,
David
--
David Gould, Personal Trainer
Register of Kettlebell Professionals
INWA Nordic Walking Instructor
Optimise Fitness Ltd -- fit for life
01264 720709
www.optimisefitness.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
