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

[Marcel Holtmann <marcel@holtmann.org>]

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