On 05/06/2012 10:14 PM, Simon Wunderlich wrote:
Your patch does the trick, although I think this ugly function could use a rewrite. First counting bytes and then allocating this size to do exactly the same thing again is not really good style. If you would like to volunteer to do this job (or plan to work more on vis), please tell me, otherwise I'll put it in on my TODO list.
While I'am already at it, I guess I can also volunteer to do some more vis work :)
Besides cleanup, are there more ideas about the vis? A nice feature I can think about would be adding some freely chosen identification string (for example the hostname) to the vis data, this could make big graphs much more readable. I wonder though if this would be possible without breaking compatiblity.
I have some questions about the code though:
- Is there any reason vis_seq_print_text() allocates a buffer at all instead of just printing the data directy into the seq_file? Looking at the seq_printf implementation, there doesn't seem to be a problem calling it while holding the lock. - In many places in the vis code hlist_for_each_entry_rcu() is used to iterate over the hash lists, even though all access to vis_hash is guarded by the vis_hash_lock, so it seems to be okay to just use hlist_for_each_entry(). In some functions, vis_seq_print_text() being one of them, rcu_read_lock/unlock pairs could be removed as well with this change. Do I overlook something?
I'll also drop by the batman IRC channel.
Matthias