Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sip_inv: Additional multipart support (#2919) #2920

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

gtjoseph
Copy link
Contributor

FYI... Although not strictly needed, I created both pjsip_tdata_get_sdp_info and pjsip_tdata_get_sdp_info2 to mirror the existing pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2 functions. I can eliminate pjsip_tdata_get_sdp_info2 if that's better. Also double check code formatting. I lost my "pjproject" Eclipse code formatter and had to recreate it from scratch. :)


sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists. So...

  • The existing pjsip_rdata_get_sdp_info2 was refactored to
    find the sdp in any body, multipart or non-multipart, and
    from either an rdata or tdata. The new function is
    pjsip_get_sdp_info. This new function detects whether the
    pjsip_msg->body->data is the text representation of the sdp
    from an rdata or an existing pjmedia_sdp_session object
    from a tdata, or whether pjsip_msg->body is a multipart
    body containing either of the two sdp formats.

  • The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
    functions are now wrappers that get the body and Content-Type
    header from the rdata and call pjsip_get_sdp_info.

  • Two new wrappers named pjsip_tdata_get_sdp_info and
    pjsip_tdata_get_sdp_info2 have been created that get the body
    from the tdata and call pjsip_get_sdp_info.

  • For testing purposes, a new field "create_multipart" was added
    to the pjsip_inv_session structure to allow forcing all bodies
    created to be multipart.

  • The internal create_sdp_body function now takes an option to
    indicate that a multipart body should be created.

  • inv_offer_answer_test.c was updated to use the create_multipart
    field and additional tests were added.

Fixes #2919

pj_bool_t create_multipart; /**< Create multipart
bodies. Should be
used for testing
only. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this setting because:

  • Potential behavioral change affecting app or upper level API. For example, currently pjsua seems to always assume that Invite module will always create non-multipart body, see how this pjsua_process_msg_data() implementation creates multipart body, it does not check whether the existing body, which is generated by Invite module, is a multipart (as specified here, nested multipart is not recommended). Some apps may assume the same.
  • Even if the pjsip_inv_session structure is public (as opposed to transparent), as far as I know, modifying/writing the field directly should be avoided, it should be done via a function (or an option/flag), which does not seem to be provided here.

Also, as currently this setting seems to be used by a test app only, so perhaps better to handle multipart body generation in the test app if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. That flag really was there for internal testing only as the comment states. I wanted the existing unit tests in inv_offer_answer_test to be able to exercise the multipart logic in sip_inv. Having the test do the multipart handling would defeat the purpose. I can just remove the extra tests and the field if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the app should generate the multipart body (instead of handling the whole multipart things) and leave sip_inv to generate non-multipart body as is (so the new field create_multipart can be avoided), something like this: 2920_diff.txt.
.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha. Let me try that and see if it still exercises the code paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I removed the create_multipart option and applied the diff to inv_offer_answer_test. Seems to work fine. Anything else I need to do?

Copy link
Member

@nanangizz nanangizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me. Thanks!

} pjsip_sdp_info;

/**
* For backwards compatibility and completemess,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completeness

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completemess

That was a Freudian slip :)

*
* @return PJ_SUCCESS on success.
*/
PJ_DEF(pj_status_t) pjsip_create_multipart_sdp_body( pj_pool_t *pool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PJ_DECL. Also below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. Cut and paste error.

sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists.  So...

* The existing pjsip_rdata_get_sdp_info2 was refactored to
  find the sdp in any body, multipart or non-multipart, and
  from either an rdata or tdata.  The new function is
  pjsip_get_sdp_info.  This new function detects whether the
  pjsip_msg->body->data is the text representation of the sdp
  from an rdata or an existing pjmedia_sdp_session object
  from a tdata, or whether pjsip_msg->body is a multipart
  body containing either of the two sdp formats.

* The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
  functions are now wrappers that get the body and Content-Type
  header from the rdata and call pjsip_get_sdp_info.

* Two new wrappers named pjsip_tdata_get_sdp_info and
  pjsip_tdata_get_sdp_info2 have been created that get the body
  from the tdata and call pjsip_get_sdp_info.

* inv_offer_answer_test.c was updated to test multipart scenarios.

Fixes pjsip#2919
@nanangizz nanangizz merged commit 69a89a2 into pjsip:master Dec 22, 2021
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request Dec 22, 2021
Adding upstream patch for pull request...
pjsip/pjproject#2920
---------------------------------------------------------------

sip_inv:  Additional multipart support (#2919)

sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists.  So...

* The existing pjsip_rdata_get_sdp_info2 was refactored to
  find the sdp in any body, multipart or non-multipart, and
  from either an rdata or tdata.  The new function is
  pjsip_get_sdp_info.  This new function detects whether the
  pjsip_msg->body->data is the text representation of the sdp
  from an rdata or an existing pjmedia_sdp_session object
  from a tdata, or whether pjsip_msg->body is a multipart
  body containing either of the two sdp formats.

* The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
  functions are now wrappers that get the body and Content-Type
  header from the rdata and call pjsip_get_sdp_info.

* Two new wrappers named pjsip_tdata_get_sdp_info and
  pjsip_tdata_get_sdp_info2 have been created that get the body
  from the tdata and call pjsip_get_sdp_info.

* inv_offer_answer_test.c was updated to test multipart scenarios.

ASTERISK-29804

Change-Id: I483c7c3d413280c9e247a96ad581278347f9c71b
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request Dec 27, 2021
Adding upstream patch for pull request...
pjsip/pjproject#2920
---------------------------------------------------------------

sip_inv:  Additional multipart support (#2919)

sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists.  So...

* The existing pjsip_rdata_get_sdp_info2 was refactored to
  find the sdp in any body, multipart or non-multipart, and
  from either an rdata or tdata.  The new function is
  pjsip_get_sdp_info.  This new function detects whether the
  pjsip_msg->body->data is the text representation of the sdp
  from an rdata or an existing pjmedia_sdp_session object
  from a tdata, or whether pjsip_msg->body is a multipart
  body containing either of the two sdp formats.

* The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
  functions are now wrappers that get the body and Content-Type
  header from the rdata and call pjsip_get_sdp_info.

* Two new wrappers named pjsip_tdata_get_sdp_info and
  pjsip_tdata_get_sdp_info2 have been created that get the body
  from the tdata and call pjsip_get_sdp_info.

* inv_offer_answer_test.c was updated to test multipart scenarios.

ASTERISK-29804

Change-Id: I483c7c3d413280c9e247a96ad581278347f9c71b
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request Dec 27, 2021
Adding upstream patch for pull request...
pjsip/pjproject#2920
---------------------------------------------------------------

sip_inv:  Additional multipart support (#2919)

sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists.  So...

* The existing pjsip_rdata_get_sdp_info2 was refactored to
  find the sdp in any body, multipart or non-multipart, and
  from either an rdata or tdata.  The new function is
  pjsip_get_sdp_info.  This new function detects whether the
  pjsip_msg->body->data is the text representation of the sdp
  from an rdata or an existing pjmedia_sdp_session object
  from a tdata, or whether pjsip_msg->body is a multipart
  body containing either of the two sdp formats.

* The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
  functions are now wrappers that get the body and Content-Type
  header from the rdata and call pjsip_get_sdp_info.

* Two new wrappers named pjsip_tdata_get_sdp_info and
  pjsip_tdata_get_sdp_info2 have been created that get the body
  from the tdata and call pjsip_get_sdp_info.

* inv_offer_answer_test.c was updated to test multipart scenarios.

ASTERISK-29804

Change-Id: I483c7c3d413280c9e247a96ad581278347f9c71b
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request Dec 27, 2021
Adding upstream patch for pull request...
pjsip/pjproject#2920
---------------------------------------------------------------

sip_inv:  Additional multipart support (#2919)

sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists.  So...

* The existing pjsip_rdata_get_sdp_info2 was refactored to
  find the sdp in any body, multipart or non-multipart, and
  from either an rdata or tdata.  The new function is
  pjsip_get_sdp_info.  This new function detects whether the
  pjsip_msg->body->data is the text representation of the sdp
  from an rdata or an existing pjmedia_sdp_session object
  from a tdata, or whether pjsip_msg->body is a multipart
  body containing either of the two sdp formats.

* The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
  functions are now wrappers that get the body and Content-Type
  header from the rdata and call pjsip_get_sdp_info.

* Two new wrappers named pjsip_tdata_get_sdp_info and
  pjsip_tdata_get_sdp_info2 have been created that get the body
  from the tdata and call pjsip_get_sdp_info.

* inv_offer_answer_test.c was updated to test multipart scenarios.

ASTERISK-29804

Change-Id: I483c7c3d413280c9e247a96ad581278347f9c71b
@gtjoseph gtjoseph deleted the master-more-multipart branch January 13, 2022 13:16
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request Jul 13, 2022
Adding upstream patch for pull request...
pjsip/pjproject#2920
---------------------------------------------------------------

sip_inv:  Additional multipart support (#2919)

sip_inv.c:inv_check_sdp_in_incoming_msg() deals with multipart
message bodies in rdata correctly. In the case where early media is
involved though, the existing sdp has to be retrieved from the last
tdata sent in this transaction. This, however, always assumes that
the sdp sent is in a non-multipart body. While there's a function
to retrieve the sdp from multipart and non-multpart rdata bodies,
no similar function for tdata exists.  So...

* The existing pjsip_rdata_get_sdp_info2 was refactored to
  find the sdp in any body, multipart or non-multipart, and
  from either an rdata or tdata.  The new function is
  pjsip_get_sdp_info.  This new function detects whether the
  pjsip_msg->body->data is the text representation of the sdp
  from an rdata or an existing pjmedia_sdp_session object
  from a tdata, or whether pjsip_msg->body is a multipart
  body containing either of the two sdp formats.

* The exsting pjsip_rdata_get_sdp_info and pjsip_rdata_get_sdp_info2
  functions are now wrappers that get the body and Content-Type
  header from the rdata and call pjsip_get_sdp_info.

* Two new wrappers named pjsip_tdata_get_sdp_info and
  pjsip_tdata_get_sdp_info2 have been created that get the body
  from the tdata and call pjsip_get_sdp_info.

* inv_offer_answer_test.c was updated to test multipart scenarios.

ASTERISK-29804

Change-Id: I483c7c3d413280c9e247a96ad581278347f9c71b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sip_inv:inv_check_sdp_in_incoming_msg doesn't handle multipart bodies in tx_data
4 participants