[PATCH] udevng: Add OFONO_PATHNAME property to set modem dbus path name

[PATCH] udevng: Add OFONO_PATHNAME property to set modem dbus path name

[Pau Espin Pedrol]

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

The current udevng.c implementation sets dbus path names for modems
based on type and a number incremented seuqntially for each new modem
found. As a result, the dbus path for a given device is non
deterministic, since it depends on the devices available during ofono
startup.

Furthermore, if a modem crashes and reboots while in operation, then
udev will trigger a remove event followed by a create event, and the
same modem will now be given a different name (as the sequence number is
bigger).

This is non suitable for systems handling several modems which want to
identify them easily based on its path.

This patch introduces a way to be able to set persistent names for
specific devices while still permitting previous dynamic naming
methodology.

One can set a persistent name using udev rules for the target device
which set the OFONO_PATHNAME env property. If ofono finds this property
set, it will use its value as the dbus path name for the modem.

Example:
$ cat /etc/udev/rules.d/90-local.rules
SUBSYSTEMS=="usb", DEVPATH=="/devices/pci0000:00/0000:00:1a.0/usb3/3-1/3-1.1/3-1.1.4/3-1.1.4.1/3-1.1.4.1.1", ENV{OFONO_PATHNAME}="foo"

$ udevadm info -p /devices/pci0000:00/0000:00:1a.0/usb3/3-1/3-1.1/3-1.1.4/3-1.1.4.1/3-1.1.4.1.1
...
E: OFONO_PATHNAME=foo
...

