From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1Q3GTc-0003qp-5y for mharc-grub-devel@gnu.org; Fri, 25 Mar 2011 19:31:32 -0400 Received: from [140.186.70.92] (port=57858 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q3GTa-0003qj-1a for grub-devel@gnu.org; Fri, 25 Mar 2011 19:31:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q3GTY-0006ea-PH for grub-devel@gnu.org; Fri, 25 Mar 2011 19:31:29 -0400 Received: from smarthost02.mail.zen.net.uk ([212.23.3.141]:44783) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q3GTY-0006eU-Eg for grub-devel@gnu.org; Fri, 25 Mar 2011 19:31:28 -0400 Received: from [82.69.40.219] (helo=riva.pelham.vpn.ucam.org) by smarthost02.mail.zen.net.uk with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1Q3GTU-0004j1-UO; Fri, 25 Mar 2011 23:31:25 +0000 Received: from cjwatson by riva.pelham.vpn.ucam.org with local (Exim 4.72) (envelope-from ) id 1Q3GTT-0000v6-BE; Fri, 25 Mar 2011 23:31:23 +0000 Date: Fri, 25 Mar 2011 23:31:23 +0000 From: Colin Watson To: Vladimir =?utf-8?Q?'=CF=86-coder=2Fphcoder'?= Serbinenko Message-ID: <20110325233122.GC9163@riva.ucam.org> References: <4D8D1397.5030802@dell.com> <4D8D1D64.2040801@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4D8D1D64.2040801@gmail.com> User-Agent: Mutt/1.5.20 (2009-06-14) X-Originating-Smarthost02-IP: [82.69.40.219] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 212.23.3.141 Cc: grub-devel@gnu.org, Mario Limonciello Subject: Re: [PATCH] grub-setup Modify the conditionality of the copy of the partition table X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Mar 2011 23:31:31 -0000 On Fri, Mar 25, 2011 at 11:55:32PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote: > On 25.03.2011 23:13, Mario Limonciello wrote: > > + * util/grub-setup.c: Conditionalize the partition map copy on floppy > > + support, not on whether the target contains partitions. > > + > > + Otherwise, the BIOS on Dell Latitude E series laptops will freeze > > + during POST if an invalid partition table is contained in the PBR > > + of the active partition when GRUB is installed to a partition. > > It's wrong to assume that no floppies contain partition tables. Also > --allow-floppy is usually used for USB sticks which sometimes appear as > floppies on some BIOSes and they pretty much contain the partition > table. Bottom line is: if you see a partition table: preserve it. Reading from floppies requires the floppy_probe function in boot.S, doesn't it, which collides with the partition table? But it makes sense to keep dest_partmap in the condition for safety anyway (and it saves a call to grub_util_biosdisk_is_floppy), so we'd end up with: if (dest_partmap || (!allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk))) memcpy (boot_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC, tmp_img + GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC, GRUB_BOOT_MACHINE_PART_END - GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC); ? > But it's ok to preserve this contents if floppy support is disabled > even if no partition table is discovered. In the case Mario discovered, there was indeed no partition table (all zeroes). Writing the floppy-probing code into that area caused it to fail. > A small logic lecture: > > Floppy support is: allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk) > Negation of it is either !(allow_floppy || grub_util_biosdisk_is_floppy (dest_dev->disk)) or !allow_floppy && !grub_util_biosdisk_is_floppy (dest_dev->disk), not what you wrote. Agreed. The latter form is found elsewhere in grub-setup. > Please don't move around unrelated parts. The parts Mario moved around weren't unrelated. Since the case at hand is that of installing to a partition boot record, this if block will run: if (! dest_partmap) { grub_util_warn (_("Attempting to install GRUB to a partitionless disk or to a partition. This is a BAD idea.")); goto unable_to_embed; } In order to preserve the partition table (or lack thereof) in this case, it was necessary to move the memcpy up above that test. Cheers, -- Colin Watson [cjwatson@ubuntu.com]