* [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
@ 2007-10-31 7:20 ` Łeandro Sales
2007-10-31 23:17 ` Leandro
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Łeandro Sales @ 2007-10-31 7:20 UTC (permalink / raw)
To: dccp
Hi Gerrit,
based on conversations with you, Arnaldo and Ian, please consider
the following patches related to ccid-4 initial implementation and
code share with ccid-3. Also, please reset the ccid4 branch to have
the latest dccp branch code before apply these patches series. It
would be interesting if you merge the patches related to code share
between ccid-3 and ccid-4 into the dccp branch to eliminate some of
our job and reduce (or even avoid) code repetition, in case that
ccid-3 is changed.
Some considerations about the criterious that I used to share a
function/struct/consts between ccid-3 and ccid-4.
1 - Certify whether the function is common (checking just the
functions in ccid-4 copied from ccid-3);
2 - Certify whether both functions (in ccid-3 and ccid-4) still
common; (Me or Tommi can already changed it for ccid-4)
3 - Certify whether the both functions doesn't call a ccid specific function;
i.e: ccid3_hc_tx_packet_sent could be shared, but it calls
ccid3_hc_tx_update_s, which is a ccid-3 specific function. Then
ccid3_hc_tx_packet_sent is not a candidate to be shared between
ccid-{3/4}.
i.e: ccid3_hc_rx_packet_recv
i.e: ccid3_hc_rx_insert_options
4 - Certify whether the function has a low probability to be changed
for ccid-4 implementation in the future;
i.e 1: Albeit ccid3_hc_tx_send_packet could be common, ccid-4 will
probably have a particular way to decide when send a packet
i.e 2: ccid4_first_li will certainly change because the short loss
interval length is calculated as N/K, instead of using just N
(ccid3_first_li).
i.e 3: ccid3_hc_tx_parse_options could be common, but until the
IETF folks do not decide if DROPPED_PACKET_OPTION will be into the
ccid-3, I decided to NOT share it.
5 - Common sense: functions such as ccid3_hc_tx_init and
ccid3_hc_tx_exit are very particular, probably we will want to have
specific procedures to go into ccid-3, different to ccid-4 and
vice-versa.
Patch #1: [CCID-4] Changes to Kconfig and Makefile to add ccid-4 code;
Patch #2: [CCID-4] New ccid-4 based on ccid-3 source code;
Patch #3: [CCID-4] Adjustments of the ccid-4 copyright and kernel
module authors;
Patch #4: [CCID-4] Set packet size to 1460 as per ccid-4 draft;
Patch #5: [CCID-4] Enforces a minimum interval of 10 milliseconds as
per CCID-4 draft;
Patch #6: [CCID-4] Reduce allowed sending rate by a factor that
accounts for packet header size;
Patch #7: [CCID-3/4] Initial lib for sharing common code between ccid-3/ccid-4;
Patch #8: [CCID-3/4] Share common header files and defines between
ccid-3/ccid-4;
Patch #9: [CCID-4] Adapt CCID-4 according to the latest changes to CCID-3;
Patch #10: [CCID-3/4] Share ccid-3 options enum and struct via ccid34_lib;
Patch #11: [CCID-3/4] Share TFRC sender states via ccid34_lib;
Patch #12: [CCID-3/4] Share TFRC receiver states via ccid34_lib;
Patch #13: [CCID-3/4] Share TFRC feedback types struct via ccid34_lib;
Patch #14: [CCID-4] Basic implementation for ccid-4 dropped packet
option as per ccid-4 draft;
Patch #15: [CCID-3/4] Share ccid3_hc_tx_sock struct and ccid3_hc_tx_sk
function via ccid34_lib;
Patch #16: [CCID-3/4] Share ccid3_hc_rx_sock and ccid3_hc_rx_sk via ccid34_lib;
Patch #17: [CCID-3/4] Share ccid3_tx_state_name function via ccid34_lib;
Patch #18: [CCID-3/4] Share rfc3390_initial_rate function via ccid34_lib;
Patch #19: [CCID-3/4] Share ccid3_hc_tx_idle_rtt function via ccd34_lib;
Patch #20: [CCID-3/4] Share ccid3_hc_tx_update_win_count function via
ccid34_lib;
Patch #21: [CCID-3/4] Share ccid3_rx_state_name function via ccid34_lib;
Patch #22: [DCCP] Minor adjustments to probe.c to use ccid34_lib;
Patch #23: [CCID-4] Include ccid-4 to be visible for the DCCP feature
negotiation mechanism;
Patch #24: [DCCP] Final adjustments to probe.c to use ccid34_lib;
Patch #25: [CCID-3/4] Final adjustments to ccid3.c, ccid4.c and
lib/ccid34_lib.c.
Leandro.
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
2007-10-31 7:20 ` Łeandro Sales
@ 2007-10-31 23:17 ` Leandro
2007-11-01 10:39 ` Gerrit Renker
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Leandro @ 2007-10-31 23:17 UTC (permalink / raw)
To: dccp
Hi Gerrit,
based on conversations with you, Arnaldo and Ian, please consider the following patches related to ccid-4 initial implementation and code share with ccid-3. Also, please reset the ccid4 branch to have the latest dccp branch code before apply these patches series. It would be interesting if you merge the patches related to code share between ccid-3 and ccid-4 into the dccp branch to eliminate some of our job and reduce (or even avoid) code repetition, in case that ccid-3 is changed.
Some considerations about the criterious that I used to share a function/struct/consts between ccid-3 and ccid-4. This set of patches also consider your comments about use tfrc_xxx rather than tfrc_xxx.
1 - Certify whether the function is common (checking just the functions in ccid-4 copied from ccid-3);
2 - Certify whether both functions (in ccid-3 and ccid-4) still common; (Me or Tommi can already changed it for ccid-4)
3 - Certify whether the both functions doesn't call a ccid specific function;
i.e: ccid3_hc_tx_packet_sent could be shared, but it calls ccid3_hc_tx_update_s, which is a ccid-3 specific function. Then ccid3_hc_tx_packet_sent is not a candidate to be shared between ccid-{3/4}.
i.e: ccid3_hc_rx_packet_recv
i.e: ccid3_hc_rx_insert_options
4 - Certify whether the function has a low probability to be changed for ccid-4 implementation in the future;
i.e 1: Albeit ccid3_hc_tx_send_packet could be common, ccid-4 will probably have a particular way to decide when send a packet
i.e 2: ccid4_first_li will certainly change because the short loss interval length is calculated as N/K, instead of using just N (ccid3_first_li).
i.e 3: ccid3_hc_tx_parse_options could be common, but until the IETF folks do not decide if DROPPED_PACKET_OPTION will be into the ccid-3, I decided to NOT share it.
5 - Common sense: functions such as ccid3_hc_tx_init and ccid3_hc_tx_exit are very particular, probably we will want to have specific procedures to go into ccid-3, different to ccid-4 and vice-versa.
Patch #1: [CCID-4] Changes to Kconfig and Makefile to add ccid-4 code;
Patch #2: [CCID-4] New ccid-4 based on ccid-3 source code;
Patch #3: [CCID-4] Adjustments of the ccid-4 copyright and kernel module authors;
Patch #4: [CCID-4] Set packet size to 1460 as per ccid-4 draft;
Patch #5: [CCID-4] Enforces a minimum interval of 10 milliseconds as per CCID-4 draft;
Patch #6: [CCID-4] Reduce allowed sending rate by a factor that accounts for packet header size;
Patch #7: [CCID-3/4] Initial lib for sharing common code between ccid-3/ccid-4;
Patch #8: [CCID-3/4] Share common header files and defines between ccid-3/ccid-4;
Patch #9: [CCID-4] Adapt CCID-4 according to the latest changes to CCID-3;
Patch #10: [CCID-3/4] Share ccid-3 options enum and struct via tfrc_ccids;
Patch #11: [CCID-3/4] Share TFRC sender states via tfrc_ccids;
Patch #12: [CCID-3/4] Share TFRC receiver states via tfrc_ccids;
Patch #13: [CCID-3/4] Share TFRC feedback types struct via tfrc_ccids;
Patch #14: [CCID-4] Basic implementation for ccid-4 dropped packet option as per ccid-4 draft;
Patch #15: [CCID-3/4] Share ccid3_hc_tx_sock struct and ccid3_hc_tx_sk function via tfrc_ccids;
Patch #16: [CCID-3/4] Share ccid3_hc_rx_sock and ccid3_hc_rx_sk via tfrc_ccids;
Patch #17: [CCID-3/4] Share ccid3_tx_state_name function via tfrc_ccids;
Patch #18: [CCID-3/4] Share rfc3390_initial_rate function via tfrc_ccids;
Patch #19: [CCID-3/4] Share ccid3_hc_tx_idle_rtt function via tfrc_ccids;
Patch #20: [CCID-3/4] Share ccid3_hc_tx_update_win_count function via tfrc_ccids;
Patch #21: [CCID-3/4] Share ccid3_rx_state_name function via tfrc_ccids;
Patch #22: [DCCP] Minor adjustments to probe.c to use tfrc_ccids;
Patch #23: [CCID-4] Include ccid-4 to be visible for the DCCP feature negotiation mechanism;
Patch #24: [DCCP] Final adjustments to probe.c to use tfrc_ccids;
Patch #25: [CCID-3/4] Final adjustments to ccid3.c, ccid4.c and lib/tfrc_ccids.c.
Leandro.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
2007-10-31 7:20 ` Łeandro Sales
2007-10-31 23:17 ` Leandro
@ 2007-11-01 10:39 ` Gerrit Renker
2007-11-01 21:36 ` Łeandro Sales
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Gerrit Renker @ 2007-11-01 10:39 UTC (permalink / raw)
To: dccp
The new ccid4 branch is now online. With regard to merging between ccid3/4, this will
take some time. I have a day job to do and there may be a few changes in the ccid3 code
(will be announced on the list), but I'll let you know.
root@turnip> git pull git://eden-feed.erg.abdn.ac.uk/dccp_exp ccid4
Updating de1f427..b1a9b84
Fast forward
include/linux/dccp.h | 1 +
net/dccp/ccid.c | 3 +
net/dccp/ccids/Kconfig | 77 ++++-
net/dccp/ccids/Makefile | 4 +
net/dccp/ccids/ccid3.c | 480 +++++++++------------
net/dccp/ccids/ccid3.h | 135 +------
net/dccp/ccids/ccid4.c | 917 +++++++++++++++++++++++++++++++++++++++
net/dccp/ccids/ccid4.h | 64 +++
net/dccp/ccids/lib/Makefile | 3 +-
net/dccp/ccids/lib/tfrc_ccids.c | 68 +++
net/dccp/ccids/lib/tfrc_ccids.h | 198 +++++++++
net/dccp/feat.c | 59 +++-
net/dccp/probe.c | 21 +-
13 files changed, 1604 insertions(+), 426 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
` (2 preceding siblings ...)
2007-11-01 10:39 ` Gerrit Renker
@ 2007-11-01 21:36 ` Łeandro Sales
2007-11-03 11:21 ` Gerrit Renker
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Łeandro Sales @ 2007-11-01 21:36 UTC (permalink / raw)
To: dccp
2007/11/1, Gerrit Renker <gerrit@erg.abdn.ac.uk>:
> The new ccid4 branch is now online. With regard to merging between ccid3/4, this will
> take some time.
Ok, no problem.
> I have a day job to do and there may be a few changes in the ccid3 code
> (will be announced on the list), but I'll let you know.
Ok, please let me know when you are read.
>
> root@turnip> git pull git://eden-feed.erg.abdn.ac.uk/dccp_exp ccid4
>
> Updating de1f427..b1a9b84
> Fast forward
> include/linux/dccp.h | 1 +
> net/dccp/ccid.c | 3 +
> net/dccp/ccids/Kconfig | 77 ++++-
> net/dccp/ccids/Makefile | 4 +
> net/dccp/ccids/ccid3.c | 480 +++++++++------------
> net/dccp/ccids/ccid3.h | 135 +------
> net/dccp/ccids/ccid4.c | 917 +++++++++++++++++++++++++++++++++++++++
> net/dccp/ccids/ccid4.h | 64 +++
> net/dccp/ccids/lib/Makefile | 3 +-
> net/dccp/ccids/lib/tfrc_ccids.c | 68 +++
> net/dccp/ccids/lib/tfrc_ccids.h | 198 +++++++++
> net/dccp/feat.c | 59 +++-
> net/dccp/probe.c | 21 +-
> 13 files changed, 1604 insertions(+), 426 deletions(-)
>
Thank you,
Leandro.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
` (3 preceding siblings ...)
2007-11-01 21:36 ` Łeandro Sales
@ 2007-11-03 11:21 ` Gerrit Renker
2007-11-05 14:51 ` Łeandro Sales
2007-11-08 8:12 ` Gerrit Renker
6 siblings, 0 replies; 8+ messages in thread
From: Gerrit Renker @ 2007-11-03 11:21 UTC (permalink / raw)
To: dccp
I've had a look at the recent set of patches, some more detailed comments below.
Basically, I think it is too early to think of merging: it will constrain your
work, at the moment there is really not much being shared, and to me it is not
clear what shape the CCID4 code will eventually take.
As it is done currently, I can't see much benefit: there are only three
functions in tfrc_ccids.c:
* rfc3390_initial_rate
* tfrc_hc_tx_idle_rtt
* tfrc_hc_tx_update_win_count
The first and last of the above functions are declared inline. Inlines inside a
source file are not often considered a good idea; according to current wisdom one
should leave it to gcc to decide whether to make a function inline or not.
If you do want to declare these two inline, you can put them into the header file --
which then leaves you with only a single function in tfrc_ccids.c.
If there is really not more that you want to share between the CCIDs, then
there is not much benefit in allocating an extra file; on the contrary, it just
increases the confusion ("what is this file doing?"). Besides, there is already
such a file for this purpose -- check net/dccp/ccids/lib/tfrc_module.c
With regard to header files, there is a similar observation: due to putting
everything into tfrc_ccids.h, ccid3.h now becomes a one-liner:
#include "lib/tfrc_ccids.h",
so that you might as well remove ccid3.h entirely; ccid4.h has three more lines,
when these are instead put at the top of ccid4.c, then ccid4.h is also no longer
necessary.
There is a disparity: almost no code is shared, while CCID3 and CCID4 use an
exactly identical socket structure. When two different pieces of code share the same
underlying data structure, then it is likely possible to share more code currently done.
Hence I am asking you what your plans are: keep the strategy of only sharing functions
that previously appeared in both ccid3.c and ccid4.c -- in this case the number of new
files can be reduced; or extend the work to also share functions which have the same
functionality in both files.
Individual points:
> 1 - Certify whether the function is common (checking just the functions
> in ccid-4 copied from ccid-3);
>
> 2 - Certify whether both functions (in ccid-3 and ccid-4) still common;
> (Me or Tommi can already changed it for ccid-4)
Looking at the code, this seems to be just one category, of those functions which are
100% identical in both files.
> 3 - Certify whether the both functions doesn't call a ccid specific function;
> i.e: ccid3_hc_tx_packet_sent could be shared, but it calls
> ccid3_hc_tx_update_s, which is a ccid-3 specific function.
> Then ccid3_hc_tx_packet_sent is not a candidate to be shared
> between ccid-{3/4}. i.e: ccid3_hc_rx_packet_recv i.e:
> ccid3_hc_rx_insert_options
It is possible to still share code here; by using a generic function which takes
a function pointer to the CCID-specific function as argument. Realising this will
reduce the amount of duplicated functionality, but requires careful analysis. May
be able to help here, depending on how things progress.
> 4 - Certify whether the function has a low probability to be changed
> for ccid-4 implementation in the future;
>
> * although ccid3_hc_tx_send_packet could be common, ccid-4 will
> probably have a particular way to decide when send a packet;
> * ccid4_first_li will certainly change because the short loss interval
> length is calculated as N/K, instead of using just N (ccid3_first_li);
Ok, these functions really do different things and so keeping them separate seems the
right thing to do.
> * ccid3_hc_tx_parse_options could be common, but until the IETF
> folks do not decide if DROPPED_PACKET_OPTION will be into the ccid-3,
> I decided to NOT share it.
Can't see the point: CCID3 does not use the experimental Dropped Packets Option
(since it doesn't even use Loss Intervals), so there would be no harm in sharing
this function -- at worst code which is not executed; whereas in the other case
you have an almost identical function in two different places.
> 5 - Common sense: functions such as ccid3_hc_tx_init and ccid3_hc_tx_exit
> are very particular, probably we will want to have specific
> procedures to go into ccid-3, different to ccid-4 and vice-versa.
So same category as the first two in (4).
To conclude and repeat, I think that it is better at this stage if you focus on getting y
our CCID4 implementation to work; and decide afterwards about what to share and what not.
It will be much easier to do: as it is often better to write the `introduction' section
to some document _after_ the document has been constructed/written, since only then it is
fully clear what the document is all about.
Last comment - I think that future changes to ccid3.c/ccid4.c are regrettably unavoidable,
as the standardisation still depends on many work-in-progress Internet Drafts
(such as rfc3448bis, ccid4 draft) which are constantly changing (hence "work in progress").
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
` (4 preceding siblings ...)
2007-11-03 11:21 ` Gerrit Renker
@ 2007-11-05 14:51 ` Łeandro Sales
2007-11-08 8:12 ` Gerrit Renker
6 siblings, 0 replies; 8+ messages in thread
From: Łeandro Sales @ 2007-11-05 14:51 UTC (permalink / raw)
To: dccp
Hi Gerrit,
2007/11/3, Gerrit Renker <gerrit@erg.abdn.ac.uk>:
> I've had a look at the recent set of patches, some more detailed comments below.
> Basically, I think it is too early to think of merging: it will constrain your
> work, at the moment there is really not much being shared, and to me it is not
> clear what shape the CCID4 code will eventually take.
>
> As it is done currently, I can't see much benefit: there are only three
> functions in tfrc_ccids.c:
>
> * rfc3390_initial_rate
> * tfrc_hc_tx_idle_rtt
> * tfrc_hc_tx_update_win_count
>
> The first and last of the above functions are declared inline. Inlines inside a
> source file are not often considered a good idea; according to current wisdom one
> should leave it to gcc to decide whether to make a function inline or not.
>
The inline declarations were done just to maintain the current code,
which was current using inline. But, in fact I can remove the inline
declaration.
> If you do want to declare these two inline, you can put them into the header file --
> which then leaves you with only a single function in tfrc_ccids.c.
>
The idea of my patches is to propose an initial code sharing between
ccid3/ccid4 ("towards ccid3/4 integration"). After these patches, we
can work to share more code between ccid3/ccid4 to tfrc_ccids.c, like
you suggested using generic code.
> If there is really not more that you want to share between the CCIDs, then
> there is not much benefit in allocating an extra file; on the contrary, it just
> increases the confusion ("what is this file doing?"). Besides, there is already
> such a file for this purpose -- check net/dccp/ccids/lib/tfrc_module.c
>
But it exists. One example is the function which handles
DROPPED_PACKET_OPTION, that you mentioned. I just didn't try to share
more function because I intended to make all of you check the code and
discuss the directions and see if it is worthy to continue the job or
not.
> With regard to header files, there is a similar observation: due to putting
> everything into tfrc_ccids.h, ccid3.h now becomes a one-liner:
> #include "lib/tfrc_ccids.h",
> so that you might as well remove ccid3.h entirely; ccid4.h has three more lines,
> when these are instead put at the top of ccid4.c, then ccid4.h is also no longer
> necessary.
>
I maintain the ccid{3/4}.h in case we need to use them eventually in the future.
> There is a disparity: almost no code is shared, while CCID3 and CCID4 use an
> exactly identical socket structure. When two different pieces of code share the same
> underlying data structure, then it is likely possible to share more code currently done.
>
Yes, this is the idea. The patches just starts up the job! :)
> Hence I am asking you what your plans are: keep the strategy of only sharing functions
> that previously appeared in both ccid3.c and ccid4.c -- in this case the number of new
> files can be reduced; or extend the work to also share functions which have the same
> functionality in both files.
>
The idea is to extend the work to also share functions which have the
same functionality, but for archive this, it is necessary to analyze
more carefully the code, as you mentioned below.
> Individual points:
> > 1 - Certify whether the function is common (checking just the functions
> > in ccid-4 copied from ccid-3);
> >
> > 2 - Certify whether both functions (in ccid-3 and ccid-4) still common;
> > (Me or Tommi can already changed it for ccid-4)
> Looking at the code, this seems to be just one category, of those functions which are
> 100% identical in both files.
>
> > 3 - Certify whether the both functions doesn't call a ccid specific function;
> > i.e: ccid3_hc_tx_packet_sent could be shared, but it calls
> > ccid3_hc_tx_update_s, which is a ccid-3 specific function.
> > Then ccid3_hc_tx_packet_sent is not a candidate to be shared
> > between ccid-{3/4}. i.e: ccid3_hc_rx_packet_recv i.e:
> > ccid3_hc_rx_insert_options
> It is possible to still share code here; by using a generic function which takes
> a function pointer to the CCID-specific function as argument. Realising this will
> reduce the amount of duplicated functionality, but requires careful analysis. May
> be able to help here, depending on how things progress.
>
Ok.
> > 4 - Certify whether the function has a low probability to be changed
> > for ccid-4 implementation in the future;
> >
> > * although ccid3_hc_tx_send_packet could be common, ccid-4 will
> > probably have a particular way to decide when send a packet;
> > * ccid4_first_li will certainly change because the short loss interval
> > length is calculated as N/K, instead of using just N (ccid3_first_li);
> Ok, these functions really do different things and so keeping them separate seems the
> right thing to do.
>
Ok.
> > * ccid3_hc_tx_parse_options could be common, but until the IETF
> > folks do not decide if DROPPED_PACKET_OPTION will be into the ccid-3,
> > I decided to NOT share it.
> Can't see the point: CCID3 does not use the experimental Dropped Packets Option
> (since it doesn't even use Loss Intervals), so there would be no harm in sharing
> this function -- at worst code which is not executed; whereas in the other case
> you have an almost identical function in two different places.
>
Yes, I can say that I was in doubt whether to share this function or
not and I you are right on this, I will submit a patch to share this
function.
>
> > 5 - Common sense: functions such as ccid3_hc_tx_init and ccid3_hc_tx_exit
> > are very particular, probably we will want to have specific
> > procedures to go into ccid-3, different to ccid-4 and vice-versa.
> So same category as the first two in (4).
>
Ok.
> To conclude and repeat, I think that it is better at this stage if you focus on getting y
> our CCID4 implementation to work; and decide afterwards about what to share and what not.
>
> It will be much easier to do: as it is often better to write the `introduction' section
> to some document _after_ the document has been constructed/written, since only then it is
> fully clear what the document is all about.
>
Ok, good decision! :)
> Last comment - I think that future changes to ccid3.c/ccid4.c are regrettably unavoidable,
> as the standardisation still depends on many work-in-progress Internet Drafts
> (such as rfc3448bis, ccid4 draft) which are constantly changing (hence "work in progress").
>
Leandro.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/25] Towards CCID3/CCID4 code integration
2007-10-31 5:54 [PATCH 0/25] Towards CCID3/CCID4 code integration Leandro Melo de Sales
` (5 preceding siblings ...)
2007-11-05 14:51 ` Łeandro Sales
@ 2007-11-08 8:12 ` Gerrit Renker
6 siblings, 0 replies; 8+ messages in thread
From: Gerrit Renker @ 2007-11-08 8:12 UTC (permalink / raw)
To: dccp
| > As it is done currently, I can't see much benefit: there are only three
| > functions in tfrc_ccids.c:
| >
| > * rfc3390_initial_rate
| > * tfrc_hc_tx_idle_rtt
| > * tfrc_hc_tx_update_win_count
| >
| > The first and last of the above functions are declared inline. Inlines inside a
| > source file are not often considered a good idea; according to current wisdom one
| > should leave it to gcc to decide whether to make a function inline or not.
| >
|
| The inline declarations were done just to maintain the current code,
| which was current using inline. But, in fact I can remove the inline
| declaration.
|
These functions were originally declared locally in ccid3.c, as `static
inline'. The situation is now different - this file is a library, and
the symbols are exported with EXPORT_SYMBOL. When the functions were
first introduced for ccid3.c there were some discussions as to what to
inline and what not; I believe that the current mixture in ccid3.c is
fairly consistent, but the situation is different when functions are
moved/shared. Actually, all three of the above could appear in a header
file, since tfrc_hc_tx_idle_rtt is also really short.
| The idea of my patches is to propose an initial code sharing between
| ccid3/ccid4 ("towards ccid3/4 integration"). After these patches, we
| can work to share more code between ccid3/ccid4 to tfrc_ccids.c, like
| you suggested using generic code.
|
Again, I think it is better for the moment if you use whatever helps you
to get your implementation going, and leave aside the desire to merge
As far as I can see, the CCID4 code is not even finished yet.
Then there are several other things which will change, too:
* rfc3448bis is currently being revised, next IETF will probably see a
new version, then people will discuss, and even more changes;
* these changes will somehow influence CCID4, too (code and specification);
* the CCID4 draft is a moving target, too rev01 came out in June, but
Floyd's webpage already has a new revision online.
I think that there is absolutely no harm if you start off with code
duplication, there is even a quote by Torvalds that "duplicated code
is good". While you are implementing, you will automatically note the
patterns and identify where the same thing happens twice.
But doing this at the start is likely to just create confusion.
An example is in the two nearly empty header-files ccid{3,4}.h: why do
you want to keep these for "future use"? If redundant, they can go, but
if you keep things for future use they will pile up and in the end no
one knows why it is still there.
^ permalink raw reply [flat|nested] 8+ messages in thread