$ mdbus2 -s org.ofono
/
/bluetooth
/bluetooth/profile
/bluetooth/profile/dun_gw
/bluetooth/profile/hfp_ag
/bluetooth/profile/hfp_hf
/foo
/mbm_0
---
 plugins/udevng.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 9b78ab47..e9f27977 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -46,6 +46,7 @@ enum modem_type {
 struct modem_info {
 	char *syspath;
 	char *devname;
+	char *pathname;
 	char *driver;
 	char *vendor;
 	char *model;
@@ -1302,6 +1303,7 @@ static void destroy_modem(gpointer data)
 
 	g_free(modem->syspath);
 	g_free(modem->devname);
+	g_free(modem->pathname);
 	g_free(modem->driver);
 	g_free(modem->vendor);
 	g_free(modem->model);
@@ -1382,7 +1384,7 @@ static struct udev_device* get_serial_modem_device(struct udev_device *dev)
  */
 static void add_serial_device(struct udev_device *dev)
 {
-	const char *syspath, *devpath, *devname, *devnode;
+	const char *syspath, *devpath, *devname, *devnode, *pathname;
 	struct modem_info *modem;
 	struct serial_device_info *info;
 	const char *subsystem;
@@ -1396,6 +1398,7 @@ static void add_serial_device(struct udev_device *dev)
 	}
 
 	driver = udev_device_get_property_value(mdev, "OFONO_DRIVER");
+	pathname = udev_device_get_property_value(mdev, "OFONO_PATHNAME");
 
 	syspath = udev_device_get_syspath(mdev);
 	devname = udev_device_get_devnode(mdev);
@@ -1415,6 +1418,7 @@ static void add_serial_device(struct udev_device *dev)
 		modem->type = MODEM_TYPE_SERIAL;
 		modem->syspath = g_strdup(syspath);
 		modem->devname = g_strdup(devname);
+		modem->pathname = g_strdup(pathname);
 		modem->driver = g_strdup(driver);
 
 		g_hash_table_replace(modem_list, modem->syspath, modem);
@@ -1440,7 +1444,8 @@ static void add_serial_device(struct udev_device *dev)
 
 static void add_device(const char *syspath, const char *devname,
 			const char *driver, const char *vendor,
-			const char *model, struct udev_device *device)
+			const char *model, const char *pathname,
+			struct udev_device *device)
 {
 	struct udev_device *usb_interface;
 	const char *devpath, *devnode, *interface, *number;
@@ -1477,6 +1482,7 @@ static void add_device(const char *syspath, const char *devname,
 		modem->driver = g_strdup(driver);
 		modem->vendor = g_strdup(vendor);
 		modem->model = g_strdup(model);
+		modem->pathname = g_strdup(pathname);
 
 		modem->sysattr = get_sysattr(driver);
 
@@ -1512,8 +1518,8 @@ static void add_device(const char *syspath, const char *devname,
 
 	DBG("%s", syspath);
 	DBG("%s", devpath);
-	DBG("%s (%s) %s [%s] ==> %s %s", devnode, driver,
-					interface, number, label, sysattr);
+	DBG("%s (%s) %s [%s] ==> %s %s %s",
+		devnode, driver, interface, number, label, sysattr, pathname);
 
 	info = g_try_new0(struct device_info, 1);
 	if (info == NULL)
@@ -1611,7 +1617,7 @@ static struct {
 static void check_usb_device(struct udev_device *device)
 {
 	struct udev_device *usb_device;
-	const char *syspath, *devname, *driver;
+	const char *syspath, *devname, *driver, *pathname;
 	const char *vendor = NULL, *model = NULL;
 
 	usb_device = udev_device_get_parent_with_subsystem_devtype(device,
@@ -1630,6 +1636,8 @@ static void check_usb_device(struct udev_device *device)
 	vendor = udev_device_get_property_value(usb_device, "ID_VENDOR_ID");
 	model = udev_device_get_property_value(usb_device, "ID_MODEL_ID");
 
+	pathname = udev_device_get_property_value(usb_device, "OFONO_PATHNAME");
+
 	driver = udev_device_get_property_value(usb_device, "OFONO_DRIVER");
 	if (!driver) {
 		struct udev_device *usb_interface =
@@ -1685,7 +1693,7 @@ static void check_usb_device(struct udev_device *device)
 			return;
 	}
 
-	add_device(syspath, devname, driver, vendor, model, device);
+	add_device(syspath, devname, driver, vendor, model, pathname, device);
 }
 
 static void check_device(struct udev_device *device)
@@ -1723,7 +1731,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
 
 	DBG("driver=%s", modem->driver);
 
-	modem->modem = ofono_modem_create(NULL, modem->driver);
+	modem->modem = ofono_modem_create(modem->pathname, modem->driver);
 	if (modem->modem == NULL)
 		return TRUE;
 
-- 
2.16.1

Re: [PATCH] udevng: Add OFONO_PATHNAME property to set modem dbus path name

[Denis Kenzior]

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

Hi Pau,

On 02/13/2018 01:53 PM, Pau Espin Pedrol wrote:
> The current udevng.c implementation sets dbus path names for modems
> based on type and a number incremented seuqntially for each new modem
> found. As a result, the dbus path for a given device is non
> deterministic, since it depends on the devices available during ofono
> startup.

Why should it be deterministic?

> 
> Furthermore, if a modem crashes and reboots while in operation, then
> udev will trigger a remove event followed by a create event, and the
> same modem will now be given a different name (as the sequence number is
> bigger).

Yes, and?

> 
> This is non suitable for systems handling several modems which want to
> identify them easily based on its path.
> 

This sounds like a very specific non-real world usecase, right?  I mean 
why don't you simply compare the IMEI if you really care?

> This patch introduces a way to be able to set persistent names for
> specific devices while still permitting previous dynamic naming
> methodology.
> 
> One can set a persistent name using udev rules for the target device
> which set the OFONO_PATHNAME env property. If ofono finds this property
> set, it will use its value as the dbus path name for the modem.

I'm hesitant to take this patch as udev logic is already complex and 
this doesn't seem to be relevant for anyone else besides your test system.

> 
> Example:
> $ cat /etc/udev/rules.d/90-local.rules
> SUBSYSTEMS=="usb", DEVPATH=="/devices/pci0000:00/0000:00:1a.0/usb3/3-1/3-1.1/3-1.1.4/3-1.1.4.1/3-1.1.4.1.1", ENV{OFONO_PATHNAME}="foo"
> 
> $ udevadm info -p /devices/pci0000:00/0000:00:1a.0/usb3/3-1/3-1.1/3-1.1.4/3-1.1.4.1/3-1.1.4.1.1
> ...
> E: OFONO_PATHNAME=foo
> ...
> 
> $ mdbus2 -s org.ofono
> /
> /bluetooth
> /bluetooth/profile
> /bluetooth/profile/dun_gw
> /bluetooth/profile/hfp_ag
> /bluetooth/profile/hfp_hf
> /foo

How are you making sure that dbus path is valid?   ofono_modem_create 
can fail after all...

> /mbm_0
> ---
>   plugins/udevng.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 

Regards,
-Denis

Re: [PATCH] udevng: Add OFONO_PATHNAME property to set modem dbus path name

[Pau Espin Pedrol]

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

Hi Denis,

On 14/02/18 18:04, Denis Kenzior wrote:
> 
> This sounds like a very specific non-real world usecase, right?  I mean 
> why don't you simply compare the IMEI if you really care?

First of all, I like thinking that I'm part of the real world!

As far as I understand the IMEI is available in 
org.ofono.Modem.GetProperties "Serial" property. To be able to access 
this information, I need to previously power on the modem (SetProperty 
Powered true), otherwise it doesn't show up.
Which means that for an ofono user, it is a requirement to power on the 
modem and thus use it before being able to identify it.

Simple case: Several modems attached to a PC, in order to find the one 
I'm interested in I need to power on all of them and find which one 
matches a given IMEI. Really? Do I need this if I know where is my 
device always physically attached? Are you going through all your 
network interfaces and check against its MAC address to find the one you 
are interested in? Or you configure your network manager based on 
interface names? Would you enjoy it if your enp0s25 interface was 
suddenly named enp1s67? Specially if you have iptables rules, I bet 
that's going to be funny.

More complex case (my scenario): Again, I repeat: To identify a modem, 
as of now, you seem to require to power it on (use it, change its state) 
before identifying it. Now think of a system which has a config file 
with modems available on the system (with its dbus paths) and a 
resources database of modems (and other types of resources) currently in 
use. Now imagine several processes reserving any specific subset of 
modems with specific capabilities (in parallel) against the resources 
database, then using them and among the use cases they power off and 
power them on. Even if you have locking mechanism in the database, it's 
a nightmare, because again, you must use a modem before identifying it, 
meaning you either race condition with other processes using the modem 
you are identifying (eg. by powering it on, or leaving it powered on 
after identifying it, or while powering it off after identifying while 
other process is starting to use it).

Then the easy way: Each modem is assigned a unique persistent path, and 
that's the one always being presented and used. No need to identify it, 
I already know which modem is which. If I have several system setups, I 
can assign same names in all systems for easy use, etc.

So the question should be better: Why should it NOT be deterministic?


> 
> How are you making sure that dbus path is valid?   ofono_modem_create 
> can fail after all...

Yes, it can fail while validating the path (dbus_validate_path) which is 
expected if a bad OFONO_PATHMNAME is supplied. If it's fine for you, I 
can add an error message there stating that dbus_validate_path failed 
and printing the pathname. Agree?

-- 
- Pau Espin Pedrol <pespin@sysmocom.de>         http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte

Re: [PATCH] udevng: Add OFONO_PATHNAME property to set modem dbus path name

[Denis Kenzior]

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

Hi Pau,

> As far as I understand the IMEI is available in 
> org.ofono.Modem.GetProperties "Serial" property. To be able to access 
> this information, I need to previously power on the modem (SetProperty 
> Powered true), otherwise it doesn't show up.
> Which means that for an ofono user, it is a requirement to power on the 
> modem and thus use it before being able to identify it.

Correct, but why is this a problem?  Under normal circumstances the 
modems are auto-powered by connman anyway.  That's the only reason that 
oFono doesn't do this automagically.

> 
> Simple case: Several modems attached to a PC, in order to find the one 
> I'm interested in I need to power on all of them and find which one 
> matches a given IMEI. Really? Do I need this if I know where is my 
> device always physically attached? Are you going through all your 
> network interfaces and check against its MAC address to find the one you 
> are interested in? Or you configure your network manager based on 
> interface names? Would you enjoy it if your enp0s25 interface was 
> suddenly named enp1s67? Specially if you have iptables rules, I bet 
> that's going to be funny.
> 
> More complex case (my scenario): Again, I repeat: To identify a modem, 
> as of now, you seem to require to power it on (use it, change its state) 
> before identifying it. Now think of a system which has a config file 
> with modems available on the system (with its dbus paths) and a 
> resources database of modems (and other types of resources) currently in 
> use. Now imagine several processes reserving any specific subset of 
> modems with specific capabilities (in parallel) against the resources 
> database, then using them and among the use cases they power off and 
> power them on. Even if you have locking mechanism in the database, it's 
> a nightmare, because again, you must use a modem before identifying it, 
> meaning you either race condition with other processes using the modem 
> you are identifying (eg. by powering it on, or leaving it powered on 
> after identifying it, or while powering it off after identifying while 
> other process is starting to use it).
oFono doesn't expose hardware details, only hardware capabilities.  So I 
understand how this is helpful for your usecase, but I'm not convinced 
this is relevant for what oFono was designed to do.

> 
> Then the easy way: Each modem is assigned a unique persistent path, and 
> that's the one always being presented and used. No need to identify it, 
> I already know which modem is which. If I have several system setups, I 
> can assign same names in all systems for easy use, etc. >
> So the question should be better: Why should it NOT be deterministic?
> 

In general oFono does not use deterministic paths, the Manager APIs 
should be used to query the list of objects.  In fact the docs even 
specifically state that the modem object path might have a variable 
prefix (e.g. a random number) and that variable prefix can change across 
oFono boots.  This is mainly to encourage UI/client writers to use the 
APIs properly.

We have used hardcoded paths in one or two cases for synthetic test 
cases (e.g. stktest) but those are highly special and not user-visible 
anyhow.

> 
>>
>> How are you making sure that dbus path is valid?   ofono_modem_create 
>> can fail after all...
>
> Yes, it can fail while validating the path (dbus_validate_path) which is 
> expected if a bad OFONO_PATHMNAME is supplied. If it's fine for you, I 
> can add an error message there stating that dbus_validate_path failed 
> and printing the pathname. Agree?

Go ahead and send a V2, I'll let it sit for a week or so to see if 
anyone else gives any feedback.

Regards,
-Denis

Re: [PATCH] udevng: Add OFONO_PATHNAME property to set modem dbus path name

[Christophe Ronco]

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

Hi Pau, hi Denis,


On 02/14/2018 09:31 PM, Denis Kenzior wrote:
> Hi Pau,
>
>> As far as I understand the IMEI is available in 
>> org.ofono.Modem.GetProperties "Serial" property. To be able to access 
>> this information, I need to previously power on the modem 
>> (SetProperty Powered true), otherwise it doesn't show up.
>> Which means that for an ofono user, it is a requirement to power on 
>> the modem and thus use it before being able to identify it.
>
> Correct, but why is this a problem?  Under normal circumstances the 
> modems are auto-powered by connman anyway.  That's the only reason 
> that oFono doesn't do this automagically.
>
>>
>> Simple case: Several modems attached to a PC, in order to find the 
>> one I'm interested in I need to power on all of them and find which 
>> one matches a given IMEI. Really? Do I need this if I know where is 
>> my device always physically attached? Are you going through all your 
>> network interfaces and check against its MAC address to find the one 
>> you are interested in? Or you configure your network manager based on 
>> interface names? Would you enjoy it if your enp0s25 interface was 
>> suddenly named enp1s67? Specially if you have iptables rules, I bet 
>> that's going to be funny.
>>
>> More complex case (my scenario): Again, I repeat: To identify a 
>> modem, as of now, you seem to require to power it on (use it, change 
>> its state) before identifying it. Now think of a system which has a 
>> config file with modems available on the system (with its dbus paths) 
>> and a resources database of modems (and other types of resources) 
>> currently in use. Now imagine several processes reserving any 
>> specific subset of modems with specific capabilities (in parallel) 
>> against the resources database, then using them and among the use 
>> cases they power off and power them on. Even if you have locking 
>> mechanism in the database, it's a nightmare, because again, you must 
>> use a modem before identifying it, meaning you either race condition 
>> with other processes using the modem you are identifying (eg. by 
>> powering it on, or leaving it powered on after identifying it, or 
>> while powering it off after identifying while other process is 
>> starting to use it).
> oFono doesn't expose hardware details, only hardware capabilities.  So 
> I understand how this is helpful for your usecase, but I'm not 
> convinced this is relevant for what oFono was designed to do.
>
>>
>> Then the easy way: Each modem is assigned a unique persistent path, 
>> and that's the one always being presented and used. No need to 
>> identify it, I already know which modem is which. If I have several 
>> system setups, I can assign same names in all systems for easy use, 
>> etc. >
>> So the question should be better: Why should it NOT be deterministic?
>>
>
> In general oFono does not use deterministic paths, the Manager APIs 
> should be used to query the list of objects.  In fact the docs even 
> specifically state that the modem object path might have a variable 
> prefix (e.g. a random number) and that variable prefix can change 
> across oFono boots.  This is mainly to encourage UI/client writers to 
> use the APIs properly.
>
> We have used hardcoded paths in one or two cases for synthetic test 
> cases (e.g. stktest) but those are highly special and not user-visible 
> anyhow.
>
>>
>>>
>>> How are you making sure that dbus path is valid? ofono_modem_create 
>>> can fail after all...
>>
>> Yes, it can fail while validating the path (dbus_validate_path) which 
>> is expected if a bad OFONO_PATHMNAME is supplied. If it's fine for 
>> you, I can add an error message there stating that dbus_validate_path 
>> failed and printing the pathname. Agree?
>
> Go ahead and send a V2, I'll let it sit for a week or so to see if 
> anyone else gives any feedback.

I have the same use case on my boards: several modems and I need to know 
where they physically are to be able to power the one I want and to 
physically reset a none working modem (using gpios corresponding to its 
hardware position). So I have a patch in Ofono to do that.
I have added a ModemSyspath property in org.ofono.Modem displaying dbus 
path. The patch is good enough for me but maybe not for upstream. As 
information comes from udevng plugin, this property might be in a 
specific interface added in udevng plugin and not in org.ofono.Modem.

This is just another way to have needed information (without changing 
modem name). I'll send my patch.

Christophe

[PATCH] add syspath property to modem properties

[Christophe Ronco]

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

---
 plugins/udevng.c |  2 ++
 src/modem.c      | 74 ++++++++++++++++++++++++++++++++------------------------
 2 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 2279bbe9..d5139586 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1330,6 +1330,8 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
 			continue;
 
 		if (driver_list[i].setup(modem) == TRUE) {
+			ofono_modem_set_string(modem->modem, "Syspath",
+								syspath);
 			ofono_modem_register(modem->modem);
 			return FALSE;
 		}
diff --git a/src/modem.c b/src/modem.c
index b1e8d3e2..7759b567 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -773,6 +773,41 @@ ofono_bool_t ofono_modem_get_online(struct ofono_modem *modem)
 	return modem->online;
 }
 
+static gboolean get_modem_property(struct ofono_modem *modem, const char *name,
+					enum property_type type,
+					void *value)
+{
+	struct modem_property *property;
+
+	DBG("modem %p property %s", modem, name);
+
+	property = g_hash_table_lookup(modem->properties, name);
+
+	if (property == NULL) {
+		DBG("property %s does not exist", name);
+		return FALSE;
+	}
+
+	if (property->type != type) {
+		DBG("property %s wrong type", name);
+		return FALSE;
+	}
+
+	switch (property->type) {
+	case PROPERTY_TYPE_STRING:
+		*((const char **) value) = property->value;
+		return TRUE;
+	case PROPERTY_TYPE_INTEGER:
+		memcpy(value, property->value, sizeof(int));
+		return TRUE;
+	case PROPERTY_TYPE_BOOLEAN:
+		memcpy(value, property->value, sizeof(ofono_bool_t));
+		return TRUE;
+	default:
+		return FALSE;
+	}
+}
+
 void __ofono_modem_append_properties(struct ofono_modem *modem,
 						DBusMessageIter *dict)
 {
@@ -783,6 +818,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 	struct ofono_devinfo *info;
 	dbus_bool_t emergency = ofono_modem_get_emergency_mode(modem);
 	const char *strtype;
+	char *value;
 
 	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
 				&modem->online);
@@ -823,6 +859,13 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 						&info->svn);
 	}
 
+	if (get_modem_property(modem, "Syspath", PROPERTY_TYPE_STRING,
+							&value) == TRUE) {
+		ofono_dbus_dict_append(dict, "ModemSyspath",
+						DBUS_TYPE_STRING,
+						&value);
+	}
+
 	interfaces = g_new0(char *, g_slist_length(modem->interface_list) + 1);
 	for (i = 0, l = modem->interface_list; l; l = l->next, i++)
 		interfaces[i] = l->data;
@@ -1731,37 +1774,6 @@ static int set_modem_property(struct ofono_modem *modem, const char *name,
 	return 0;
 }
 
-static gboolean get_modem_property(struct ofono_modem *modem, const char *name,
-					enum property_type type,
-					void *value)
-{
-	struct modem_property *property;
-
-	DBG("modem %p property %s", modem, name);
-
-	property = g_hash_table_lookup(modem->properties, name);
-
-	if (property == NULL)
-		return FALSE;
-
-	if (property->type != type)
-		return FALSE;
-
-	switch (property->type) {
-	case PROPERTY_TYPE_STRING:
-		*((const char **) value) = property->value;
-		return TRUE;
-	case PROPERTY_TYPE_INTEGER:
-		memcpy(value, property->value, sizeof(int));
-		return TRUE;
-	case PROPERTY_TYPE_BOOLEAN:
-		memcpy(value, property->value, sizeof(ofono_bool_t));
-		return TRUE;
-	default:
-		return FALSE;
-	}
-}
-
 int ofono_modem_set_string(struct ofono_modem *modem,
 				const char *key, const char *value)
 {
-- 
2.11.0

Re: [PATCH] add syspath property to modem properties

[Pau Espin Pedrol]

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

Hi Christophe,

Thanks for providing feedback on the topic. As I expected, I'm not alone 
and other people also have this requirement of being able to easily 
identify modems.

The patch you provide is enough for my needs, as I can easily identify a 
modem from the list without having to change its state (powering them 
on). I tested it and seems to be working fine:

  mdbus2 -s org.ofono / org.ofono.Manager.GetModems
([('/mbm_1', {'Online': <false>, 'Powered': <false>, 'Lockdown': 
<false>, 'Emergency': <false>, 'ModemSyspath': 
<'/sys/devices/pci0000:00/0000:00:1d.0/usb4/4-1/4-1.4'>, 'Interfaces': 
<@as []>, 'Features': <@as []>, 'Type': <'hardware'>}), ('/gobi_0', 
{'Online': <false>, 'Powered': <false>, 'Lockdown': <false>, 
'Emergency': <false>, 'ModemSyspath': 
<'/sys/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.2'>, 'Interfaces': 
<@as []>, 'Features': <@as []>, 'Type': <'hardware'>})],)

So I'd like seeing merged either something Christophe patch or mine (or 
both). I'll send a v2 of my patch as suggested and let you deliver on 
the best approach. If not good solution is provided, I'd need to keep it 
in a separate fork, which imho would be really sad as it seems other 
people may potentially use this kind of feature.

Regards,

-- 
- Pau Espin Pedrol <pespin@sysmocom.de>         http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte

Re: [PATCH] add syspath property to modem properties

[Denis Kenzior]

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

Hi Christophe,

On 02/15/2018 02:50 AM, Christophe Ronco wrote:
> ---
>   plugins/udevng.c |  2 ++
>   src/modem.c      | 74 ++++++++++++++++++++++++++++++++------------------------
>   2 files changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 2279bbe9..d5139586 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1330,6 +1330,8 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
>   			continue;
>   
>   		if (driver_list[i].setup(modem) == TRUE) {
> +			ofono_modem_set_string(modem->modem, "Syspath",

Use camel case, so "SysPath" or "SystemPath"

> +								syspath);
>   			ofono_modem_register(modem->modem);
>   			return FALSE;
>   		}
> diff --git a/src/modem.c b/src/modem.c
> index b1e8d3e2..7759b567 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -773,6 +773,41 @@ ofono_bool_t ofono_modem_get_online(struct ofono_modem *modem)
>   	return modem->online;
>   }
>   
> +static gboolean get_modem_property(struct ofono_modem *modem, const char *name,
> +					enum property_type type,
> +					void *value)
> +{
> +	struct modem_property *property;
> +
> +	DBG("modem %p property %s", modem, name);
> +
> +	property = g_hash_table_lookup(modem->properties, name);
> +
> +	if (property == NULL) {
> +		DBG("property %s does not exist", name);
> +		return FALSE;
> +	}
> +
> +	if (property->type != type) {
> +		DBG("property %s wrong type", name);
> +		return FALSE;
> +	}
> +
> +	switch (property->type) {
> +	case PROPERTY_TYPE_STRING:
> +		*((const char **) value) = property->value;
> +		return TRUE;
> +	case PROPERTY_TYPE_INTEGER:
> +		memcpy(value, property->value, sizeof(int));
> +		return TRUE;
> +	case PROPERTY_TYPE_BOOLEAN:
> +		memcpy(value, property->value, sizeof(ofono_bool_t));
> +		return TRUE;
> +	default:
> +		return FALSE;
> +	}
> +}
> +
>   void __ofono_modem_append_properties(struct ofono_modem *modem,
>   						DBusMessageIter *dict)
>   {
> @@ -783,6 +818,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>   	struct ofono_devinfo *info;
>   	dbus_bool_t emergency = ofono_modem_get_emergency_mode(modem);
>   	const char *strtype;
> +	char *value;
>   
>   	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
>   				&modem->online);
> @@ -823,6 +859,13 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>   						&info->svn);
>   	}
>   
> +	if (get_modem_property(modem, "Syspath", PROPERTY_TYPE_STRING,
> +							&value) == TRUE) {
> +		ofono_dbus_dict_append(dict, "ModemSyspath",
> +						DBUS_TYPE_STRING,
> +						&value);
> +	}
> +

