[Asterisk-code-review] RFC: ao2 reference storage location logging. (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: RFC: ao2 reference storage location logging.
......................................................................


Patch Set 3:

> I don't have any issue with what is attempting to be accomplished
 > here. My concerns are the following:
 > 
 > (1) The plethora of new API calls requires everyone to learn the
 > 'new' way to use ao2 in order to get better tracking. The API
 > proposed - while there isn't anything wrong - puts the ao2 API in
 > the camp of many older APIs that have '_ex' or '_s'
 > prepended/appended everywhere. Ideally, we'd have a way to
 > accomplish the goals here without requiring all API calls that
 > modify a reference count from being renamed. I get that there may
 > not be an easy mechanism for this, since the pairing of reference
 > count bumps can be difficult to match up (and may not always be
 > known apriori), but some of the simpler ones may be able to be
 > handled by simply knowing the pointer address of the object being
 > modified - or the address of the function that performed the
 > manipulation.

So part of this question do we want to have _s macro's?  These are all convenience macro's, none are technically required but are nice in object code.

Using conventional API's with debugstorage:
myobj->field = ao2_bump_full(value1, "", &myobj->field);
if (myobj->field != value2) {
  ao2_ref_full(value2, +1, "", &myobj->field);
  ao2_ref_full(myobj->field, -1, "", &myobj->field);
  myobj->field = value2;
}
if (myobj->field) {
  ao2_ref_full(myobj->field, -1, "", &myobj->field);
}

Same using AO2 Storage API:
ao2_s_set(&myobj->field, value1);
ao2_s_replace(&myobj->field, value2);
ao2_s_cleanup(&myobj->field);

Storage API's are much easier to use within objects for init / updating / cleanup of member fields, but not as useful for functions that return bumped objects.  The current plan is that higher level Storage API's will be implemented as macro's using ao2_s_getter or __ao2_s_getter.

I do agree about getting rid of the '_full' versions of functions but I'd like to think it through / get the API right.

 > (2) Since this is in master, I think it's reasonable to propose a
 > breaking API change in ao2. While this is painful, as it will
 > absolutely break the crap out of third party modules, that is - for
 > better or worse - what master is for. It's at least worth debating,
 > given the scope of the change - although I'd hate to think of all
 > of the lines of code that would have to be updated to take
 > advantage of this.

Actually some of the API have already changed since 13.  __ao2_alloc and __ao2_ref now include the file/line/func parameters for 14 since addition of the asterisk.conf option to enabled refdebug.  Only 19 lines of code outside of astobj2* directly use __ao2_alloc/__ao2_ref/__ao2_find/__ao2_callback, so the impact of adding a parameter to them will be low.

The biggest benefit is probably from updating format*.c / codec.c, especially where references might be part of RTP processing.

I'll think about the astobj2.h interfaces some more, write up an email to asterisk-dev requesting comments.

-- 
To view, visit https://gerrit.asterisk.org/3141
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacb0a51cefaa98c83eab18aa2b7d18850bb33951
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No