Re: [PATCH 5/7] dlm: device interface

[Greg KH <greg@kroah.com>]

On Mon, Apr 25, 2005 at 11:13:03PM +0800, David Teigland wrote:
> +#ifndef __KERNEL
> +#define __user
> +#endif

What is this for?

> +/* struct passed to the lock write */
> +struct dlm_lock_params {
> +	uint8_t mode;
> +	uint16_t flags;
> +	uint32_t lkid;
> +	uint32_t parent;

These are crossing the userspace/kernelspace boundry, please mark them
__u8, __u16, and __u32.

> +	struct dlm_range range;
> +	uint8_t namelen;
> +        void __user *castparam;
> +	void __user *castaddr;
> +	void __user *bastparam;
> +        void __user *bastaddr;
> +	struct dlm_lksb __user *lksb;
> +	char lvb[DLM_LVB_LEN];
> +	char name[1];

Mix of tabs and spaces.

> +/*
> + * ioctls to create/remove lockspaces, and check how many
> + * outstanding ASTs there are against a particular LS.
> + */

Your comment is wrong, based on the code.

> +static int dlm_ioctl(struct inode *inode, struct file *file,
> +		     uint command, ulong u)
> +{
> +	struct file_info *fi = file->private_data;
> +	int status = -EINVAL;
> +	int count;
> +	struct list_head *tmp_list;
> +
> +	switch (command) {
> +
> +		/* Are there any ASTs for us to read?
> +		 * Warning, this returns the number of messages (ASTs)
> +		 * in the queue, NOT the number of bytes to read
> +		 */
> +	case FIONREAD:
> +		count = 0;
> +		spin_lock(&fi->fi_ast_lock);
> +		list_for_each(tmp_list, &fi->fi_ast_list)
> +			count++;
> +		spin_unlock(&fi->fi_ast_lock);
> +		status = put_user(count, (int *)u);
> +		break;

Is this really needed?

thanks,

greg k-h