There's no need for {}.  And why don't you make things much easier on 
yourself and simply use ofono_modem_get_string()

>   	interfaces = g_new0(char *, g_slist_length(modem->interface_list) + 1);
>   	for (i = 0, l = modem->interface_list; l; l = l->next, i++)
>   		interfaces[i] = l->data;
> @@ -1731,37 +1774,6 @@ static int set_modem_property(struct ofono_modem *modem, const char *name,
>   	return 0;
>   }
>   
> -static gboolean get_modem_property(struct ofono_modem *modem, const char *name,
> -					enum property_type type,
> -					void *value)
> -{
> -	struct modem_property *property;
> -
> -	DBG("modem %p property %s", modem, name);
> -
> -	property = g_hash_table_lookup(modem->properties, name);
> -
> -	if (property == NULL)
> -		return FALSE;
> -
> -	if (property->type != type)
> -		return FALSE;
> -
> -	switch (property->type) {
> -	case PROPERTY_TYPE_STRING:
> -		*((const char **) value) = property->value;
> -		return TRUE;
> -	case PROPERTY_TYPE_INTEGER:
> -		memcpy(value, property->value, sizeof(int));
> -		return TRUE;
> -	case PROPERTY_TYPE_BOOLEAN:
> -		memcpy(value, property->value, sizeof(ofono_bool_t));
> -		return TRUE;
> -	default:
> -		return FALSE;
> -	}
> -}
> -
>   int ofono_modem_set_string(struct ofono_modem *modem,
>   				const char *key, const char *value)
>   {
> 

Regards,
-Denis

Re: [PATCH] add syspath property to modem properties

[Christophe Ronco]

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

Hi Denis,

I didn't think this patch could be something used on Upstream Ofono.
If it is, I also have remarks on my own patch:
  - shouldn't it be two patches (one for plugin directory, another for 
src directory)?
  - what is an acceptable name for the new DBUS property (currently it 
is ModemSyspath):
    - ModemSysPath?
    - ModemSystemPath?
    - SystemPath?
  - what about modems that don't come from udevng plugin? Property is 
