Re: a bug or a feature

[João Valverde <j () v6e pt>]

On 03/11/21 10:50, Zoran Bošnjak wrote:
> Hello wireshark developers,
> I would appreciate some clarification about "making preferences obsolete".
>
> As this problem reports (which is also in accordance with README.dissector)
> https://gitlab.com/wireshark/wireshark/-/issues/17697
> ... is to make each removed preference explicitely obsolete with a call to
> prefs_register_obsolete_preference.
>
> By calling this function, the misconfiguration warning is suppressed.
>
> The reported issue is easy to fix and also easy to modify as suggested by reviewer, but I have some general questions 
> about the preferences. Also, I am not sure if this fix is really wanted, but rather a register_obsolete needs a review.
>
> 1. The original warning looks like a correct behaviour. It's informative and it correctly indicates a misconfiguration 
> to the user.
> Why would one want to suppress the warning?
>
> 2. If each such warning is to be suppressed, then why logging it in the first place?
>
> 3. The mechanism is fragile. It only handles the change once. Once the preference is made obsolete, the call to
> prefs_register_obsolete_preference remains in the code forever. This raises the question:
> How do you re-introduce the same preference back, in particular:
> Do you remove the register_obsolete call from the code or keep it in the code?
> If it's "remove the call": then it was unnecessary in the first place, because the call is exactly
> the negation of a preference being currently present (which we already known from the rest of the code)
> If it's "to keep the call": then it's wrong, because the preference is present and the call says
> it's obsolete.

I assume the intention is to keep the obsolete call indefinitely. It's 
not intended to be re-introduced with a different behavior either. I 
think you raise some valid points but why is the option "keep the call" 
wrong? A more wordy name might be "no_longer_actively_used" instead of 
deprecated. Does that fix whatever is wrong?
> 4. A patch to the reported problem was attempted
> https://gitlab.com/zoranbosnjak/wireshark/-/commit/3e6fe4e0c3eca2b21151176c988ea36adac10b40
> In this case, there is a closed set (256) of possible preferences, so that all not-supported categories can be made 
> obsolete in advance, which effectively suppresses the warnings, regardless of the supported category set.
> A reviewer suggested a "lean" approach which suffers the remove/add problem and it requires hardcoding a category list.
> If the reported issue really is a problem, why "lean" approach? I can not forsee any noticable burden for the preference 
> engine. It's like trying to optimize for assembler, rather than source C code (the dissector C code is generated in this case and 
> is therefore at the lower layer which does not need such optimization, at the cost of the real source code being suboptimal and in 
> this case even (potentially) wrong). Please clarify the reasoning behind this.
>
> regards,
> Zoran
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>               mailto:wireshark-dev-request () wireshark org?subject=unsubscribe

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
            mailto:wireshark-dev-request () wireshark org?subject=unsubscribe