From owner-freebsd-geom@FreeBSD.ORG Sun Apr 15 07:04:39 2007 Return-Path: X-Original-To: freebsd-geom@freebsd.org Delivered-To: freebsd-geom@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4FA2116A400; Sun, 15 Apr 2007 07:04:39 +0000 (UTC) (envelope-from ed@hoeg.nl) Received: from palm.hoeg.nl (mx0.hoeg.nl [83.98.131.211]) by mx1.freebsd.org (Postfix) with ESMTP id E3BCF13C45B; Sun, 15 Apr 2007 07:04:38 +0000 (UTC) (envelope-from ed@hoeg.nl) Received: by palm.hoeg.nl (Postfix, from userid 1000) id CEE0E1CC40; Sun, 15 Apr 2007 09:04:36 +0200 (CEST) Date: Sun, 15 Apr 2007 09:04:36 +0200 From: Ed Schouten To: Rink Springer Message-ID: <20070415070436.GQ81821@hoeg.nl> References: <20070414115959.GN81821@hoeg.nl> <20070414160629.GA82214@rink.nu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="CfiwpigK2vfmwpN3" Content-Disposition: inline In-Reply-To: <20070414160629.GA82214@rink.nu> User-Agent: Mutt/1.5.15 (2007-04-06) Cc: Laurens Timmermans , FreeBSD GEOM Subject: Re: New GEOM module: geom_xboxhd X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Apr 2007 07:04:39 -0000 --CfiwpigK2vfmwpN3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable * Rink Springer wrote: > Some comments: >=20 > (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 ;-) Thanks. Fixed that. :) > (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. I now took a different approach; I added OFF_MAX to the offsets[] list, so the scratch space can also be added the same way as the other partitions. > (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 X= BOX > kernel config file only, possibly commented out in LINT. We could also check the magic of the Xbox partitions. The first 4 bytes of the partitions should always be "FATX". I personally do not favour this, because that would mean this module won't match if you newfs one of the Xbox partitions by accident. Adding it to LINT wouldn't hurt, because LINT is only used for compile-time testing, right? > 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! >=20 > Have you tried what sysinstall(8) does when it sees such a layout? It'd > be a shame if it nuked the partition table... :-) Not yet :-) --=20 Ed Schouten WWW: http://g-rave.nl/ --CfiwpigK2vfmwpN3 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (FreeBSD) iD8DBQFGIc6E52SDGA2eCwURAnxHAJ49M2Ixr3RV/cDXnQ6rL7Ifso+RWwCfQWPj XYIs0cSu7fcglQRKfb/iyaY= =Tata -----END PGP SIGNATURE----- --CfiwpigK2vfmwpN3--