not set for these modems, is it ok? Do we want a generic property name 
to be able to also modify these plugins to give an information about 
hardware identification (something like: HardwareId, HardwarePath)? Or 
maybe SystemPath is also ok for these modems.

Let me know what you think about that and I will rework my patch to 
integrate your remarks,

Christophe

On 02/15/2018 04:59 PM, Denis Kenzior wrote:
> Hi Christophe,
>
> On 02/15/2018 02:50 AM, Christophe Ronco wrote:
>> ---
>>   plugins/udevng.c |  2 ++
>>   src/modem.c      | 74 
>> ++++++++++++++++++++++++++++++++------------------------
>>   2 files changed, 45 insertions(+), 31 deletions(-)
>>
>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>> index 2279bbe9..d5139586 100644
>> --- a/plugins/udevng.c
>> +++ b/plugins/udevng.c
>> @@ -1330,6 +1330,8 @@ static gboolean create_modem(gpointer key, 
>> gpointer value, gpointer user_data)
>>               continue;
>>             if (driver_list[i].setup(modem) == TRUE) {
>> +            ofono_modem_set_string(modem->modem, "Syspath",
>
> Use camel case, so "SysPath" or "SystemPath"
>
>> +                                syspath);
>>               ofono_modem_register(modem->modem);
>>               return FALSE;
>>           }
>> diff --git a/src/modem.c b/src/modem.c
>> index b1e8d3e2..7759b567 100644
>> --- a/src/modem.c
>> +++ b/src/modem.c
>> @@ -773,6 +773,41 @@ ofono_bool_t ofono_modem_get_online(struct 
>> ofono_modem *modem)
>>       return modem->online;
>>   }
>>   +static gboolean get_modem_property(struct ofono_modem *modem, 
>> const char *name,
>> +                    enum property_type type,
>> +                    void *value)
>> +{
>> +    struct modem_property *property;
>> +
>> +    DBG("modem %p property %s", modem, name);
>> +
>> +    property = g_hash_table_lookup(modem->properties, name);
>> +
>> +    if (property == NULL) {
>> +        DBG("property %s does not exist", name);
>> +        return FALSE;
>> +    }
>> +
>> +    if (property->type != type) {
>> +        DBG("property %s wrong type", name);
>> +        return FALSE;
>> +    }
>> +
>> +    switch (property->type) {
>> +    case PROPERTY_TYPE_STRING:
>> +        *((const char **) value) = property->value;
>> +        return TRUE;
>> +    case PROPERTY_TYPE_INTEGER:
>> +        memcpy(value, property->value, sizeof(int));
>> +        return TRUE;
>> +    case PROPERTY_TYPE_BOOLEAN:
>> +        memcpy(value, property->value, sizeof(ofono_bool_t));
>> +        return TRUE;
>> +    default:
>> +        return FALSE;
>> +    }
>> +}
>> +
>>   void __ofono_modem_append_properties(struct ofono_modem *modem,
>>                           DBusMessageIter *dict)
>>   {
>> @@ -783,6 +818,7 @@ void __ofono_modem_append_properties(struct 
>> ofono_modem *modem,
>>       struct ofono_devinfo *info;
>>       dbus_bool_t emergency = ofono_modem_get_emergency_mode(modem);
>>       const char *strtype;
>> +    char *value;
>>         ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
>>                   &modem->online);
>> @@ -823,6 +859,13 @@ void __ofono_modem_append_properties(struct 
>> ofono_modem *modem,
>>                           &info->svn);
>>       }
>>   +    if (get_modem_property(modem, "Syspath", PROPERTY_TYPE_STRING,
>> +                            &value) == TRUE) {
>> +        ofono_dbus_dict_append(dict, "ModemSyspath",
>> +                        DBUS_TYPE_STRING,
>> +                        &value);
>> +    }
>> +
>
> There's no need for {}.  And why don't you make things much easier on 
> yourself and simply use ofono_modem_get_string()
>
>>       interfaces = g_new0(char *, 
>> g_slist_length(modem->interface_list) + 1);
>>       for (i = 0, l = modem->interface_list; l; l = l->next, i++)
>>           interfaces[i] = l->data;
>> @@ -1731,37 +1774,6 @@ static int set_modem_property(struct 
>> ofono_modem *modem, const char *name,
>>       return 0;
>>   }
>>   -static gboolean get_modem_property(struct ofono_modem *modem, 
>> const char *name,
>> -                    enum property_type type,
>> -                    void *value)
>> -{
>> -    struct modem_property *property;
>> -
>> -    DBG("modem %p property %s", modem, name);
>> -
>> -    property = g_hash_table_lookup(modem->properties, name);
>> -
>> -    if (property == NULL)
>> -        return FALSE;
>> -
>> -    if (property->type != type)
>> -        return FALSE;
>> -
>> -    switch (property->type) {
>> -    case PROPERTY_TYPE_STRING:
>> -        *((const char **) value) = property->value;
>> -        return TRUE;
>> -    case PROPERTY_TYPE_INTEGER:
>> -        memcpy(value, property->value, sizeof(int));
>> -        return TRUE;
>> -    case PROPERTY_TYPE_BOOLEAN:
>> -        memcpy(value, property->value, sizeof(ofono_bool_t));
>> -        return TRUE;
>> -    default:
>> -        return FALSE;
>> -    }
>> -}
>> -
>>   int ofono_modem_set_string(struct ofono_modem *modem,
>>                   const char *key, const char *value)
>>   {
>>
>
> Regards,
> -Denis
>

Re: [PATCH] add syspath property to modem properties

[Denis Kenzior]

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

Hi Christophe,

No top posting please.

On 02/16/2018 09:40 AM, Christophe Ronco wrote:
> Hi Denis,
> 
> I didn't think this patch could be something used on Upstream Ofono.
> If it is, I also have remarks on my own patch:
>   - shouldn't it be two patches (one for plugin directory, another for 
> src directory)?

