From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1I7rUH-0004GE-3T for mharc-grub-devel@gnu.org; Mon, 09 Jul 2007 07:33:05 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1I7rUE-0004G4-CJ for grub-devel@gnu.org; Mon, 09 Jul 2007 07:33:02 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1I7rUC-0004Fs-F7 for grub-devel@gnu.org; Mon, 09 Jul 2007 07:33:01 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1I7rUC-0004Fp-9U for grub-devel@gnu.org; Mon, 09 Jul 2007 07:33:00 -0400 Received: from proxy1.bredband.net ([195.54.101.71]) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1I7rUB-0004R1-Q2 for grub-devel@gnu.org; Mon, 09 Jul 2007 07:33:00 -0400 Received: from localhost.localdomain (213.113.223.232) by proxy1.bredband.net (7.3.121.3) id 46815119004632C0 for grub-devel@gnu.org; Mon, 9 Jul 2007 13:32:56 +0200 From: Johan Rydberg To: The development of GRUB 2 References: <8c0c43de0706261836j84bdc88q59d166036741e3f5@mail.gmail.com> <8c0c43de0706280704v2481f636kcec12debeafbbf55@mail.gmail.com> <8c0c43de0707032014k645ccb3ftfe9ab40637798481@mail.gmail.com> Date: Mon, 09 Jul 2007 13:56:51 +0200 In-Reply-To: <8c0c43de0707032014k645ccb3ftfe9ab40637798481@mail.gmail.com> (Alex Roman's message of "Tue, 3 Jul 2007 23:14:40 -0400") Message-ID: <87y7hpdc3g.fsf@gnu.org> User-Agent: Gnus/5.110004 (No Gnus v0.4) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" X-detected-kernel: Genre and OS details not recognized. Subject: Re: CD-ROM booting staus update X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jul 2007 11:33:03 -0000 --=-=-= Content-Transfer-Encoding: quoted-printable "Alex Roman" writes: > I've cleaned up the code and created a patch which will enable allow > booting from the first CD-ROM on a system. This is my first patch so I > checked my code many times... hopefully it won't cause unwanted side > effects, and hopefully my patch is in the correct format. Hi Alex, I haven't seen any reviews of this patch, so I'll give it a go. Remember that I do not do any active GRUB2 development at the moment, so I'll mostly comment on style related issues. > + void EXPORT_FUNC(grub_eltorito_boot) (int drive, void *addr, int size) = __attribute__ ((noreturn));=20 > + If this line is longer than 72 characters, please try to break it. > + /* We can't do this for CD drives */ > + if ( ! (drive & 0xe0) ) > + { > + if (grub_biosdisk_get_diskinfo_standard (drive, > + &data->cylinders, > + &data->heads, > + &data->sectors) !=3D 0) > + { > + grub_free (data); > + return grub_error (GRUB_ERR_BAD_DEVICE, "cannot get C/H/S values= "); > + } > +=20=20 > + if (! total_sectors) > + total_sectors =3D data->cylinders * data->heads * data->sectors; > + } I guess the outer braces are misaligned because of whitespace issues (tab vs space)? Otherwise you need to indent them two steps further. >=20=20 > disk->total_sectors =3D total_sectors; > disk->data =3D data; > @@ -167,20 +181,23 @@ > unsigned segment) > { > struct grub_biosdisk_data *data =3D disk->data; > -=20=20 > + Whitespace alterations. No need for those. > - dap->block =3D sector; > - > + dap->block =3D sector; > +=20=20=20=20=20=20 Same here. > +/* > + * Decode and print a Boot Record Volume Descriptor structure > + */ Even though the license header in each file prefixes each line with a star, we normally don't do that for other multiline comments. > +/* > + * The main command function > + */ If you ask me, no need for this comment. > +static grub_err_t > +grub_cmd_eltorito (struct grub_arg_list *state __attribute__ ((unused)), > + int argc __attribute__ ((unused)), > + char **args __attribute__ ((unused))) > +{ > + /* A few structure pointers that we'll need. */ > + grub_eltorito_boot_record_vol_descr brvd; > + grub_eltorito_validation_entry ve; > + grub_eltorito_default_entry de; > + > + /* Some other variables we'll need. */ > + grub_device_t cd_dev; > + grub_disk_t disk; > + grub_err_t err; Instead of commenting that you have declared those variables, comment WHY. If a comment is needed at all. The rule of thumb is to comment why something is done, not how. > +struct grub_eltorito_validation_entry_tag=20 > +{ > + // Header ID, must be 0x01. > + grub_uint8_t header_id; Please do not use C++ style single-line comments. Besides these cosmetic comments I think the overall patch looks just fine. Great work Alex. ~j --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2.2 (GNU/Linux) iD8DBQBGkiKG3CqIy3K3X2ERAgz1AKCq8s8EoEqUaCB5nUPZNKrshFiiiQCePXsO n0JKBq0CqDF2fI2S9DyVk2I= =noOZ -----END PGP SIGNATURE----- --=-=-=--