osmo-mgw[master]: client: add unified function to generate MGCP messages

[Neels Hofmeyr]

Patch Set 5: Code-Review-1

(12 comments)

-1 for mem leaks and missing rc eval. The rest is up to you to reflect on.

https://gerrit.osmocom.org/#/c/4146/5//COMMIT_MSG
Commit Message:

Line 22: Change-Id I15e1af68616309555d0ed9ac5da027c9833d42e3
(I usually do it as

  Depends: libosmocore I12345678abcdef

)


https://gerrit.osmocom.org/#/c/4146/5/include/osmocom/mgcp_client/mgcp_client.h
File include/osmocom/mgcp_client/mgcp_client.h:

Line 61: 	/* See MGCP_MSG_PRESENCE_* constands above */
'constants'

(and maybe drop the 'above', just in case we were to move those defines around in some distant future)


Line 67:         char *audio_ip;
reading this, there is a potential ownership problem here. Or is there?

Given a struct mgcp_msg is passed around several functions, the char *audio_ip must be owned by the struct mgcp_msg. Ok, we can do it by a talloc that uses the struct mgcp_msg as talloc ctx.

Above char endpoint[] solves it by providing a fixed buffer. Here we could do the same as

  char audio_ip[INET_ADDRSTRLEN];

This would cause more memcpy()s but less talloc()s.

Please clarify what is intended...


Line 97: OSMO_DEPRECATED("Use mgcp_msg_gen() instead")
all instances of OSMO_DEPRECATED() I can find in current code follow after the function definition, i.e. between ')' and ';'. Am I missing something?

Ah, there is one like this libosmocore/gsm/abis_nm.c. If it works above the function, I find it more readable actually; so I would favor above, unless others disagree and prefer staying consistent with the other code style we have.


https://gerrit.osmocom.org/#/c/4146/5/src/libosmo-mgcp-client/mgcp_client.c
File src/libosmo-mgcp-client/mgcp_client.c:

Line 662: 		     "Invalid command verb, can not generate MGCP message");
leaking msgb alloc'd above


Line 669: 		     "One or more missing mandatory fields, can not generate MGCP message");
leaking msgb


Line 678: 	msgb_printf(msg, " MGCP 1.0\r\n");
evaluate return code


Line 708: 	
whitespace


Line 709: 	if (rc != 0) {
so IIUC msgb_printf() returns 0 on success, and you first go through all composition steps; if one of them failed anywhere, adding up the rc brings a nonzero here and we can return an error.

I would have expected a

  if (rc)
    goto out_err;

after each msgb_printf() step ... but now that I think of it, it's probably fine to do it this way. After all we don't need to optimize for a msgb-too-small case.


https://gerrit.osmocom.org/#/c/4146/5/tests/mgcp_client/mgcp_client_test.c
File tests/mgcp_client/mgcp_client_test.c:

Line 164: 	strcpy(ip_buf, "192.168.100.23");
ok, it's safe.
(for cosmetics I'd prefer if we used osmo_strlcpy() always and everywhere. as you see fit.)

btw, it's also possible to init with a string constant above like

  char *ip_buf[INET_ADDRSTRLEN] = "yada yada";

and BTW furthermore, for a char* you can just assign a string constant in the first place:

  mgcp_msg.audio_ip = "yada yada";

and furthermore, why not:

  struct mgcp_msg mgcp_msg = {
    .audio_ip = "my string constant",
    .endpoint = "23 at mgw",
    ...
  };

Even if audio_ip is changed to a char[INET_ADDRSTRLEN] this still works, but only during this struct init.

BTW ... also possible anywhere in code flow:

  mgcp_msg = (struct mgcp_msg){
    .audio_ip = "constant",
    .endpoint = "constant",
  };

i.e. provide an inline struct initialization to assign to the struct.

enough lecturing :)


Line 180: 	printf("%s\n", msg->data);
(wondering if this should have a cast to (char*) ... does it cause warnings?)


Line 210: 
(still: a test that hits buffer size limits?)


-- 
To view, visit https://gerrit.osmocom.org/4146
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I29c5e2fb972896faeb771ba040f015592487fcbe
Gerrit-PatchSet: 5
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes