Hey Maxim,
I'm adding the mailing list to the CCs.
> > +#include <stdio.h>
> > +#include <assert.h>
> > +
> > +#include "odp_ipfragreass_ip.h"
> > +#include "odp_ipfragreass_fragment.h"
> > +
> > +int fragment_ipv4_packet(odp_packet_t orig_packet, odp_packet_t *out,
> > + int *out_len)
> > +{
> > + uint32_t orig_len = odp_packet_len(orig_packet);
> > + odph_ipv4hdr_t *orig_header = odp_packet_data(orig_packet);
> > + uint32_t header_len = ipv4hdr_ihl(*orig_header);
> > + uint32_t bytes_remaining = orig_len - header_len;
> > +
> > + if (bytes_remaining <= MTU)
> > + return -1;
> > +
> > + /*
> > + * The main fragmentation loop (continue until all bytes from the
> > + * original payload have been assigned to a fragment)
> > + */
> > + odp_packet_t frag = orig_packet;
>
> 1. why to you need second variable for the same packet handle?
For readability! You're right that I could call the "orig_packet" parameter
"frag" and be done with it, but it's really much clearer if rather than
reusing the same variable for two effectively unrelated purposes, I use the
wonderful ability granted to us by the programming gods to associate values
with names that describe their purpose. And best of all, for all but the most
primitive or idiotic of compilers, it's completely free! (And, actually,
probably makes the optimiser's life easier.)
> 2. Variable should be declared at the begining of the scope.
Is this a hard requirement? I noticed that it wasn't picked up by checkpatch,
and I can't see it outlined in the kernel's coding style guidelines either. I
don't think this would cause a total readability disaster here, but I do
think it would make the code at least a little harder to read. Mentally, for
me, the variable declarations are clearly split between auxiliary data
required for the whole function (declared at the top), and data that is
instrumental in operating the big ol' while loop (declared just before the
loop).
Admittedly, the function more or less /is/ the loop here though, and perhaps
you could argue that this function is too long as per the kernel style
guidelines anyway. I use a similar style in other places in the code, though,
where I think it's hugely more useful. For instance, in the function
"add_fraglist_to_fraglist". It's a big function, so is helped significantly
by this. You could definitely argue that it's too long, but it sort of needs
to be. (In some part, at least, because in my testing the optimiser failed to
perform tail call optimisation.)
> 3. For easy reading it's better to have _pkt postfix or prefix to to handles.
> Like pkt_orig or orig_pkt. Like we do in implementation.
> That is optional but it's very easy to see if it's variable or handle.
Hmm... isn't this what we have types for? I'm not a fan of hungarian notation,
and while I think this convention is a bit more sensible, it seems somewhat
unnecessary if variables are properly named. In this context, I think it's
fairly obvious that a variable named "frag" is a packet of some sort (whether
it's an ODP handle or a custom struct or whatever -- distinctions which the
"_pkt" pre/postfix doesn't actually resolve). If there are places in the code
where you specifically think that adding "pkt" would be helpful, though, I'd
be more than happy to consider such changes there.
> 4. Coding style needs to be corrected.
???
> 5. Do you have linaro email address? Can you plese send patch from it.
I had some discussions about this yesterday, and it is my understanding that
I should be able to submit the patch from my ARM email address. Perhaps this
needs some further dialogue, though?
> 6. I'm not really convinced about testing of that example. As future work I
> think it might be odp helper. And tools like Snort and Suricate can use
> that odp heper for reassembly instead their own implementation. In that case
> we need to proove that algorithm work as expected
> and highlight benefits doing it with ODP. Benefits like faster speed. And
> proving that it works might be processing pcap file with ip fragments with
> your
> program. And as result tcpdump will show no fragmetation on generated pcap.
> Example also should show that code does not reorder or do other not needed
> modifications with packets.
Yes, perhaps this would integrate nicely into ODP(H) in future. The checking
performed within the program is fairly decent, and does check the reassembled
packets against the original copies, but I agree that more extensive testing
(including performance tuning and measurement) could provide fertile ground
for future work.
> 7. Will you be able to support this app in next 6 months or later?
That depends on your definition of "support". Within the next few months,
potentially, but much later than that, probably not.
