GNOME Bugzilla – Bug 702238
oFono backend crashes when adding modems
Last modified: 2018-09-21 16:03:29 UTC
Created attachment 246791 [details] [review] Patch that uses dup_strv to workaround bug in vala compiler When the folks oFono backend gets notified about a new modem, it uses GVariant's get_strv to retrieve features, which results, due to a bug in the Vala compiler, in the generated C code freeing both the container array and its elements, which is wrong, as g_variant_get_strv uses (transfer container) for the returned value. The fix for the vala compiler doesn't seem to be trivial, so while it's not fixed, a workaround is to use g_variant_dup_strv instead of get_strv so that the generated C code frees what it needs to free.
Created attachment 246792 [details] [review] Previous patch with fixed commit message
Thanks for the patch! Is it possible to create a small test case that can reproduce this issue (such that the patch fixes the test)?
Also, can you add a FIXME comment to the patch with a link to the Vala bug number, so we can revert the change once the Vala bug’s fixed please?
Created attachment 247037 [details] [review] Add FIXME with reference to Vala bug
Ok, so just missing the test program. I guess you want a test that calls the method in the oFono backend, right? If so, I guess just a program that calls 'folks-inspect personas' with a 3G modem plugged would do, since that is how I was getting the crash. Is that enough?
(In reply to comment #5) > Ok, so just missing the test program. I guess you want a test that calls the > method in the oFono backend, right? If so, I guess just a program that calls > 'folks-inspect personas' with a 3G modem plugged would do, since that is how I > was getting the crash. Is that enough? Can you add a small wrapper that fakes the presence of a 3G modem in oFono so this can be effectively checked without needing the hardware? And please make sure that any test is sufficiently sandboxed that it won't affect the host user's settings or state. Our tests already set up a private D-Bus session bus, etc., to provide isolation, but you'll want to be careful that you maintain this type of behavior for the oFono tests as well.
Created attachment 247799 [details] [review] Patch with a 1st attempt at sandboxed test Since ofono uses the system bus, I'm not able to make the test script start ofonod on the fake system bus. I guess we can provide a config file for the fake system bus, but seems ofono daemon still needs root privileges. Will do another try, but will appreciate more eyes looking at the patch
(In reply to comment #7) > Since ofono uses the system bus, I'm not able to make the test script start > ofonod on the fake system bus. I guess we can provide a config file for the > fake system bus, but seems ofono daemon still needs root privileges. Hmm, you might have to mock the ofono daemon instead then, using python-dbusmock, or an approach similar to what folks’ libsocialweb tests do.
Created attachment 247898 [details] [review] Almost fully working test suite Adding this patch for further review. Only thing not working is the starting of the fake system daemon, which I think is because of the config file, so working on that to hopefully make it fully work soon
Created attachment 247965 [details] [review] Fully working oFono test suite for the fix There was no need to really use a fake system bus, just running a session one and setting DBUS_SYSTEM_BUS_ADDRESS env var is enough, so now the patch is fully working. Feel free to review
Review of attachment 247965 [details] [review]: Looking good. If possible, you should get rid of the Bash scripts and re-use Simon’s work on Vala-based D-Bus mocking from tests/lib/test-case.vala. It would probably be helpful if you poked him about it, since I believe he has ideas for it which he never got time to implement; and he’s a lot more familiar with that code than I am. ::: tests/ofono/Makefile.am @@ +9,3 @@ + -I$(top_srcdir)/folks \ + -I$(top_srcdir)/tests/lib \ + -I$(top_srcdir)/tests/lib/key-file \ key-file shouldn’t be necessary here. ::: tests/ofono/ofono-test.vala @@ +120,3 @@ + try + { + backend.prepare.end (r); Dodgy indentation. @@ +124,3 @@ + } + catch (GLib.Error e) + { Dodgy indentation. ::: tests/tools/with-system-bus-ofono.sh @@ +42,3 @@ +cleanup + +exit $e You shouldn’t need this file, or dbus-session.sh. You can use the private_bus_up() method (and friends) in tests/lib/test-case.vala to handle that all without resorting to Bash. You might need to modify test-case.vala to support a fake system bus though.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/folks/issues/64.