Re: switching to proto_tree_add_subtree()

[Evan Huus <eapache () gmail com>]

On Tue, Jul 29, 2014 at 6:47 PM, Jeff Morriss <jeff.morriss.ws () gmail com>
wrote:

> On 07/29/14 15:39, darkjames-ws () darkjames pl wrote:
>
> > On Tue, Jul 29, 2014 at 09:18:18PM +0200, darkjames-ws () 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'?
> > >
> > >   <strike>c/ 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-request () wireshark org?subject=unsubscribe