yes, all the normal patch submission guidelines apply.  You should also 
include a doc patch.

>   - what is an acceptable name for the new DBUS property (currently it 
> is ModemSyspath):
>     - ModemSysPath?
>     - ModemSystemPath?
>     - SystemPath?

Since you want to put it on the .Modem interface the 'Modem' prefix is 
redundant.  SystemPath should be okay, or maybe SysfsPath

>   - what about modems that don't come from udevng plugin? Property is 
> not set for these modems, is it ok? Do we want a generic property name 
> to be able to also modify these plugins to give an information about 
> hardware identification (something like: HardwareId, HardwarePath)? Or 
> maybe SystemPath is also ok for these modems.

Or simply mark this property [optional] and be done with it.

Go ahead and resubmit and lets see what feedback we get.

Regards,
-Denis

[PATCH 0/3] Add SystemPath property to modem interface

[Christophe Ronco]

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

Here is the patch serie as discussed in this thread of mails.

Christophe Ronco (3):
  udevng: Add modem string SystemPath
  modem: Add SystemPath dbus property
  doc: Add SystemPath to Modem interface

 doc/modem-api.txt | 8 ++++++++
 plugins/udevng.c  | 2 ++
 src/modem.c       | 6 ++++++
 3 files changed, 16 insertions(+)

