Re: [tabled patch] abstract out TCP-write code

[Pete Zaitcev <zaitcev@xxxxxxxxxx>]

On Wed, 22 Sep 2010 20:09:08 -0400
Jeff Garzik <jeff@xxxxxxxxxx> wrote:

> @@ -1410,7 +1224,7 @@ static void tcp_cli_event(int fd, short events, void *userdata)
>  
>  	do {
>  		loop = cli->evt_table[cli->state](cli, events);
> -		loop |= cli_write_run_compl();
> +		loop |= atcp_write_run_compl(&cli->wst);
>  	} while (loop);
>  }

This cannot be right. Please see commit
d1a45fca7908b7128ed4fe2ab611111f02ee938f:

    tabled: fix running completions over disposed cli
    
    Miracluously this never actually crashed on me, but I added unrelated
    debugging printout into the dispatch routine and it printed weird
    values. Then it dawned on me that a state change function may dispose
    of the struct cli, in which case cli_write_run_compl is use-after-free.
    
    It may seem that checking if the old state was evt_dispose before
    running cli_write_run_compl is an expedient fix, but that does not
    work, because we do not always dispose of the cli in such case.
    If the cli to be disposed still has anything in the queue, we
    need to continue to deliver events, and for that we have to
    run outstanding completions.
    
    So, we go a longer route and re-hook the list of completions
    to a per-server global instead of a client. The patch is straight-
    forward. The only thing we need to be careful is to make sure
    that no outstanding completions are left in the queue before
    freeing a client struct. This is ensured by force-running completions.

    One other necessary change was to add a back poiter from a completion
    to the current client. This is because one caller needed the client
    pointer (object_get_more).

Speaking of backpointers, I think it would be much cleaner
to get rid of two-argument format for callback. It stinks of
special-casing. Either throw a back pointer into the first word
of buf, or create some kind of object/struct passed as
argument to atcp_writeq(), that's what I would do.

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html