lkml.org 
[lkml]   [2002]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: LVM 1.0.5 patch for Linux 2.4.19-rc3
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.
-
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/
\
 
 \ /
  Last update: 2005-03-22 13:27    [W:0.072 / U:0.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site