[Asterisk-code-review] json: Take advantage of new API's. (asterisk[16])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/10194 )

Change subject: json: Take advantage of new API's.
......................................................................


Patch Set 2:

(2 comments)

https://gerrit.asterisk.org/#/c/10194/1/main/json.c
File main/json.c:

https://gerrit.asterisk.org/#/c/10194/1/main/json.c@619
PS1, Line 619: 	return ast_json_pack("{s: o, s: o, s: o}",
> It looks like this change causes a non-NULL return, apparently "s?" causes invalid UTF to become NUL […]
Neither of the patches to jansson fix the bug I was pointing out.  The error struct is set with the NULL pointer error message regardless of if it is allowed or not.  Thus there cannot be any further errors reported.

"{s:s?}"

pack_string() determines if the NULL is allowed by checking for the '?' in the format.  It then calls read_string() but doesn't tell that function that a NULL string is acceptable.  read_string() calls set_error() for the NULL string.  Now no other errors will be set by set_error() for the pack operation.

After looking at read_string() again, it can determine itself if the NULL is optional by checking if the t variable is '?' before calling set_error().

As far as the invalid UTF-8 error on optional strings is concerned, read_string() is not setting the s->has_error = 1 flag which would cause the pack to fail and we would report the error.


https://gerrit.asterisk.org/#/c/10194/1/main/json.c@750
PS1, Line 750: 		"number", json_party_number(&party->number),
             : 		"name", json_party_name(&party->name),
             : 		"subaddress", json_party_subaddress(&party->subaddress));
> json_party_number / json_party_name / json_party_address all return NULL if !valid so I think this i […]
Yes, you are correct.  Not sure how I missed that.



-- 
To view, visit https://gerrit.asterisk.org/10194
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-MessageType: comment
Gerrit-Change-Id: I8382d28d7d83ee0ce13334e51ae45dbc0bdaef48
Gerrit-Change-Number: 10194
Gerrit-PatchSet: 2
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 21 Sep 2018 20:48:32 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180921/64d46d29/attachment.html>