Re: unchanged functions between ccid-3 and ccid-4

Re: unchanged functions between ccid-3 and ccid-4

[@ 2007-09-21 2:28 UTC (permalink / raw)]

2007/9/20, Ian McDonald <ian.mcdonald@jandi.co.nz>:
> On 9/21/07, ツ Leandro Sales <leandroal@gmail.com> wrote:
> > Hello Gerrit,
> >
> >    From now and as I was talking to Ian two days ago, after our
> > patchset for initial code of ccid-4, I'm identifying functions between
> > ccid-4 and ccid-3 that will remains unchangeable. What do you think if
> > we try to define something like a common_ccid34.{c/h} and make common
> > functions between ccid-4 and ccid-3 available into this source file?
> > Or this will make the code a little bit confused?
> >
> > Leandro.
> >
> Leandro,
>
> Firstly - I think these discussions are better on list. Can we open to list.
>
> Thinking some more about this I've thought about this some more. I
> think what we're bettter to do is to get CCID4 code to use CCID3 code
> rather than create a new file. This is because CCID4 references CCID3
> but not the other way around.
>
> It will also be messy if we then get CCID5 etc.
>
> So what I suggest you might need to do is shift more ccid3
> declarations into ccid3.h and included this. It might also need
> removal of some static from function declarations and adding of
> EXPORT_SYMBOL_GPL to existing CCID3 code.
>
> This way you are still sharing code but it is clear CCID4 is based on CCID3.
>
> Regards,
>
> Ian
> --
> Web1: http://wand.net.nz/~iam4/
> Web2: http://www.jandi.co.nz
> Blog: http://iansblog.jandi.co.nz
>

Exactly! I was thinking about this since my first patchset sent via an
URL, where I commented about this. The problem of doing like you
suggested (move some code from ccid3.c to ccid3.h) is it will require
an approval about you and the others, such as gerrit and arnaldo, at
least if this is the right process to follow. Anyway, I'm already
identifying the candidate codes to be shared between ccid3 and ccid4
if you didn't this yet. Another option is wait for gerrit suggestion
on this issue and discuss more about that and define a strategy to
avoid code repetition.

Leandro.

Re: unchanged functions between ccid-3 and ccid-4

[Arnaldo Carvalho de Melo]

Em Thu, Sep 20, 2007 at 11:28:52PM -0300, ツ Leandro Sales escreveu:
> 2007/9/20, Ian McDonald <ian.mcdonald@jandi.co.nz>:
> > On 9/21/07, ツ Leandro Sales <leandroal@gmail.com> wrote:
> > > Hello Gerrit,
> > >
> > >    From now and as I was talking to Ian two days ago, after our
> > > patchset for initial code of ccid-4, I'm identifying functions between
> > > ccid-4 and ccid-3 that will remains unchangeable. What do you think if
> > > we try to define something like a common_ccid34.{c/h} and make common
> > > functions between ccid-4 and ccid-3 available into this source file?
> > > Or this will make the code a little bit confused?
> > >
> > > Leandro.
> > >
> > Leandro,
> >
> > Firstly - I think these discussions are better on list. Can we open to list.
> >
> > Thinking some more about this I've thought about this some more. I
> > think what we're bettter to do is to get CCID4 code to use CCID3 code
> > rather than create a new file. This is because CCID4 references CCID3
> > but not the other way around.
> >
> > It will also be messy if we then get CCID5 etc.
> >
> > So what I suggest you might need to do is shift more ccid3
> > declarations into ccid3.h and included this. It might also need

This part is OK if the functions are really small

> > removal of some static from function declarations and adding of
> > EXPORT_SYMBOL_GPL to existing CCID3 code.

This one is not because then we would have to load ccid3.ko even when
not using ccid3 fully. Conceptually it is better to have a
dccp_ccid3_lib.ko kernel module like we have now for the tfrc equation
and loss interval code, dccp_tfrc_lib.ko. ccid3.ko and
ccid4.ko would require dccp_ccid3_lib.ko.

> > This way you are still sharing code but it is clear CCID4 is based on CCID3.

The scheme I propose also keeps this property.

> Exactly! I was thinking about this since my first patchset sent via an
> URL, where I commented about this. The problem of doing like you
> suggested (move some code from ccid3.c to ccid3.h) is it will require
> an approval about you and the others, such as gerrit and arnaldo, at
> least if this is the right process to follow.

Don't worry too much about "approvals" at this stage, I think you should
do as you think its ok and discuss here mostly aspects of protocol
implementation, i.e. compliancy with the RFCs, etc.

Reorganizing the resulting source code to match Linux kernel guidelines
is something we should try to do from the start, but also should not get
in the way of having code that works first.

> Anyway, I'm already identifying the candidate codes to be shared
> between ccid3 and ccid4 if you didn't this yet. Another option is wait
> for gerrit suggestion on this issue and discuss more about that and
> define a strategy to avoid code repetition.

