Re: [RFC, RFT] LDM support (aka Windows Dynamic Volumes)

From: Ivan Voras <ivoras_at_freebsd.org>
Date: Mon, 12 Mar 2012 14:10:11 +0100
On 11/03/2012 10:35, Andrey V. Elsukov wrote:
> Hi, All
> 
> i wrote GEOM_PART_LDM class. It provides basic support of Logical Disk Manager
> partitioning scheme [1]. Since LDM metadata is not documented i used several
> articles found in the web and linux implementation as reference [2].

Seems ok, but there are a lot of "magic numbers" sprinkled around, which
could possibly be better expressed as named constants / macros. E.g. in
ldm_privhdr_parse(), ldm_tochdr_check(), as the array size in line 434,
in the switch block in lines 635, etc. I don't think it's a big problem,
though.

You could use SLIST_FOREACH_SAFE in ldm_vmdb_free(), but that's also not
a problem.

You have a lot of deeply nested loops in ldm_vmdb_parse() and others,
which may be "flattened" by removing the inner loops into separate
functions (so you don't run into the 80 column limit so often), but
again, not a problem.

Looks ok.




Received on Mon Mar 12 2012 - 12:10:36 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:24 UTC