svn commit: r40167 - head/en_US.ISO8859-1/books/arch-handbook/driverbasics

[John Baldwin]

On Tuesday, November 27, 2012 10:30:40 am Eitan Adler wrote:
> Author: eadler
> Date: Tue Nov 27 15:30:40 2012
> New Revision: 40167
> URL: http://svnweb.freebsd.org/changeset/doc/40167
> 
> Log:
>   Modernize the example echo module and fix some style(9) issues in the
>   code.
>   
>   Reviewed by:	alfred (code)
>   Reviewed by:	gonzo (code)
>   Reviewed by:	ehaupt (code)
>   Approved by:	bcr (mentor)
> 
> Modified:
>   head/en_US.ISO8859-1/books/arch-handbook/driverbasics/chapter.xml

A few notes:

In echo_read():

	amt = MIN(uio->uio_resid, echomsg->len - uio->uio_offset);

You should probably make 'amt' a ssize_t since uio_resid is now a ssize_t.  
Also, I'm not sure it is safe to remove the check for uio_offset being > 
echmosg->len.  On 32-bit platforms, uio_offset is an off_t (64-bit), but 'len' 
is an int and you are assigning it to an int (or ssize_t).  I think you can 
get a uio_offset such that you overflow and when you truncate to 32-bits you 
end up with a positive value for 'echomsg->len - uio->uio_offset'.  This 
coupled with a uio_resid larger than 'echomsg->len' (easy to do) can result in 
a buffer overrun.  I would probably write this as:

	amt = MIN(uio->uio_resid, uio->uio_offset >= echomsg->len ? 0 :
	    echomsg->len - uio->uio_offset);

Rather than the previous version that tested the result of the subtraction.

You should also not adjust uio_offset manually.  uiomove() already does that.

In echo_write():

The duplicate 'Copy the string in ...' comment at the top of this function 
should be removed.

It's not clear to me why you forbade random access?  Though it is fine if you 
want to do that.

style(9) says single-line comments shouldn't span multiple lines, hence:

	/*
	 * This is new message, reset length
	 */

should be:

	/* This is a new message, reset length. */

IIRC, It is a null character, not a NULL character (NULL is only for 
pointers).  Also, I think that should be in an else, so putting all that 
together:

	/*
	 * For new messages, reset the length.  If appending, start
	 * at the existing null terminator.
	 */
	if (uio->uio_offset == 0)
		echomsg->len = 0;
	else
		echomsg->len--;

Again, do not adjust uio_offset manually, you just did it twice.  In fact, 
this will do the wrong thing if you get an EFAULT error.  The right thing to 
do to handle partial copies, etc. is this:

	error = uiomove(...);

	/* Now we need to null terminate, then record the length. */
	echomsg->len = uio->uio_offset + 1;
	echomsg->msg[uio->uio_offset] = '\0';

Other pre-existing nits:

This driver shouldn't reference 'kmalloc' in a comment, and that entire 
comment can probably just be removed.

The DEV_MODULE() macro should have spaces after the comma operator.

-- 
John Baldwin