Skip to contentSkip to conversation

don't rely on field order in namedev_parse

udev Applied Greg KH 3 participants 9 messages Dec 15, 2003 – Dec 17, 2003

lore
Share & tools
lore

Greg KH

author
#0 lore raw
Quoted 9 lines · Kay Sievers

Hi,
here is a patch for the namedev-parser not to rely on the order of the fields given in the rule. The error handling now is relaxed, if a field is given twice, the last one wins. If a key is unknown we use it as a sysfs attribute.

It is not as efficient as the current version,
cause we need to loop over the list of known fields for every given field, but we are free to sort the fields in udev.rules.
What do you think?

I like the idea, it really cleans up the code a lot. But it looks like it breaks the fact that we might want to test for the sysfs file "name" in a rule, right?

namedev.h line 51

Quoted 12 lines

diff -Nru a/namedev.h b/namedev.h

namedev.h +0 -5
@@ -44,11 +44,6 @@
 #define ID_SIZE		50
 #define PLACE_SIZE	50
 
-#define TYPE_LABEL	"LABEL"
-#define TYPE_NUMBER	"NUMBER"
-#define TYPE_TOPOLOGY	"TOPOLOGY"
-#define TYPE_REPLACE	"REPLACE"
-#define TYPE_CALLOUT	"CALLOUT"

Why remove these?

thanks,

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#1 lore raw
Quoted 6 lines · Greg KH
Quoted 5 lines · Kay Sievers

Hi,
here is a patch for the namedev-parser not to rely on the order of the fields given in the rule. The error handling now is relaxed, if a field is given twice, the last one wins. If a key is unknown we use it as a sysfs attribute.

Quoted 8 lines
Quoted 4 lines

It is not as efficient as the current version,
cause we need to loop over the list of known fields for every given field, but we are free to sort the fields in udev.rules.
What do you think?

I like the idea, it really cleans up the code a lot. But it looks like it breaks the fact that we might want to test for the sysfs file "name" in a rule, right?

Yes, that's not nice. Two options come to my mind:

o prepend SYSFS_ to the match, like:
LABEL, BUS="usb", SYSFS_model="Creative Labs WebCam*", NAME="video%n" This is my favorite. It's close to the current and says clearly what it does.

o switch to case sensitive field name match
and hope nobody puts uppercase 'ID', 'BUS', 'NAME' in the kernel It's more like a text adventure :)

What do you think?

Quoted 7 lines

Review

Quoted 5 lines
(quoted diff) +0 -5
-#define TYPE_LABEL	"LABEL"
-#define TYPE_NUMBER	"NUMBER"
-#define TYPE_TOPOLOGY	"TOPOLOGY"
-#define TYPE_REPLACE	"REPLACE"
-#define TYPE_CALLOUT	"CALLOUT"

Why remove these?

Oh I just wrote the code without it in mind and later simply removed it (being lazy). The field name strings are also embedded in the code,
we may use #define's for the names of the fields too, then I'm happy :)

thanks,
Kay

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

author
#2 lore raw
In reply to Greg KH
Quoted 5 lines · Kay Sievers

Yes, that's not nice. Two options come to my mind:

o prepend SYSFS_ to the match, like:
LABEL, BUS="usb", SYSFS_model="Creative Labs WebCam*", NAME="video%n" This is my favorite. It's close to the current and says clearly what it does.

Yeah, I like this one too.

Quoted 11 lines
Quoted 7 lines

Review

Quoted 5 lines
(quoted diff) +0 -5
-#define TYPE_LABEL	"LABEL"
-#define TYPE_NUMBER	"NUMBER"
-#define TYPE_TOPOLOGY	"TOPOLOGY"
-#define TYPE_REPLACE	"REPLACE"
-#define TYPE_CALLOUT	"CALLOUT"

Why remove these?

Oh I just wrote the code without it in mind and later simply removed it (being lazy). The field name strings are also embedded in the code,
we may use #define's for the names of the fields too, then I'm happy :)

Ok, that sounds good.

thanks,

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#3 lore raw
In reply to Greg KH
Quoted 6 lines · Greg KH
Quoted 3 lines · Kay Sievers

o prepend SYSFS_ to the match, like:
LABEL, BUS="usb", SYSFS_model="Creative Labs WebCam*", NAME="video%n" This is my favorite. It's close to the current and says clearly what it does.

Yeah, I like this one too.

Here is a try.
Please have a look at it.

My motto of the day: Try to attach the patch before hitting the 'y'-key :)

Kay

