> On July 9, 2014, 9:45 p.m., Corey Farrell wrote: > > /team/group/media_formats-reviewed-trunk/main/bridge_channel.c, line 324 > > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62646#file62646line324> > > > > Check still needed or can we just assert?
This one is a bit interesting, in that the formats on the channel get saved off, an attempt to make the channel compatible with everything else occurs - where new formats are placed on the channel - leading to this path getting hit. I have mixed feelings about this: there's a lot going on leading up to this point, and I'm not sure we can always predict that there will never be a case where a format won't be NULL. We can try asserts and restore these checks if needed. > On July 9, 2014, 9:45 p.m., Corey Farrell wrote: > > /team/group/media_formats-reviewed-trunk/main/codec_builtin.c, line 118 > > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62649#file62649line118> > > > > I'm not sure AUDIO is appropriate. Maybe UNKNOWN is better, or even a > > new AST_MEDIA_TYPE_NULL? If this isn't audio, we're going to get more NULL format hits. A lot of things assume that there is a single audio format on the channel; consider every single format that is passed to the translation core. Whether or not that's appropriate is another question; but if the point of this is to try and prevent seg faults from a channel with no formats getting abused, not using audio is going to lead to crashing. > On July 9, 2014, 9:45 p.m., Corey Farrell wrote: > > /team/group/media_formats-reviewed-trunk/main/format_cache.c, line 437 > > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62650#file62650line437> > > > > This procedure could previously unset a format: > > ast_format_cache_set("ulaw", NULL); > > > > Is that not needed? > > > > Also ast_format_get_name is called many times, it would be better to > > call it once and save to char *name; It isn't needed. The name field is actually broken: you can set a format by some random name, but that name is not cached with a format; hence, you cannot look up a format using the name you provided. Instead, you need to use ast_format_create_named. That will allow you to assign a custom name to format, which is equivalent to what this used to do. Caching the result of ast_format_get_name is unlikely to be useful; compilers tend to handle simple accessors just fine. > On July 9, 2014, 9:45 p.m., Corey Farrell wrote: > > /team/group/media_formats-reviewed-trunk/main/format_cache.c, line 442 > > <https://reviewboard.asterisk.org/r/3734/diff/2/?file=62650#file62650line442> > > > > I think just assert for !format. No, this safety check is useful. It prevents someone from creating a named empty format and attempting to cache it. Making an API robust - particularly when those checks do not sit in a critical path - is not a bad thing :-) - Matt ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3734/#review12535 ----------------------------------------------------------- On July 9, 2014, 7:37 p.m., Matt Jordan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3734/ > ----------------------------------------------------------- > > (Updated July 9, 2014, 7:37 p.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > This patch does two things: > > * It updates a few of the unit tests for some of the API changes. In > particular, it focuses on adding some tests for formats with attributes and > their expected behaviour. A few other non-format related unit tests were > updated as well to handle off nominals detected during testing. > > * It adds an 'ast_format_none' format. This format is a dummy format that can > be used instead of a NULL pointer to prevent having to put NULL dereference > checks into every place in the codebase. Channels are no assigned this format > immediately upon creation, and their default capabilities are set to have it. > As this format's codec has no translation (nor a representation in the RTP > engine), it _shouldn't_ cause harm. > > * A few NULL checks were put in anyway into key areas in a few modules. These > were ones that were hit hard by the unit tests and prone to crashing if > presented a NULL format. > > > Diffs > ----- > > /team/group/media_formats-reviewed-trunk/tests/test_format_cap.c 418259 > /team/group/media_formats-reviewed-trunk/tests/test_format_cache.c 418259 > /team/group/media_formats-reviewed-trunk/tests/test_core_format.c 418259 > /team/group/media_formats-reviewed-trunk/tests/test_cel.c 418259 > /team/group/media_formats-reviewed-trunk/main/format_cap.c 418259 > /team/group/media_formats-reviewed-trunk/main/format_cache.c 418259 > /team/group/media_formats-reviewed-trunk/main/codec_builtin.c 418259 > /team/group/media_formats-reviewed-trunk/main/codec.c 418259 > /team/group/media_formats-reviewed-trunk/main/channel.c 418259 > /team/group/media_formats-reviewed-trunk/main/bridge_channel.c 418259 > /team/group/media_formats-reviewed-trunk/include/asterisk/format_cache.h > 418259 > /team/group/media_formats-reviewed-trunk/include/asterisk/codec.h 418259 > > Diff: https://reviewboard.asterisk.org/r/3734/diff/ > > > Testing > ------- > > Unit tests pass. > > There is a FRACK on shutdown, but it doesn't appear to be caused by this > patch (things didn't run long enough without this patch before) > > > Thanks, > > Matt Jordan > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
