[PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[jussi.pakkanen]

[-- Attachment #1: Type: text/plain, Size: 7512 bytes --]

From: Jussi Pakkanen <jussi.pakkanen@canonical.com>

Hello

In testing it is sometimes useful to be able to replace the system ofono daemon
instance with our own. This patch makes this possible using dbus' name replacement
feature. This patch has the plumbing changes to make it possible to set the
name replacement settings. The next patch allows you to set these parameters
from the command line. The default behaviour does not change, i.e. the service
name is not replaceable and the daemon will not try to replace an existing ofono
instance.

---
 dundee/main.c        |  2 +-
 gdbus/gdbus.h        | 12 +++++++++---
 gdbus/mainloop.c     | 22 ++++++++++++++--------
 src/main.c           |  3 ++-
 tools/auto-enable.c  |  2 +-
 tools/huawei-audio.c |  2 +-
 tools/stktest.c      |  2 +-
 7 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/dundee/main.c b/dundee/main.c
index 791425b..2264d42 100644
--- a/dundee/main.c
+++ b/dundee/main.c
@@ -201,7 +201,7 @@ int main(int argc, char **argv)
 
 	dbus_error_init(&error);
 
-	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, DUNDEE_SERVICE, &error);
+	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, DUNDEE_SERVICE, 0, &error);
 	if (conn == NULL) {
 		if (dbus_error_is_set(&error) == TRUE) {
 			ofono_error("Unable to hop onto D-Bus: %s",
diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 551c306..0eedd1e 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -35,6 +35,7 @@ typedef enum GDBusMethodFlags GDBusMethodFlags;
 typedef enum GDBusSignalFlags GDBusSignalFlags;
 typedef enum GDBusPropertyFlags GDBusPropertyFlags;
 typedef enum GDBusSecurityFlags GDBusSecurityFlags;
+typedef enum GDBusNameBehaviourFlags GDBusNameBehaviourFlags;
 
 typedef struct GDBusArgInfo GDBusArgInfo;
 typedef struct GDBusMethodTable GDBusMethodTable;
@@ -52,13 +53,13 @@ typedef gboolean (* GDBusSignalFunction) (DBusConnection *connection,
 					DBusMessage *message, void *user_data);
 
 DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
-							DBusError *error);
+							GDBusNameBehaviourFlags name_flags, DBusError *error);
 
 DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
-							DBusError *error);
+							GDBusNameBehaviourFlags name_flags, DBusError *error);
 
 gboolean g_dbus_request_name(DBusConnection *connection, const char *name,
-							DBusError *error);
+							GDBusNameBehaviourFlags name_flags, DBusError *error);
 
 gboolean g_dbus_set_disconnect_function(DBusConnection *connection,
 				GDBusWatchFunction function,
@@ -115,6 +116,11 @@ enum GDBusSecurityFlags {
 	G_DBUS_SECURITY_FLAG_ALLOW_INTERACTION = (1 << 2),
 };
 
+enum GDBusNameBehaviourFlags {
+	G_DBUS_NAME_ALLOW_REPLACEMENT  = (1 << 0),
+	G_DBUS_NAME_TRY_REPLACEMENT    = (1 << 1),
+};
+
 struct GDBusArgInfo {
 	const char *name;
 	const char *signature;
diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index 435fb93..6417818 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -252,13 +252,13 @@ static inline void setup_dbus_with_main_loop(DBusConnection *conn)
 }
 
 static gboolean setup_bus(DBusConnection *conn, const char *name,
-						DBusError *error)
+						GDBusNameBehaviourFlags name_flags, DBusError *error)
 {
 	gboolean result;
 	DBusDispatchStatus status;
 
 	if (name != NULL) {
-		result = g_dbus_request_name(conn, name, error);
+		result = g_dbus_request_name(conn, name, name_flags, error);
 
 		if (error != NULL) {
 			if (dbus_error_is_set(error) == TRUE)
@@ -278,7 +278,7 @@ static gboolean setup_bus(DBusConnection *conn, const char *name,
 }
 
 DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
-							DBusError *error)
+							GDBusNameBehaviourFlags name_flags, DBusError *error)
 {
 	DBusConnection *conn;
 
@@ -292,7 +292,7 @@ DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
 	if (conn == NULL)
 		return NULL;
 
-	if (setup_bus(conn, name, error) == FALSE) {
+	if (setup_bus(conn, name, name_flags, error) == FALSE) {
 		dbus_connection_unref(conn);
 		return NULL;
 	}
@@ -301,7 +301,7 @@ DBusConnection *g_dbus_setup_bus(DBusBusType type, const char *name,
 }
 
 DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
-							DBusError *error)
+							GDBusNameBehaviourFlags name_flags, DBusError *error)
 {
 	DBusConnection *conn;
 
@@ -315,7 +315,7 @@ DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
 	if (conn == NULL)
 		return NULL;
 
-	if (setup_bus(conn, name, error) == FALSE) {
+	if (setup_bus(conn, name, name_flags, error) == FALSE) {
 		dbus_connection_unref(conn);
 		return NULL;
 	}
@@ -324,12 +324,18 @@ DBusConnection *g_dbus_setup_private(DBusBusType type, const char *name,
 }
 
 gboolean g_dbus_request_name(DBusConnection *connection, const char *name,
-							DBusError *error)
+							GDBusNameBehaviourFlags name_flags, DBusError *error)
 {
 	int result;
+	unsigned int dbus_flags = DBUS_NAME_FLAG_DO_NOT_QUEUE;
+
+	if (name_flags & G_DBUS_NAME_TRY_REPLACEMENT)
+		dbus_flags |= DBUS_NAME_FLAG_REPLACE_EXISTING;
+	if (name_flags & G_DBUS_NAME_ALLOW_REPLACEMENT)
+		dbus_flags |= DBUS_NAME_FLAG_ALLOW_REPLACEMENT;
 
 	result = dbus_bus_request_name(connection, name,
-					DBUS_NAME_FLAG_DO_NOT_QUEUE, error);
+					dbus_flags, error);
 
 	if (error != NULL) {
 		if (dbus_error_is_set(error) == TRUE)
diff --git a/src/main.c b/src/main.c
index d6349cb..d2042c5 100644
--- a/src/main.c
+++ b/src/main.c
@@ -168,6 +168,7 @@ int main(int argc, char **argv)
 	DBusConnection *conn;
 	DBusError error;
 	guint signal;
+	GDBusNameBehaviourFlags name_flags = 0;
 
 #ifdef NEED_THREADS
 	if (g_thread_supported() == FALSE)
@@ -217,7 +218,7 @@ int main(int argc, char **argv)
 
 	dbus_error_init(&error);
 
-	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, OFONO_SERVICE, &error);
+	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, OFONO_SERVICE, name_flags, &error);
 	if (conn == NULL) {
 		if (dbus_error_is_set(&error) == TRUE) {
 			ofono_error("Unable to hop onto D-Bus: %s",
diff --git a/tools/auto-enable.c b/tools/auto-enable.c
index 87fb0a8..86c4b39 100644
--- a/tools/auto-enable.c
+++ b/tools/auto-enable.c
@@ -527,7 +527,7 @@ int main(int argc, char **argv)
 
 	dbus_error_init(&err);
 
-	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, &err);
+	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, 0, &err);
 	if (conn == NULL) {
 		if (dbus_error_is_set(&err) == TRUE) {
 			fprintf(stderr, "%s\n", err.message);
diff --git a/tools/huawei-audio.c b/tools/huawei-audio.c
index 9997a58..8f95ade 100644
--- a/tools/huawei-audio.c
+++ b/tools/huawei-audio.c
@@ -810,7 +810,7 @@ int main(int argc, char **argv)
 
 	dbus_error_init(&err);
 
-	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, &err);
+	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, 0, &err);
 	if (conn == NULL) {
 		if (dbus_error_is_set(&err) == TRUE) {
 			fprintf(stderr, "%s\n", err.message);
diff --git a/tools/stktest.c b/tools/stktest.c
index c015e09..4e7e645 100644
--- a/tools/stktest.c
+++ b/tools/stktest.c
@@ -4501,7 +4501,7 @@ int main(int argc, char **argv)
 
 	dbus_error_init(&err);
 
-	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, &err);
+	conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, 0, &err);
 	if (conn == NULL) {
 		if (dbus_error_is_set(&err) == TRUE) {
 			fprintf(stderr, "%s\n", err.message);
-- 
1.9.1

[PATCH 2/2] Can set name replacement with command line arguments.

[jussi.pakkanen]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

From: Jussi Pakkanen <jussi.pakkanen@canonical.com>

---
 src/main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/main.c b/src/main.c
index d2042c5..489a9d8 100644
--- a/src/main.c
+++ b/src/main.c
@@ -133,6 +133,8 @@ static gchar *option_plugin = NULL;
 static gchar *option_noplugin = NULL;
 static gboolean option_detach = TRUE;
 static gboolean option_version = FALSE;
+static gboolean option_try_replace = FALSE;
+static gboolean option_allow_replace = FALSE;
 
 static gboolean parse_debug(const char *key, const char *value,
 					gpointer user_data, GError **error)
@@ -156,6 +158,10 @@ static GOptionEntry options[] = {
 	{ "nodetach", 'n', G_OPTION_FLAG_REVERSE,
 				G_OPTION_ARG_NONE, &option_detach,
 				"Don't run as daemon in background" },
+	{ "replace", 0, 0, G_OPTION_ARG_NONE, &option_try_replace,
+				"Try to replace existing service", NULL },
+	{ "allow-replace", 0, 0, G_OPTION_ARG_NONE, &option_allow_replace,
+				"Allow dbus service name to be replaced", NULL },
 	{ "version", 'v', 0, G_OPTION_ARG_NONE, &option_version,
 				"Show version information and exit" },
 	{ NULL },
@@ -203,6 +209,12 @@ int main(int argc, char **argv)
 		}
 	}
 
+	if (option_try_replace == TRUE)
+		name_flags |= G_DBUS_NAME_TRY_REPLACEMENT;
+
+	if (option_allow_replace == TRUE)
+		name_flags |= G_DBUS_NAME_ALLOW_REPLACEMENT;
+
 	event_loop = g_main_loop_new(NULL, FALSE);
 
 #ifdef NEED_THREADS
-- 
1.9.1

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

Hi Jussi,

> In testing it is sometimes useful to be able to replace the system ofono daemon
> instance with our own. This patch makes this possible using dbus' name replacement
> feature. This patch has the plumbing changes to make it possible to set the
> name replacement settings. The next patch allows you to set these parameters
> from the command line. The default behaviour does not change, i.e. the service
> name is not replaceable and the daemon will not try to replace an existing ofono
> instance.

what is this useful for. We have been running oFono for more than 4 years and BlueZ with D-Bus for over 10 years and never had the need for doing this. So I do not understand why we would support this.

> ---
> dundee/main.c        |  2 +-
> gdbus/gdbus.h        | 12 +++++++++---
> gdbus/mainloop.c     | 22 ++++++++++++++————

And gdbus/ changes must always be in a separate patch. This code is used by ConnMan, oFono, BlueZ etc.

Regards

Marcel

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Jussi Pakkanen]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

On 02.04.2014 21:39, Marcel Holtmann wrote:

>> In testing it is sometimes useful to be able to replace the system
>> ofono daemon instance with our own. This patch makes this possible
>> using dbus' name replacement feature. This patch has the plumbing
>> changes to make it possible to set the name replacement settings.
>> The next patch allows you to set these parameters from the command
>> line. The default behaviour does not change, i.e. the service name
>> is not replaceable and the daemon will not try to replace an
>> existing ofono instance.
>
> what is this useful for. We have been running oFono for more than 4
> years and BlueZ with D-Bus for over 10 years and never had the need
> for doing this. So I do not understand why we would support this.

This helps in an issue that comes up in system-wide automated testing.
There are some tests that we want to run different ofono instances. All 
these tests need to run in the same instance and without root 
privileges. The normal approach would be to run the tests under a 
private dbus session. However this becomes problematic when the thing we 
are testing requires other services that are only provided by the real 
system bus. Permitting name transfer allows us to replace only the ofono 
instance and do so without root privileges (installing a custom dbus 
conf file that permits name replacement during testing is straightforward).

> And gdbus/ changes must always be in a separate patch. This code is
> used by ConnMan, oFono, BlueZ etc.

If the patch is otherwise acceptable, I'll reformat and resubmit.

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

On 03.04.2014 10:38, Jussi Pakkanen wrote:
> On 02.04.2014 21:39, Marcel Holtmann wrote:
> 
>>> In testing it is sometimes useful to be able to replace the system
>>> ofono daemon instance with our own. This patch makes this possible
>>> using dbus' name replacement feature. This patch has the plumbing
>>> changes to make it possible to set the name replacement settings.
>>> The next patch allows you to set these parameters from the command
>>> line. The default behaviour does not change, i.e. the service name
>>> is not replaceable and the daemon will not try to replace an
>>> existing ofono instance.
>>
>> what is this useful for. We have been running oFono for more than 4
>> years and BlueZ with D-Bus for over 10 years and never had the need
>> for doing this. So I do not understand why we would support this.
> 
> This helps in an issue that comes up in system-wide automated testing.
> There are some tests that we want to run different ofono instances. All
> these tests need to run in the same instance and without root
> privileges. The normal approach would be to run the tests under a
> private dbus session. However this becomes problematic when the thing we
> are testing requires other services that are only provided by the real
> system bus. Permitting name transfer allows us to replace only the ofono
> instance and do so without root privileges (installing a custom dbus
> conf file that permits name replacement during testing is straightforward).


Indeed, and this combined with commit 5f765259 one can easily run a
series of tests (let's say dialer UI, messaging..) with different
phonesim configurations changing the number of modems and phonesim .xml
files for each modem.


 -- Antti

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

Hi Jussi,

>>> In testing it is sometimes useful to be able to replace the system
>>> ofono daemon instance with our own. This patch makes this possible
>>> using dbus' name replacement feature. This patch has the plumbing
>>> changes to make it possible to set the name replacement settings.
>>> The next patch allows you to set these parameters from the command
>>> line. The default behaviour does not change, i.e. the service name
>>> is not replaceable and the daemon will not try to replace an
>>> existing ofono instance.
>> 
>> what is this useful for. We have been running oFono for more than 4
>> years and BlueZ with D-Bus for over 10 years and never had the need
>> for doing this. So I do not understand why we would support this.
> 
> This helps in an issue that comes up in system-wide automated testing.
> There are some tests that we want to run different ofono instances. All these tests need to run in the same instance and without root privileges. The normal approach would be to run the tests under a private dbus session. However this becomes problematic when the thing we are testing requires other services that are only provided by the real system bus. Permitting name transfer allows us to replace only the ofono instance and do so without root privileges (installing a custom dbus conf file that permits name replacement during testing is straightforward).

the custom D-Bus configuration needs to be installed as root. So you need the root access right there. And if oFono is not running as root, then the same oFono user can start and stop it. I do not see what problem you are trying to solve.

So just kill the current ofonod process using kill(1) and start the new one. If you have access to the process that is allowed to own the name, you can also terminate the current process.

Regards

Marcel

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

Hi Jussi,

>>>> In testing it is sometimes useful to be able to replace the system
>>>> ofono daemon instance with our own. This patch makes this possible
>>>> using dbus' name replacement feature. This patch has the plumbing
>>>> changes to make it possible to set the name replacement settings.
>>>> The next patch allows you to set these parameters from the command
>>>> line. The default behaviour does not change, i.e. the service name
>>>> is not replaceable and the daemon will not try to replace an
>>>> existing ofono instance.
>>> 
>>> what is this useful for. We have been running oFono for more than 4
>>> years and BlueZ with D-Bus for over 10 years and never had the need
>>> for doing this. So I do not understand why we would support this.
>> 
>> This helps in an issue that comes up in system-wide automated testing.
>> There are some tests that we want to run different ofono instances. All
>> these tests need to run in the same instance and without root
>> privileges. The normal approach would be to run the tests under a
>> private dbus session. However this becomes problematic when the thing we
>> are testing requires other services that are only provided by the real
>> system bus. Permitting name transfer allows us to replace only the ofono
>> instance and do so without root privileges (installing a custom dbus
>> conf file that permits name replacement during testing is straightforward).
> 
> 
> Indeed, and this combined with commit 5f765259 one can easily run a
> series of tests (let's say dialer UI, messaging..) with different
> phonesim configurations changing the number of modems and phonesim .xml
> files for each modem.

this argument does not really fly with me. You can just disable/enable the phonesim modem over D-Bus and it allows you to connect to a total different phonesim instance with a new phonesim.xml.

Or go the route of plugins/stktest.c and tools/stktest.c that we used for unit testing SIM Toolkit.

And here as well. If you are allowed to own the D-Bus well know name, you normally can kill the current process and start a new one of your choice.

Regards

Marcel

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

Hi Antti,
>
> Indeed, and this combined with commit 5f765259 one can easily run a
> series of tests (let's say dialer UI, messaging..) with different
> phonesim configurations changing the number of modems and phonesim .xml
> files for each modem.

I see what you needed that commit for now.  While I can kind of see your
usecase, I really wonder whether simply creating a large phonesim.conf
and grouping modems there would be a less intrusive way to get what you
want.

Phonesim can have an arbitrary number of instances (I once tested 100 at
once), and one can assign an object path using phonesim.conf.  Your test
application can then find such groupings and accomplish essentially the
same thing as you're trying to do now.

Regards,
-Denis

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

On 03.04.2014 20:55, Marcel Holtmann wrote:
>> Indeed, and this combined with commit 5f765259 one can easily run
>> a series of tests (let's say dialer UI, messaging..) with
>> different phonesim configurations changing the number of modems and
>> phonesim .xml files for each modem.
> 
> this argument does not really fly with me. You can just
> disable/enable the phonesim modem over D-Bus and it allows you to
> connect to a total different phonesim instance with a new
> phonesim.xml.

Oh, didn't know that. That sounds great!
We were under the impression that the phonesim instances have to be
running before phonesim plugin loads and the modem .xml configurations
can't be changed on the fly.

This new information now removes the need to be able to restart ofono
just for updating the phonesim modem .xml configurations.


> Or go the route of plugins/stktest.c and tools/stktest.c that we used
> for unit testing SIM Toolkit.

The only problem that remains is that there seems to be no way of
changing the number of modems on the fly as they are read upon phonesim
plugin initialization from phonesim.conf.

Now looking at stktest.c I see it opens a socket to communicate with an
external process.


Would it be acceptable if we implement the following:

Upon phonesim plugin init we check if a configuration file exists at
    /etc/ofono/phonesim-control.conf

The file would contain the following:

    [control]
    Port=715517
    LoadDefaultConfig=0

Only if the file exists phonesim plugin would then create a _listening_
socket on the specified port.

If LoadDefaultConfig is 0 the phonesim plugin would not load
/etc/ofono/phonesim.conf.

That socket would be used to issue control commands:
    ADD [sim name] [Address] [Port]
    REMOVE [sim name]
    RESET

ADD    - adds a modem, fields are consistent with phonesim.conf
REMOVE - remove [sim name] modem
RESET  - clear all modems,
         if LoadDefaultConfig = 1 then reload the phonesim.conf
         if LoadDefaultConfig = 0 then remove all modems

I would be intrigued to have LOAD [configuration file], but having it
possible to order a root process to read arbitrary files is probably too
much. :)


> And here as well. If you are allowed to own the D-Bus well know name,
> you normally can kill the current process and start a new one of your
> choice.

The idea was that we would not have to allow root access to the
automated tests to be able to change the phonesim configuration. If we
can figure out a solution how to change the number of modems phonesim
plugin exposes on the fly then by all means there is no need for the
original patches.


 -- Antti

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

On 04.04.2014 00:40, Denis Kenzior wrote:
> Hi Antti,
>>
>> Indeed, and this combined with commit 5f765259 one can easily run a
>> series of tests (let's say dialer UI, messaging..) with different
>> phonesim configurations changing the number of modems and phonesim .xml
>> files for each modem.
> 
> I see what you needed that commit for now.  While I can kind of see your
> usecase, I really wonder whether simply creating a large phonesim.conf
> and grouping modems there would be a less intrusive way to get what you
> want.

That previous commit is something that is useful if ofonod with phonesim
is ran with unpriviledged uid on top of a private system bus, which is
totally doable, and works in situations where you have to test let's say
a low level binding library which expects ofono to be found on the
system bus.

Our original idea was to run all of our tests on top of private system
and session buses and in this situation the environment variable to
change the phonesim configuration file location would have been all we
need (and anyway, I personally don't like hardcoded configuration file
paths;)

But we ran into trouble trying to run the whole unity8 stack on top of
private session and system buses (mainly getting all the necessary
services on both the system and session bus to run on there as well was
exploding the test setup complexity) so we though about another
solutions and figured out that keeping the rest of the system running as
is and only replacing ofonod instance on real system-bus would be pretty
straight forward technically (such small patches:). Jussi sent the
patches to the mailing list so that we could get your opinion on them
and turns out (thanks Marcel!) that there might be better solutions
available.


 -- Antti

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

On 04.04.2014 02:45, Antti Kaijanmäki wrote:
> Only if the file exists phonesim plugin would then create a _listening_
> socket on the specified port.

Actually, are plugins able to register arbitrary dbus-objects under
org.ofono?

Something like /phonesim with an org.ofono.phonesim.Control interface
would be much nicer and would save the trouble of writing custom socket
communication to a root service.

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

Hi Antti,

> Oh, didn't know that. That sounds great!
> We were under the impression that the phonesim instances have to be
> running before phonesim plugin loads and the modem .xml configurations
> can't be changed on the fly.

What modem .xml configurations?  I'm lost now.

You have phonesim plugin inside oFono that simply creates modem driver
instances that essentially know two things:
  - IP Address
  - Port number.

When you enable the modem on path /phonesimN, a TCP connection is
established.
When you disable the modem, a TCP connection is killed.

> 
> This new information now removes the need to be able to restart ofono
> just for updating the phonesim modem .xml configurations.
> 

Just restart phonesim on the same port with a new XML file and be done
with it.

> 
>> Or go the route of plugins/stktest.c and tools/stktest.c that we used
>> for unit testing SIM Toolkit.
> 
> The only problem that remains is that there seems to be no way of
> changing the number of modems on the fly as they are read upon phonesim
> plugin initialization from phonesim.conf.

Why is this a problem?

> 
> Now looking at stktest.c I see it opens a socket to communicate with an
> external process.
> 

If by external process you mean oFono... then yes.  The setup is exactly
the same as phonesim, just automated.  Think of stktest as a phonesim
instance with a very limited AT command set, but tells oFono when to
connect ;)

> 
> Would it be acceptable if we implement the following:
> 
> Upon phonesim plugin init we check if a configuration file exists at
>     /etc/ofono/phonesim-control.conf
> 
> The file would contain the following:
> 
>     [control]
>     Port=715517
>     LoadDefaultConfig=0
> 
> Only if the file exists phonesim plugin would then create a _listening_
> socket on the specified port.
> 
> If LoadDefaultConfig is 0 the phonesim plugin would not load
> /etc/ofono/phonesim.conf.
> 
> That socket would be used to issue control commands:
>     ADD [sim name] [Address] [Port]
>     REMOVE [sim name]
>     RESET

You want to create and remove phonesim modems on the fly? Why?  That
seems utterly pointless.

Regards,
-Denis

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

Hi Antti,

On 04/03/2014 07:09 PM, Antti Kaijanmäki wrote:
> On 04.04.2014 02:45, Antti Kaijanmäki wrote:
>> Only if the file exists phonesim plugin would then create a _listening_
>> socket on the specified port.
> 
> Actually, are plugins able to register arbitrary dbus-objects under
> org.ofono?
> 

Yes, plugins can do whatever they wish.  See plugins/smart-messaging.c
for an example.

Regards,
-Denis

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On 04.04.2014 00:40, Denis Kenzior wrote:
> Phonesim can have an arbitrary number of instances (I once tested 100 at
> once), and one can assign an object path using phonesim.conf.  Your test
> application can then find such groupings and accomplish essentially the
> same thing as you're trying to do now.

What we are trying to do is a full stack system testing where the
individual components that are being tested don't have any modifications
or adaptations when they are run under test suite.

The number of modems returned by org.ofono.Manager.GetModems() must
represent the total number of modems available to the system and thus we
need to have exactly two modems when testing dual-sim features etc.

The need to change the number of modems during testing comes from the
fact that our test suite has the single and multimodem tests together
and runs them one after another and we need to be able to set up the
environment appropriately in between individual test cases.

 -- Antti

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

Hi Antti,

> What we are trying to do is a full stack system testing where the
> individual components that are being tested don't have any modifications
> or adaptations when they are run under test suite.

Okay that sort of makes sense

> 
> The number of modems returned by org.ofono.Manager.GetModems() must
> represent the total number of modems available to the system and thus we
> need to have exactly two modems when testing dual-sim features etc.
> 

Sounds like your system is a bit inflexible, but okay.

> The need to change the number of modems during testing comes from the
> fact that our test suite has the single and multimodem tests together
> and runs them one after another and we need to be able to set up the
> environment appropriately in between individual test cases.
> 

Why don't you simply write a plugin that handles all of this?  E.g.
canonical_tester that creates two modem instances.  If you must insist
on having exactly 1 or 2 modems, then just add a DBus interface to
switch between modes.

Adding a control mechanism for controlling the number of phonesim
instances seems like total overkill.

Regards,
-Denis

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

On 04.04.2014 03:11, Denis Kenzior wrote:
> Hi Antti,
> 
>> Oh, didn't know that. That sounds great!
>> We were under the impression that the phonesim instances have to be
>> running before phonesim plugin loads and the modem .xml configurations
>> can't be changed on the fly.
> 
> What modem .xml configurations?  I'm lost now.

The XML files you pass to ofono-phonesim through command line parameter.


> You have phonesim plugin inside oFono that simply creates modem driver
> instances that essentially know two things:
>   - IP Address
>   - Port number.
> 
> When you enable the modem on path /phonesimN, a TCP connection is
> established.
> When you disable the modem, a TCP connection is killed.

Yes, got it now. If we want to change the modem configuration we simply
have to power down the modem, kill the old phonesim instance, start a
new one with another .xml and power the modem back on.


>> Now looking at stktest.c I see it opens a socket to communicate with an
>> external process.
>>
> 
> If by external process you mean oFono... then yes.  The setup is exactly
> the same as phonesim, just automated.  Think of stktest as a phonesim
> instance with a very limited AT command set, but tells oFono when to
> connect ;)

skttest is a oFono plugin which connects to an external process stktool,
yes, very much like phonesim plugin connects to a phonesim instance. But
what I was first proposing actually makes phonsim plugin (oFono) to
listen for connections from external processes.

But as you pointed out in your another reply that a plugin can do what
ever it wants then we don't need this custom listening socket, but we
can implement a proper dbus-interface instead.

Re: [PATCH 1/2] Allow users to specify dbus name replacement behaviour.

[Antti =?unknown-8bit?q?Kaijanm=C3=A4ki?=]

[-- Attachment #1: Type: text/plain, Size: 3478 bytes --]

On 04.04.2014 03:25, Denis Kenzior wrote:
>> The number of modems returned by org.ofono.Manager.GetModems() must
>> represent the total number of modems available to the system and thus we
>> need to have exactly two modems when testing dual-sim features etc.
>>
> 
> Sounds like your system is a bit inflexible, but okay.

The system is not inflexible, but we need to be able to set up the
environment for each test case according to the test plan.

As an example, not actual test plans:


= Test 1 =

Prerequisites:
 - number of modems: 1
   - modem 1
     - ID: Personal

Step 1:
Incoming call comes in on modem 1.
Verify:
 - notification of incoming call is shown
 - incoming call number is shown
 - the notification does not contain the ID of the modem
   receiving the call.

Step 2:
Accept the incoming call through the notification
Verify:
 - call interface shows up
 - call interface shows the incoming number
 - call interface does not show the modem ID

Step 3:
Hang up the call
Verify:
 - call is terminated
 - call interface turns to dialer interface
 - dialer interface does not show the Modem ID


= Test 1 =

Prerequisites:
 - number of modems: 2
   - modem 1
     - ID: Personal
   - modem 2
     - ID: Work

Step 1:
Incoming call comes in on modem 2.
Verify:
 - notification of incoming call is shown
 - incoming call number is shown
 - the notification contains the Modem ID "Work"

Step 2:
Accept the incoming call through the notification
Verify:
 - call interface shows up
 - call interface shows the incoming number
 - call interface shows the Modem ID "Work"

Step 3:
Hang up the call
Verify:
 - call is terminated
 - call interface turns to dialer interface
 - dialer interface allows to choose between two modems
   with IDs "Personal" and "Work"

Now, we have testing framework in place that we can do these tests in an
automated manner, testing the whole stack down from ofono up to the
final application that handles the phone calls, inspecting different
components that are visible on the screen no matter where they come from
(notifications from the notification system, applications, any shell
component..)

Now to run these tests in arbitrary order we need to be able to set up
the testing environment for each test individually.

For Test 1 we need oFono to expose a single modem for the duraction of
the test and through that modem we emulate the phone call and also
verify that the call gets terminated when the button in call interface
is pushed.

Test 2 is ran straight after Test 1 and for that we need to change the
number of modems to two and have them individually configured with
proper phonesim .xml files in between the two tests.


>> The need to change the number of modems during testing comes from the
>> fact that our test suite has the single and multimodem tests together
>> and runs them one after another and we need to be able to set up the
>> environment appropriately in between individual test cases.
>>
> 
> Why don't you simply write a plugin that handles all of this?  E.g.
> canonical_tester that creates two modem instances.  If you must insist
> on having exactly 1 or 2 modems, then just add a DBus interface to
> switch between modes.
> 
> Adding a control mechanism for controlling the number of phonesim
> instances seems like total overkill.

Yes, agreed. The DBus interface is the way to go.