Re: [Wireshark-dev] switching to proto_tree_add_subtree()
proto_tree_add_subtree should be proto_tree_add_text + proto_item_add_subtree (where the return value of proto_tree_add_text is input to proto_item_add_subtree). When I originally created proto_tree_add_subtree, I thought it would be more optimized to include the function body of proto_tree_add_text, rather than just call the function itself. It seems like Martin found a subtly in why that shouldn't be done (that I still don't understand). However, the tree_item variable is intended to add as the return value for the hidden proto_tree_add_text, which dissectors can (and do) use to append more text or set the length to. So perhaps the simplest fix is to just have proto_tree_add_subtree be what it really was intended to be - just proto_tree_add_text (as the function call) + proto_item_add_subtree. Michael -Original Message- From: darkjames-ws darkjames...@darkjames.pl To: Developer support list for Wireshark wireshark-dev@wireshark.org Sent: Wed, Jul 30, 2014 1:32 am Subject: Re: [Wireshark-dev] switching to proto_tree_add_subtree() Hi, On Tue, Jul 29, 2014 at 06:47:53PM -0400, Jeff Morriss wrote: However, I don't quite understand why for tree!=NULL but not visible, proto_tree_add_text() returns tree. I can see this in the code, we call TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not visible. But what sense does this make for the caller? I think it's because while proto_trees are allowed to be NULL, we don't want (or the API is not ready for) proto_items to be NULL. (IOW that return tree quoted above is (ab)using the fact that a proto_item is a proto_tree.) Does this mean that this code in add_subtree_format() should be setting *tree_item to 'tree' (instead of NULL) here: /* Make sure pi is initialized in case TRY_TO_FAKE_THIS_ITEM bails */ if (tree_item != NULL) *tree_item = NULL; Wouldn't be it simpler if we just remove 'tree_item' argument? Cause I understand that after change *tree_item is always the same as return value? Regards, Jakub. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
On Wed, Jul 30, 2014 at 11:23:55AM -0400, Jeff Morriss wrote: On 07/30/14 01:30, darkjames...@darkjames.pl wrote: On Tue, Jul 29, 2014 at 06:47:53PM -0400, Jeff Morriss wrote: Does this mean that this code in add_subtree_format() should be setting *tree_item to 'tree' (instead of NULL) here: /* Make sure pi is initialized in case TRY_TO_FAKE_THIS_ITEM bails */ if (tree_item != NULL) *tree_item = NULL; Wouldn't be it simpler if we just remove 'tree_item' argument? Cause I understand that after change *tree_item is always the same as return value? Well, no, they'll only return the same thing if TRY_TO_FAKE_THIS_ITEM short-cuts us out, right? 1138 proto_tree_add_subtree_format(proto_tree *tree, tvbuff_t *tvb, gint start, gint length, gint idx, proto_item **tree_item, const char *format, ... ) ... 1159 pt = proto_item_add_subtree(pi, idx); 1160 if (tree_item != NULL) 1161 *tree_item = pi; 1162 1163 return pt; And proto_item_add_subtree(pi, ...) returns pi, so *tree_item == pt == pi. Anyway, sorry, I'm fine with this API. If you want to ever make proto_item != proto_tree without even more refactoring it makes sense. BR, Jakub ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
On 07/30/14 14:02, darkjames...@darkjames.pl wrote: On Wed, Jul 30, 2014 at 11:23:55AM -0400, Jeff Morriss wrote: On 07/30/14 01:30, darkjames...@darkjames.pl wrote: On Tue, Jul 29, 2014 at 06:47:53PM -0400, Jeff Morriss wrote: Does this mean that this code in add_subtree_format() should be setting *tree_item to 'tree' (instead of NULL) here: /* Make sure pi is initialized in case TRY_TO_FAKE_THIS_ITEM bails */ if (tree_item != NULL) *tree_item = NULL; Wouldn't be it simpler if we just remove 'tree_item' argument? Cause I understand that after change *tree_item is always the same as return value? Well, no, they'll only return the same thing if TRY_TO_FAKE_THIS_ITEM short-cuts us out, right? 1138 proto_tree_add_subtree_format(proto_tree *tree, tvbuff_t *tvb, gint start, gint length, gint idx, proto_item **tree_item, const char *format, ... ) ... 1159 pt = proto_item_add_subtree(pi, idx); 1160 if (tree_item != NULL) 1161 *tree_item = pi; 1162 1163 return pt; And proto_item_add_subtree(pi, ...) returns pi, so *tree_item == pt == pi. Anyway, sorry, I'm fine with this API. If you want to ever make proto_item != proto_tree without even more refactoring it makes sense. https://code.wireshark.org/review/3271 ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
Hi, On Mon, Jul 28, 2014 at 10:47:43PM +0200, Martin Kaiser wrote: However, I don't quite understand why for tree!=NULL but not visible, proto_tree_add_text() returns tree. I can see this in the code, we call TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not visible. But what sense does this make for the caller? Just to make filtering works. We're keeping array of interesting fields, stored in tree-tree_data-interesting_hfids. In common case like this: foobar_tree = proto_tree_add_subtree_format(tree, ..., Foobar); proto_tree_add_item(foobar_tree, hf_foobar_xyz, ...); proto_tree_add_item(foobar_tree, hf_foobar_aaa, ...); when foobar_tree == NULL, proto_tree_add_item() will fast return in if (!tree) check inside TRY_TO_FAKE_THIS_ITEM_OR_FREE Cheers, Jakub. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
Thus wrote darkjames...@darkjames.pl (darkjames...@darkjames.pl):
Hi,
On Mon, Jul 28, 2014 at 10:47:43PM +0200, Martin Kaiser wrote:
However, I don't quite understand why for tree!=NULL but not visible,
proto_tree_add_text() returns tree. I can see this in the code, we call
TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not
visible. But what sense does this make for the caller?
Just to make filtering works.
We're keeping array of interesting fields, stored in
tree-tree_data-interesting_hfids.
In common case like this:
foobar_tree = proto_tree_add_subtree_format(tree, ..., Foobar);
proto_tree_add_item(foobar_tree, hf_foobar_xyz, ...);
proto_tree_add_item(foobar_tree, hf_foobar_aaa, ...);
when foobar_tree == NULL, proto_tree_add_item() will fast return in if
(!tree) check inside TRY_TO_FAKE_THIS_ITEM_OR_FREE
This case is ok, I understand that this makes things faster if we don't
have a tree.
I'm confused about this block in TRY_TO_FAKE_THIS_ITEM_OR_FREE
if (!(PTREE_DATA(tree)-visible)) { \
if (PTREE_FINFO(tree)) { \
if ((hfinfo-ref_type != HF_REF_TYPE_DIRECT) \
(hfinfo-type != FT_PROTOCOL || \
PTREE_DATA(tree)-fake_protocols)) { \
free_block; \
/* just return tree back to the caller */\
return tree; \
If tree is not visible (and fake_protocols is set, which seems to be the
default), we return the tree itself.
proto_item *it = proto_tree_add_text(tree, tvb, 0, -1, foobar);
If tree!=NULL !(PTREE_DATA(tree)-visible) the return value it==tree
Why does this make sense?
Regards,
Martin
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
Hi,
On Tue, Jul 29, 2014 at 08:33:57PM +0200, Martin Kaiser wrote:
I'm confused about this block in TRY_TO_FAKE_THIS_ITEM_OR_FREE
if (!(PTREE_DATA(tree)-visible)) { \
if (PTREE_FINFO(tree)) { \ ### [1] Sake workaround for some
bugs (details: 00c05ed3f)
if ((hfinfo-ref_type != HF_REF_TYPE_DIRECT) \
(hfinfo-type != FT_PROTOCOL || \
PTREE_DATA(tree)-fake_protocols)) { \
free_block; \
/* just return tree back to the caller */\
return tree; \
If tree is not visible (and fake_protocols is set, which seems to be the
default), we return the tree itself.
proto_item *it = proto_tree_add_text(tree, tvb, 0, -1, foobar);
If tree!=NULL !(PTREE_DATA(tree)-visible) the return value it==tree
Why does this make sense?
Ok, what value you propose to return instead of 'tree'?
IMHO we could return:
a/ tree root (right now not so good idea cause of [1] line)
b/ some fake item.
strikec/ NULL/strike (you cannot cause it will make filtering stop
working).
Right now AFAIK some API should be carefully used when we're faking tree - like
proto_item_set_len() or proto_item_append_string() (it might be can of open
bugs),
Still, currently it works quite well, isn't it?
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
On Tue, Jul 29, 2014 at 09:18:18PM +0200, darkjames...@darkjames.pl wrote:
Hi,
On Tue, Jul 29, 2014 at 08:33:57PM +0200, Martin Kaiser wrote:
I'm confused about this block in TRY_TO_FAKE_THIS_ITEM_OR_FREE
if (!(PTREE_DATA(tree)-visible)) { \
if (PTREE_FINFO(tree)) { \ ### [1] Sake workaround for some
bugs (details: 00c05ed3f)
if ((hfinfo-ref_type != HF_REF_TYPE_DIRECT) \
(hfinfo-type != FT_PROTOCOL || \
PTREE_DATA(tree)-fake_protocols)) { \
free_block; \
/* just return tree back to the caller */\
return tree; \
If tree is not visible (and fake_protocols is set, which seems to be the
default), we return the tree itself.
proto_item *it = proto_tree_add_text(tree, tvb, 0, -1, foobar);
If tree!=NULL !(PTREE_DATA(tree)-visible) the return value it==tree
Why does this make sense?
Ok, what value you propose to return instead of 'tree'?
strikec/ NULL/strike (you cannot cause it will make filtering stop
working).
Ah! We could do if (it == NULL) { it = tree; } when creating subtree, for leaf
we don't need a fix.
It still lot of work to do, but I'm +0.5 for it ;-)
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
On 07/29/14 15:39, darkjames...@darkjames.pl wrote:
On Tue, Jul 29, 2014 at 09:18:18PM +0200, darkjames...@darkjames.pl wrote:
Hi,
On Tue, Jul 29, 2014 at 08:33:57PM +0200, Martin Kaiser wrote:
I'm confused about this block in TRY_TO_FAKE_THIS_ITEM_OR_FREE
if (!(PTREE_DATA(tree)-visible)) { \
if (PTREE_FINFO(tree)) { \ ### [1] Sake workaround for some
bugs (details: 00c05ed3f)
if ((hfinfo-ref_type != HF_REF_TYPE_DIRECT) \
(hfinfo-type != FT_PROTOCOL || \
PTREE_DATA(tree)-fake_protocols)) { \
free_block; \
/* just return tree back to the caller */\
return tree; \
If tree is not visible (and fake_protocols is set, which seems to be the
default), we return the tree itself.
proto_item *it = proto_tree_add_text(tree, tvb, 0, -1, foobar);
If tree!=NULL !(PTREE_DATA(tree)-visible) the return value it==tree
Why does this make sense?
Ok, what value you propose to return instead of 'tree'?
strikec/ NULL/strike (you cannot cause it will make filtering stop
working).
Ah! We could do if (it == NULL) { it = tree; } when creating subtree, for leaf
we don't need a fix.
It still lot of work to do, but I'm +0.5 for it ;-)
Going back to Martin's initial problem...
However, I don't quite understand why for tree!=NULL but not visible,
proto_tree_add_text() returns tree. I can see this in the code, we call
TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not
visible. But what sense does this make for the caller?
I think it's because while proto_trees are allowed to be NULL, we don't
want (or the API is not ready for) proto_items to be NULL. (IOW that
return tree quoted above is (ab)using the fact that a proto_item is a
proto_tree.)
Does this mean that this code in add_subtree_format() should be setting
*tree_item to 'tree' (instead of NULL) here:
/* Make sure pi is initialized in case TRY_TO_FAKE_THIS_ITEM bails */
if (tree_item != NULL)
*tree_item = NULL;
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
On Tue, Jul 29, 2014 at 6:47 PM, Jeff Morriss jeff.morriss...@gmail.com
wrote:
On 07/29/14 15:39, darkjames...@darkjames.pl wrote:
On Tue, Jul 29, 2014 at 09:18:18PM +0200, darkjames...@darkjames.pl
wrote:
Hi,
On Tue, Jul 29, 2014 at 08:33:57PM +0200, Martin Kaiser wrote:
I'm confused about this block in TRY_TO_FAKE_THIS_ITEM_OR_FREE
if (!(PTREE_DATA(tree)-visible)) { \
if (PTREE_FINFO(tree)) { \ ### [1] Sake workaround for
some bugs (details: 00c05ed3f)
if ((hfinfo-ref_type != HF_REF_TYPE_DIRECT) \
(hfinfo-type != FT_PROTOCOL || \
PTREE_DATA(tree)-fake_protocols)) { \
free_block; \
/* just return tree back to the caller
*/\
return tree; \
If tree is not visible (and fake_protocols is set, which seems to be the
default), we return the tree itself.
proto_item *it = proto_tree_add_text(tree, tvb, 0, -1, foobar);
If tree!=NULL !(PTREE_DATA(tree)-visible) the return value it==tree
Why does this make sense?
Ok, what value you propose to return instead of 'tree'?
strikec/ NULL/strike (you cannot cause it will make filtering stop
working).
Ah! We could do if (it == NULL) { it = tree; } when creating subtree, for
leaf we don't need a fix.
It still lot of work to do, but I'm +0.5 for it ;-)
Going back to Martin's initial problem...
However, I don't quite understand why for tree!=NULL but not visible,
proto_tree_add_text() returns tree. I can see this in the code, we call
TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not
visible. But what sense does this make for the caller?
I think it's because while proto_trees are allowed to be NULL, we don't
want (or the API is not ready for) proto_items to be NULL. (IOW that
return tree quoted above is (ab)using the fact that a proto_item is a
proto_tree.)
Does this mean that this code in add_subtree_format() should be setting
*tree_item to 'tree' (instead of NULL) here:
/* Make sure pi is initialized in case TRY_TO_FAKE_THIS_ITEM bails */
if (tree_item != NULL)
*tree_item = NULL;
That's my understanding, to restore the old behaviour at least for now
until we settle how it should really behave.
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] switching to proto_tree_add_subtree()
Hi, On Tue, Jul 29, 2014 at 06:47:53PM -0400, Jeff Morriss wrote: However, I don't quite understand why for tree!=NULL but not visible, proto_tree_add_text() returns tree. I can see this in the code, we call TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not visible. But what sense does this make for the caller? I think it's because while proto_trees are allowed to be NULL, we don't want (or the API is not ready for) proto_items to be NULL. (IOW that return tree quoted above is (ab)using the fact that a proto_item is a proto_tree.) Does this mean that this code in add_subtree_format() should be setting *tree_item to 'tree' (instead of NULL) here: /* Make sure pi is initialized in case TRY_TO_FAKE_THIS_ITEM bails */ if (tree_item != NULL) *tree_item = NULL; Wouldn't be it simpler if we just remove 'tree_item' argument? Cause I understand that after change *tree_item is always the same as return value? Regards, Jakub. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] switching to proto_tree_add_subtree()
Hi all, I'm sending out some unsorted thoughts, hoping that you can help me understand the issue I'm seeing. After the switch to proto_tree_add_subtree(), I found that some output of the DVB-CI dissector was different than before. Replacing ti = proto_tree_add_text(tree, tvb, offset, tvb_data_len, Resource ID: 0x%04x, res_id); res_tree = proto_item_add_subtree(ti, ett_dvbci_res); with res_tree = proto_tree_add_subtree_format(tree, tvb, offset, tvb_data_len, ett_dvbci_res, ti, Resource ID: 0x%04x, res_id); Does not give 100% identical behaviour. When tree!=NULL but not set to visible, ti will be !=NULL in the fist case and ==NULL after the change to proto_tree_add_subtree_format() I came across this in a function that does something like check if input is valid, return NULL on error create proto_item from the input create tree populate tree return proto_item The caller would then check the returned proto_item for NULL to see if the input could be processed. This started failing when proto_tree_add_subtree_format() is used. My gut feeling is that proto_tree_add_subtree_format() does the right thing and I should fix my dissector... However, I don't quite understand why for tree!=NULL but not visible, proto_tree_add_text() returns tree. I can see this in the code, we call TRY_TO_FAKE_THIS_ITEM(), which returns the tree itself when it's not visible. But what sense does this make for the caller? Best regards, Martin ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
