On Tuesday 05 January 2010 12:20:58 Linus Lüssing wrote:
Hmm, so far we are having too modes only, vis server being enabled or disabled. But in those packet functions we are talking about types (two ones only so far again) instead. In the second one I found it ok to use the defines, but for the proc.c switching, I found a simple 0 or 1 more intuitive, as you already know that this function is for enabling/disabling the vis server and you don't have to wonder about a certain vis-types field in the vis-packet format. Hmm, I'm also wondering, are there any plans for introducing other vis_types? (What would you think about changing the 8 bits vis_type field to flags instead?)
I think using defines instead of hard-coded values always is the better choice. Even if it is more intuitive for you now you never know when this code is going to be extended. In addition, you still want to understand the code in 6 months and/or make it easier of others to understand.
Whether it makes sense to rename more variables / defines is another discussion and belongs into another patch. :)
Damn it, you are right :). I assume, that it'd be ok to remove the default value being set in vis_init and explicitly setting this field every time a new vis packet gets generated in generate_vis_packet() (instead of updating this variable from proc.c too)?
I think a default value is good to have. ;-) generate_vis_packet() looks like a good place to update the packet's variable.
Yes, sorry, you're right I'd removed the server/client/enabled/disabled things and added just 0/1 as input values for proc to make the syntax similar to the aggregation stuff and simple in general for the kernel module. I'm going to put that in a seperate patch.
Great.
Regards, Marek