[asterisk-dev] ast_flags now uint64_t ? should it be reverted ?

[Steve Murphy]

On Wed, 2007-07-18 at 07:13 -0700, Luigi Rizzo wrote:
> Hi,
> the following change modified ast_flags to uint64_t
> 
> 	http://svn.digium.com/view/asterisk?view=rev&revision=75400
> 
> causing a rather massive change, both in-tree and to third party
> code, all for the benefit of a single application, app_dial.c
> 
> I don't think this change is a good idea, partly because of the overall
> increase in memory and processing time (especially on low-end boxes),
> but mainly because it might open the way to hard-to-detect bugs
> because of truncation in assignments.
> 
> I'd suggest to have this change reverted, and if that one application
> (app_dial.c) needs a larger set of flags, consider storing some of
> them in a different variable.
> 
> As an example, in app_dial.c, there is no reason to have
> DIAL_STILLGOING and DIAL_NOFORWARDHTML stored together
> with the various OPT_* flags. This can be fixed trivially.
> 

Luigi--

I tried to follow this advise, but it just plain scares me.
I don't know what your definition of "trivial" is, but scanning every
bit of code in app_dial, and all the functions defined therein, and 
studying how and which flags are tested, set, and copied for under
what conditions, did not strike me as "trivial"!

As best I can tell (I did not fully finish my analysis; I gave up
with the determination to come back if other options failed), the 
DIAL_STILLGOING and DIALNOFORWARDHTML flags are set in both peerflags
and chanlist flags, along with these others, which I gathered either in 
groups, or one by one:

OPT_IGNORE_FORWARDING
OPT_ORIGINAL_CLID
OPT_DTMF_EXIT
OPT_GO_ON
OPT_CALLEE_TRANSFER
OPT_CALLER_TRANSFER
OPT_CALLEE_HANGUP
OPT_CALLER_HANGUP
OPT_CALLEE_MONITOR
OPT_CALLER_MONITOR
OPT_CALLEE_PARK
OPT_CALLER_PARK

Theoretically, I could change the value of DIAL_STILLGOING and
DIALNOFORWARDHTML,
to any value not equal to any in the above list, and of course, not
equal to
each other, and probably be OK. It's the "theoretically", and the
"probably" 
that bother me. And heaven help us if some maintainer finds a bug and
adds another flag to the list later, that accidentally conflicts with
whatever values we choose! Folding flag values to open up an option
value is kinda dangerous.

Whoever inserted the STILLGOING and NOFORWARDHTML flag values, who
decided not to make them collide with existing option flag values, I
would label as "wise". And I am not eager to join the other side of the
camp. Especially for a hack that's only good for 1 spare flag (I'm using
up one of them).

But my own offer to do make a different app_parse_args function that
would dump into 64-bit flags train-wrecked also; I ended up making
64-bit versions of the parse_args func, the ast_flags structure, and I
had to make 64 bit versions of the ast_set_flag, ast_test_flag, etc.
macros. I had to change all the ast_set_flags, ast_test_flag calls, that
dealt with option flags, peer flags, and chan_list flags, and leave the
stuff that dealt with channel flags and feature flags alone. The size of
this mod was bigger and more involved than what's in trunk right now.
I'll post the diffs on the original bug report (10206). My plan is to
revert the mods in my workspace and try something else!

How about this? I like the idea of splitting things up into
caller/callee options
in a new app, app_dial2. We could further break it down into more option
flag subsets, with a max of 3 or 4 option sets, if possible, and pass
each as a separate argument in the app call. Shall I do the analysis?
I'd rather do the "right" thing, than make an (uglier) temporary hack
right now. I would tend to keep the current option letters the same, but
I think it would be best to make the T and the t options end up being
just "t" in their respective option sets, same for W & w, and so on.

Steve Critchfield's idea wasn't bad; it would work; but I'm more for
uniformity. Unless we change all the apps to work that way, I'm not
feeling warm and cozy about it. And if the dialplan needs to do one or
more lines of setup to call an app, then it's clumsy, and won't fly
well.


murf

-- 
Steve Murphy
Software Developer
Digium
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3227 bytes
Desc: not available
Url : http://lists.digium.com/pipermail/asterisk-dev/attachments/20070719/ed4dc23e/attachment-0001.bin