Re: mtd/drivers/mtd rfd_ftl.c,1.1,1.2

Re: mtd/drivers/mtd rfd_ftl.c,1.1,1.2

[Jörn Engel]

On Thu, 23 June 2005 15:50:15 +0100, sean@infradead.org wrote:
> +
> +	for (i=0; i<SECTOR_SIZE; i++) {
> +		if (buf[i])
> +			break;
> +	}
> +
> +	if (i < SECTOR_SIZE) {
> +		rc = do_writesect(dev, sector, buf, &old_addr);
> +		if (rc)
> +			goto err;
> +	}
> +	else 
> +		part->sector_map[sector] = -1;

I don't like this part too much.  Effectively, you're using "i" as a
state variable.  In this specific case, it's not too bad, but my
experience with state variable is that they always hurt.

Maybe something like this:

+	for (i=0; i<SECTOR_SIZE; i++) {
+		if (!buf[i])
+			continue;
+		rc = do_writesect(dev, sector, buf, &old_addr);
+		if (rc)
+			goto err;
+	}
+
+	if (i == SECTOR_SIZE)
+		part->sector_map[sector] = -1;

Or even:

+	for (i=0; i<SECTOR_SIZE; i++) {
+		if (!buf[i])
+			continue;
+		rc = do_writesect(dev, sector, buf, &old_addr);
+		if (rc)
+			goto err;
+		goto hit;
+	}
+
+	/* no hit */
+	part->sector_map[sector] = -1;
+hit:

Now we have one additional goto and your cs professor would fail you
in class, but code flow and data is nicely seperated.

Jörn

-- 
Mac is for working, 
Linux is for Networking, 
Windows is for Solitaire! 
-- stolen from dc