-- 
2.7.4

[PATCH 1/3] udevng: Add modem string SystemPath

[Christophe Ronco]

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

---
 plugins/udevng.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 9b78ab4..3c7d99e 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1732,6 +1732,8 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
 			continue;
 
 		if (driver_list[i].setup(modem) == TRUE) {
+			ofono_modem_set_string(modem->modem, "SystemPath",
+								syspath);
 			ofono_modem_register(modem->modem);
 			return FALSE;
 		}
-- 
2.7.4

[PATCH 2/3] modem: Add SystemPath dbus property

[Christophe Ronco]

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

---
 src/modem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/modem.c b/src/modem.c
index d5fda7c..0cee861 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -792,6 +792,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 	struct ofono_devinfo *info;
 	dbus_bool_t emergency = ofono_modem_get_emergency_mode(modem);
 	const char *strtype;
+	const char *system_path;
 
 	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
 				&modem->online);
@@ -832,6 +833,11 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 						&info->svn);
 	}
 
+	system_path = ofono_modem_get_string(modem, "SystemPath");
+	if (system_path)
+		ofono_dbus_dict_append(dict, "SystemPath", DBUS_TYPE_STRING,
+					&system_path);
+
 	interfaces = g_new0(char *, g_slist_length(modem->interface_list) + 1);
 	for (i = 0, l = modem->interface_list; l; l = l->next, i++)
 		interfaces[i] = l->data;
