Re: [PATCH] EVMS core 2/4: evms.h

[Christoph Hellwig <hch@infradead.org>]

> diff -Naur linux-2.5.40/include/linux/evms/evms.h linux-2.5.40-evms/include/linux/evms/evms.h
> --- linux-2.5.40/include/linux/evms/evms.h	Sun Jul 17 18:46:18 1994
> +++ linux-2.5.40-evms/include/linux/evms/evms.h	Tue Oct  1 15:30:14 2002

What about drivers/md?

> @@ -0,0 +1,613 @@
> +/* -*- linux-c -*- */
> +/*
> + *   Copyright (c) International Business Machines  Corp., 2000

Has this file _really_ not been touched for two years??

> +/*
> + * linux/include/linux/evms/evms.h
> + *
> + * EVMS kernel header file
> + *
> + */

This comment sais exactly nothing.  You aswell just remove it..

> +
> +#ifndef __EVMS_INCLUDED__
> +#define __EVMS_INCLUDED__
> +
> +#include <linux/blk.h>
> +#include <linux/genhd.h>
> +#include <linux/fs.h>
> +#include <linux/iobuf.h>

I can't see the need for this include anywhere in the file.  Please check
whether you need all these includes.
> +
> +/**
> + * general defines section
> + **/
> +#define FALSE                           0
> +#define TRUE                            1

Please just use 0/1 directly just like everyone else..

> +#define DEV_PATH			"/dev"
> +#define EVMS_DIR_NAME			"evms"
> +#define EVMS_DEV_NAME			"block_device"
> +#define EVMS_DEV_NODE_PATH		DEV_PATH "/" EVMS_DIR_NAME "/"
> +#define EVMS_DEVICE_NAME		DEV_PATH "/" EVMS_DIR_NAME "/" EVMS_DEV_NAME

The kernel doesn't know about device names at all.

> +
> +/**
> + * list_for_each_entry_safe -	iterate over list safe against removal of list entry
> + * @pos:        the type * to use as a loop counter.
> + * @n: 	       	another type * to use as temporary storage
> + * @head:       the head for your list.
> + * @member:     the name of the list_struct within the struct.
> + */
> +#define list_for_each_entry_safe(pos, n, head, member)                      \
> +        for (pos = list_entry((head)->next, typeof(*pos), member),          \
> +		    n = list_entry(pos->member.next, typeof(*pos), member); \
> +		&pos->member != (head); 				    \
> +		pos = n,                                                    \
> +		    n = list_entry(pos->member.next, typeof(*pos), member))
> +/**
> + * list_member - tests whether a list member is currently on a list
> + * @member: 	member to evaulate
> + */
> +static inline int
> +list_member(struct list_head *member)
> +{
> +	return ((!member->next || !member->prev) ? FALSE : TRUE);
> +}

This should go into list.h

> +
> +/**
> + * kernel logging levels defines 
> + **/
> +#define EVMS_INFO_CRITICAL              0
> +#define EVMS_INFO_SERIOUS               1
> +#define EVMS_INFO_ERROR                 2
> +#define EVMS_INFO_WARNING               3
> +#define EVMS_INFO_DEFAULT               5
> +#define EVMS_INFO_DETAILS               6
> +#define EVMS_INFO_DEBUG                 7
> +#define EVMS_INFO_EXTRA                 8
> +#define EVMS_INFO_ENTRY_EXIT            9
> +#define EVMS_INFO_EVERYTHING            10

What about just using normal logging levels?

> +/**
> + * helpful PROCFS macro
> + **/
> +#ifdef CONFIG_PROC_FS
> +#define PROCPRINT(msg, args...) (sz += sprintf(page + sz, msg, ## args));\
> +			if (sz < off)\
> +				off -= sz, sz = 0;\
> +			else if (sz >= off + count)\
> +				goto out
> +#endif

I think this really wants to be converted to the seq_file interface..

> +
> +/**
> + * PluginID convenience macros
> + *
> + * An EVMS PluginID is a 32-bit number with the following bit positions:
> + * Top 16 bits: OEM identifier. See IBM_OEM_ID.
> + * Next 4 bits: Plugin type identifier. See evms_plugin_code.
> + * Lowest 12 bits: Individual plugin identifier within a given plugin type.
> + **/
> +#define SetPluginID(oem, type, id) ((oem << 16) | (type << 12) | id)
> +#define GetPluginOEM(pluginid) (pluginid >> 16)
> +#define GetPluginType(pluginid) ((pluginid >> 12) & 0xf)
> +#define GetPluginID(pluginid) (pluginid & 0xfff)

What is the prupose of OEM IDs?

> +struct evms_plugin_header {
> +	u32 id;
> +	struct evms_version version;
> +	struct evms_version required_services_version;
> +	struct evms_plugin_fops *fops;
> +	struct list_head headers;
> +};

What is the required services version?

> +struct evms_plugin_fops {

What about evms_plugin_ops?

> +	int (*ioctl) (struct evms_logical_node *, struct inode *,
> +		      struct file *, u32, unsigned long);
> +	int (*direct_ioctl) (struct inode *, struct file *,
> +			     u32, unsigned long);

Umm, why do you need two ioctl handlers?

> +/**
> + * convenience macros to use plugin's fops entry points  
> + **/
> +#define DISCOVER(node, list) ((plugin)->fops->discover(list))
> +#define END_DISCOVER(node, list) ((plugin)->fops->end_discover(list))
> +#define DELETE(node) ((node)->plugin->fops->delete(node))
> +#define SUBMIT_IO(node, bio) ((node)->plugin->fops->submit_io(node, bio))
> +#define INIT_IO(node, rw_flag, start_sec, num_secs, buf_addr) ((node)->plugin->fops->init_io(node, rw_flag, start_sec, num_secs, buf_addr))
> +#define IOCTL(node, inode, file, cmd, arg)    ((node)->plugin->fops->ioctl(node, inode, file, cmd, arg))
> +#define DIRECT_IOCTL(plugin, inode, file, cmd, arg)   ((plugin)->fops->direct_ioctl(inode, file, cmd, arg))

Do you really need those wrapper?  They just obsfucate the code

> +/**
> + * struct evms_pool_mgmt - anchor block for private pool management
> + * @cachep: 		kmem_cache_t variable
> + * @member_size: 	size of each element in the pool
> + * @head:
> + * @waiters: 		count of waiters
> + * @wait_queue: 	list of waiters
> + * @name: 		name of the pool (must be less than 20 chars)
> + *
> + * anchor block for private pool management
> + **/
> +struct evms_pool_mgmt {
> +	kmem_cache_t *cachep;
> +	int member_size;
> +	void *head;
> +	atomic_t waiters;
> +	wait_queue_head_t wait_queue;
> +	u8 *name;
> +};

What's the pruipose of this?  Is it really evms-specific?

> +
> +/*
> + * Notes:  
> + *	All of the following kernel thread functions belong to EVMS base.
> + *	These functions were copied from md_core.c
> + */

What about moving them to the core kernel code so everyone
can use them?

> +/* Have to include this at the end, since it depends
> + * on structures and definitions in this file.
> + */
> +#include <linux/evms/evms_ioctl.h>

Just remove this and make the individual sources include it