Code review comment for lp:~awe/phablet-extras/ofono-gprs-support

91 +static char printBuf[PRINTBUF_SIZE];

574 +static char printBuf[PRINTBUF_SIZE];

910 +static char printBuf[PRINTBUF_SIZE];

2390 +static char printBuf[PRINTBUF_SIZE];

Do we really need a global string? Would avoid calling clear.

1467 + * $AOSP/hardware/ril/libril/ril.cpp
1468 + */
1469 +#define startRequest sprintf(printBuf, "(")
1470 +#define closeRequest sprintf(printBuf, "%s)", printBuf)
1471 +#define printRequest(token, req) \
1472 + ofono_debug("[%04d]> %s %s", token, ril_request_id_to_string(req), printBuf)
1473 +
1474 +#define startResponse sprintf(printBuf, "%s {", printBuf)
1475 +#define closeResponse sprintf(printBuf, "%s}", printBuf)
1476 +#define printResponse ofono_debug("%s", printBuf)
1477 +
1478 +#define clearPrintBuf printBuf[0] = 0
1479 +#define removeLastChar printBuf[strlen(printBuf)-1] = 0
1480 +#define appendPrintBuf(x...) sprintf(printBuf, x)

91 +static char printBuf[PRINTBUF_SIZE];

Coding style here might be an issue when pushing this code upstream. I'd guess it'd be better to use print_buf instead.

114 + if (message->buf_len < 36) {
115 + DBG("Parcel is less then minimum DataCallResponseV6 size!");

Missing "decode_ril_error(&error, "FAIL");".

126 + /* TODO: make conditional */
127 + appendPrintBuf("[%04d]< %s",
128 + message->serial_no,
129 + ril_request_id_to_string(message->req));
130 + startResponse;
131 + /* TODO: make conditional */

Mind moving this code a bit bellow so it can be near the rest of the append/close/print logic? Otherwise it's hard to track which message it's actually covering (in case of another message setup later).

135 + /* RIL version */
136 + version = parcel_r_int32(&rilp);
137 +
138 + /* Size of call list */
139 + /* TODO: What if there's more than 1 call in the list?? */
140 + num = parcel_r_int32(&rilp);

Isn't the reply just a RIL_Data_Call_Response_v6 struct? If so, mind adding a comment saying that the version and num arguments were not described correctly by the Android ril.h file?

175 + if (status != 0) {
176 + DBG("Reply failure; status %d", status);
177 + gcd->state = STATE_IDLE;
178 + goto error;
179 + }

203 + if (ip_addrs[0] == NULL) {
204 + DBG("Invalid IP address field returned: %s", raw_ip_addrs);
205 + goto error;
206 + }

217 + if (split_ip_addr[0] == NULL) {
218 + ofono_error("Invalid IP address; can't strip prefix: %s",
219 + ip_addrs[0]);
220 + goto error;
221 + }

230 + if (gateways[0] == NULL) {
231 + DBG("Invalid gateways field returned: %s", raw_gws);
232 + goto error;
233 + }

Don't you also need to update error.error here?

Also, in case of error, we're not releasing any of the strings allocated with parcel_r_string (your code just free them in case it all works).

266 + guchar profile[5] = { 0x00 };

Not used.

329 + DBG("Invalid protocol");

Mind doing a better error message here? Otherwise it will be hard to get from the ofono log.

334 + appendPrintBuf("%s %s,%s,%s,%s,%s,%s,%s",
335 + printBuf,
336 + tech,
337 + DATA_PROFILE_DEFAULT,
338 + ctx->apn,
339 + ctx->username,
340 + ctx->password,
341 + CHAP_PAP_OK,
342 + protocol);
343 +
344 + ret = g_ril_send(gcd->ril,
345 + request,
346 + rilp.data,
347 + rilp.size,
348 + ril_setup_data_call_cb, cbd, g_free);
349 +
350 + /* TODO: make conditional */
351 + closeRequest;
352 + printRequest(ret, request);
353 + /* TODO: make conditional */

Mind containing all the printbuf related function calls together? Easier to read and understand.

375 + appendPrintBuf("[%04d]< %s",
376 + message->serial_no,
377 + ril_request_id_to_string(message->req));

Why just the append without any clear, response or print?

720 + if (cb)
721 + cb(&error, status, cbd->data);
722 + return;
723 +
724 +error:
725 + if (cb)
726 + cb(&error, -1, cbd->data);

Could probably simplify by setting up status in case of an error.