-- 
2.7.4

[PATCH 3/3] doc: Add SystemPath to Modem interface

[Christophe Ronco]

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

---
 doc/modem-api.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/modem-api.txt b/doc/modem-api.txt
index 89e6191..e03c64a 100644
--- a/doc/modem-api.txt
+++ b/doc/modem-api.txt
@@ -95,6 +95,14 @@ Properties	boolean Powered [readwrite]
 			String representing the software version number of the
 			modem device.
 
+		string SystemPath [readonly, optional]
+
+			String giving a consistent hardware path for the modem
+			device. It will not change in case of reboot of modem
+			or ofono.
+			For modems detected by udev events, gives modem sysfs
+			path.
+
 		array{string} Features [readonly]
 
 			List of currently enabled features. It uses simple
-- 
2.7.4

Re: [PATCH 3/3] doc: Add SystemPath to Modem interface

[Jonas Bonn]

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

On 02/19/2018 03:57 PM, Christophe Ronco wrote:
> ---
>   doc/modem-api.txt | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/doc/modem-api.txt b/doc/modem-api.txt
> index 89e6191..e03c64a 100644
> --- a/doc/modem-api.txt
> +++ b/doc/modem-api.txt
> @@ -95,6 +95,14 @@ Properties	boolean Powered [readwrite]
>   			String representing the software version number of the
>   			modem device.
>   
> +		string SystemPath [readonly, optional]
> +
> +			String giving a consistent hardware path for the modem
> +			device. It will not change in case of reboot of modem
> +			or ofono.

