cmdlineparts.c

cmdlineparts.c

[Rainer Weikusat]

Hi.

I have rewritten cmdlineparts.c to no longer cause aligment traps on
ARM, to enable freeing of now-unsed cmdline_mtd_parition structs after
the real partitioning info has been passed to an upper layer and to
get rid of the recursion (using kmalloc instead has the [theoretical]
advantage that OOM problems can be dealt with gracefully instead of
causing stack overflows with possibly drastic consequences). Is there
any interest in this? [Mainly, I intend to use this for a project I am
currently working on].

Re: cmdlineparts.c

[Thomas Gleixner]

On Tue, 2004-07-06 at 08:53, Rainer Weikusat wrote:
> Hi.
> 
> I have rewritten cmdlineparts.c to no longer cause aligment traps on
> ARM, to enable freeing of now-unsed cmdline_mtd_parition structs after
> the real partitioning info has been passed to an upper layer and to
> get rid of the recursion (using kmalloc instead has the [theoretical]
> advantage that OOM problems can be dealt with gracefully instead of
> causing stack overflows with possibly drastic consequences). Is there
> any interest in this? [Mainly, I intend to use this for a project I am
> currently working on].

Sure, provide a patch :)

tglx

cmdlineparts.c [PATCH]

[Rainer Weikusat]

