Hi Gerrit and all.
Thank you for your fast reply. My comments follow below
On Sat, Sep 12, 2009 at 15:32, Gerrit Renker <gerrit@xxxxxxxxxxxxxx> wrote:
> Hi Ivo,
>
> you have made a really good job of sticking to code conventions (see other
> posting). There are a few things that needed tending to in the first patch.
>
> (1) Version changes
> ===================
> It seems that you applied something like s/\*\*/*/g to the first instance of
> the patch, in order to remove duplicate asterisks. This caused a problem:
>
> --- tfrc_sp_receiver_01.patch 2009/09/12 08:37:12 1.1
> +++ tfrc_sp_receiver_01.patch 2009/09/08 17:34:40
> @@ -1001,8 +1001,8 @@ Index: dccp_tree_work4/net/dccp/ccids/li
> +
> +#endif
> +
> -+extern int tfrc_sp_tx_hist_add(struct tfrc_tx_hist_entry **headp, u64 seqno);
> -+extern void tfrc_sp_tx_hist_purge(struct tfrc_tx_hist_entry **headp);
> ++extern int tfrc_sp_tx_hist_add(struct tfrc_tx_hist_entry *headp, u64 seqno);
> ++extern void tfrc_sp_tx_hist_purge(struct tfrc_tx_hist_entry *headp);
>
> I have reverted the bug, also to minimise the difference to the existing (non
> TFRC-SP) files.
>
>
> (2) Other changes that I edited out
> ===================================
> (Other than whitespace changes.)
>
> net/dccp/ccids/lib/loss_interval_sp.c
> -------------------------------------
> I replaced the following dead code
> if ((tfrc_lh_slab != NULL))
> return 0;
>
> if (tfrc_lh_slab != NULL) {
> kmem_cache_destroy(tfrc_lh_slab);
> tfrc_lh_slab = NULL;
> }
> return -ENOBUFS;
> with
> return tfrc_lh_slab == NULL ? -ENOBUFS : 0;
>
> Also separated the conditions
> + if ((len <= 0) ||
> + (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))) {
> back into
> if (len <= 0)
> return false;
>
> if (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))
> return false;
>
Thanks!
> I removed the following unnecessary inclusion:
> +#include <linux/random.h>
>
I should not have included that header now, it'll be only necessary in
another patch
>
> The following function pokes a hole in thei so far "abstract" data type;
> the convention has been to access the internals of the struct only via
> get-functions:
>
> static inline struct tfrc_loss_interval
> *tfrc_lh_get_loss_interval(struct tfrc_loss_hist *lh, const u8 i)
> {
> BUG_ON(i >= lh->counter);
> return lh->ring[LIH_INDEX(lh->counter - i - 1)];
> }
>
> (You use it in patch 3/5 to gain access to li_ccval and li_losses.
> Better would be to have two separate accessor functions.)
>
Okay, I will fix this.
>
> net/dccp/ccids/lib/tfrc_equation_sp.c
> -------------------------------------
> This is a prime candidate for removal. After editing out the whitespace
> differences, I found that it is 100% identical with tfrc_equation.c.
>
> The result of this editing has been uploaded to
> http://eden-feed.erg.abdn.ac.uk/tfrc_sp_receiver_01.patch
>
One future patch will need to modify this file, but now it's really an
exact copy.
--
Ivo Augusto Andrade Rocha Calado
MSc. Candidate
Embedded Systems and Pervasive Computing Lab - http://embedded.ufcg.edu.br
Systems and Computing Department - http://www.dsc.ufcg.edu.br
Electrical Engineering and Informatics Center - http://www.ceei.ufcg.edu.br
Federal University of Campina Grande - http://www.ufcg.edu.br
PGP: 0x03422935
Quidquid latine dictum sit, altum viditur.
--
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