Given that many modems are USB devices, I don't think you can actually 
promise this.  I can envision a (slightly pathological) hardware design 
whereby the presence or absence of an _external_ device at startup may 
change the device enumeration order and thereby the syspath...

/Jonas

> +			For modems detected by udev events, gives modem sysfs
> +			path.
> +
>   		array{string} Features [readonly]
>   
>   			List of currently enabled features. It uses simple
> 

Re: [PATCH 3/3] doc: Add SystemPath to Modem interface

[Denis Kenzior]

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

Hi,

>> +        string SystemPath [readonly, optional]
>> +
>> +            String giving a consistent hardware path for the modem
>> +            device. It will not change in case of reboot of modem
>> +            or ofono.
> 
> Given that many modems are USB devices, I don't think you can actually 
> promise this.  I can envision a (slightly pathological) hardware design 
> whereby the presence or absence of an _external_ device at startup may 
> change the device enumeration order and thereby the syspath...
> 

Right, this can depend on the kernel version also.  I wouldn't claim any 
guarantees at the D-Bus API level as it is outside our control.

If someone chooses to use it as a consistent path, then they do it at 
their own risk.

Regards,
-Denis

[PATCH 1/1] doc: Add SystemPath to Modem interface

[Christophe Ronco]

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

---
 doc/modem-api.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/modem-api.txt b/doc/modem-api.txt
index 89e6191..ff9facd 100644
--- a/doc/modem-api.txt
+++ b/doc/modem-api.txt
@@ -95,6 +95,13 @@ Properties	boolean Powered [readwrite]
 			String representing the software version number of the
 			modem device.
 
+		string SystemPath [readonly, optional]
+
+			String representing the system path for the modem
+			device.
+			For modems detected by udev events, this corresponds to
+			the modem sysfs path.
+
 		array{string} Features [readonly]
 
 			List of currently enabled features. It uses simple
-- 
2.7.4

Re: [PATCH 0/3] Add SystemPath property to modem interface

[Denis Kenzior]

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

Hi Christophe,

On 02/19/2018 08:57 AM, Christophe Ronco wrote:
> Here is the patch serie as discussed in this thread of mails.
> 
> Christophe Ronco (3):
>    udevng: Add modem string SystemPath
>    modem: Add SystemPath dbus property
>    doc: Add SystemPath to Modem interface
> 
>   doc/modem-api.txt | 8 ++++++++
>   plugins/udevng.c  | 2 ++
>   src/modem.c       | 6 ++++++
>   3 files changed, 16 insertions(+)
> 

I went ahead and applied all these (with the updated doc 3/3 patch).

Regards,
-Denis

[PATCH] modem: ofono_modem_create: log invalid paths

[Pau Espin Pedrol]

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

---
 src/modem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/modem.c b/src/modem.c
index d5fda7ca..698a7ca8 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -1876,8 +1876,10 @@ struct ofono_modem *ofono_modem_create(const char *name, const char *type)
 	else
 		snprintf(path, sizeof(path), "/%s", name);
 
-	if (!dbus_validate_path(path, NULL))
+	if (!dbus_validate_path(path, NULL)) {
+		DBG("Invalid dbus path for modem: %s", path);
 		return NULL;
+	}
 
 	modem = g_try_new0(struct ofono_modem, 1);
 
-- 
2.16.1