This patch ensures that the same fill function is called once so to
prevent any possible issues.
Nevertheless, calling a fill function repeatedly in
''fill_active_slots'' will not lead to any obvious change in existing
behavior, though performance might be affected.
''add_fill_action'' checks if the function to be added has already been
added. Allocation of memory for the list ''fill_chain*'' is postponed
until the check passes, unlike previously.
Signed-off-by: Tay Ray Chuan <redacted>
---
http.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index ee58799..cdedeb6 100644
--- a/http.c
+++ b/http.c
@@ -408,13 +408,17 @@ static struct fill_chain *fill_cfg = NULL;
void add_fill_function(void *data, int (*fill)(void *))
{
- struct fill_chain *new = xmalloc(sizeof(*new));
+ struct fill_chain *new;
struct fill_chain **linkp = &fill_cfg;
+ for (;*linkp; linkp = &(*linkp)->next)
+ if ((*linkp)->fill == fill)
+ return;
+
+ new = xmalloc(sizeof(*new));
new->data = data;
new->fill = fill;
new->next = NULL;
- while (*linkp)
- linkp = &(*linkp)->next;
+
*linkp = new;
}
--
1.6.2.rc1
Tay Ray Chuan [off-list ref] writes:
This patch ensures that the same fill function is called once so to
prevent any possible issues.
Nevertheless, calling a fill function repeatedly in
''fill_active_slots'' will not lead to any obvious change in existing
behavior, though performance might be affected.
''add_fill_action'' checks if the function to be added has already been
added. Allocation of memory for the list ''fill_chain*'' is postponed
until the check passes, unlike previously.
Could you care to explain the following a bit better?
- what "possible issues" you are addressing;
- what changes in the behaviour that are not "obvious" we would be
suffering from, if we apply this patch;
- in what situation the performance _might_ be affected, in what way and
to what extent.
If the patch author does not have clear answers to these questions, how
can others decide if it is worth reading the patch to judge if it is worth
applying?
In other words, I'd expect you to explain the issues like this:
add_fill_function() adds the same fill function twice on the fill_cfg
list; this causes THIS and THAT breakages when the fill function is
called twice.
Ignore add_fill_function() when fill_cfg list already has the function
registered on it to fix this issue. Note however that the new code may
behave very inefficiently under this situation:
- XYZZY happens, then
- FROTZ happens, and then
- NITFOL happens.
In such a case we end up doing FROTZ repeatedly, and ...; we might
want to later optimize this, but a correctly working code is more
important than efficient code that works most of the time but silently
breaks in some cases.
We need to iterate over the existing entries in fill_cfg list to find
duplicates, which may look like an overhead, but the existing function
already needed to do so to queue the new entry at the end anyway, so
this is nothing new.
Hi,
On Sun, Mar 8, 2009 at 4:18 AM, Junio C Hamano [off-list ref] wrote:
Could you care to explain the following a bit better?
- what "possible issues" you are addressing;
- what changes in the behaviour that are not "obvious" we would be
suffering from, if we apply this patch;
- in what situation the performance _might_ be affected, in what way and
to what extent.
(note: "repeatedly" here means looping over it, eg. while(condition)
fill_function(). )
Thanks for taking the time to give this clear and detailed example explanation.
However, at this point of time, I couldn't come up with a convincing
instance of where
*a fill function is added twice or more, and as a result
*something breaks as a result of invoking the function repeatedly
that was why I used the word "possible" as in "possible issues",
because this patch doesn't solve any existing issues (at least none
that I know of now).
Calling a fill function repeatedly won't break behaviour, because fill
functions (those that are currently defined in git) are designed to be
called repeatedly. But it's just useless to call the same fill
function repeatedly without any reason.
So should I still address the "THIS and THAT breakages"?
--
Cheers,
Ray Chuan