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

> 127 - if (message->req != RIL_UNSOL_DATA_CALL_LIST_CHANGED) {
> 128 - ofono_error("ril_gprs_update_calls: invalid message received %d",
> 129 - message->req);
> 130 - return;
> 131 - }
>
> Why did you remove the above? g_ril_unsol_parse_data_call_list is also not
> checking if the message is indeed UNSOL_DATA_CALL_LIST_CHANGED.

I removed it because it wasn't needed. If you look at the code in gril.c that handles unsolicited messages, it's not really possible for the wrong message to be passed to a registered listener, as that's the whole purpose of registering for a particular message.

> 1017 + g_free(reply);
> 1611 + g_free(call);
> 1621 + g_free(unsol);
>
> Not an issue, but as you're checking for call and unsol just before free, you
> could include the g_free call as part of the if scope.

Done.

> 901 +#define OFONO_EINVAL(error) do { \
> 902 + error->type = OFONO_ERROR_TYPE_FAILURE; \
> 903 + error->error = -EINVAL; \
> 904 +} while (0)
> 905 +
> 906 +#define OFONO_NO_ERROR(error) do { \
> 907 + error->type = OFONO_ERROR_TYPE_NO_ERROR; \
> 908 + error->error = 0; \
> 909 +} while (0)
> 910 +
>
> These would be better if defined somewhere in a common ofono code, not just
> for ril.

Done, moved these to <ofono/type.h>.
>
> 2018 +static const struct ril_msg reply_data_call_invalid_2 = {
> 2019 + .buf = (gchar *) &reply_data_call_invalid_parcel1,
>
> 2052 +static const struct ril_msg reply_data_call_invalid_3 = {
> 2053 + .buf = (gchar *) &reply_data_call_invalid_parcel2,
>
> 2084 +static const struct ril_msg reply_data_call_invalid_4 = {
> 2085 + .buf = (gchar *) &reply_data_call_invalid_parcel3,
>
> 2115 +static const struct ril_msg reply_data_call_invalid_5 = {
> 2116 + .buf = (gchar *) &reply_data_call_invalid_parcel4,
>
> 2147 +static const struct ril_msg reply_data_call_invalid_6 = {
> 2148 + .buf = (gchar *) &reply_data_call_invalid_parcel5,
>
> 2176 +static const struct ril_msg reply_data_call_invalid_7 = {
> 2177 + .buf = (gchar *) &reply_data_call_invalid_parcel6,
>
> Would you mind changing the test id to match the same number used by both
> structs? Otherwise it gets quite confusing when reading the code, as
> data_call_invalid_2 is part of invalid_parcel_1, instead of invalid_parcel_2
> (which is a natural reading by our brain).

Yea, I thought about this an since the first test case didn't actually have parcel_data, I thought it would be weird having the first parcel data variable start with "2". Oh well, not a big deal. Re-number per your suggestion.

> Other than those minor comments, code looks good (although it could
> technically be separated in more MRs, as we discussed).
>
> Will now test in both mako and maguro.