Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 14 Apr 2007 18:06:29 +0200
From:      Rink Springer <rink@freebsd.org>
To:        Ed Schouten <ed@fxq.nl>
Cc:        Rink Springer <rink@freebsd.org>, Laurens Timmermans <laurens@timkapel.nl>, FreeBSD GEOM <freebsd-geom@freebsd.org>
Subject:   Re: New GEOM module: geom_xboxhd
Message-ID:  <20070414160629.GA82214@rink.nu>
In-Reply-To: <20070414115959.GN81821@hoeg.nl>
References:  <20070414115959.GN81821@hoeg.nl>

next in thread | previous in thread | raw e-mail | index | archive | help
Hi Ed,

On Sat, Apr 14, 2007 at 01:59:59PM +0200, Ed Schouten wrote:
> I've placed the code in a Git repository, which can be viewed online:
>=20
> 	http://g-rave.nl/gitweb?p=3Dgeom_xboxhd;a=3Dtree

Some comments:

(1) g_xboxhd_checkmagic(): You look at uint32_t* buf, which you have just
    g_free()'d; this seems wrong. I know this is debugging code, but you
    could just get rid of these lines all the same ;-)

(2) I consider the for-loop in line 125 a bit kludgy - why not add all
    'standard' partitions (of which you know they are always there) in
    this loop, and if the disk was larger, add a final paritition
    containing the rest? This makes the code prettier to read, in my
    opinion.

(3) This may be personal preference, but I consider a check of 4 bytes on
    address 0x600 a bit weak - usually, the first 63 sectors or so after the
    boot sector are skipped during partitioning (most general boot
    managers reside there), so there may be no telling what's in there.
    However, there doesn't seem to be a better way to identify Xbox
    harddisks, which makes me wonder whether we should keep this in the XBOX
    kernel config file only, possibly commented out in LINT.

Anyway, apart from this, it looks functional and something the xbox port
could very well use. I'd like to get this in the tree, so people with
more geom-fu are very welcome to take a look!

Have you tried what sysinstall(8) does when it sees such a layout? It'd
be a shame if it nuked the partition table... :-)

Regards,

--=20
Rink P.W. Springer                                - http://rink.nu
"It is such a quiet thing, to fall. But yet a far more terrible thing,
 to admit it."                                    - Darth Traya



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070414160629.GA82214>