[PATCH] libudev: device - add devtype support

[PATCH] libudev: device - add devtype support

[Marcel Holtmann]

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

Hi Kay,

I was playing a little bit with libudev and I actually need the DEVTYPE
from uevent for various tasks. Especially with USB and Bluetooth, the
subsystem value is too generic.

Attached is a patch that implements udev_device_get_devtype() and also
udev_device_get_parent_with_devtype(). Please double check that I did it
the right way.

Regards

Marcel


[-- Attachment #2: 0001-libudev-device-add-devtype-support.patch --]
[-- Type: text/x-patch, Size: 7783 bytes --]

From 9a1f276ac8f47e082512a938deb2f04b62b63f5c Mon Sep 17 00:00:00 2001
From: Marcel Holtmann <marcel@holtmann.org>
Date: Fri, 2 Jan 2009 04:14:47 +0100
Subject: [PATCH] libudev: device - add devtype support

---
 udev/lib/exported_symbols  |    2 +
 udev/lib/libudev-device.c  |   53 +++++++++++++++++++++++++++++++++++++++++++-
 udev/lib/libudev-monitor.c |    2 +
 udev/lib/libudev-private.h |    1 +
 udev/lib/libudev.h         |    2 +
 udev/lib/test-libudev.c    |    4 +++
 6 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/udev/lib/exported_symbols b/udev/lib/exported_symbols
index a2f0ca7..6b07842 100644
--- a/udev/lib/exported_symbols
+++ b/udev/lib/exported_symbols
@@ -17,6 +17,7 @@ udev_device_new_from_devnum
 udev_device_new_from_subsystem_sysname
 udev_device_get_parent
 udev_device_get_parent_with_subsystem
+udev_device_get_parent_with_devtype
 udev_device_ref
 udev_device_unref
 udev_device_get_udev
@@ -26,6 +27,7 @@ udev_device_get_devnode
 udev_device_get_sysname
 udev_device_get_sysnum
 udev_device_get_subsystem
+udev_device_get_devtype
 udev_device_get_devlinks_list_entry
 udev_device_get_properties_list_entry
 udev_device_get_action
diff --git a/udev/lib/libudev-device.c b/udev/lib/libudev-device.c
index 055263b..06227be 100644
--- a/udev/lib/libudev-device.c
+++ b/udev/lib/libudev-device.c
@@ -40,6 +40,7 @@ struct udev_device {
 	const char *sysnum;
 	char *devnode;
 	char *subsystem;
+	char *devtype;
 	char *driver;
 	char *action;
 	char *devpath_old;
@@ -59,6 +60,7 @@ struct udev_device {
 	dev_t devnum;
 	unsigned int parent_set:1;
 	unsigned int subsystem_set:1;
+	unsigned int devtype_set:1;
 	unsigned int devlinks_uptodate:1;
 	unsigned int envp_uptodate:1;
 	unsigned int driver_set:1;
@@ -204,7 +206,9 @@ int udev_device_read_uevent_file(struct udev_device *udev_device)
 			continue;
 		pos[0] = '\0';
 
-		if (strncmp(line, "MAJOR=", 6) == 0)
+		if (strncmp(line, "DEVTYPE=", 8) == 0)
+			udev_device_set_devtype(udev_device, &line[8]);
+		else if (strncmp(line, "MAJOR=", 6) == 0)
 			maj = strtoull(&line[6], NULL, 10);
 		else if (strncmp(line, "MINOR=", 6) == 0)
 			min = strtoull(&line[6], NULL, 10);
@@ -553,6 +557,22 @@ struct udev_device *udev_device_get_parent_with_subsystem(struct udev_device *ud
 	return parent;
 }
 
+struct udev_device *udev_device_get_parent_with_devtype(struct udev_device *udev_device, const char *devtype)
+{
+	struct udev_device *parent;
+
+	parent = udev_device_get_parent(udev_device);
+	while (parent != NULL) {
+		const char *parent_devtype;
+
+		parent_devtype = udev_device_get_devtype(parent);
+		if (parent_devtype != NULL && strcmp(parent_devtype, devtype) == 0)
+			break;
+		parent = udev_device_get_parent(parent);
+	}
+	return parent;
+}
+
 /**
  * udev_device_get_udev:
  * @udev_device: udev device
@@ -605,6 +625,7 @@ void udev_device_unref(struct udev_device *udev_device)
 	free(udev_device->sysname);
 	free(udev_device->devnode);
 	free(udev_device->subsystem);
+	free(udev_device->devtype);
 	udev_list_cleanup_entries(udev_device->udev, &udev_device->devlinks_list);
 	udev_list_cleanup_entries(udev_device->udev, &udev_device->properties_list);
 	free(udev_device->action);
@@ -724,6 +745,25 @@ const char *udev_device_get_subsystem(struct udev_device *udev_device)
 }
 
 /**
+ * udev_device_get_devtype:
+ * @udev_device: udev device
+ *
+ * Retrieve the devtype string of the udev device.
+ *
+ * Returns: the devtype name of the udev device, or #NULL if it can not be determined
+ **/
+const char *udev_device_get_devtype(struct udev_device *udev_device)
+{
+	if (udev_device == NULL)
+		return NULL;
+	if (!udev_device->devtype_set) {
+		udev_device->devtype_set = 1;
+		udev_device_read_uevent_file(udev_device);
+	}
+	return udev_device->devtype;
+}
+
+/**
  * udev_device_get_devlinks_list_entry:
  * @udev_device: udev device
  *
@@ -963,6 +1003,17 @@ int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsy
 	return 0;
 }
 
+int udev_device_set_devtype(struct udev_device *udev_device, const char *devtype)
+{
+	free(udev_device->devtype);
+	udev_device->devtype = strdup(devtype);
+	if (udev_device->devtype == NULL)
+		return -ENOMEM;
+	udev_device->devtype_set = 1;
+	udev_device_add_property(udev_device, "DEVTYPE", udev_device->devtype);
+	return 0;
+}
+
 int udev_device_set_devnode(struct udev_device *udev_device, const char *devnode)
 {
 	free(udev_device->devnode);
diff --git a/udev/lib/libudev-monitor.c b/udev/lib/libudev-monitor.c
index 502fe24..e4cb807 100644
--- a/udev/lib/libudev-monitor.c
+++ b/udev/lib/libudev-monitor.c
@@ -325,6 +325,8 @@ struct udev_device *udev_monitor_receive_device(struct udev_monitor *udev_monito
 		} else if (strncmp(key, "SUBSYSTEM=", 10) == 0) {
 			udev_device_set_subsystem(udev_device, &key[10]);
 			subsystem_set = 1;
+		} else if (strncmp(key, "DEVTYPE=", 8) == 0) {
+			udev_device_set_devtype(udev_device, &key[8]);
 		} else if (strncmp(key, "DEVNAME=", 8) == 0) {
 			udev_device_set_devnode(udev_device, &key[8]);
 		} else if (strncmp(key, "DEVLINKS=", 9) == 0) {
diff --git a/udev/lib/libudev-private.h b/udev/lib/libudev-private.h
index 5e09188..0d752bb 100644
--- a/udev/lib/libudev-private.h
+++ b/udev/lib/libudev-private.h
@@ -56,6 +56,7 @@ extern struct udev_list_entry *udev_get_properties_list_entry(struct udev *udev)
 /* libudev-device */
 extern int udev_device_set_syspath(struct udev_device *udev_device, const char *syspath);
 extern int udev_device_set_subsystem(struct udev_device *udev_device, const char *subsystem);
+extern int udev_device_set_devtype(struct udev_device *udev_device, const char *devtype);
 extern int udev_device_set_devnode(struct udev_device *udev_device, const char *devnode);
 extern int udev_device_add_devlink(struct udev_device *udev_device, const char *devlink);
 extern void udev_device_cleanup_devlinks_list(struct udev_device *udev_device);
diff --git a/udev/lib/libudev.h b/udev/lib/libudev.h
index a9e399c..b96e494 100644
--- a/udev/lib/libudev.h
+++ b/udev/lib/libudev.h
@@ -63,11 +63,13 @@ extern struct udev_device *udev_device_new_from_devnum(struct udev *udev, char t
 extern struct udev_device *udev_device_new_from_subsystem_sysname(struct udev *udev, const char *subsystem, const char *sysname);
 extern struct udev_device *udev_device_get_parent(struct udev_device *udev_device);
 extern struct udev_device *udev_device_get_parent_with_subsystem(struct udev_device *udev_device, const char *subsystem);
+extern struct udev_device *udev_device_get_parent_with_devtype(struct udev_device *udev_device, const char *devtype);
 extern struct udev_device *udev_device_ref(struct udev_device *udev_device);
 extern void udev_device_unref(struct udev_device *udev_device);
 extern struct udev *udev_device_get_udev(struct udev_device *udev_device);
 extern const char *udev_device_get_devpath(struct udev_device *udev_device);
 extern const char *udev_device_get_subsystem(struct udev_device *udev_device);
+extern const char *udev_device_get_devtype(struct udev_device *udev_device);
 extern const char *udev_device_get_syspath(struct udev_device *udev_device);
 extern const char *udev_device_get_sysname(struct udev_device *udev_device);
 extern const char *udev_device_get_sysnum(struct udev_device *udev_device);
diff --git a/udev/lib/test-libudev.c b/udev/lib/test-libudev.c
index 5120626..fd12bd9 100644
--- a/udev/lib/test-libudev.c
+++ b/udev/lib/test-libudev.c
@@ -67,6 +67,10 @@ static void print_device(struct udev_device *device)
 	if (str != NULL)
 		printf("subsystem: '%s'\n", str);
 
+	str = udev_device_get_devtype(device);
+	if (str != NULL)
+		printf("devtype:   '%s'\n", str);
+
 	str = udev_device_get_driver(device);
 	if (str != NULL)
 		printf("driver:    '%s'\n", str);
-- 
1.5.6.3

Re: [PATCH] libudev: device - add devtype support

[Kay Sievers]

On Fri, Jan 2, 2009 at 04:22, Marcel Holtmann <marcel@holtmann.org> wrote:
> I was playing a little bit with libudev and I actually need the DEVTYPE
> from uevent for various tasks. Especially with USB and Bluetooth, the
> subsystem value is too generic.
>
> Attached is a patch that implements udev_device_get_devtype() and also
> udev_device_get_parent_with_devtype(). Please double check that I did it
> the right way.

Looks good. Applied.

Thanks,
Kay

Re: [PATCH] libudev: device - add devtype support

[Marcel Holtmann]

Hi Kay,

> > I was playing a little bit with libudev and I actually need the DEVTYPE
> > from uevent for various tasks. Especially with USB and Bluetooth, the
> > subsystem value is too generic.
> >
> > Attached is a patch that implements udev_device_get_devtype() and also
> > udev_device_get_parent_with_devtype(). Please double check that I did it
> > the right way.
> 
> Looks good. Applied.

one minor thing that came to my mind is that DEVTYPE and subsystem are
actually kinda coupled. So a DEVTYPE="host" has a different semantic for
USB than for Bluetooth subsystem for example. Not sure if we actually
care or just add a udev_device_get_parent_with_subsystem_devtype()
function to give applications a choice if they wanna care.

Regards

Marcel

Re: [PATCH] libudev: device - add devtype support

[Kay Sievers]

On Fri, Jan 2, 2009 at 09:22, Marcel Holtmann <marcel@holtmann.org> wrote:
>> > I was playing a little bit with libudev and I actually need the DEVTYPE
>> > from uevent for various tasks. Especially with USB and Bluetooth, the
>> > subsystem value is too generic.
>> >
>> > Attached is a patch that implements udev_device_get_devtype() and also
>> > udev_device_get_parent_with_devtype(). Please double check that I did it
>> > the right way.
>>
>> Looks good. Applied.
>
> one minor thing that came to my mind is that DEVTYPE and subsystem are
> actually kinda coupled. So a DEVTYPE="host" has a different semantic for
> USB than for Bluetooth subsystem for example. Not sure if we actually
> care or just add a udev_device_get_parent_with_subsystem_devtype()
> function to give applications a choice if they wanna care.

You mean replacing:
  udev_device_get_parent_with_devtype(..., *devtype)
by:
  udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
?

Sounds sensible, because in most cases you don't want to check for
parents of a different subsystem. As you are using it, want to send a
patch?

Thanks,
Kay

Re: [PATCH] libudev: device - add devtype support

[Marcel Holtmann]

Hi Kay,

> >> > I was playing a little bit with libudev and I actually need the DEVTYPE
> >> > from uevent for various tasks. Especially with USB and Bluetooth, the
> >> > subsystem value is too generic.
> >> >
> >> > Attached is a patch that implements udev_device_get_devtype() and also
> >> > udev_device_get_parent_with_devtype(). Please double check that I did it
> >> > the right way.
> >>
> >> Looks good. Applied.
> >
> > one minor thing that came to my mind is that DEVTYPE and subsystem are
> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
> > USB than for Bluetooth subsystem for example. Not sure if we actually
> > care or just add a udev_device_get_parent_with_subsystem_devtype()
> > function to give applications a choice if they wanna care.
> 
> You mean replacing:
>   udev_device_get_parent_with_devtype(..., *devtype)
> by:
>   udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
> ?
> 
> Sounds sensible, because in most cases you don't want to check for
> parents of a different subsystem. As you are using it, want to send a
> patch?

I am thinking of keeping both. So just adding ...subsystem_devtype() and
keeping also the original one. My reason for it is that in some case you
already know the subsystem you are looking at (or don't care in). So no
point in doing a lookup with two checks.

And yes, I can write a patch for it.

Regards

Marcel

Re: [PATCH] libudev: device - add devtype support

[Kay Sievers]

On Fri, Jan 2, 2009 at 14:46, Marcel Holtmann <marcel@holtmann.org> wrote:
>> >> > I was playing a little bit with libudev and I actually need the DEVTYPE
>> >> > from uevent for various tasks. Especially with USB and Bluetooth, the
>> >> > subsystem value is too generic.
>> >> >
>> >> > Attached is a patch that implements udev_device_get_devtype() and also
>> >> > udev_device_get_parent_with_devtype(). Please double check that I did it
>> >> > the right way.
>> >>
>> >> Looks good. Applied.
>> >
>> > one minor thing that came to my mind is that DEVTYPE and subsystem are
>> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
>> > USB than for Bluetooth subsystem for example. Not sure if we actually
>> > care or just add a udev_device_get_parent_with_subsystem_devtype()
>> > function to give applications a choice if they wanna care.
>>
>> You mean replacing:
>>   udev_device_get_parent_with_devtype(..., *devtype)
>> by:
>>   udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
>> ?
>>
>> Sounds sensible, because in most cases you don't want to check for
>> parents of a different subsystem. As you are using it, want to send a
>> patch?
>
> I am thinking of keeping both. So just adding ...subsystem_devtype() and
> keeping also the original one. My reason for it is that in some case you
> already know the subsystem you are looking at (or don't care in). So no
> point in doing a lookup with two checks.

We could make it accept NULL for the subsystem in that case?

Kay

Re: [PATCH] libudev: device - add devtype support

[David Zeuthen]

On Fri, 2009-01-02 at 09:22 +0100, Marcel Holtmann wrote:
> Hi Kay,
> 
> > > I was playing a little bit with libudev and I actually need the DEVTYPE
> > > from uevent for various tasks. Especially with USB and Bluetooth, the
> > > subsystem value is too generic.
> > >
> > > Attached is a patch that implements udev_device_get_devtype() and also
> > > udev_device_get_parent_with_devtype(). Please double check that I did it
> > > the right way.
> > 
> > Looks good. Applied.
> 
> one minor thing that came to my mind is that DEVTYPE and subsystem are
> actually kinda coupled. So a DEVTYPE="host" has a different semantic for
> USB than for Bluetooth subsystem for example. Not sure if we actually
> care or just add a udev_device_get_parent_with_subsystem_devtype()
> function to give applications a choice if they wanna care.

Isn't DEVTYPE in the environment already? If so, just use the new API
added here

http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;hð89350234e39b868a5e3df71a8f8c036aaae4fd

instead of Marcel's patch?

    David

Re: [PATCH] libudev: device - add devtype support

[Kay Sievers]

On Fri, Jan 2, 2009 at 17:17, David Zeuthen <david@fubar.dk> wrote:
> On Fri, 2009-01-02 at 09:22 +0100, Marcel Holtmann wrote:
>> > > I was playing a little bit with libudev and I actually need the DEVTYPE
>> > > from uevent for various tasks. Especially with USB and Bluetooth, the
>> > > subsystem value is too generic.
>> > >
>> > > Attached is a patch that implements udev_device_get_devtype() and also
>> > > udev_device_get_parent_with_devtype(). Please double check that I did it
>> > > the right way.
>> >
>> > Looks good. Applied.
>>
>> one minor thing that came to my mind is that DEVTYPE and subsystem are
>> actually kinda coupled. So a DEVTYPE="host" has a different semantic for
>> USB than for Bluetooth subsystem for example. Not sure if we actually
>> care or just add a udev_device_get_parent_with_subsystem_devtype()
>> function to give applications a choice if they wanna care.
>
> Isn't DEVTYPE in the environment already?

Yeah, it's a property, just like SUBSYSTEM is. But It's a "core
property", like a "subsystem subtype" which sounds fine to support in
the API.

> If so, just use the new API added here
>
> http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;hð89350234e39b868a5e3df71a8f8c036aaae4fd
>
> instead of Marcel's patch?

In many cases you just walk along the parent devices, and lookup a
specific device, without introducing dependencies on the specific
devpath position, like the "two directories up"-crap many tools do.
The enumeration API, which searches over all /sys devices does not
really fit here.

Kay

Re: [PATCH] libudev: device - add devtype support

[Marcel Holtmann]

Hi David,

> > > > I was playing a little bit with libudev and I actually need the DEVTYPE
> > > > from uevent for various tasks. Especially with USB and Bluetooth, the
> > > > subsystem value is too generic.
> > > >
> > > > Attached is a patch that implements udev_device_get_devtype() and also
> > > > udev_device_get_parent_with_devtype(). Please double check that I did it
> > > > the right way.
> > > 
> > > Looks good. Applied.
> > 
> > one minor thing that came to my mind is that DEVTYPE and subsystem are
> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
> > USB than for Bluetooth subsystem for example. Not sure if we actually
> > care or just add a udev_device_get_parent_with_subsystem_devtype()
> > function to give applications a choice if they wanna care.
> 
> Isn't DEVTYPE in the environment already? If so, just use the new API
> added here
> 
> http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;hð89350234e39b868a5e3df71a8f8c036aaae4fd
> 
> instead of Marcel's patch?

the use case here is different. You need udev_device_get_parent... ones
if you are receiving your device via a monitor. Using the enumeration
API only produces overhead.

The other main issue was that DEVTYPE has to be read from the uevent
sysfs file like MAJOR and MINOR in some cases.

An example where this is useful is for all the USB and SDIO composite
devices where you have to apply different policies if they share the
same parent. One example is a WiFi/WiMAX combo where the actual physical
radio is shared and thus only one can be active at a time. For the
kernel these two look like independent device, but in userspace we have
to apply certain policies. Otherwise nothing will work.

Also the problem with USB is that the parent is most times from DEVTYPE
usb_interface and that doesn't help. You have to go all up to usb_device
before you know that it is actually the same physical device.

Regards

Marcel

Re: [PATCH] libudev: device - add devtype support

[Kay Sievers]

On Fri, Jan 2, 2009 at 15:03, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Fri, Jan 2, 2009 at 14:46, Marcel Holtmann <marcel@holtmann.org> wrote:
>>> >> > I was playing a little bit with libudev and I actually need the DEVTYPE
>>> >> > from uevent for various tasks. Especially with USB and Bluetooth, the
>>> >> > subsystem value is too generic.
>>> >> >
>>> >> > Attached is a patch that implements udev_device_get_devtype() and also
>>> >> > udev_device_get_parent_with_devtype(). Please double check that I did it
>>> >> > the right way.
>>> >>
>>> >> Looks good. Applied.
>>> >
>>> > one minor thing that came to my mind is that DEVTYPE and subsystem are
>>> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
>>> > USB than for Bluetooth subsystem for example. Not sure if we actually
>>> > care or just add a udev_device_get_parent_with_subsystem_devtype()
>>> > function to give applications a choice if they wanna care.
>>>
>>> You mean replacing:
>>>   udev_device_get_parent_with_devtype(..., *devtype)
>>> by:
>>>   udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
>>> ?
>>>
>>> Sounds sensible, because in most cases you don't want to check for
>>> parents of a different subsystem. As you are using it, want to send a
>>> patch?
>>
>> I am thinking of keeping both. So just adding ...subsystem_devtype() and
>> keeping also the original one. My reason for it is that in some case you
>> already know the subsystem you are looking at (or don't care in). So no
>> point in doing a lookup with two checks.
>
> We could make it accept NULL for the subsystem in that case?

I guess, we should just have
udev_device_get_parent_with_subsystem_devtype(), and drop the both
other ones, and accept NULL as parameters, if you want to match only
one. You are right that subsystem and devtype belong together, so we
should probably reflect that in the API. Any problems with such
change?

Thanks,
Kay

Re: [PATCH] libudev: device - add devtype support

[Marcel Holtmann]

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

Hi Kay,

> >>> >> > I was playing a little bit with libudev and I actually need the DEVTYPE
> >>> >> > from uevent for various tasks. Especially with USB and Bluetooth, the
> >>> >> > subsystem value is too generic.
> >>> >> >
> >>> >> > Attached is a patch that implements udev_device_get_devtype() and also
> >>> >> > udev_device_get_parent_with_devtype(). Please double check that I did it
> >>> >> > the right way.
> >>> >>
> >>> >> Looks good. Applied.
> >>> >
> >>> > one minor thing that came to my mind is that DEVTYPE and subsystem are
> >>> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
> >>> > USB than for Bluetooth subsystem for example. Not sure if we actually
> >>> > care or just add a udev_device_get_parent_with_subsystem_devtype()
> >>> > function to give applications a choice if they wanna care.
> >>>
> >>> You mean replacing:
> >>>   udev_device_get_parent_with_devtype(..., *devtype)
> >>> by:
> >>>   udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
> >>> ?
> >>>
> >>> Sounds sensible, because in most cases you don't want to check for
> >>> parents of a different subsystem. As you are using it, want to send a
> >>> patch?
> >>
> >> I am thinking of keeping both. So just adding ...subsystem_devtype() and
> >> keeping also the original one. My reason for it is that in some case you
> >> already know the subsystem you are looking at (or don't care in). So no
> >> point in doing a lookup with two checks.
> >
> > We could make it accept NULL for the subsystem in that case?
> 
> I guess, we should just have
> udev_device_get_parent_with_subsystem_devtype(), and drop the both
> other ones, and accept NULL as parameters, if you want to match only
> one. You are right that subsystem and devtype belong together, so we
> should probably reflect that in the API. Any problems with such
> change?

I think that make sense. So I opted for allowing devtype being NULL
while subsystem is required. I tried to come up with a good reason why
you would look for devtype and not subsystem and couldn't find it right
now. The USB case is double information since they prefixed their
devtypes with "usb_", but that is really an USB subsystem problem. For
Bluetooth I am not doing this at all and neither does SCSI. In future no
subsystem should.

So patch 0001 renames ..with_devtype() into ..with_subsystem_devtype()
and then patch 0002 removes ..with_subsystem() and replaces its usage.

Regards

Marcel


[-- Attachment #2: 0001-libudev-device-lookup-subsystem-and-devtype-toget.patch --]
[-- Type: text/x-patch, Size: 3209 bytes --]

From 53eb85ecb5bff49449b79174734f7d32d85c5af6 Mon Sep 17 00:00:00 2001
From: Marcel Holtmann <marcel@holtmann.org>
Date: Sat, 3 Jan 2009 11:10:10 +0100
Subject: [PATCH] libudev: device - lookup subsystem and devtype together

---
 udev/lib/exported_symbols |    2 +-
 udev/lib/libudev-device.c |   17 +++++++++++++----
 udev/lib/libudev.h        |    2 +-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/udev/lib/exported_symbols b/udev/lib/exported_symbols
index 6b07842..88d5c48 100644
--- a/udev/lib/exported_symbols
+++ b/udev/lib/exported_symbols
@@ -17,7 +17,7 @@ udev_device_new_from_devnum
 udev_device_new_from_subsystem_sysname
 udev_device_get_parent
 udev_device_get_parent_with_subsystem
-udev_device_get_parent_with_devtype
+udev_device_get_parent_with_subsystem_devtype
 udev_device_ref
 udev_device_unref
 udev_device_get_udev
diff --git a/udev/lib/libudev-device.c b/udev/lib/libudev-device.c
index a25716d..7c80359 100644
--- a/udev/lib/libudev-device.c
+++ b/udev/lib/libudev-device.c
@@ -557,17 +557,26 @@ struct udev_device *udev_device_get_parent_with_subsystem(struct udev_device *ud
 	return parent;
 }
 
-struct udev_device *udev_device_get_parent_with_devtype(struct udev_device *udev_device, const char *devtype)
+struct udev_device *udev_device_get_parent_with_subsystem_devtype(struct udev_device *udev_device, const char *subsystem, const char *devtype)
 {
 	struct udev_device *parent;
 
+	if (subsystem == NULL)
+		return NULL;
+
 	parent = udev_device_get_parent(udev_device);
 	while (parent != NULL) {
+		const char *parent_subsystem;
 		const char *parent_devtype;
 
-		parent_devtype = udev_device_get_devtype(parent);
-		if (parent_devtype != NULL && strcmp(parent_devtype, devtype) == 0)
-			break;
+		parent_subsystem = udev_device_get_subsystem(parent);
+		if (parent_subsystem != NULL && strcmp(parent_subsystem, subsystem) == 0) {
+			if (devtype == NULL)
+				break;
+			parent_devtype = udev_device_get_devtype(parent);
+			if (parent_devtype != NULL && strcmp(parent_devtype, devtype) == 0)
+				break;
+		}
 		parent = udev_device_get_parent(parent);
 	}
 	return parent;
diff --git a/udev/lib/libudev.h b/udev/lib/libudev.h
index b96e494..bac362a 100644
--- a/udev/lib/libudev.h
+++ b/udev/lib/libudev.h
@@ -63,7 +63,7 @@ extern struct udev_device *udev_device_new_from_devnum(struct udev *udev, char t
 extern struct udev_device *udev_device_new_from_subsystem_sysname(struct udev *udev, const char *subsystem, const char *sysname);
 extern struct udev_device *udev_device_get_parent(struct udev_device *udev_device);
 extern struct udev_device *udev_device_get_parent_with_subsystem(struct udev_device *udev_device, const char *subsystem);
-extern struct udev_device *udev_device_get_parent_with_devtype(struct udev_device *udev_device, const char *devtype);
+extern struct udev_device *udev_device_get_parent_with_subsytem_devtype(struct udev_device *udev_device, const char *subsystem, const char *devtype);
 extern struct udev_device *udev_device_ref(struct udev_device *udev_device);
 extern void udev_device_unref(struct udev_device *udev_device);
 extern struct udev *udev_device_get_udev(struct udev_device *udev_device);
-- 
1.5.6.3


[-- Attachment #3: 0002-libudev-device-remove-udev_device_get_parent_with.patch --]
[-- Type: text/x-patch, Size: 4242 bytes --]

From 9ad3a8779559099a0b55c361b46a861a407786db Mon Sep 17 00:00:00 2001
From: Marcel Holtmann <marcel@holtmann.org>
Date: Sat, 3 Jan 2009 11:13:51 +0100
Subject: [PATCH] libudev: device - remove udev_device_get_parent_with_subsystem

---
 extras/usb_id/usb_id.c    |    6 +++---
 udev/lib/exported_symbols |    1 -
 udev/lib/libudev-device.c |   16 ----------------
 udev/lib/libudev.h        |    1 -
 4 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/extras/usb_id/usb_id.c b/extras/usb_id/usb_id.c
index 28352eb..9ee4e6e 100644
--- a/extras/usb_id/usb_id.c
+++ b/extras/usb_id/usb_id.c
@@ -192,7 +192,7 @@ static int usb_id(struct udev_device *dev)
 	dbg(udev, "syspath %s\n", udev_device_get_syspath(dev));
 
 	/* usb interface directory */
-	dev_interface = udev_device_get_parent_with_subsystem(dev, "usb");
+	dev_interface = udev_device_get_parent_with_subsystem_devtype(dev, "usb", NULL);
 	if (dev_interface == NULL) {
 		info(udev, "unable to access usb_interface device of '%s'\n",
 		     udev_device_get_syspath(dev));
@@ -218,7 +218,7 @@ static int usb_id(struct udev_device *dev)
 	     udev_device_get_syspath(dev_interface), if_class_num, protocol);
 
 	/* usb device directory */
-	dev_usb = udev_device_get_parent_with_subsystem(dev_interface, "usb");
+	dev_usb = udev_device_get_parent_with_subsystem_devtype(dev_interface, "usb", NULL);
 	if (!dev_usb) {
 		info(udev, "unable to find parent 'usb' device of '%s'\n",
 		     udev_device_get_syspath(dev));
@@ -232,7 +232,7 @@ static int usb_id(struct udev_device *dev)
 		int host, bus, target, lun;
 
 		/* get scsi device */
-		dev_scsi = udev_device_get_parent_with_subsystem(dev, "scsi");
+		dev_scsi = udev_device_get_parent_with_subsystem_devtype(dev, "scsi", NULL);
 		if (dev_scsi == NULL) {
 			info(udev, "unable to find parent 'scsi' device of '%s'\n",
 			     udev_device_get_syspath(dev));
diff --git a/udev/lib/exported_symbols b/udev/lib/exported_symbols
index 88d5c48..abf5051 100644
--- a/udev/lib/exported_symbols
+++ b/udev/lib/exported_symbols
@@ -16,7 +16,6 @@ udev_device_new_from_syspath
 udev_device_new_from_devnum
 udev_device_new_from_subsystem_sysname
 udev_device_get_parent
-udev_device_get_parent_with_subsystem
 udev_device_get_parent_with_subsystem_devtype
 udev_device_ref
 udev_device_unref
diff --git a/udev/lib/libudev-device.c b/udev/lib/libudev-device.c
index 7c80359..0cb5921 100644
--- a/udev/lib/libudev-device.c
+++ b/udev/lib/libudev-device.c
@@ -541,22 +541,6 @@ struct udev_device *udev_device_get_parent(struct udev_device *udev_device)
 	return udev_device->parent_device;
 }
 
-struct udev_device *udev_device_get_parent_with_subsystem(struct udev_device *udev_device, const char *subsystem)
-{
-	struct udev_device *parent;
-
-	parent = udev_device_get_parent(udev_device);
-	while (parent != NULL) {
-		const char *parent_subsystem;
-
-		parent_subsystem = udev_device_get_subsystem(parent);
-		if (parent_subsystem != NULL && strcmp(parent_subsystem, subsystem) == 0)
-			break;
-		parent = udev_device_get_parent(parent);
-	}
-	return parent;
-}
-
 struct udev_device *udev_device_get_parent_with_subsystem_devtype(struct udev_device *udev_device, const char *subsystem, const char *devtype)
 {
 	struct udev_device *parent;
diff --git a/udev/lib/libudev.h b/udev/lib/libudev.h
index bac362a..63b9b79 100644
--- a/udev/lib/libudev.h
+++ b/udev/lib/libudev.h
@@ -62,7 +62,6 @@ extern struct udev_device *udev_device_new_from_syspath(struct udev *udev, const
 extern struct udev_device *udev_device_new_from_devnum(struct udev *udev, char type, dev_t devnum);
 extern struct udev_device *udev_device_new_from_subsystem_sysname(struct udev *udev, const char *subsystem, const char *sysname);
 extern struct udev_device *udev_device_get_parent(struct udev_device *udev_device);
-extern struct udev_device *udev_device_get_parent_with_subsystem(struct udev_device *udev_device, const char *subsystem);
 extern struct udev_device *udev_device_get_parent_with_subsytem_devtype(struct udev_device *udev_device, const char *subsystem, const char *devtype);
 extern struct udev_device *udev_device_ref(struct udev_device *udev_device);
 extern void udev_device_unref(struct udev_device *udev_device);
-- 
1.5.6.3

Re: [PATCH] libudev: device - add devtype support

[Kay Sievers]

On Sat, Jan 3, 2009 at 11:19, Marcel Holtmann <marcel@holtmann.org> wrote:
>> >>> >> > I was playing a little bit with libudev and I actually need the DEVTYPE
>> >>> >> > from uevent for various tasks. Especially with USB and Bluetooth, the
>> >>> >> > subsystem value is too generic.
>> >>> >> >
>> >>> >> > Attached is a patch that implements udev_device_get_devtype() and also
>> >>> >> > udev_device_get_parent_with_devtype(). Please double check that I did it
>> >>> >> > the right way.
>> >>> >>
>> >>> >> Looks good. Applied.
>> >>> >
>> >>> > one minor thing that came to my mind is that DEVTYPE and subsystem are
>> >>> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
>> >>> > USB than for Bluetooth subsystem for example. Not sure if we actually
>> >>> > care or just add a udev_device_get_parent_with_subsystem_devtype()
>> >>> > function to give applications a choice if they wanna care.
>> >>>
>> >>> You mean replacing:
>> >>>   udev_device_get_parent_with_devtype(..., *devtype)
>> >>> by:
>> >>>   udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
>> >>> ?
>> >>>
>> >>> Sounds sensible, because in most cases you don't want to check for
>> >>> parents of a different subsystem. As you are using it, want to send a
>> >>> patch?
>> >>
>> >> I am thinking of keeping both. So just adding ...subsystem_devtype() and
>> >> keeping also the original one. My reason for it is that in some case you
>> >> already know the subsystem you are looking at (or don't care in). So no
>> >> point in doing a lookup with two checks.
>> >
>> > We could make it accept NULL for the subsystem in that case?
>>
>> I guess, we should just have
>> udev_device_get_parent_with_subsystem_devtype(), and drop the both
>> other ones, and accept NULL as parameters, if you want to match only
>> one. You are right that subsystem and devtype belong together, so we
>> should probably reflect that in the API. Any problems with such
>> change?
>
> I think that make sense. So I opted for allowing devtype being NULL
> while subsystem is required. I tried to come up with a good reason why
> you would look for devtype and not subsystem and couldn't find it right
> now. The USB case is double information since they prefixed their
> devtypes with "usb_", but that is really an USB subsystem problem. For
> Bluetooth I am not doing this at all and neither does SCSI. In future no
> subsystem should.

Yeah, sounds all fine. Patches are applied.

Thanks a lot,
Kay

Re: [PATCH] libudev: device - add devtype support

[Marcel Holtmann]

Hi Kay,

> >> >>> >> > I was playing a little bit with libudev and I actually need the DEVTYPE
> >> >>> >> > from uevent for various tasks. Especially with USB and Bluetooth, the
> >> >>> >> > subsystem value is too generic.
> >> >>> >> >
> >> >>> >> > Attached is a patch that implements udev_device_get_devtype() and also
> >> >>> >> > udev_device_get_parent_with_devtype(). Please double check that I did it
> >> >>> >> > the right way.
> >> >>> >>
> >> >>> >> Looks good. Applied.
> >> >>> >
> >> >>> > one minor thing that came to my mind is that DEVTYPE and subsystem are
> >> >>> > actually kinda coupled. So a DEVTYPE="host" has a different semantic for
> >> >>> > USB than for Bluetooth subsystem for example. Not sure if we actually
> >> >>> > care or just add a udev_device_get_parent_with_subsystem_devtype()
> >> >>> > function to give applications a choice if they wanna care.
> >> >>>
> >> >>> You mean replacing:
> >> >>>   udev_device_get_parent_with_devtype(..., *devtype)
> >> >>> by:
> >> >>>   udev_device_get_parent_with_subsystem_devtype(..., *subsystem,  *devtype)
> >> >>> ?
> >> >>>
> >> >>> Sounds sensible, because in most cases you don't want to check for
> >> >>> parents of a different subsystem. As you are using it, want to send a
> >> >>> patch?
> >> >>
> >> >> I am thinking of keeping both. So just adding ...subsystem_devtype() and
> >> >> keeping also the original one. My reason for it is that in some case you
> >> >> already know the subsystem you are looking at (or don't care in). So no
> >> >> point in doing a lookup with two checks.
> >> >
> >> > We could make it accept NULL for the subsystem in that case?
> >>
> >> I guess, we should just have
> >> udev_device_get_parent_with_subsystem_devtype(), and drop the both
> >> other ones, and accept NULL as parameters, if you want to match only
> >> one. You are right that subsystem and devtype belong together, so we
> >> should probably reflect that in the API. Any problems with such
> >> change?
> >
> > I think that make sense. So I opted for allowing devtype being NULL
> > while subsystem is required. I tried to come up with a good reason why
> > you would look for devtype and not subsystem and couldn't find it right
> > now. The USB case is double information since they prefixed their
> > devtypes with "usb_", but that is really an USB subsystem problem. For
> > Bluetooth I am not doing this at all and neither does SCSI. In future no
> > subsystem should.
> 
> Yeah, sounds all fine. Patches are applied.

you even fixed my typo. Just found that one a few minutes ago. Thanks :)

Regards

Marcel