[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