Comment # 5
on bug 8472
from Evan Huus
(In reply to comment #4)
> Hi Michael and Evan,
>
> thank you for reviewing the patch!
>
> (In reply to comment #2)
> > Also, please attach a sample capture to test with.
>
> Done.
>
> (In reply to comment #1)
> > Do you really need the [abs|rel]_time_to_str calls? Can't you just use
> > proto_tree_add_time and make those fields filterable instead of using
> > proto_tree_add_text?
>
> Each packet contains several sub-components. Each sub-component contains a
> type (hostname, time, interval, …), a length and the actual payload.
>
> The {abs,rel}_time_to_str() / proto_tree_add_text() calls are used in two
> places. The first use is for the tree node representing the entire
> sub-component. The type, length and payload are then added as children to
> this node. These leaf nodes are added with proto_tree_add_time() at the end
> of the dissect_collectd_integer() function, so the filtering does work.
>
> proto_tree_add_text() is used so the "item" is added to the protocol tree
> with the "header field" only once. Should I use
> proto_tree_add_time_format_value() instead?
No, that's fine. There's just enough space between the two changes that the
patch has them as different hunks, so it wasn't obvious that they went
together. What you're doing here is pretty standard.
> The second use is more cosmetic: Each packet can contain multiple metrics.
> Each VALUES sub-component marks the end of the information for one metric.
> If some information about a metric is identical for the previous metric,
> e.g. the same hostname is used, this information is not re-transmitted. So
> in many packets the hostname, interval, … is transmitted only once.
>
> The VALUES sub-component includes a "simulation" in the tree which shows the
> currently active values for all the appropriate fields, so that
> understanding which values are actually transmitted is easier and doesn't
> involve scanning the packet dissection backwards to find the last INTERVAL
> sub-component or similar.
>
> Here, too, _add_text() is used in order to skip referencing the "header
> field". If you prefer, I can switch that to _add_time_format_value() too, if
> you say that this is preferred.
Assuming I've followed this correctly (since it seems a bit complicated):
- Can these implied values carry across multiple packets, or is it only between
sub-components of a single packet?
- My initial inclination would be to make these fields use the same correct hf
references as the actual values, but then to set them as generated using the
PROTO_ITEM_SET_GENERATED macro. I think that's the standard way of doing
something like this. In this case you shouldn't even need
_add_time_format_value, you should just be able to _add_time, since the hidden
flag will take care of distinguishing it from 'normal' time fields.
Cheers,
Evan
You are receiving this mail because:
- You are watching all bug changes.