http: add_fill_function checks if function has been added

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.

---
 http.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

index ee58799..cdedeb6 100644
@@ -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

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,

(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

Your above explanation is good, but it was given after I asked ;-). Making unnecessary call repeatedly counts as a breakage your patch fixes, right?

I didn't look at the callers of add_fill_function(), but "fill" takes a callback data and different invocation of add_fill_function() could be passing different callback data. In such a case, doesn't it feel wrong to omit the "duplicated" calls to register the fill callback? Your patch makes me suspect that it _might_ be better to fix the callers not to call the function repeatedly when they know they only want one-shot invocation.

Hi,

Omitting duplicate calls in fill_active_slots does not mean that repeated calls of the fill functions won't take place.

In the current use instances in http-push and http-walker, run_active_slot and finish_all_active_slots (which calls run_active_slot) will always be called, because they are the functions that actually start the http requests[1], and that means the fill functions will be called repeated, because

run_active_slot, in its while loop, repeatedly calls
step_active_slots calls
fill_active_slots calls
our fill functions.

fill_active_slots is the only direct caller of fill functions (duplicate ones included).

So we can be sure that the fill functions are called repeatedly (at least for the slot's active duration).

[1] Actually calling step_active_slots will start http requests, without you having to call run_active_slot.

Cheers,
Ray Chuan

That is not what I was worried about. There are two callsites to add_fill_function().

http-push.c:2439: add_fill_function(NULL, fill_active_slot);
http-walker.c:936: add_fill_function(walker, (int (*)(void *)) fill_active_slot);

The caller in http-push.c always passes NULL as the callback data for the fill_active_slot callback, which means that the callback function, as long as it does not depend on the state other than the callback function parameter (which is always NULL), will do the same thing whether you queue it only once or as many times as add_fill_function() was called, which the improvement your patch brings in.

But it is not immediately obvious in http-walker.c callchain if multiple calls to add_fill_function(), if they were ever made, give the same "walker" as the callback data. Your patch only looks at the function but not the callback data for its filtering. Doesn't it mean that if a caller made two calls to add_fill_function() with walker#1 and then walker#2 as the callback data, you would discard the request for walker#2 and call the callback function fill_active_slot() with walker#1 inside run_active_slot repeatedly, without ever calling it for walker#2?

I do not know if that actually happens in the current http-walker.c codepath, or the caller _happens to_ call the add_fill_function() only once and making my worry a non-issue, but as a patch submitter, you would know the answer to the question.

If it is the former, then your change is introducing a bug. If it is the latter, your change won't break anything immediately, but still introduces a potential bug waiting to happen, as the API looks as if you can call add_fill_function() to ask for callback functions to be called with different callback data and the caller can rely on both of them to be called at appropriate times, but in reality that guarantee does not hold.

Hi,

I'm sorry I didn't catch this from your earlier message; on a re-reading I caught it.

You're right, in addition to checking whether the callback is the same, I should also have checked the callback data, before discarding the invocation of add_fill_function as a duplicate.

That indeed is true, but afaik, none of the two instances of the API usage does that -- calling callback functions with different callback data to change behaviour. The fill functions are basically used to "clear" the request queue.

Look at the fill function in http-walker.c. Yes, it's true that callback data is passed on by the fill function, unlike the one in http-push.c, which is passed NULL. But it doesn't really seem to use the callback data. It loops through the request queue to find requests that have yet to start (ie. their state is WAITING), and starts them if they haven't start yet. The request object (struct file_request) already contains a pointer to the callback data (walker), so it doesn't really need it.

Nevertheless, I'll make add_fill_function additionally check the callback data.

Cheers,
Ray Chuan