See? Wait for Gerrit, wait for Arnaldo, reach a consensus on code layout
first... nah, get something that works, when and if people send
suggestions you react, but don't get lack of feedback on these matters
to make you stop coding.

- Arnaldo

Re: unchanged functions between ccid-3 and ccid-4

[@ 2007-09-21 13:20 UTC (permalink / raw)]

2007/9/21, Arnaldo Carvalho de Melo <acme@ghostprotocols.net>:
> Em Thu, Sep 20, 2007 at 11:28:52PM -0300, ツ Leandro Sales escreveu:
> > 2007/9/20, Ian McDonald <ian.mcdonald@jandi.co.nz>:
> > > On 9/21/07, ツ Leandro Sales <leandroal@gmail.com> wrote:
> > > > Hello Gerrit,
> > > >
> > > >    From now and as I was talking to Ian two days ago, after our
> > > > patchset for initial code of ccid-4, I'm identifying functions between
> > > > ccid-4 and ccid-3 that will remains unchangeable. What do you think if
> > > > we try to define something like a common_ccid34.{c/h} and make common
> > > > functions between ccid-4 and ccid-3 available into this source file?
> > > > Or this will make the code a little bit confused?
> > > >
> > > > Leandro.
> > > >
> > > Leandro,
> > >
> > > Firstly - I think these discussions are better on list. Can we open to list.
> > >
> > > Thinking some more about this I've thought about this some more. I
> > > think what we're bettter to do is to get CCID4 code to use CCID3 code
> > > rather than create a new file. This is because CCID4 references CCID3
> > > but not the other way around.
> > >
> > > It will also be messy if we then get CCID5 etc.
> > >
> > > So what I suggest you might need to do is shift more ccid3
> > > declarations into ccid3.h and included this. It might also need
>
> This part is OK if the functions are really small
>

OK

> > > removal of some static from function declarations and adding of
> > > EXPORT_SYMBOL_GPL to existing CCID3 code.
>
> This one is not because then we would have to load ccid3.ko even when
> not using ccid3 fully. Conceptually it is better to have a
> dccp_ccid3_lib.ko kernel module like we have now for the tfrc equation
> and loss interval code, dccp_tfrc_lib.ko. ccid3.ko and
> ccid4.ko would require dccp_ccid3_lib.ko.
>

OK

> > > This way you are still sharing code but it is clear CCID4 is based on CCID3.
>
> The scheme I propose also keeps this property.
>
> > Exactly! I was thinking about this since my first patchset sent via an
> > URL, where I commented about this. The problem of doing like you
> > suggested (move some code from ccid3.c to ccid3.h) is it will require
> > an approval about you and the others, such as gerrit and arnaldo, at
> > least if this is the right process to follow.
>
> Don't worry too much about "approvals" at this stage, I think you should
> do as you think its ok and discuss here mostly aspects of protocol
> implementation, i.e. compliancy with the RFCs, etc.
>
> Reorganizing the resulting source code to match Linux kernel guidelines
> is something we should try to do from the start, but also should not get
> in the way of having code that works first.
>

Nice to know this.

> > Anyway, I'm already identifying the candidate codes to be shared
> > between ccid3 and ccid4 if you didn't this yet. Another option is wait
> > for gerrit suggestion on this issue and discuss more about that and
> > define a strategy to avoid code repetition.
>
> See? Wait for Gerrit, wait for Arnaldo, reach a consensus on code layout
> first... nah, get something that works, when and if people send
> suggestions you react, but don't get lack of feedback on these matters
> to make you stop coding.
>

Ok, I'll try to always discuss with you about my work on the ccid-4
implementation.

> - Arnaldo
>

Leandro

Re: unchanged functions between ccid-3 and ccid-4

[Gerrit Renker]

 
|  > Anyway, I'm already identifying the candidate codes to be shared
|  > between ccid3 and ccid4 if you didn't this yet. Another option is wait
|  > for gerrit suggestion on this issue and discuss more about that and
|  > define a strategy to avoid code repetition.
|  
|  See? Wait for Gerrit, wait for Arnaldo, reach a consensus on code layout
|  first... nah, get something that works, when and if people send
|  suggestions you react, but don't get lack of feedback on these matters
|  to make you stop coding.
|  
I'll say the same - the repository is yours, just keep working on it until
you have soomething that works for you - when there are actual problems with
the CCID3 code, please however mail them to the list.

I just remembered a very crude, but technically correct hack: if you want
to avoid the work of linking, header dependencies, one can include entire
source files using the preprocessor macro, e.g. in ccid4.c:

#include "ccid3.c"	/* works as expected, all code is imported */

