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.
> > > - /* 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, ...
Best regards,
Sven
signature.asc
Description: This is a digitally signed message part.
