Networking headers in volume managment code?
> +/*
> + * string used when validating & logging redundant metadata
> + */
> +u8 *evms_primary_string = "primary";
> +EXPORT_SYMBOL(evms_primary_string);
> +u8 *evms_secondary_string = "secondary";
> +EXPORT_SYMBOL(evms_secondary_string);
Why do you export strings? Wouldn't a simple cpp symbol do it?
Also the symbol names are bigger than the actual string they
represent.. Looks a little pointless :)
> +/**
> + * SYSCTL - EVMS folder definitions/variables
> + **/
> +#ifdef CONFIG_PROC_FS
Needs to be checked for CONFIG_SYSCTL instead.
> +/**********************************************************/
> +/* START -- arch ioctl32 support */
> +/**********************************************************/
IMHO this is the wrong place. What about an conditionally compiled
evms_ioctl32.c file?
> +/**
> + * find_next_volume - locates first or next logical volume
> + * @lv: current logical volume
> + *
> + * returns the next logical volume or NULL
> + **/
All user of this look like they better used list_for_each?
> +
> +/**
> + * find_next_volume_safe - locates first or next logical volume (safe for removes)
> + * @next_lv: ptr to next logical volume
> + *
> + * returns the next logical volume or NULL
> + **/
Dito with list_for_each_safe
> +/**
> + * lookup_volume - finds a logical volume by minor number
> + * @minor: minor number of logical volume to be found
> + *
> + * returns the logical volume of the specified minor or NULL.
> + **/
> +static struct evms_logical_volume *
> +lookup_volume(int minor)
Very bad if you ever want to be able to use more than one major
number.
> +/**********************************************************/
> +/* START -- exported functions/Common Services */
> +/**********************************************************/
> +
> +/**
> + * evms_cs_get_version - returns the current EVMS version
> + * @major: returned major value
> + * @minor: returned minor value
> + **/
> +void
> +evms_cs_get_version(int *major, int *minor)
> +{
> + *major = EVMS_MAJOR_VERSION;
> + *minor = EVMS_MINOR_VERSION;
> +}
> +
> +EXPORT_SYMBOL(evms_cs_get_version);
Scap this. Modules under linux aren't binary compatible.
> +int
> +evms_cs_check_version(struct evms_version *required,
> + struct evms_version *actual)
> +{
> + if ((required->major != actual->major) ||
> + (required->minor > actual->minor) ||
> + ((required->minor == actual->minor) &&
> + (required->patchlevel > actual->patchlevel)))
> + return (-EINVAL);
> + return 0;
> +}
> +
> +EXPORT_SYMBOL(evms_cs_check_version);
Dito.
> +
> +/**
> + * evms_cs_allocate_logical_node - allocates an evms logical node structure
> + * @pp: address of the pointer which will contain the address of newly allocated node
> + *
> + * allocates and zeros an evms_logical_node structure.
> + *
> + * returns: 0 if sucessful
> + * -ENOMEM if unsuccessful
> + **/
> +int
> +evms_cs_allocate_logical_node(struct evms_logical_node **pp)
> +{
> + *pp = kmalloc(sizeof (struct evms_logical_node), GFP_KERNEL);
> + if (*pp == NULL) {
> + return -ENOMEM;
> + }
> + memset(*pp, 0, sizeof (struct evms_logical_node));
> + return 0;
A helper for kmalloc + memset looks rather pointles..
> +#define CRC_POLYNOMIAL 0xEDB88320L
> +static u32 crc_table[256];
> +static u32 crc_table_built = FALSE;
> +
> +/**
> + * build_crc_table
> + *
> + * initialzes the internal crc table
> + **/
> +static void
> +build_crc_table(void)
> +{
> + u32 i, j, crc;
> +
> + for (i = 0; i <= 255; i++) {
> + crc = i;
> + for (j = 8; j > 0; j--) {
> + if (crc & 1)
> + crc = (crc >> 1) ^ CRC_POLYNOMIAL;
> + else
> + crc >>= 1;
> + }
> + crc_table[i] = crc;
> + }
> + crc_table_built = TRUE;
> +}
Is this a different crc from the lib/crc32.c one?
> + done = FALSE;
> + while (!done) {
> + new_entry = mempool_alloc(evms_io_notify_pool, GFP_NOIO);
> + if (!new_entry) {
> + schedule();
> + continue;
> + }
Umm..
> +int
> +evms_cs_volume_request_in_progress(kdev_t dev,
> + int operation, int *current_count)
> +{
> + struct evms_logical_volume *volume = lookup_volume(minor(dev));
> + if (!volume || !volume->node) {
> + return -ENODEV;
> + }
> + if (operation > 0) {
> + atomic_inc(&volume->requests_in_progress);
> + } else if (operation < 0) {
> + atomic_dec(&volume->requests_in_progress);
> + }
> + if (current_count) {
> + *current_count = atomic_read(&volume->requests_in_progress);
> + }
> + return 0;
This function should be three ones for the different functionality.
Also kdev_t won't last long for block devices..
> +/**
> + * is_busy - determines if a block_devices is currently in use
> + * @dev: device to check
> + *
> + * determines if a block_device is in use or not
> + *
> + * returns: 0 = device is not in use
> + * -EBUSY if device is in use
> + * -ENOMEM if unable to get a bdev
> + **/
> +static int
> +is_busy(kdev_t dev)
> +{
> + struct block_device *bdev;
> +
> + bdev = bdget(kdev_t_to_nr(dev));
> + if (!bdev)
> + return (-ENOMEM);
> + if (bd_claim(bdev, is_busy))
> + return (-EBUSY);
> + bd_release(bdev);
> + return (0);
I don't think this is_busy check is a good idea. Anyways
it should be better something like this (then in block_dev.c):
int bd_busy(struct block_device *bdev)
{
int res = 0;
spin_lock(&bdev_lock);
if (bdev->bd_holder)
res = -EBUSY;
spin_unlock(&bdev_lock);
return res;
}
> +static int
> +evms_ioctl_cmd_get_info_level(void *arg)
> +{
> + /* copy info to userspace */
> + if (copy_to_user(arg, &evms_info_level, sizeof (evms_info_level)))
> + return -EFAULT;
> +
> + return 0;
> +}
>
>
> +
> +/**
> + * evms_ioctl_cmd_set_info_level
> + * @arg: int value
> + *
> + * sets the evms info (syslog logging) level
> + *
> + * returns: 0 = success
> + * otherwise error code
> + **/
> +static int
> +evms_ioctl_cmd_set_info_level(void *arg)
> +{
> + int temp;
> +
> + /* copy info from userspace */
> + if (copy_from_user(&temp, arg, sizeof (temp)))
> + return -EFAULT;
> + evms_info_level = temp;
> +
> + return 0;
> +}
Didn't you already export these two through /proc?
> + if (qv->command) {
> + /* After setting the volume to
> + * a quiesced state, there could
> + * be threads (on SMP systems)
> + * that are executing in the
> + * function, evms_handle_request,
> + * between the "wait_event" and the
> + * "atomic_inc" lines. We need to
> + * provide a "delay" sufficient
> + * to allow those threads to
> + * to reach the atomic_inc's
> + * before executing the while loop
> + * below. The "schedule" call should
> + * provide this.
> + */
> + schedule();
> + /* wait for outstanding requests to complete */
> + while (atomic_read(&volume->requests_in_progress) > 0)
> + schedule();
ever heard of waitqueues and wait_event?
> +/**
> + * evms_ioctl_cmd_rediscover_volumes
> + * @inode: vfs ioctl parameter
> + * @file: vfs ioctl parameter
> + * @cmd: vfs ioctl parame
Looks like even the EVMS list snipped the rest of the mail :)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/