JSON initial codec state command and response by Roog · Pull Request #973 · baresip/baresip

Roog · 2020-05-04T11:11:19Z

Hi!
Thanks for answering the 'issue' #959 . "MQTT / TCP on connection client state request".

As mentioned before in the "issue" the idea here is to have a command with a JSON-formatted response. The command could be used when you are starting an api connection to Baresip, via TCP or MQTT. Similair like how '/uastat' and '/callstat' is used today, so an external application could get up and running without a lot of parsing of those commands. The new 'response' could also be the default response on registration and call events...

I should start with an apology, I don't have a deep knowledge of C-programming. So the pull-request is a very early one, maybe @alfredh you can put me in the right direction. Please let me know if I should look up something before you give a proper response, I don't want to bother you with too trivial things..

How memory is working and how the code i wrote is effecting that, I couldn't tell you..
I'm probably not using 'pointers' and references in the correct way..
I implemented a command in the debug_cmd, and I hope the placement of the other functions is ok/correct, maybe there is a better place or even in some custom module?
- menu '/apistate' command added in debug_cmd.c
- cmd_api_uastate added in devug_cmd.c
- ua_state_json_api added in ua.c
- account_json_api added in account.c
- reg_json_api added in reg.c
I will add some comments on specific rows, where I could not get the code to work in the way I'm thinking. There is some things that is not working in the way I think.
This "object" only contains the user-agent, settings and account information. Call information will come later and maybe even in another function / "object". Depending how this goes.

{
    "cuser":"rogersan-0x7f81ded10c30",
    "selected_ua":true,
    "aor":"sip:rogersan@domain.com",
    "display_name":"Roger San",
    "settings":{
        "sip_nat":"outbound",
        "sip_nat_outbound":[
            "sip:primary.example.com",
            "sip:secondary.example.com"
        ],
        "stun_host":"",
        "stun_port":0,
        "stun_user":"rogersan",
        "stun_pass":"***",
        "answer_mode":"manual",
        "call_transfer":true,
        "packet_time":20
    },
    "registration":{
        "interval":3697,
        "q_value":0.000000,
        "id":1,
        "scode":999,
        "af":"v4"
    }
}

Best regards
Roger

- menu command added '/apistate' - cmd_api_uastate added in devug_cmd.c - ua_state_json_api added in ua.c - account_json_api added in account.c - reg_json_api added in reg.c

src/ua.c

Roog · 2020-05-04T11:16:46Z

src/ua.c

