Re: Preparing btier for kernel inclusion

[Dan Carpenter <dan.carpenter@oracle.com>]

This is staging quality code.  (Not very good).  The staging tree is
closed for the 3.9 window so it couldn't make it in until 3.10.

*) Clean up the indenting.
*) Run checkpatch.pl over this and fix the warnings.
*) Don't include .c files into other .c files.
*) Get rid of homemade print macros like TIERERR().  Use dev_err()
   or pr_err().
*) Delete all compat code with older kernels like
	#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,0,0)
*) Change Yoda code to the if (NULL = odinfo) to normal format
	if (!odinfo)
		return -ENOMEM;
   Or "if (NULL != dev->backdev[0]->blocklist)" should be:
	if (dev->backdev[0]->blocklist)
		memcpy( ...
*) Run Sparse and Smatch over the code.  This should complain about
   some poor error handling.  For example, printing "no memory"
   followed by a dereference instead of actual error handling.  Or
   in the ioctl() it returns with the lock held.

*) Make magic numbers a define.
	/* Allow max 24 devices to be configured */ 
	devicenames = kmalloc(sizeof(char) * 26, GFP_KERNEL);

	for (count = 0; count <= 25; count++) {
		devicenames[count] = 97 + count;
	}

   Where does the 97 come from?  Also it's hella messy to use
   24, 25, and 26!  The for loop should be written in the normal
   way:

	#define MAX_BTEIR_DEVS 26

	for (i = 0; i < MAX_BTEIR_DEVS; i++) {

   Btw, use "i" instead of "count" for the iterator.  Use "count"
   for counting.

   Some of the magic numbers are just wrong:
	res = snprintf(buf, 1023, "%s\n", msg);
   1023 should have been PAGE_SIZE.  If the size argument to
   snprintf() is non-zero then snprintf() adds a NUL terminator.
   No one ever creates a 1023 char buffer.  Also the return value
   is the number of bytes that would have been printed if there
   were enough space (not counting the NUL terminator).  Consider
   using scnprintf().

*) Use normal kernel style comments.
	/* single line comment */

	/*
	 * Multi line
	 * comment.
	 */

*) Some of functions could use more comments.  What does
   allocated_on_device() return?  I would have assumed from the name
   that it returns a bool, but actually it returns a u64.

*) It scares me that when list_for_each_safe() is used
   unnecessarily.  A lot of people assume it has to do with locking
   but it doesn't.  It's for when you remove a list item.  This is
   wrong:

	list_for_each_safe(pos, q, &device_list) {
		count++;
	}

*) Put a blank line between declarations and code.

*) Use temp variables to make lines shorter:

-	dev->backdev[count]->bitlistsize -		dev->backdev[count]->devmagic->bitlistsize;

+	back = dev->backdev[count];
+	back->bitlistsize = back->devmagic->bitlistsize;

*) Never return -1 as a error code.  Return proper error codes at
   every level.

*) The TIER_DEREGISTER ioctl takes a kernel pointer from user space
   which is a bug.  It should be doing copy_from_user().

There are a lot of other messy things about this code, but that
should be enough to get started.

My advice is that people will take you a lot more seriously if you
clean it up and make a good first impression.  The block layer
people are crotchety.

Also, when you send patches for review, send it as a patch which can
be reviewed without leaving the email client.  That way we can put
comments inline.

regards,
dan carpenter