This is a (lengthy) patch that transforms cmdlinepart.c
as I have described. I've also added a header to stop gcc from
complaining about use of undeclared functions from physmap.c, but I do
not quite know what to do with it. And I don't really have an idea
about what to do with the 'author' and 'copyright' parts, either,
because I normally avoid putting my (or anybody elses' fwiw) name
anywhere.

I hope to a found (and squashed) all the really stupid bugs ...

--- cmdlinepart.c.orig	2004-07-07 16:57:46.000000000 +0800
+++ cmdlinepart.c	2004-07-07 16:04:59.000000000 +0800
@@ -3,7 +3,9 @@
  *
  * Read flash partition table from command line
  *
- * Copyright 2002 SYSGO Real-Time Solutions GmbH
+ * (c) 2004 SNC AG
+ *
+ * Portions copyright 2002 SYSGO Real-Time Solutions GmbH
  *
  * The format for the command line is as follows:
  * 
@@ -41,300 +43,334 @@
 #define dbg(x)
 #endif
 
-
-/* special size referring to all the remaining space in a partition */
+/*
+  special size referring to all the remaining space in a partition
+*/
 #define SIZE_REMAINING 0xffffffff
 
 struct cmdline_mtd_partition {
-	struct cmdline_mtd_partition *next;
+	struct cmdline_mtd_partition *next, *prev;
 	char *mtd_id;
-	int num_parts;
+	int n_parts;
 	struct mtd_partition *parts;
 };
 
+struct part_info {
+	struct part_info *p;
+	
+	char const *name;
+	u32 ofs, size;
+	unsigned name_len, mask;
+};
+
 /* mtdpart_setup() parses into here */
 static struct cmdline_mtd_partition *partitions;
 
 /* the command line passed to mtdpart_setupd() */
-static char *cmdline;
+static char const *cmdline;
 static int cmdline_parsed = 0;
 
+static int parse_part_spec(char const **s, struct part_info *info)
 /*
- * Parse one partition definition for an MTD. Since there can be many
- * comma separated partition definitions, this function calls itself 
- * recursively until no more partition definitions are found. Nice side
- * effect: the memory to keep the mtd_partition structs and the names
- * is allocated upon the last definition being found. At that point the
- * syntax has been verified ok.
- */
-static struct mtd_partition * newpart(char *s, 
-                                      char **retptr,
-                                      int *num_parts,
-                                      int this_part, 
-                                      unsigned char **extra_mem_ptr, 
-                                      int extra_mem_size)
+  Parse a single partition spec into *info and advance the
+  position in the definition string as appropriate. Return
+  0 if successful, otherwise -1.
+
+  NB: This parser is horribly broken in case of most
+  syntactical errors. OTOH, it's much smaller because
+  of that.
+*/
 {
-	struct mtd_partition *parts;
-	unsigned long size;
-	unsigned long offset = 0;
-	char *name;
-	int name_len;
-	unsigned char *extra_mem;
-	char delim;
-	unsigned int mask_flags;
-
-	/* fetch the partition size */
-	if (*s == '-')
-	{	/* assign all remaining space to this partition */
-		size = SIZE_REMAINING;
-		s++;
-	}
-	else
-	{
-		size = memparse(s, &s);
-		if (size < PAGE_SIZE)
-		{
-			printk(KERN_ERR ERRP "partition size too small (%lx)\n", size);
-			return 0;
-		}
-	}
+	char const *p;
 
-	/* fetch partition name and flags */
-	mask_flags = 0; /* this is going to be a regular partition */
-	delim = 0;
-        /* check for offset */
-        if (*s == '@') 
-	{
-                s++;
-                offset = memparse(s, &s);
-        }
-        /* now look for name */
-	if (*s == '(')
-	{
-		delim = ')';
-	}
-		
-	if (delim)
-	{
-		char *p;
-
-	    	name = ++s;
-		if ((p = strchr(name, delim)) == 0)
-		{
-			printk(KERN_ERR ERRP "no closing %c found in partition name\n", delim);
-			return 0;
-		}
-		name_len = p - name;
-		s = p + 1;
-	}
-	else
-	{
-	    	name = NULL;
-		name_len = 13; /* Partition_000 */
-	}
-   
-	/* record name length for memory allocation later */
-	extra_mem_size += name_len + 1;
-
-        /* test for options */
-        if (strncmp(s, "ro", 2) == 0) 
-	{
-		mask_flags |= MTD_WRITEABLE;
-		s += 2;
-        }
-
-	/* test if more partitions are following */
-	if (*s == ',')
-	{
-		if (size == SIZE_REMAINING)
-		{
-			printk(KERN_ERR ERRP "no partitions allowed after a fill-up partition\n");
-			return 0;
-		}
-		/* more partitions follow, parse them */
-		if ((parts = newpart(s + 1, &s, num_parts, 
-		                     this_part + 1, &extra_mem, extra_mem_size)) == 0)
-		  return 0;
-	}
-	else
-	{	/* this is the last partition: allocate space for all */
-		int alloc_size;
-
-		*num_parts = this_part + 1;
-		alloc_size = *num_parts * sizeof(struct mtd_partition) +
-			     extra_mem_size;
-		parts = kmalloc(alloc_size, GFP_KERNEL);
-		if (!parts)
-		{
-			printk(KERN_ERR ERRP "out of memory\n");
-			return 0;
+	/*
+	  size
+	*/
+	p = *s;
+	if (*p == '-') {
+	    info->size = SIZE_REMAINING;
+	    ++p;
+	} else {
+	    info->size = memparse((char *)p, (char **)&p);
+	    
+	    if (info->size < PAGE_SIZE) {
+		printk(KERN_ERR ERRP "size %x too small\n",
+		       (unsigned)info->size);
+		return -1;
+	    }
+	}
+
+	/*
+	  offset
+	*/
+	info->ofs = 0;
+	if (*p == '@') ++p, info->ofs = memparse((char *)p, (char **)&p);
+
+	/*
+	  name
+	*/
+	if (*p == '(') {
+		info->name = ++p ;
+		p = strchr(p, ')');
+		if (!p) {
+			printk(KERN_ERR ERRP
+			       "closing delimiter missing in partition name\n");
+			return -1;
 		}
-		memset(parts, 0, alloc_size);
-		extra_mem = (unsigned char *)(parts + *num_parts);
-	}
-	/* enter this partition (offset will be calculated later if it is zero at this point) */
-	parts[this_part].size = size;
-	parts[this_part].offset = offset;
-	parts[this_part].mask_flags = mask_flags;
-	if (name)
-	{
-		strncpy(extra_mem, name, name_len);
-		extra_mem[name_len] = 0;
-	}
-	else
-	{
-		sprintf(extra_mem, "Partition_%03d", this_part);
+
+		info->name_len = p++ - info->name;
+	} else {
+		info->name = "";
+		info->name_len = 0;
+	}
+
+	/*
+	  masked flags
+	*/
+	info->mask = 0;
+	if (*p == 'r' && p[1] == 'o') {
+		info->mask = MTD_WRITEABLE;
+		p += 2;
 	}
-	parts[this_part].name = extra_mem;
-	extra_mem += name_len + 1;
 
-	dbg(("partition %d: name <%s>, offset %x, size %x, mask flags %x\n",
-	     this_part, 
-	     parts[this_part].name,
-	     parts[this_part].offset,
-	     parts[this_part].size,
-	     parts[this_part].mask_flags));
-
-	/* return (updated) pointer to extra_mem memory */
-	if (extra_mem_ptr)
-	  *extra_mem_ptr = extra_mem;
+	*s = p;
+	return 0;
+}
+
+static struct mtd_partition *cleanup(struct part_info *p)
+{
+    struct part_info *pp;
 
-	/* return (updated) pointer command line string */
-	*retptr = s;
+    while ((pp = p)) {
+	p = p->p;
+	kfree(pp);
+    }
 
-	/* return partition table */
-	return parts;
+    return NULL;
 }
 
-/* 
- * Parse the command line. 
- */
-static int mtdpart_setup_real(char *s)
+static struct mtd_partition *parse_mtd_parts(char const **s, unsigned *n_parts)
+/*
+  Parse partition definitions associated with a single mtd id
+  and advance the position in the definition string as
+  appropriate. Return a pointer to an array of partition
+  definitions in case of success and NULL otherwise. If
+  successfull, set *n_parts to the number of partitions.
+*/
 {
-	cmdline_parsed = 1;
+	struct mtd_partition *parts, *p;
+	char *names;
+	struct part_info *first, *last, *cur;
+	unsigned l_names, n;
+	int rc;
+
+	/*
+	  Create a temporary list of partition information
+	  structures.
+	*/
+	parts = NULL;
+	first = last = NULL;
+	n = l_names = 0;
+	while (1) {
+		cur = kmalloc(sizeof(*cur), GFP_KERNEL);
+		if (!cur) {
+			printk(KERN_ERR ERRP
+			       "out of memory during mtdpart parsing\n");
 
-	for( ; s != NULL; )
-	{
-		struct cmdline_mtd_partition *this_mtd;
-		struct mtd_partition *parts;
-	    	int mtd_id_len;
-		int num_parts;
-		char *p, *mtd_id;
-
-	    	mtd_id = s;
-		/* fetch <mtd-id> */
-		if (!(p = strchr(s, ':')))
-		{
-			printk(KERN_ERR ERRP "no mtd-id\n");
-			return 0;
+			/*
+			  In theory, allocation failures are of
+			  temporary nature and retrying the operation
+			  later may prove successful. In practice, the
+			  calling code does not support this, but having
+			  the option doesn't hurt.
+			*/
+			cmdline_parsed = 0;
+			return cleanup(first);
 		}
-		mtd_id_len = p - mtd_id;
 
-		dbg(("parsing <%s>\n", p+1));
+		rc = parse_part_spec(s, cur);
+		if (rc == -1) return cleanup(first);
 
-		/* 
-		 * parse one mtd. have it reserve memory for the
-		 * struct cmdline_mtd_partition and the mtd-id string.
-		 */
-		parts = newpart(p + 1,		/* cmdline */
-				&s,		/* out: updated cmdline ptr */
-				&num_parts,	/* out: number of parts */
-				0,		/* first partition */
-				(unsigned char**)&this_mtd, /* out: extra mem */
-				mtd_id_len + 1 + sizeof(*this_mtd));
-		if(!parts)
-		{
-			/*
-			 * An error occurred. We're either:
-			 * a) out of memory, or
-			 * b) in the middle of the partition spec
-			 * Either way, this mtd is hosed and we're
-			 * unlikely to succeed in parsing any more
-			 */
-			 return 0;
-		 }
-
-		/* enter results */	    
-		this_mtd->parts = parts;
-		this_mtd->num_parts = num_parts;
-		this_mtd->mtd_id = (char*)(this_mtd + 1);
-		strncpy(this_mtd->mtd_id, mtd_id, mtd_id_len);
-		this_mtd->mtd_id[mtd_id_len] = 0;
-
-		/* link into chain */
-		this_mtd->next = partitions;	    	
-		partitions = this_mtd;
+		++n;
+		l_names += cur->name_len + 1;
+		if (last) last->p = cur;
+		else first = cur;
+		last = cur;
+		cur->p = NULL;
+
+		if (**s != ',') break;
+		++*s;
+	}
+
+	parts = p = kmalloc(n * sizeof(*p) + l_names, GFP_KERNEL);
+	if (!parts) {
+		printk(KERN_ERR ERRP
+		       "could not allocate partition definitions\n");
+		
+		cmdline_parsed = 0;
+		return cleanup(first);
+	}
+
+	/*
+	  Build the partition array to be returned.
+	*/
+	cur = first;
+	names = (char *)(p + n);
+	do {
+		memcpy(names, cur->name, cur->name_len);
+		names[cur->name_len] = 0;
+		p->name = names;
+		names += cur->name_len + 1;
+
+		p->size = cur->size;
+		p->offset = cur->ofs;
+		p->mask_flags = cur->mask;
+		p->mtdp = NULL;
+
+		first = cur->p;
+		kfree(cur);
+		++p;
+	} while ((cur = first));
+
+	*n_parts = n;
+	return parts;
+}
 
-		dbg(("mtdid=<%s> num_parts=<%d>\n", 
-		     this_mtd->mtd_id, this_mtd->num_parts));
+static void mtdpart_setup_real(void)
+/*
+  Parse commandline partition definitions. Build a list of
+  cmdline_mtd_partition structures, each completly describing
+  the partioning for a single flash device and store that
+  in 'partitions'. In case of errors, free unused allocated
+  space and return.
+*/
+{
+	struct cmdline_mtd_partition *cpart;
+	struct mtd_partition *parts;
+	char const *p, *mtd_id;
+	unsigned mtd_id_len, n;
+
+	cmdline_parsed = 1;
+	p = cmdline;
+	n = 0;
+	while (1) {
+		/*
+		  Parse partition defns.
+		*/
+		mtd_id = p;
+		p = strchr(p, ':');
+		if (!p || p == mtd_id) {
+			printk(KERN_ERR ERRP "no mtd id\n");
+			return;
+		}
+		mtd_id_len = p - mtd_id;
 		
+		++p;
+		parts = parse_mtd_parts(&p, &n);
+		if (!parts) return;
+
+		/*
+		  Set up description.
+		*/
+		cpart = kmalloc(sizeof(*cpart) + mtd_id_len + 1, GFP_KERNEL);
+		if (!cpart) {
+			printk(KERN_ERR ERRP
+			       "could not allocate commandline partition description\n");
+
+			cmdline_parsed = 0;
+			kfree(parts);
+			return;
+		}
+		cpart->mtd_id = (char *)(cpart + 1);
+		memcpy(cpart->mtd_id, mtd_id, mtd_id_len);
+		cpart->mtd_id[mtd_id_len] = 0;
+		cpart->parts = parts;
+		cpart->n_parts = n;
+
+		cpart->next = partitions;
+		if (partitions) partitions->prev = cpart;
+		cpart->prev = NULL;
+		partitions = cpart;
+
+		/*
+		  More work?
+		*/
+		switch (*p) {
+		default:
+			printk(KERN_ERR ERRP
+			       "garbage at end of partition defn (%c)\n", *p);
+			
+		case 0:
+			return;
 
-		/* EOS - we're done */
-		if (*s == 0)
+		case ';':
+			++p;
 			break;
-
-		/* does another spec follow? */
-		if (*s != ';')
-		{
-			printk(KERN_ERR ERRP "bad character after partition (%c)\n", *s);
-			return 0;
 		}
-		s++;
 	}
-	return 1;
 }
 
-/*
- * Main function to be called from the MTD mapping driver/device to
- * obtain the partitioning information. At this point the command line
- * arguments will actually be parsed and turned to struct mtd_partition
- * information. It returns partitions for the requested mtd device, or
- * the first one in the chain if a NULL mtd_id is passed in.
- */
 int parse_cmdline_partitions(struct mtd_info *master, 
                              struct mtd_partition **pparts,
                              const char *mtd_id)
+/*
+  Find partition list for mtd_id, if any. Return the number
+  of partitions found in case of success, and a pointer to
+  the partition array in *pparts. Return -EINVAL in case
+  there was none.
+*/
 {
-	unsigned long offset;
-	int i;
-	struct cmdline_mtd_partition *part;
-
-	if(!cmdline)
-		return -EINVAL;
-
-	/* parse command line */
-	if (!cmdline_parsed)
-		mtdpart_setup_real(cmdline);
-
-	for(part = partitions; part; part = part->next)
-	{
-		if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
-		{
-			for(i = 0, offset = 0; i < part->num_parts; i++)
-			{
-				if (!part->parts[i].offset)
-				  part->parts[i].offset = offset;
-				else
-				  offset = part->parts[i].offset;
-				if (part->parts[i].size == SIZE_REMAINING)
-				  part->parts[i].size = master->size - offset;
-				if (offset + part->parts[i].size > master->size)
-				{
-					printk(KERN_WARNING ERRP 
-					       "%s: partitioning exceeds flash size, truncating\n",
-					       part->mtd_id);
-					part->parts[i].size = master->size - offset;
-					part->num_parts = i;
-				}
-				offset += part->parts[i].size;
-			}
-			*pparts = part->parts;
-			return part->num_parts;
-		}
-	}
-	return -EINVAL;
-}
+	struct cmdline_mtd_partition *p;
+	struct mtd_partition *parts;
+	u32 ofs;
+	unsigned n;
+
+	/*
+	  Parse command line if not already done and find
+	  the list of partitions asked for.
+	*/
+	if (!cmdline) return -EINVAL;
+	if (!cmdline_parsed) mtdpart_setup_real();
+
+	p = partitions;
+	while (p && strcmp(p->mtd_id, mtd_id) != 0) p = p->next;
+	if (!p) return -EINVAL;
+	*pparts = parts = p->parts;
+
+	/*
+	  Fixup offsets and sizes.
+	*/
+	n = p->n_parts;
+	ofs = 0;
+	do {
+	    if (!parts->offset) parts->offset = ofs;
+	    else ofs = parts->offset;
+
+	    if (parts->size == SIZE_REMAINING) break;
+	    if (ofs + parts->size > master->size) {
+		printk(KERN_WARNING ERRP
+		       "%s: partitioning exceeds flash size, truncating\n",
+		       mtd_id);
+
+		break;
+	    }
+
+	    ofs += parts->size;
+	    ++parts;
+	} while (--n);
+	if (n) parts->size = master->size - ofs;
+
+	/*
+	  Free now unused list head.
+	*/
+	n = p->n_parts;
+	if (p->prev) p->prev->next = p->next;
+	else partitions = p->next;
+	if (p->next) p->next->prev = p->prev;
+	kfree(p);
 
+	return n;
+}
 
 /* 
  * This is the handler for our kernel parameter, called from 

Re: cmdlineparts.c [PATCH]

[Marius Groeger]

On Wed, 7 Jul 2004, Rainer Weikusat wrote:

> not quite know what to do with it. And I don't really have an idea
> about what to do with the 'author' and 'copyright' parts, either,

You should claim the MODULE_AUTHOR() tag for yourself. OTOH, it is
good practice to just leave the existing copyright notices in the head
and add yours to the list. Is it OK for you to use this hunk?

--- cmdlinepart.c.org   2004-07-07 16:48:53.000000000 +0200
+++ cmdlinepart.c       2004-07-07 16:49:50.000000000 +0200
@@ -3,7 +3,8 @@
  *
  * Read flash partition table from command line
  *
- * Copyright 2002 SYSGO Real-Time Solutions GmbH
+ * Copyright 2002 SYSGO AG
+ * Copyright 2004 SNC AG
  *
  * The format for the command line is as follows:
  *

Regards,
Marius

-- 
Marius Groeger <mgroeger@sysgo.com>           Project Manager
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.imerva.com

Re: cmdlineparts.c [PATCH]

[Rainer Weikusat]

Marius Groeger <mgroeger@sysgo.com> writes:
> On Wed, 7 Jul 2004, Rainer Weikusat wrote:
>> And I don't really have an idea about what to do with the 'author'
>> and 'copyright' parts, either,
>
> You should claim the MODULE_AUTHOR() tag for yourself. OTOH, it is
> good practice to just leave the existing copyright notices in the head
> and add yours to the list. Is it OK for you to use this hunk?
>
> --- cmdlinepart.c.org   2004-07-07 16:48:53.000000000 +0200
> +++ cmdlinepart.c       2004-07-07 16:49:50.000000000 +0200
> @@ -3,7 +3,8 @@
>   *
>   * Read flash partition table from command line
>   *
> - * Copyright 2002 SYSGO Real-Time Solutions GmbH
> + * Copyright 2002 SYSGO AG
> + * Copyright 2004 SNC AG
>   *
>   * The format for the command line is as follows:
>   *
>

Certainly.