linux-hotplug.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [udev] don't rely on field order in namedev_parse
@ 2003-12-15 22:36 Greg KH
  2003-12-16  0:21 ` Kay Sievers
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Greg KH @ 2003-12-15 22:36 UTC (permalink / raw)
  To: linux-hotplug

On Fri, Dec 12, 2003 at 08:36:03PM +0100, Kay Sievers wrote:
> 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? 

> diff -Nru a/namedev.h b/namedev.h
> --- a/namedev.h	Fri Dec 12 20:21:01 2003
> +++ b/namedev.h	Fri Dec 12 20:21:01 2003
> @@ -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_id\x1278&alloc_id371&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [udev] don't rely on field order in namedev_parse
@ 2003-12-12 19:36 Kay Sievers
  0 siblings, 0 replies; 10+ messages in thread
From: Kay Sievers @ 2003-12-12 19:36 UTC (permalink / raw)
  To: linux-hotplug

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

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?

thanks,
Kay

[-- Attachment #2: 01-remove-field-ordering.diff --]
[-- Type: text/plain, Size: 8741 bytes --]

diff -Nru a/namedev.h b/namedev.h
--- a/namedev.h	Fri Dec 12 20:21:01 2003
+++ b/namedev.h	Fri Dec 12 20:21:01 2003
@@ -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"
 #define CALLOUT_MAXARG	8
 
 struct config_device {
diff -Nru a/namedev_parse.c b/namedev_parse.c
--- a/namedev_parse.c	Fri Dec 12 20:21:01 2003
+++ b/namedev_parse.c	Fri Dec 12 20:21:01 2003
@@ -45,7 +45,7 @@
 		return -ENODEV;
 
 	/* eat any whitespace */
-	while (isspace(*string))
+	while (isspace(*string) || *string == ',')
 		++string;
 
 	/* split based on '=' */
@@ -71,19 +71,6 @@
 	return 0;
 }
 
-static int get_value(const char *left, char **orig_string, char **ret_string)
-{
-	int retval;
-	char *left_string;
-
-	retval = get_pair(orig_string, &left_string, ret_string);
-	if (retval)
-		return retval;
-	if (strcasecmp(left_string, left) != 0)
-		return -ENODEV;
-	return 0;
-}
-
 void dump_config_dev(struct config_device *dev)
 {
 	switch (dev->type) {
@@ -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;
@@ -184,199 +166,155 @@
 		if (*temp == COMMENT_CHARACTER)
 			continue;
 
+		/* eat the whitespace */
+		while (isspace(*temp))
+			++temp;
+
 		memset(&dev, 0x00, sizeof(struct config_device));
 
-		/* parse the line */
+		/* get the method */
 		temp2 = strsep(&temp, ",");
-		if (strcasecmp(temp2, TYPE_LABEL) == 0) {
-			/* label type */
+
+		if (strcasecmp(temp2, "LABEL") == 0) {
 			dev.type = LABEL;
+			goto keys;
+		}
 
-			/* BUS="bus" */
-			retval = get_value("BUS", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.bus, temp3);
+		if (strcasecmp(temp2, "NUMBER") == 0) {
+			dev.type = NUMBER;
+			goto keys;
+		}
+
+		if (strcasecmp(temp2, "TOPOLOGY") == 0) {
+			dev.type = TOPOLOGY;
+			goto keys;
+		}
+
+		if (strcasecmp(temp2, "REPLACE") == 0) {
+			dev.type = REPLACE;
+			goto keys;
+		}
 
-			/* file="value" */
-			temp2 = strsep(&temp, ",");
+		if (strcasecmp(temp2, "CALLOUT") == 0) {
+			dev.type = CALLOUT;
+			goto keys;
+		}
+
+		dbg_parse("unknown type of method '%s'", temp2);
+		goto error;
+keys:
+		/* get all known keys */
+		while (1) {
 			retval = get_pair(&temp, &temp2, &temp3);
 			if (retval)
 				break;
-			strfieldcpy(dev.sysfs_file, temp2);
-			strfieldcpy(dev.sysfs_value, temp3);
 
-			/* NAME="new_name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("NAME", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.name, temp3);
+			if (strcasecmp(temp2, "BUS") == 0) {
+				strfieldcpy(dev.bus, temp3);
+				continue;
+			}
+
+			if (strcasecmp(temp2, "ID") == 0) {
+				strfieldcpy(dev.id, temp3);
+				continue;
+			}
+
+			if (strcasecmp(temp2, "PLACE") == 0) {
+				strfieldcpy(dev.place, temp3);
+				continue;
+			}
+
+			if (strcasecmp(temp2, "KERNEL") == 0) {
+				strfieldcpy(dev.kernel_name, temp3);
+				continue;
+			}
+
+			if (strcasecmp(temp2, "PROGRAM") == 0) {
+				strfieldcpy(dev.exec_program, temp3);
+				continue;
+			}
+
+			if (strcasecmp(temp2, "NAME") == 0) {
+				strfieldcpy(dev.name, temp3);
+				continue;
+			}
 
-			/* SYMLINK="name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("SYMLINK", &temp, &temp3);
-			if (retval == 0)
+			if (strcasecmp(temp2, "SYMLINK") == 0) {
 				strfieldcpy(dev.symlink, temp3);
+				continue;
+			}
 
+			/* unknown key taken as sysfs attribute */
+			strfieldcpy(dev.sysfs_file, temp2);
+			strfieldcpy(dev.sysfs_value, temp3);
+		}
+
+		/* check presence of keys according to method type */
+		switch (dev.type) {
+		case LABEL:
 			dbg_parse("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);
-		}
-
-		if (strcasecmp(temp2, TYPE_NUMBER) == 0) {
-			/* number type */
-			dev.type = NUMBER;
-
-			/* BUS="bus" */
-			retval = get_value("BUS", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.bus, temp3);
-
-			/* ID="id" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("ID", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.id, temp3);
-
-			/* NAME="new_name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("NAME", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.name, temp3);
-
-			/* SYMLINK="name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("SYMLINK", &temp, &temp3);
-			if (retval == 0)
-				strfieldcpy(dev.symlink, temp3);
-
+			if ((*dev.name == '\0') ||
+			    (*dev.bus == '\0') ||
+			    (*dev.sysfs_file == '\0') ||
+			    (*dev.sysfs_value == '\0'))
+				goto error;
+			break;
+		case NUMBER:
 			dbg_parse("NUMBER name='%s', bus='%s', id='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.id, dev.symlink);
-		}
-
-		if (strcasecmp(temp2, TYPE_TOPOLOGY) == 0) {
-			/* number type */
-			dev.type = TOPOLOGY;
-
-			/* BUS="bus" */
-			retval = get_value("BUS", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.bus, temp3);
-
-			/* PLACE="place" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("PLACE", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.place, temp3);
-
-			/* NAME="new_name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("NAME", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.name, temp3);
-
-			/* SYMLINK="name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("SYMLINK", &temp, &temp3);
-			if (retval == 0)
-				strfieldcpy(dev.symlink, temp3);
-
+			if ((*dev.name == '\0') ||
+			    (*dev.bus == '\0') ||
+			    (*dev.id == '\0'))
+				goto error;
+			break;
+		case TOPOLOGY:
 			dbg_parse("TOPOLOGY name='%s', bus='%s', "
 				  "place='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.place, dev.symlink);
-		}
-
-		if (strcasecmp(temp2, TYPE_REPLACE) == 0) {
-			/* number type */
-			dev.type = REPLACE;
-
-			/* KERNEL="kernel_name" */
-			retval = get_value("KERNEL", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.kernel_name, temp3);
-
-			/* NAME="new_name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("NAME", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.name, temp3);
-
-			/* SYMLINK="name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("SYMLINK", &temp, &temp3);
-			if (retval == 0)
-				strfieldcpy(dev.symlink, temp3);
-
+			if ((*dev.name == '\0') ||
+			    (*dev.bus == '\0') ||
+			    (*dev.place == '\0'))
+				goto error;
+			break;
+		case REPLACE:
 			dbg_parse("REPLACE name='%s', kernel_name='%s', symlink='%s'",
 				  dev.name, dev.kernel_name, dev.symlink);
-		}
-
-		if (strcasecmp(temp2, TYPE_CALLOUT) == 0) {
-			/* number type */
-			dev.type = CALLOUT;
-
-			/* BUS="bus" */
-			retval = get_value("BUS", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.bus, temp3);
-
-			/* PROGRAM="executable" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("PROGRAM", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.exec_program, temp3);
-
-			/* ID="id" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("ID", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.id, temp3);
-
-			/* NAME="new_name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("NAME", &temp, &temp3);
-			if (retval)
-				break;
-			strfieldcpy(dev.name, temp3);
-
-			/* SYMLINK="name" */
-			temp2 = strsep(&temp, ",");
-			retval = get_value("SYMLINK", &temp, &temp3);
-			if (retval == 0)
-				strfieldcpy(dev.symlink, temp3);
-
+			if ((*dev.name == '\0') ||
+			    (*dev.kernel_name == '\0'))
+				goto error;
+			break;
+		case CALLOUT:
 			dbg_parse("CALLOUT name='%s', bus='%s', program='%s', "
 				  "id='%s', symlink='%s'",
 				  dev.name, dev.bus, dev.exec_program,
 				  dev.id, dev.symlink);
+			if ((*dev.name == '\0') ||
+			    (*dev.bus == '\0') ||
+			    (*dev.id == '\0') ||
+			    (*dev.exec_program == '\0'))
+				goto error;
+			break;
+		default:
+			dbg_parse("xxx default method");
+			goto error;
 		}
 
 		retval = add_config_dev(&dev);
 		if (retval) {
 			dbg("add_config_dev returned with error %d", retval);
-			goto exit;
+			continue;
 		}
 	}
-	dbg_parse("%s:%d:%Zd: error parsing '%s'", udev_rules_filename,
+error:
+	dbg_parse("%s:%d:%Zd: field missing or parse error", udev_rules_filename,
 		  lineno, temp - line, temp);
 exit:
 	fclose(fd);
 	return retval;
-}	
-
+}
 
 int namedev_init_permissions(void)
 {

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2003-12-17 18:26 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-12-15 22:36 [udev] don't rely on field order in namedev_parse Greg KH
2003-12-16  0:21 ` Kay Sievers
2003-12-16  0:27 ` Greg KH
2003-12-16  1:36 ` Kay Sievers
2003-12-16 23:40 ` Greg KH
2003-12-17  0:50 ` Kay Sievers
2003-12-17  0:59 ` Greg KH
2003-12-17 17:04 ` Roman Kagan
2003-12-17 18:26 ` Greg KH
  -- strict thread matches above, loose matches on Subject: below --
2003-12-12 19:36 Kay Sievers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).