[Asterisk-code-review] RFC sdp: Initial SDP creation (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change. ( https://gerrit.asterisk.org/5119 )

Change subject: RFC sdp: Initial SDP creation
......................................................................


Patch Set 3: Code-Review-1

(13 comments)

https://gerrit.asterisk.org/#/c/5119/3/include/asterisk/sdp_options.h
File include/asterisk/sdp_options.h:

Line 66: struct ast_sdp_options {
I'm not 100% sure how I feel about the options structure no longer being opaque. My previous thought had been that within the SDP code, ast_sdp_options would be the most likely structure to change between releases. Keeping the structure opaque meant that we could feel more free to change the structure without worrying as much about ABI breakage. However, given the increased size of the structure, I can see the appeal of not having to have quite so many getters/setters.


Line 120: 
If we move forward with making ast_sdp_options non-opaque (transparent?), then everything from this line down can just be removed since you can just set and get the fields of the structure directly.


https://gerrit.asterisk.org/#/c/5119/3/include/asterisk/sdp_state.h
File include/asterisk/sdp_state.h:

PS3, Line 58: /*!
            :  * \brief Get the local topology
            :  *
            :  */
            : struct ast_stream_topology *ast_sdp_state_get_local_topology(struct ast_sdp_state *sdp_state);
            : 
            : /*!
            :  * \brief Get the sdp_state options
            :  *
            :  */
            : struct ast_sdp_options *ast_sdp_state_get_options(struct ast_sdp_state *sdp_state);
I think both of these functions should return const structures. This way, it's clear that users should only expect to read information from the returned structures and not try to modify them.


https://gerrit.asterisk.org/#/c/5119/3/include/asterisk/stream.h
File include/asterisk/stream.h:

PS3, Line 206: /*!
             :  * \brief Get the opaque stream data
             :  *
             :  * \param stream The media stream
             :  *
             :  * \retval non-NULL success
             :  * \retval NULL failure
             :  *
             :  * \since 15
             :  */
             : void *ast_stream_get_data(struct ast_stream *stream);
             : 
             : /*!
             :  * \brief Set the opaque stream data
             :  *
             :  * \param stream The media stream
             :  * \param data Opaque data
             :  * \param data_free_fn Callback to free data when stream is freed. May be NULL for no action.
             :  *
             :  * \return data
             :  *
             :  * \since 15
             :  */
             : void *ast_stream_set_data(struct ast_stream *stream, void *data,
             : 	ast_stream_data_free_fn data_free_fn);
Here's something to chew on. Do we think that there will ever be multiple parties interested in storing their own private data on a stream? For instance, we know that the SDP code is going to store  the RTP instance on the stream. But is it possible that a bridge might want to store some sort of bridge-related meta information on a stream as well? If so, should the facility be present to allow for multiple private datas to exist? If so, the datastore approach may be a reasonable one.


https://gerrit.asterisk.org/#/c/5119/3/main/sdp.c
File main/sdp.c:

I'm not going to point out every single place this applies, but consider const-ifying a lot of the parameters being passed to functions where it's possible to do so.


Line 36: 	ast_debug(5, "free\n");
All of these debug lines should either be removed or expanded on so they're more clear about what's being freed. Personally, I'd prefer total removal.


PS3, Line 58: void ast_sdp_payload_free(struct ast_sdp_payload *payload)
            : {
            : 	ast_debug(5, "free\n");
            : 	return;
            : }
This function should be calling

    ast_free(payload)


Line 474: 	char rtmap[64];
The rtmap buffer isn't used for anything in this function. Its size is taken in an snprintf() call, but in that snprintf() call, you're actually writing to the tmp buffer.


PS3, Line 612: 		if (ast_sdp_m_add_format(m_line, options, rtp_code, 0, format, 0)) {
             : 			ast_sdp_m_free(m_line);
             : 			return -1;
             : 		}
This off-nominal leaks the "format" variable.


Line 726: 	if (!o_line) {
This should be

    if (!c_line)


PS3, Line 721: o_line = ast_sdp_o_alloc("user", 99999, 0, "IN4", "127.0.0.1");
             : 	if (!o_line) {
             : 		goto error;
             : 	}
             : 	c_line = ast_sdp_c_alloc("IN4", "127.0.0.1");
             : 	if (!o_line) {
             : 		goto error;
             : 	}
Since you added the media_address SDP option, we should be able to fill in an actual media address for the o and c lines instead of using the filler "IN4" and "127.0.0.1" values.


Line 731: 	if (!o_line) {
This should be

    if (!s_line)


Line 734: 	t_line = ast_sdp_t_alloc(999, 9999);
There's not a good reason to be allocating t-lines for SDPs we generate. If we later add some sort of SDP option for timing, we can allocate a t-line in that circumstance. If you're wondering why there's even a t-line structure on an SDP, it's mainly for the case where we receive a remote SDP with a t-line.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefbd877af7f5a4d3c74deead1bff8802661b0d48
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes