All,
have send an LVM 1.0.5 patch to Marcelo directly which addresses:
- an OBO error accessing the vg array
- SMP lock fixes
- using blk_ioctl()
- indenting
It is available at:
<http://people.sistina.com/~mauelshagen/lvm_patches/lvm_1.0.5+_25.07.2002.patch>
Regards,
Heinz -- The LVM Guy --
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Heinz Mauelshagen Sistina Software Inc.
Senior Consultant/Developer Am Sonnenhang 11
56242 Marienrachdorf
Germany
[email protected] +49 2626 141200
FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
On Thu, Jul 25, 2002 at 03:39:44PM +0200, Heinz J . Mauelshagen wrote:
>
> All,
> have send an LVM 1.0.5 patch to Marcelo directly which addresses:
>
> - an OBO error accessing the vg array
> - SMP lock fixes
> - using blk_ioctl()
> - indenting
Any estimate when Stephen's non-broken pvmove implementaion will be merged?
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.
Christoph Hellwig wrote:
>
> +#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..
Or of course the normal u8 u16 and u32 and infally u64, which are so
much more explicit about the fact that we are actually dealig with
bit slices.
>
> All in all this patch would be _soooo_ much easier to review if you wouldn't
> mix random indentation changes with real fixes.
Christoph applying the patch and rediffing with diffs "ingore white
space' options can help you here.
And plese note that this kind of problems wouldn't be that common
if we finally decided to make indent -kr -i8 mandatory.
On Fri, 2002-07-26 at 01:47, Marcin Dalecki wrote:
> Christoph applying the patch and rediffing with diffs "ingore white
> space' options can help you here.
> And plese note that this kind of problems wouldn't be that common
> if we finally decided to make indent -kr -i8 mandatory.
indent -kr -i8 -bri0 (-l255 is also a good idea) doesn't produce the
same output for all whitespace/wrapped inputs. A long time ago I tried
to work that way and it caused much pain.
On Thu, Jul 25, 2002 at 03:34:19PM +0100, Christoph Hellwig wrote:
> On Thu, Jul 25, 2002 at 03:39:44PM +0200, Heinz J . Mauelshagen wrote:
> >
> > All,
> > have send an LVM 1.0.5 patch to Marcelo directly which addresses:
> >
> > - an OBO error accessing the vg array
> > - SMP lock fixes
> > - using blk_ioctl()
> > - indenting
>
> Any estimate when Stephen's non-broken pvmove implementaion will be merged?
Probably next month if I get time for that.
--
Regards,
Heinz -- The LVM Guy --
*** Software bugs are stupid.
Nevertheless it needs not so stupid people to solve them ***
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Heinz Mauelshagen Sistina Software Inc.
Senior Consultant/Developer Am Sonnenhang 11
56242 Marienrachdorf
Germany
[email protected] +49 2626 141200
FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
On Fri, Jul 26, 2002 at 02:47:56AM +0200, Marcin Dalecki wrote:
> Or of course the normal u8 u16 and u32 and infally u64, which are so
> much more explicit about the fact that we are actually dealig with
> bit slices.
*nod*
> > All in all this patch would be _soooo_ much easier to review if you wouldn't
> > mix random indentation changes with real fixes.
>
> Christoph applying the patch and rediffing with diffs "ingore white
> space' options can help you here.
It's not that easy - he randomly moves the conditional statements on the
if or else lines and changes brace placement..
> And plese note that this kind of problems wouldn't be that common
> if we finally decided to make indent -kr -i8 mandatory.
nightly run on the bk repo... Larry, is that possible? :)
<advertise>
for all those who want an indent with sane defaults:
version 0.2 of the 'portable' version of NetBSD is now available.
get it from https://developer.berlios.de/project/filelist.php?group_id=192
and package it for $DISTRO.
feedback welcome.
</advertise>
On Fri, Jul 26, 2002 at 02:47:56AM +0200, Marcin Dalecki wrote:
> Christoph Hellwig wrote:
> >
> > +#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..
>
> Or of course the normal u8 u16 and u32 and infally u64, which are so
> much more explicit about the fact that we are actually dealig with
> bit slices.
>
> >
> > All in all this patch would be _soooo_ much easier to review if you wouldn't
> > mix random indentation changes with real fixes.
>
> Christoph applying the patch and rediffing with diffs "ingore white
> space' options can help you here.
> And plese note that this kind of problems wouldn't be that common
> if we finally decided to make indent -kr -i8 mandatory.
It should have been for this patch.
Obviously an error on my end.
We'll resend...
--
Regards,
Heinz -- The LVM Guy --
*** Software bugs are stupid.
Nevertheless it needs not so stupid people to solve them ***
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Heinz Mauelshagen Sistina Software Inc.
Senior Consultant/Developer Am Sonnenhang 11
56242 Marienrachdorf
Germany
[email protected] +49 2626 141200
FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-