Am Sun, 12 Sep 2010 22:11:06 +0200
schrieb Sven Eckelmann <[email protected]>:
> Marek Lindner wrote:
> > On Saturday 11 September 2010 11:26:37 Sven Eckelmann wrote:
> > > Is it possible to split the patch in those parts? It would make it easier
> > > to read it and understand the patches later.
> >
> > I'm not sure that will do much good. He managed to reorganize the code and
> > thereby remove redundancies. The second patch would probably be no bigger
> > than 3-5 lines of code.
>
> That would not be bad. But refactoring and adding two new features isn't
> something I personally like. Guessing what part could belong to which other
> part just makes it hard to find the real problems. Take for example the
> missing failure checks in the patch.
>
> Something I had in mind would be a patch which adds frag_send_skb and starts
> to use it, one that removes the duplicated code, another one that does the
> cleanup/renaming stuff, the next adds the fragmentation on routing and
> finally
> there is the defragmentation (probably skipped some steps) - you could much
> more easily check the actual change in its own context. That would for
> example
> remove the question why those lines were removed - the patch itself would
> answer that question using its commit message which states: "hey those lines
> aren't needed because we already check that fact here and there and don't
> need
> that information before".
>
> And if a patch real adds a cool feature only by changing some lines and
> without rewriting the whole code then we must have done something right.
>
> If you think different - ok, leave it that way.
No it's ok, you are right. I will split the patch.
>
> > > > - /* packet for me */
> > > > - if (is_my_mac(unicast_packet->dest)) {
> > > > - interface_rx(recv_if->soft_iface, skb, hdr_size);
> > > > - return NET_RX_SUCCESS;
> > > > - }
> > > > -
> > >
> > > There are different parts of the patch which makes ma a little bit
> > > curious - for example: why it is possible to drop that check entirely?
> > > Could that be an extra patch with an explanation why that can be
> > > dropped? Is it only valid in context of the new fragmentation handling?
> > > ...
> >
> > I ran into the same question as it looks a bit odd here but if you apply
> > the patch it all looks good. As I said: it seems he managed to purge quite
> > a bit of redundancy (fragmented and non-fragmented packets are almost
> > identical after all).
>
> Andreas Langer wrote:
> > what are the other parts which makes you curious ?
>
> That a failure in frag_reassemble_skb is a good successfully rx, that
> frag_create_buffer is magically always successful even when the called
> functions can fail, that you use skb_pull and directly afterward access the
> data pointer, that no one frees the skb when frag_reassemble_skb cannot find
> a
> matching originator, ...
I will look into it.Thanks for your hints.
regards,
andreas
