[patch] geom_vinum platform fixes
Rick C. Petty
rick-freebsd at kiwi-computer.com
Sun Mar 16 18:50:57 UTC 2008
On Sun, Mar 16, 2008 at 10:05:55AM +0100, Ulf Lilleengen wrote:
>
> I've reviewed the patch and done some modifications to it. I'll need some
> testing first though (I don't have a testbed right now since I'm travelling).
I've reviewed your patch but haven't had time to test it yet (hopefully
this week..)
> Here are some notes:
> - Removed GV_VERSION. It is not used anywhere.
Well, the point of this macro is for future changes to vinum on-disk
structure, since I added version as part of the new magic. Strictly
speaking, the macro isn't necessary (yet) and perhaps no future changes
will happen to the structures.
> - Avoid moving header includes around.
Not sure I agree. Although style(9) doesn't specify that the sys/* headers
should be alphabetized, many other places in kernel code have them sorted
as such (with certain exceptions like sys/param). It was a trivial "fix" I
had hoped to sneak in since that file was being modified anyway.
> - Declare functions static when used inside one file.
> - Style fixes.
*nod*
> - In gv_legacy_header_type, you can't be guaranteed that the hdr fields is
> not always null (although I agree in practise its highly unlikely), but
> it's hard to check this any other way, so I think it's alright :)
I can't think of any _better_ way to check. In the struct timeval fields
on amd64, I can't think of how these "zeros" would not appear. The bytes
past the end of the i386 header will always be zero because of the malloc
with M_ZERO.
> - Use macro values instead of hard-coded values when testing legacy type.
> - Pass data variable as argument rather than returning a header.
> - Avoid memory allocation where possible.
All good!
> Please let me know how it fares, and thanks for your help btw :)
I will do some testing using your patch. And thank _you_ for reviewing
this. I really look forward to your upcoming merge from p4!
-- Rick C. Petty
More information about the freebsd-geom
mailing list