Merge lp://qastaging/~awe/phablet-extras/ofono-unittest-gprs-context into lp://qastaging/phablet-extras/ofono

FAILED: Continuous integration, rev:53
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/51/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/26/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/51/rebuild

Build failure is due to the fact that I didn't add the unit test source file... I'm working on restoring it, and should have this branch updated by end-of-day.

PASSED: Continuous integration, rev:56
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/56/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/31
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/31/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/56/rebuild

I would split:
823 +static void test_invalid_setup_data_calls(void)
834 + /* Test invalid radio tech values */
847 + /* Test invalid data profile */
855 + /* Invalid APNs */
...
and so on for the other test functions into individual test cases.

Also consider the posibility of a test fixture for each of these test functions(at your discretion)
https://developer.gnome.org/glib/2.28/glib-Testing.html#g-test-add

Is it a good idea to make a free_reply (or with similar name) that would take care of freeing the complete struct and then just call that?
238 + g_strfreev(reply.dns_addresses);
239 + g_strfreev(reply.ip_addrs);
240 + g_strfreev(reply.gateways);
249 + g_free(reply.ifname);

@Sergio

Thanks for the comments, and I agree that splitting the tests out probably makes sense.

Also per our discussion yesterday, I will split grilmessages.c into three separate files: grilrequests.c, grilreplies.c, and grilevents.c.

Finally, I also mentioned that I only added unit tests for the SETUP_DATA_CALL request. I will go ahead and add unit tests for the corresponding reply as well.

I should be able to wrap up all these changes today.

On Fri, Jul 12, 2013 at 12:15 PM, Tony Espy <email address hidden> wrote:
> @Sergio
>
> Thanks for the comments, and I agree that splitting the tests out probably makes sense.
>
> Also per our discussion yesterday, I will split grilmessages.c into three separate files: grilrequests.c, grilreplies.c, and grilevents.c.
>
> Finally, I also mentioned that I only added unit tests for the SETUP_DATA_CALL request. I will go ahead and add unit tests for the corresponding reply as well.
>
> I should be able to wrap up all these changes today.

Sounds good to me :-)

FAILED: Continuous integration, rev:60
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/58/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/33/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/58/rebuild

FAILED: Continuous integration, rev:61
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/59/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/34/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/59/rebuild

Two last things need to be fixed.

First, the CI build fails due to a unit test failure. This is the test I mentioned earlier that was also failing in pbuild.

ERROR:unit/test-grilrequest.c:200:test_deactivate_data_call_valid: assertion failed: (!memcmp(rilp.data, test_data->parcel_data, test_data->parcel_size)\

Test test tries to compare wire-format parcel data ( stored in rilp.data ) to a static guchar of bytes that represent what the parcel wire format should look like.

The second issue is how the gprs-context code handles setup_data_call failures. I'm not sure if a failure callback triggers a disconnect from the core ofono code or not. If it does, then the code should be fine as is. If it doesn't, then this could potentially leave an active data call within RIL, and thus we should explicitly disconnect in this case.

OK, so it appears the failure callback doesn't trigger a disconnect, so I'll modify the code to do that if the SETUP_DATA_CALL reply triggers a message validation error.

I'll plan on landing the final bits over the weekend...

FAILED: Continuous integration, rev:62
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/60/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/35/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/60/rebuild

OK, both problems are now fixed.

Data calls are properly disconnected if a reply is received from RIL that is deemed invalid by the gril code.

The unit test failure was due to a subtle bug in parcel_w_string() which could result in invalid string terminator bytes to be included due to an incorrect offset used to write the terminating utf16 0x00000.

PASSED: Continuous integration, rev:63
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/61/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/36
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/36/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/61/rebuild

PASSED: Continuous integration, rev:64
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/62/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/37
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/37/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/62/rebuild

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.

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.

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.

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).

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.

Tested with both mako and maguro and it seems to be all good :-)

Just minor issue I got with my maguro is that the modem can't attach anymore it seems, but don't think it's related with this MR (might be that intermittent issue we had this week).

Will test a bit more with maguro...

Would be nice if you could double check with it as well.

Yeah, can't get my mako to attach anymore, with/without NM, and even after a clean install.

We might be having another issue somewhere in this code, which is not related with this MR.

Argh, *maguro*.

Ok, the issue with maguro is that it cannot attach at first, but it works after restarting ofono =\

First run, not attached:
http://paste.ubuntu.com/5917341/

Second run, attached:
http://paste.ubuntu.com/5917346/

Wonder if this is a side effect of that roaming bug you fixed (will follow up this in a bug).

Question, were you testing with just this MR, or the combined MRs? As we discussed, the attach fix was split between the two MRs due to the heavy re-factoring of gprs-context.c in this MR.

I will look at your comments above, which all seem reasonable and try and update the MR later today.

I will also do some more testing to see if I can reproduce any of the issues you ran into while testing.

Note, my fix assumes that NetworkManager always sets the ConnectionManager's 'Powered' on start-up. Perhaps there's a race condition and this isn't always happening?

Just with this MR, and without NetworkManager involved. Was just tracking the ofono state after it was properly initialized.

OK, I'll plan on giving both a shake-down tomorrow after I clean up the couple of things based on your previous comment.

This code really shouldn't have effected whether GPRS attaches or not, as it only touches the gprs-context driver.

> 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.

PASSED: Continuous integration, rev:67
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/65/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/40
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/40/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/65/rebuild

Good, thanks!