Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Christoph Hellwig <hch@infradead.org>]

Okay, more comments on the actual patch:


+	/* remove pv's */
+	for(i = 0; i < vg_ptr->pv_max; i++)
+		if(vg_ptr->pv[i]) lvm_fs_remove_pv(vg_ptr, vg_ptr->pv[i]);
+

The code was carefully indented to match Documentation/CodingStyle.
Please don't mix random indentation changes with bugfixes..

--- linux-2.4.19-rc3.orig/drivers/md/lvm-internal.h	Thu Jul 25 13:05:13 2002
+++ linux-2.4.19-rc3/drivers/md/lvm-internal.h	Thu Jul 25 13:46:01 2002
@@ -49,6 +49,10 @@
 extern vg_t *vg[];
 extern struct file_operations lvm_chr_fops;
 
+#ifndef	uchar
+typedef	unsigned char	uchar;
+#endif

Do you _really_ have to use this non-standard type?  can't you use the
BSD u_char or sysv unchar?  and typedef/#define don't really mix nicely..

+#ifdef	list_move
+				list_move(next, hash_table);
+#else
 				list_del(next);
-				list_add(next, hash_table);
+#endif				list_add(next, hash_table);

In 2.5 list_move is a inline function, in 2.4 it is not present at all (yet).
An as LVM is utterly broken on 2.5 anyway this change has _no_ use at all.

 /* variables */
-char *lvm_version =
-    "LVM version " LVM_RELEASE_NAME "(" LVM_RELEASE_DATE ")";
+char *lvm_version = "LVM version "LVM_RELEASE_NAME"("LVM_RELEASE_DATE")";

when you change this anyway, what about const char[] to squeeze out a few bytes?

 struct file_operations lvm_chr_fops = {
-	open:lvm_chr_open,
-	release:lvm_chr_close,
-	ioctl:lvm_chr_ioctl,
+	owner:		THIS_MODULE,
+	open:		lvm_chr_open,
+	release:	lvm_chr_close,
+	ioctl:		lvm_chr_ioctl,
 };

when you update this you could move to C99 initializers, can't you?
 
+static struct gendisk lvm_gendisk =
+{
+	major:		MAJOR_NR,
+	major_name:	LVM_NAME,
+	minor_shift:	0,

this is in .bss, you don't need to initialize to zero.

-}				/* lvm_init() */
+} /* lvm_init() */


can't you just kill those silly end-of-function comments entirely?

 	case 0:
+		down_write(&lv->lv_lock);
 		lv->lv_snapshot_use_rate = lv_rate_req.rate;
+		up_write(&lv->lv_lock);
+		down_read(&lv->lv_lock);

you are sure youreally want to drop the lock here and not downgrade it?
(yes, I'm prodding for the downgrade patch to finally get merged..)

All in all this patch would be _soooo_ much easier to review if you wouldn't
mix random indentation changes with real fixes.