01-remove-field-ordering.diff
o change the parsing to get a key from the rule and sort it into our list of known keys instead of expecting a special order
o the key to match a sysfs file must be prependend by 'SYSFS_' now to match with the new parsing.
(The config must be changed, but it's a bit more descriptive too.)
o put names of fields in define's, like the name of the methods
o update all tests and the man page

namedev.h +10 -0
diff -Nru a/namedev.h b/namedev.h
@@ -49,6 +49,16 @@
 #define TYPE_TOPOLOGY	"TOPOLOGY"
 #define TYPE_REPLACE	"REPLACE"
 #define TYPE_CALLOUT	"CALLOUT"
+
+#define FIELD_BUS	"BUS"
+#define FIELD_ID	"ID"
+#define FIELD_SYSFS	"SYSFS_"
+#define FIELD_PLACE	"PLACE"
+#define FIELD_PROGRAM	"PROGRAM"
+#define FIELD_KERNEL	"KERNEL"
+#define FIELD_NAME	"NAME"
+#define FIELD_SYMLINK	"SYMLINK"
+
 #define CALLOUT_MAXARG	8
 
 struct config_device {
namedev_parse.c +113 -170

351 lines not on this page · raw message

test/label_test +1 -1
diff -Nru a/test/label_test b/test/label_test
@@ -8,7 +8,7 @@
 export UDEV_CONFIG_FILE=$PWD/$CONFIG
 
 cat > $RULES << EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="boot_disk%n"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="boot_disk%n"
 EOF
 
 cat > $CONFIG << EOF
test/udev-test.pl +11 -11
diff -Nru a/test/udev-test.pl b/test/udev-test.pl
@@ -38,7 +38,7 @@
 		devpath  => "block/sda",
 		expected => "boot_disk" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="boot_disk%n"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="boot_disk%n"
 REPLACE, KERNEL="ttyUSB0", NAME="visor"
 EOF
 	},

@@ -48,7 +48,7 @@
 		devpath  => "block/sda/sda1",
 		expected => "boot_disk1" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="boot_disk%n"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="boot_disk%n"
 EOF
 	},
 	{

@@ -57,10 +57,10 @@
 		devpath  => "block/sda/sda1",
 		expected => "boot_disk1" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="?IBM-ESXS", NAME="boot_disk%n-1"
-LABEL, BUS="scsi", vendor="IBM-ESXS?", NAME="boot_disk%n-2"
-LABEL, BUS="scsi", vendor="IBM-ES??", NAME="boot_disk%n"
-LABEL, BUS="scsi", vendor="IBM-ESXSS", NAME="boot_disk%n-3"
+LABEL, BUS="scsi", SYSFS_vendor="?IBM-ESXS", NAME="boot_disk%n-1"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS?", NAME="boot_disk%n-2"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ES??", NAME="boot_disk%n"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXSS", NAME="boot_disk%n-3"
 EOF
 	},
 	{

@@ -167,7 +167,7 @@
 		devpath  => "block/sda",
 		expected => "lun0/disc" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="lun0/%D"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="lun0/%D"
 EOF
 	},
 	{

@@ -176,7 +176,7 @@
 		devpath  => "block/sda/sda2",
 		expected => "lun0/part2" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="lun0/%D"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="lun0/%D"
 EOF
 	},
 	{

@@ -205,7 +205,7 @@
 		devpath  => "block/sda/sda2",
 		expected => "1/2/a/b/symlink" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="1/2/node", SYMLINK="1/2/a/b/symlink"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="1/2/node", SYMLINK="1/2/a/b/symlink"
 EOF
 	},
 	{

@@ -214,7 +214,7 @@
 		devpath  => "block/sda/sda2",
 		expected => "1/2/symlink" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="1/2/a/b/node", SYMLINK="1/2/symlink"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="1/2/a/b/node", SYMLINK="1/2/symlink"
 EOF
 	},
 	{

@@ -223,7 +223,7 @@
 		devpath  => "block/sda/sda2",
 		expected => "1/2/c/d/symlink" ,
 		conf     => <<EOF
-LABEL, BUS="scsi", vendor="IBM-ESXS", NAME="1/2/a/b/node", SYMLINK="1/2/c/d/symlink"
+LABEL, BUS="scsi", SYSFS_vendor="IBM-ESXS", NAME="1/2/a/b/node", SYMLINK="1/2/c/d/symlink"
 EOF
 	},
 	{
udev.8 +5 -6
diff -Nru a/udev.8 b/udev.8
@@ -18,7 +18,7 @@
 .B udev
 reads the sysfs directory of the given device to collect device attributes
 like label, serial number or bus device number.
-These attributes are treated as a key 
+These attributes are treated as a key
 to determine a unique name for device file creation.
 .B udev
 maintains a database for devices present on the system.

@@ -87,7 +87,7 @@
 .I /etc/udev/udev.rules
 or specified by the
 .I udev_rules
-value in the 
+value in the
 .I /etc/udev/udev.conf
 file.
 .P

@@ -114,8 +114,7 @@
 device label or serial number, like USB serial number, SCSI UUID or
 file system label
 .br
-.RB "keys: " BUS ", "
-.I sysfs_attribute
+.RB "keys: " BUS ", " SYSFS_
 .TP
 .B NUMBER
 device number on the bus, like PCI bus id

@@ -130,7 +129,7 @@
 .B REPLACE
 string replacement of the kernel device name
 .br
-.RB "key: " KERNEL_NAME
+.RB "key: " KERNEL
 .P
 The methods are applied in the following order:
 .BR CALLOUT ", " LABEL ", " NUMBER ", " TOPOLOGY ", " REPLACE "."

@@ -167,7 +166,7 @@
 CALLOUT, BUS="scsi", PROGRAM="/sbin/scsi_id", ID="OEM 0815", NAME="disk1"
 
 # USB printer to be called lp_color
-LABEL, BUS="usb", serial="W09090207101241330", NAME="lp_color"
+LABEL, BUS="usb", SYSFS_serial="W09090207101241330", NAME="lp_color"
 
 # sound card with PCI bus id 00:0b.0 to be called dsp
 NUMBER, BUS="pci", ID="00:0b.0", NAME="dsp"
udev.rules.demo +2 -2
diff -Nru a/udev.rules.demo b/udev.rules.demo
@@ -1,8 +1,8 @@
 # USB camera from Fuji to be named "camera"
-LABEL, BUS="usb", vendor="FUJIFILM", NAME="camera%n"
+LABEL, BUS="usb", SYSFS_vendor="FUJIFILM", NAME="camera%n"
 
 # USB device plugged into the fourth port of the second hub to be called gps_device
-TOPOLOGY, BUS="usb", place="2.4", NAME="gps_device"
+TOPOLOGY, BUS="usb", PLACE="2.4", NAME="gps_device"
 
 # ttyUSB1 should always be called visor
 REPLACE, KERNEL="ttyUSB1", NAME="visor"
author
#4 lore raw
In reply to Greg KH
Quoted 10 lines · Kay Sievers
Quoted 6 lines · Greg KH
Quoted 3 lines · Kay Sievers

o prepend SYSFS_ to the match, like:
LABEL, BUS="usb", SYSFS_model="Creative Labs WebCam*", NAME="video%n" This is my favorite. It's close to the current and says clearly what it does.

Yeah, I like this one too.

Here is a try.
Please have a look at it.

Looks great, it reduced the ammount of code by a bit, making things easier to read. Nice job.

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#5 lore raw
In reply to Greg KH
Quoted 7 lines · Greg KH
Quoted 6 lines · Kay Sievers
Quoted 5 lines · Greg KH
Quoted 3 lines · Kay Sievers

o prepend SYSFS_ to the match, like:
LABEL, BUS="usb", SYSFS_model="Creative Labs WebCam*", NAME="video%n" This is my favorite. It's close to the current and says clearly what it does.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Yeah, I like this one too.

Here is a try.
Please have a look at it.

Looks great, it reduced the ammount of code by a bit, making things easier to read. Nice job.

Fine, thanks.
Here is a small trivial cleanup of the recent changes.

Kay

02-trivial-cleanup-parser-changes.diff
o use defines in debug strings
o replace my 'xxx' debug :)
o shorten line in man page example to not to exceed 80 chars when printed