This allows small and clear code (only the `diff' needs to be added, but I think 
the mainline guys don't like it so much :)

Re: unchanged functions between ccid-3 and ccid-4

[Tommi Saviranta]

On Fri, Sep 21, 2007 at 07:00:21 -0300, Arnaldo Carvalho de Melo wrote:
> Reorganizing the resulting source code to match Linux kernel
> guidelines is something we should try to do from the start, but also
> should not get in the way of having code that works first.

Agreed. However it seems I'm still not fully aware of the process.
Quoting another thread:

On Tue, Sep 25, 2007 at 10:01:32 +0100, Gerrit Renker wrote:
> |  - Rewrite all the patches (from me and Tommi) and resubmit to
> |  Gerrit as a unique patchset, as suggested by Ian. Besides, ask
> |  Gerrit to overwrite the current ccid4 branch;

> The ideas sound good, and I think it is a good idea to factor out what
> is not needed - just unclear which patch set to use, the latest so far
> is from Thursday?

Is overwriting an existing branch common practice, or simply used here
to cover our tracks of the initial confusion?-) Obviously we needed some
sort of starting point, and thus my mutilated copy of CCID 3 got in
early and unreviewed. Anyway, I think we should fix the existing trivial
issues (such as my mistakes to refer to non-existent RFCs instead of
Internet Drafts), overwrite the branch once more, and then start working
on splitting CCID 3.

Now that my patches have already been merged, how exactly should the
patches be dealt with? Naturally the patches need to be against the
current branch, but should or should I not start a new thread for the
patches?

Re: unchanged functions between ccid-3 and ccid-4

[@ 2007-09-26 13:33 UTC (permalink / raw)]

2007/9/26, Tommi Saviranta <wnd@iki.fi>:
> On Fri, Sep 21, 2007 at 07:00:21 -0300, Arnaldo Carvalho de Melo wrote:
> > Reorganizing the resulting source code to match Linux kernel
> > guidelines is something we should try to do from the start, but also
> > should not get in the way of having code that works first.
>
> Agreed. However it seems I'm still not fully aware of the process.
> Quoting another thread:
>
> On Tue, Sep 25, 2007 at 10:01:32 +0100, Gerrit Renker wrote:
> > |  - Rewrite all the patches (from me and Tommi) and resubmit to
> > |  Gerrit as a unique patchset, as suggested by Ian. Besides, ask
> > |  Gerrit to overwrite the current ccid4 branch;
>
> > The ideas sound good, and I think it is a good idea to factor out what
> > is not needed - just unclear which patch set to use, the latest so far
> > is from Thursday?
>
> Is overwriting an existing branch common practice, or simply used here
> to cover our tracks of the initial confusion?-) Obviously we needed some
> sort of starting point, and thus my mutilated copy of CCID 3 got in
> early and unreviewed. Anyway, I think we should fix the existing trivial
> issues (such as my mistakes to refer to non-existent RFCs instead of
> Internet Drafts), overwrite the branch once more, and then start working
> on splitting CCID 3.
>
> Now that my patches have already been merged, how exactly should the
> patches be dealt with? Naturally the patches need to be against the
> current branch, but should or should I not start a new thread for the
> patches?
>
>

I'm currently organized our patches (me and Tommi) as an unique
patchset and when it is ready, we will ask to gerrit overwrite the
current patches and apply the new patchset, as suggested by Ian.

Leandro.

Re: unchanged functions between ccid-3 and ccid-4

[Tommi Saviranta]

On Wed, Sep 26, 2007 at 10:33:21 -0300, ツ Leandro Sales wrote:
> 2007/9/26, Tommi Saviranta <wnd@iki.fi>:
> > Anyway, I think we should fix the existing trivial issues (such as
> > my mistakes to refer to non-existent RFCs instead of Internet
> > Drafts)
> 
> I'm currently organized our patches (me and Tommi) as an unique
> patchset and when it is ready, we will ask to gerrit overwrite the
> current patches and apply the new patchset, as suggested by Ian.

Does that also mean you will also take care of these (issues Ian
reported)? The patches are trivial and I have them ready, but right now
I don't know what to do with them.

Re: unchanged functions between ccid-3 and ccid-4

[@ 2007-09-26 13:51 UTC (permalink / raw)]

2007/9/26, Tommi Saviranta <wnd@iki.fi>:
> On Wed, Sep 26, 2007 at 10:33:21 -0300, ツ Leandro Sales wrote:
> > 2007/9/26, Tommi Saviranta <wnd@iki.fi>:
> > > Anyway, I think we should fix the existing trivial issues (such as
> > > my mistakes to refer to non-existent RFCs instead of Internet
> > > Drafts)
> >
> > I'm currently organized our patches (me and Tommi) as an unique
> > patchset and when it is ready, we will ask to gerrit overwrite the
> > current patches and apply the new patchset, as suggested by Ian.
>
> Does that also mean you will also take care of these (issues Ian
> reported)? The patches are trivial and I have them ready, but right now
> I don't know what to do with them.
>
>

The fact is that even it is trivial patches, our patchset became
something like patches against patches, which it is not good since the
patches is related to the same issues.