+
+	for (le = ua->regl.head; le; le = le->next) {
+		/* TODO: how to get only current ua register state? */
+		err |= reg_json_api(reg, le->data);


This one is going through all the user agents and their registration statuses. I would like to just have the one that is valid for this user-agent.

I realised that this one was listing two registrations for one user-agent / account. Not sure how i managed to get that. But that is what caused some confusion. Adding a check for this, unless it's a feature.

modules/debug_cmd/debug_cmd.c

src/account.c

src/reg.c

src/ua.c

alfredh · 2020-05-04T15:05:32Z

this looks really nice, well done for writing this code!

I have done a code review and added some comments inline.

it is mainly 2 things:

add check for optional strings
free dictionaries after encoding (mem_deref)

if I run the patch locally, here is how it looks:

$ ./baresip -e/apistate -eq

baresip is ready.
/apistate
ua: failed to encode json account (Invalid argument)
ua: failed to encode json registration (Invalid argument)
ua: failed to encode json package (Invalid argument)
{"cuser":"aeh-0x7fbfe8f31e20","selected_ua":true,"aor":"sip:aeh@iptel.org","settings":{"sip_nat_outbound":[],"stun_host":"","stun_port":0,"stun_user":"aeh","stun_pass":"***","answer_mode":"manual","call_transfer":true,"packet_time":20},"registration":{"interval":3761,"q_value":-0.000000,"id":0,"scode":0,"af":"v?"}}debug: failed to encode json (Invalid argument)

Quit
ua: stop all (forced=0)
mem: Memory leaks (55):
  0x7fbfe8f37140: nrefs=1  size=24      [90 75 f3 e8 bf 7f 00 00 50 89 f3 e8 bf 7f 00 00 ] [.u......P.......]
  0x7fbfe8f371a0: nrefs=1  size=16      [f0 71 f3 e8 bf 7f 00 00 08 00 00 00 00 00 00 00 ] [.q..............]
  0x7fbfe8f371f0: nrefs=1  size=128     [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] [................]
  0x7fbfe8f372b0: nrefs=2  size=24      [60 83 f3 e8 bf 7f 00 00 20 87 f3 e8 bf 7f 00 00 ] [`....... .......]
...

alfredh · 2020-05-04T15:06:25Z

there are also some formating warnings that should be fixed:

ccheck.py 
./src/account.c:1262: line is too wide (86 - max 79)
./src/account.c:1271: line is too wide (80 - max 79)
./src/account.c:1274: line is too wide (84 - max 79)
./src/account.c:1280: line is too wide (97 - max 79)
./src/account.c:1285: line is too wide (84 - max 79)
./src/account.c:1287: line is too wide (86 - max 79)
./src/account.c:1288: line is too wide (89 - max 79)
./src/account.c:1292: line is too wide (100 - max 79)
./src/account.c:1295: line is too wide (84 - max 79)
./src/ua.c:1251: line is too wide (82 - max 79)
./src/ua.c:1267: line is too wide (83 - max 79)
./include/baresip.h:62: line is too wide (90 - max 79)
./include/baresip.h:741: line is too wide (84 - max 79)
Statistics:
~~~~~~~~~~~
Number of files processed:     c: 243  cpp: 9  h: 49  mk: 96  m4: 0  py: 0  m: 4  s: 0  java: 0 
Number of lines with errors:   13

- Moved declarations to top of function - Cleaned up code formatting - Null check on variables passed to odict_entry_add - Renamed 'af' under 'registration' to 'ip_version'

- ua_state_json_api did not need the print function

Roog · 2020-05-05T01:27:10Z

Thank you @alfredh !

Your comments, really helped :)

I can not see any more "memory leeks", and I think i placed the mem_deref in the right places and freed up dictionaries.
I think most formatting is solved, hopefully according to the rest of the code.
Most values should be checked for NULL and casted in the correct format.

Suggested next step:

Format and add User-Agents and information to a JSON list, for multiple user-agents.
The 'selected_ua' property is not working, should only be true for the user-agent that is selected in Baresip. Comparing (ua == uag_current()) is not working, not sure what i'm doing wrong. It is always true on each iteration.
Only filter out registration-status for one user-agent at a time, while iterating through list. Seems to exhaust the list on 'round two', guess the list needs to be pointed in some way to the beginning.. Not really sure how to yet.
Is there a unique id for each 'account' added?
What is the 'cuser' exactly? Is it the computer user?

Thank you for taking your time to reply and comment!

Best regards
Roger

src/reg.c

src/ua.c

alfredh · 2020-05-05T07:47:22Z

modules/debug_cmd/debug_cmd.c

+
+		err |= ua_state_json_api(od, ua);
+		/* todo: add to list */
+		err |= json_encode_odict(pf, od);


this logic will not work. it will append the ua object to the dict and then print it.
if you have 3 uas the result will be:

1
1,2
1,2,3

instead you should build an array, and then encode it as the last step

alfredh · 2020-05-09T12:17:09Z

hi @Roog I added some more review comments, do they make sense to you ?

Roog · 2020-05-10T11:26:38Z

Hi @alfredh , they do make sense! I'm just spending a little bit time reading up on some C related things. I wanted to understand arrays and pointers in C better. Hopefully I'll have some code soon :)

alfredh · 2020-05-10T12:07:36Z

great,

regarding the function that encodes the JSON, here is an example:

static int cmd_api_uastate(struct re_printf *pf, void *unused)
{
	struct odict *od = NULL;
	struct le *le;
	int err;
	(void)unused;

	err = odict_alloc(&od, 8);
	if (err)
		return err;

	for (le = list_head(uag_list()); le && !err; le = le->next) {
		const struct ua *ua = le->data;
		struct odict *odua;

		err = odict_alloc(&odua, 8);

		err |= ua_state_json_api(odua, ua);

		err |= odict_entry_add(od, ua_aor(ua), ODICT_OBJECT, odua);

		mem_deref(odua);
	}

	err |= json_encode_odict(pf, od);
	if (err)
		warning("debug: failed to encode json (%m)\n", err);

	mem_deref(od);

	return re_hprintf(pf, "\n");
}

it adds multiple objects to the root object, using the SIP aor as the key.
this is just an idea, you can solve it differently if you want.

the output is something like this:

{
   "sip:aeh@iptel.org":{
      "cuser":"aeh-0x7fe892e49cd0",
      "selected_ua":true,
      "aor":"sip:aeh@iptel.org",
      "settings":{
         "sip_nat_outbound":[

         ],
         "stun_host":"",
         "stun_port":0,
         "stun_user":"aeh",
         "answer_mode":"manual",
         "call_transfer":true,
         "packet_time":20
      },
      "registration":{
         "interval":3759,
         "q_value":0.000000,
         "id":0,
         "scode":0,
         "ip_version":"v?"
      }
   },
   "sip:alfredh@sip.antisip.com":{
      "cuser":"alfredh-0x7fe892e4a700",
      "selected_ua":false,
      "aor":"sip:alfredh@sip.antisip.com",
      "display_name":"Anti Alf",
      "settings":{
         "sip_nat_outbound":[

         ],
         "stun_host":"",
         "stun_port":0,
         "stun_user":"alfredh",
         "answer_mode":"manual",
         "call_transfer":true,
         "packet_time":20
      },
      "registration":{
         "interval":600,
         "q_value":0.000000,
         "id":0,
         "scode":0,
         "ip_version":"v?"
      }
   }
}

I have used an online JSON validator to check the JSON output.

- For the JSON api if there is more than one registration the sip-uri or aor will be the key

- scode -> code (makes sense since it's under registration.code - ip_version -> ipv (less data to send around, wondering if IP-version is needed in the api)

Roog · 2020-05-11T01:59:35Z

Thank you for the suggestion!

I think this makes sense, an alternative would be an array, instead of an object. But with the assumption that the SIP id, is unique, your suggestion is good I think. At least something we could start with.
I managed in some way to get one "user-agent" in my accounts file to be registered twice. So for one "registration" object i got two, so there was an "id:0 and id:1" with the same aor (not listed as multiple lines in the account file) except two registrations. I've added a check for this, assuming that it is not something that should happen? So this might have confused me a bit.
I see that your "cuser" is unique per account, how do you make it so?

If you don't see anything that you'd like to change.
Maybe the next step is to question if the "debug_cmd.c" is the right location for this kind of data, thinking it can be available over TCP / HTTP and MQTT modules?

Best regards
Roger

alfredh · 2020-05-11T08:52:45Z

thanks, I have merged the code to master.

cuser (contact user) is unique per baresip instance. it is used to dispatch
incoming SIP requests to the correct UA.
it is possible to have multiple UAs with the same aor, so the sip aor is not
really unique. we can change the JSON to an array if you want, or use
a numeric index.

you are welcome to make more changes and submit a PR for review.

Many thanks for your contributions :)

(Presumably the changelog [Unreleased] section is for 1.0.0) = Baresip Changelog == [Unreleased] === Added - aac: add AAC_STREAMTYPE_AUDIO enum value - aac: add AAC_ prefix - Video mode param to call_answer(), ua_answer() and ua_hold_answer [#966] - video_stop_display() API function [#977] - module: add path to module_load() function - conf: add conf_configure_buf - test: add usage of g711.so module [#978] - JSON initial codec state command and response [#973] - account_set_video_codecs() API function [#981] - net: add fallback dns nameserver [#996] - gtk: show call_peername in notify title [#1006] - call: Added call_state() API function that returns enum state of the call [#1013] - account_set_stun_user() and account_set_stun_pass() API functions [#1015] - API functions account_stun_uri and account_set_stun_uri. [#1018] - ausine: Audio sine wave input module [#1021] - gtk/menu: replace spaces from uri [#1007] - jack: allowing jack client name to be specified in the config file [#1025] [#1020] - snapshot: Add snapshot_send and snapshot_recv commands [#1029] - webrtc_aec: 'extended_filter' config option [#1030] - avfilter: FFmpeg filter graphs integration [#1038] - reg: view proxy expiry value in reg_status [#1068] - account: add parameter rwait for re-register interval [#1069] - call, stream, menu: add cmd to set the direction of video stream [#1073] === Changed - **Using [baresip/re](https://github.com/baresip/re) fork now** - audio: move calculation to audio_jb_current_value - avformat: clean up docs - gzrtp: update docs - account: increased size of audio codec list to 16 - video: make video_sdp_attr_decode public - config: Derive default audio driver from default audio device [#1009] - jack: modifying info message on jack client creation [#1019] - call: when video stream is disabled, stop also video display [#1023] - dtls_srtp: use tls_set_selfsigned_rsa with keysize 2048 [#1062] [#1056] - rst: use a min ptime of 20ms - aac: change ptime to 4ms === Fixed - avcodec: fix H.264 interop with Firefox - winwave: waveInGetPosition is no longer supported for use as of Windows Vista [#960] - avcodec: call av_hwdevice_ctx_create before if-statement - account: use single quote instead of backtick - ice: fix segfault in connh [#980] - call: Update call->got_offer when re-INVITE or answer to re-INVITE is received [#986] - mk: Test also for /usr/lib64/libspeexdsp.so to cover Fedora/RHEL/CentOS [#992] - config: Allow distribution specific CA trust bundle locations (fixes [#993] - config: Allow distribution specific default audio device (fixes [#994] - mqtt: fix err is never read (found by clang static analyzer) - avcodec: fix err is never read (found by clang static analyzer) - gtk: notification buttons do not work on Systems [#1012] - gtk: fix dtmf_tone and add tones as feedback [#1010] - pulse: drain pulse buffers before freeing [#1016] - jack: jack_play connect all physical ports [#1028] - Makefile: do not try to install modules if build is static [#1031] - gzrtp: media_alloc function is missing [#1034] [#1022] - call: when updating video, check if video stream has been disabled [#1037] - amr: fix length check, fixes [#1011] - modules: fix search path for avdevice.h [#1043] - gtk: declare variables C89 style - config: init newly added member - menu: fix segfault in ua_event_handler [#1059] [#1061] - debug_cmd: fix OpenSSL no-deprecated [#1065] - aac: handle missing bitrate parameter in SDP format - av1: properly configure encoder === Removed - ice: remove support for ICE-lite - ice: remove ice_debug, use log level DEBUG instead - ice: make stun server optional - config: remove ice_debug option (unused) === Contributors (many thanks) - Alfred E. Heggestad - Alexander Gramner - Andrew Webster - Christian Spielberger - Christoph Huber - Davide Alberani - Ethan Funk - Juha Heinanen - mbattista - Michael Malone - Mikl Kurkov - ndilieto - Robert Scheck - Roger Sandholm - Sebastian Reimers [#966]: baresip/baresip#966 [#977]: baresip/baresip#977 [#978]: baresip/baresip#978 [#973]: baresip/baresip#973 [#981]: baresip/baresip#981 [#996]: baresip/baresip#996 [#1006]: baresip/baresip#1006 [#1013]: baresip/baresip#1013 [#1015]: baresip/baresip#1015 [#1018]: baresip/baresip#1018 [#1021]: baresip/baresip#1021 [#1007]: baresip/baresip#1007 [#1025]: baresip/baresip#1025 [#1020]: baresip/baresip#1020 [#1029]: baresip/baresip#1029 [#1030]: baresip/baresip#1030 [#1038]: baresip/baresip#1038 [#1009]: baresip/baresip#1009 [#1019]: baresip/baresip#1019 [#1023]: baresip/baresip#1023 [#1062]: baresip/baresip#1062 [#1056]: baresip/baresip#1056 [#960]: baresip/baresip#960 [#980]: baresip/baresip#980 [#986]: baresip/baresip#986 [#992]: baresip/baresip#992 [#993]: baresip/baresip#993 [#994]: baresip/baresip#994 [#1012]: baresip/baresip#1012 [#1010]: baresip/baresip#1010 [#1016]: baresip/baresip#1016 [#1028]: baresip/baresip#1028 [#1031]: baresip/baresip#1031 [#1034]: baresip/baresip#1034 [#1022]: baresip/baresip#1022 [#1037]: baresip/baresip#1037 [#1011]: baresip/baresip#1011 [#1043]: baresip/baresip#1043 [#1059]: baresip/baresip#1059 [#1061]: baresip/baresip#1061 [#1065]: baresip/baresip#1065 [#1068]: baresip/baresip#1068 [#1069]: baresip/baresip#1069 [#1073]: baresip/baresip#1073 [Unreleased]: baresip/baresip@v0.6.6...HEAD

Roog added 2 commits May 4, 2020 04:08

JSON formatted account & registration debug cmd

64aa92d

- menu command added '/apistate' - cmd_api_uastate added in devug_cmd.c - ua_state_json_api added in ua.c - account_json_api added in account.c - reg_json_api added in reg.c

Rename 'current' to 'selected_ua' in JSON response

247928a

Roog commented May 4, 2020

View reviewed changes

src/ua.c Outdated Show resolved Hide resolved

Roog commented May 4, 2020

View reviewed changes

src/ua.c Outdated Show resolved Hide resolved

Roog commented May 4, 2020

View reviewed changes