namedev_parse.c +6 -6
diff -Nru a/namedev_parse.c b/namedev_parse.c
@@ -257,7 +257,7 @@
 		/* check presence of keys according to method type */
 		switch (dev.type) {
 		case LABEL:
-			dbg_parse("LABEL name='%s', bus='%s', "
+			dbg_parse(TYPE_LABEL " name='%s', bus='%s', "
 				  "sysfs_file='%s', sysfs_value='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.sysfs_file,
 				  dev.sysfs_value, dev.symlink);

@@ -268,7 +268,7 @@
 				goto error;
 			break;
 		case NUMBER:
-			dbg_parse("NUMBER name='%s', bus='%s', id='%s', symlink='%s'",
+			dbg_parse(TYPE_NUMBER "name='%s', bus='%s', id='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.id, dev.symlink);
 			if ((*dev.name == '\0') ||
 			    (*dev.bus == '\0') ||

@@ -276,7 +276,7 @@
 				goto error;
 			break;
 		case TOPOLOGY:
-			dbg_parse("TOPOLOGY name='%s', bus='%s', "
+			dbg_parse(TYPE_TOPOLOGY "name='%s', bus='%s', "
 				  "place='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.place, dev.symlink);
 			if ((*dev.name == '\0') ||

@@ -285,14 +285,14 @@
 				goto error;
 			break;
 		case REPLACE:
-			dbg_parse("REPLACE name='%s', kernel_name='%s', symlink='%s'",
+			dbg_parse(TYPE_REPLACE "name='%s', kernel_name='%s', symlink='%s'",
 				  dev.name, dev.kernel_name, dev.symlink);
 			if ((*dev.name == '\0') ||
 			    (*dev.kernel_name == '\0'))
 				goto error;
 			break;
 		case CALLOUT:
-			dbg_parse("CALLOUT name='%s', bus='%s', program='%s', "
+			dbg_parse(TYPE_CALLOUT "name='%s', bus='%s', program='%s', "
 				  "id='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.exec_program,
 				  dev.id, dev.symlink);

@@ -303,7 +303,7 @@
 				goto error;
 			break;
 		default:
-			dbg_parse("xxx default method");
+			dbg_parse("unknown type of method");
 			goto error;
 		}
udev.8 +1 -1
diff -Nru a/udev.8 b/udev.8
@@ -178,7 +178,7 @@
 REPLACE, KERNEL="ttyUSB1", NAME="pda", SYMLINK="palmtop handheld"
 
 # multiple USB webcams with symlinks to be called webcam0, webcam1, ...
-LABEL, BUS="usb", SYSFS_model="WebCam V3", NAME="video%n", SYMLINK="webcam%n"
+LABEL, BUS="usb", SYSFS_model="XV3", NAME="video%n", SYMLINK="webcam%n"
 .fi
 .P
 Permissions and ownership for the created device files may specified at
author Applied
#6 lore raw
In reply to Greg KH
Quoted 3 lines · Kay Sievers

Fine, thanks.
Here is a small trivial cleanup of the recent changes.

Applied, thanks. I released 009 just a few minutes too early :(

thanks,

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

#7 lore raw
In reply to Greg KH

Hi,

Quoted 20 lines · Kay Sievers

01-remove-field-ordering.diff
[...]
diff -Nru a/namedev_parse.c b/namedev_parse.c

namedev_parse.c +0 -5
@@ -169,13 +156,8 @@
 		if (temp = NULL)
 			goto exit;
 		lineno++;
-
 		dbg_parse("read '%s'", temp);
 
-		/* eat the whitespace at the beginning of the line */
-		while (isspace(*temp))
-			++temp;
-
 		/* empty line? */
 		if (*temp = 0x00)
 			continue;

namedev_parse.c line 174

Quoted 9 lines
namedev_parse.c +4 -0
@@ -184,199 +166,160 @@
 		if (*temp = COMMENT_CHARACTER)
 			continue;
 
+		/* eat the whitespace */
+		while (isspace(*temp))
+			++temp;
+
 		memset(&dev, 0x00, sizeof(struct config_device));

This chunk broke parsing of blank lines and comments with blanks before '#'. Please revert it with the patch below.

Roman.

namedev_parse.c renamed from namedev_parse.c~ +4 -4
@@ -158,6 +158,10 @@
 		lineno++;
 		dbg_parse("read '%s'", temp);
 
+		/* eat the whitespace */
+		while (isspace(*temp))
+			++temp;
+
 		/* empty line? */
 		if (*temp = 0x00)
 			continue;

@@ -166,10 +170,6 @@
 		if (*temp = COMMENT_CHARACTER)
 			continue;
 
-		/* eat the whitespace */
-		while (isspace(*temp))
-			++temp;
-
 		memset(&dev, 0x00, sizeof(struct config_device));
 
 		/* get the method */

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

author
#8 lore raw
In reply to Greg KH
Quoted 3 lines · Roman Kagan

This chunk broke parsing of blank lines and comments with blanks before '#'. Please revert it with the patch below.

Thanks I've applied this. I've also added some tests to the udev-test.pl script to catch this kind of error in the future.

greg k-h

-------------------------------------------------------
This SF.net email is sponsored by: IBM Linux Tutorials. Become an expert in LINUX or just sharpen your skills. Sign up for IBM's Free Linux Tutorials. Learn everything from the bash shell to sys admin. Click now! http://ads.osdn.com/?ad_id78&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel