https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7658
--- Comment #3 from Martin Kupec <magonk@xxxxxxxxx> 2012-08-22 15:01:21 PDT ---
(In reply to comment #2)
> Hi Martin, thanks for the patch. I'm afraid I can't comment on the modification
> to proto.c - hopefully somebody else will be able to take a look at that - but
> I've given the rest of the patch a quick review. No major problems I don't
> think, but a few nits:
>
> - Is there any particular reason the LLRP_COMM_STANDARD #defines aren't
> capitalized?
Yes, great one, I have missed it. I have fixed it.
>
> - You use a couple of variadic macros. Unfortunately, not all compilers support
> those (this is documented in an obscure corner of /doc/README.developer).
Ok, It was simple to fix. I have actually used only fixed number of parameters.
>
> - Speaking of macros, you do some interesting things with them that I'm not
> necessarily comfortable with. I'll admit that I haven't taken the time to fully
> understand them yet, but it would make me a lot happier if you replaced them
> with more traditional dissector code. I'm not sure how much typing some of them
> saved you, anyways...
I can feel your concerns. It is just shortcut to make hf field variable and hf
field setting at the same time.
What is the standard way anyway?
Declare "static int hf_llrp_field_name = -1;" and than in
proto_register_llrp register the field?
My problem with this workflow is that I am forced "declare" the variable twice.
Once in the actual variable declaration and than use it in the field
declaration.
When I was writing all the parameters dissection this annoyed me, so I made
this macro. But now as all the variables are quite stable it should be possible
to just expand it manually.
>
> - There are a couple of places (the switch case for
> LLRP_IMPINJ_PARAM_REDUCED_POWER_FREQUENCY_LIST caught my eye) where you get a
> value from the TVB and loop over it, adding a new tree item each time. Before
> looping you should check the value you got against the length of the buffer, as
> otherwise it's possible to effectively hang Wireshark by putting an enormous
> value in that field.
I have sorted this out. I have moved it to separate function and made a check
there.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
