On 26 January 2017 at 16:56, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 01/26/17 18:27, Ola Liljedahl wrote: > > > > > > On 26 January 2017 at 15:19, Joe Savage <joe.sav...@arm.com > > <mailto:joe.sav...@arm.com>> wrote: > > > > > >> It will be very helpful if rehe was some README with > description about > > > >> this app, run environments and some output. So people can learn > > > >> something before looking to code. > > > > > > > > I can add one, but I don't think there's really that much to > describe. Since > > > > the example doesn't connect to the network, all that users > really need to > > > > know is that it fragments and reassembles IPv4 packets. > > > > > > you add that description here: > > > ./doc/application-api-guide/examples.dox > > > > > > Description can be about application internals. Like you create N > > > workers, use queue with nsize to reassembly packets. Use following > > > algorithm. > > > > > > And why this app is not connected to network? I think it will be > very > > > useful if you can pass some pcap file and get pcap on output. And > test > > > this program work with other program which does reassembly. That > looks > > > like good proof that it works as expected. > > > > Adding the example to the list in examples.dox seems sensible, but I > > think > > the code and comments are probably the best description of the > algorithm > > itself. > > > > As for why it isn't network connected, I wanted to keep the example > > somewhat > > bare bones to its purpose. Dealing with a real network connection is > > likely > > to add clutter that doesn't really speak to the contents of this > > specific > > example. Anyone wanting to implement this kind of functionality > > themselves > > can simply glean this information from a different example focusing > > around > > the packet I/O interface. > > > > > > > > > >> app naming might be not best. > > > > > > > > Hmm... do you have any other ideas? I didn't want it to be too > long, and both > > > > "fragmentation" and "reassembly" are unfortunately lengthy. > > > > > > > > > > ipfrag or ipv4frag? > > > > Ehh, maybe. The fragmentation doesn't really play a huge role here > > though, > > the reassembly is really the star of the show. Perhaps ipfragreass > > or just > > ipreass? > > > > > > > >> in early examples we defined max workers, but actully it's not > needed > > > >> becase you can ask odp how many workers are there with 0. > > > >> I.e. in your code it will be: > > > >> *num_workers = odp_cpumask_default_worker(cpumask, 0) > > > > > > > > I did see this in the documentation, but since the functionality > was present > > > > in other examples I thought it might be worthwhile to allow a > maximum number > > > > of workers to be set. Happy to remove this to use all available > CPUs if it > > > > is desirable though. > > > > > > we used MAX_WORKERS before odp_cpumask_default_worker() was > implemented, > > > after that we did not rewrite old example. I think it's better to > have > > > less defines in code. > > > > Ok, will do. > > > > > > > >>> + /* ODP initialisation */ > > > >>> + { > > > >> > > > >> usually we do not use additional brackets. I think here they > also need > > > >> to be removed to match common style. > > > > > > > > In that particular instance, I agree, they are unnecessary. > Would you also > > > > suggest the removal of these extra scoping brackets in other > places within > > > > this same function though? In my opinion, the extra scope they > provide in the > > > > other places make the code easier to read and reason about. > > > > > > > > > > yes, in all other places also. Brackets might be separate static > inline > > > function. > > > > A static function is something very different from an inner scope in a > > function. > > The inner scope can directly access and modify variables in outer scopes. > > A static function would have to take all of these as input and output > > parameters. Messy. > > Declaring all function variables at the top of the function regardless > > of where they are (actually) used is just confusing and increases the > > risk for errors. Why not use a standard feature of the language that is > > there to help the programmer? > > > > I think linux kernel community found that smaller functions are easier > to maintenance. If you need brackets then more likely you need to split > your function on 2 smaller. > I disagree that this is the general solution. Sometimes this make sense but if you have multiple inputs and outputs this becomes very ugly and possibly inefficient (have to pass a lot of parameters by reference so that they can be modified, passing by pointer is not helpful to optimisation). C isn't very good at returning multiple values, no tuple support. Minimising the scope of variables is a good idea. Taking your suggestion to the extreme, all variables should be global. > > We also do not use function declaration inside function and might be a > lot of things which possible to do with C. C is already a very minimalistic language. I don't understand why we can't use language features that were supported already in K&R C. > > > > Give up Maxim or we begin bombing in five minutes. > > > > We have a woman here in jail for sms about tanks about 6 years ago. Not > all people can read messages as jokes :) > This is an old joke, I hoped people would be familiar with it. > > > > I'm of the opinion that separate functions (inline or otherwise) are > too > > heavy a tool for this purpose. If the goal is to introduce some > > additional > > scoping within a fairly uncomplicated function body, and not to > > allow for > > reuse or more granular abstractions, extra braces are the ideal > > solution. > > If these really are considered inconsistent for whatever reason > > though, I can > > just dump all the nicely scoped variables into one big list, making > > the code > > less readable should it be desired. > > > > > >>> + for (int i = 0; i < FRAGLISTS; ++i) > > > >>> + init_fraglist(&fraglists[i]); > > > >>> + > > > >> > > > >> no declaration for int inside loop. > > > > Well this is an example, not part of the ODP implementation. Examples > > should be able to use whatever language or standard they want. If > > someone contributes an example of using ODP from Erlang, will we respond > > with "Not welcome, only a subset of C89 or C99 allowed!" ? Or more > > realistically, an example written in some C++ standard (perhaps C++11). > > > > Because of Linaro have to maintain examples in future. There have to be > exact rules which code do we support in example directory. For now we > did not write which applications we accept. I think maintains of some > complex Erlang program will require Erlang programmer for that. C++11 > might depend on compiler version and c++/boost will require additional > libraries with their version. I think just need to discuss and write > done rules. > > > > > > > > > > > It might be worth adding this one to the checkpatch script or to > the compiler > > > > flags as a warning. Personally, I'm really quite opposed to > needlessly > > > > cluttering up the list of variable declarations with a mere loop > counter (it > > > > increases the amount of state that the reader has to think about > when parsing > > > > the code), but am happy to oblige (in all the various places > within the code > > > > that I've used this style of loop counter declaration) if this > is part of the > > > > project's official code style guidelines. > > > > > > > > > > In C++ people always do declaration like that. > > > > Because it is allowed by the C++ standard? This feature was added to C++ > > for exactly the reasons Joe describe below. > > > > > > > On linux kernel - never. > > > > Because the Linux kernel was originally written (and might still have to > > adhere to?) a C standard that do not allow for this? > > no, it has to compile but people do not use it. I don't know why. > > > > > > We decided to follow linux kernel code style. Yes, it's > interesting why > > > checkpatch did not warn on it. I has to. > > > > So perhaps this is OK after all. > > > > > > > > This seems like a particularly silly decision, as declaring > variables as > > locally as possible generally makes code easier to reason about and > > understand, and this is a prime example of that. This is especially > > important > > for code in the example directory, where coding conventions from > > nearly 30 > > years ago are surely less relevant! But, nonetheless, as the rule is > > already > > in place, I suppose I have no choice but to use separate > > declarations in the > > name of consistency. > > > > > >
