From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1JuWSm-00030S-Ot for mharc-grub-devel@gnu.org; Fri, 09 May 2008 13:32:56 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1JuWSk-0002zi-Rk for grub-devel@gnu.org; Fri, 09 May 2008 13:32:54 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1JuWSj-0002ym-9H for grub-devel@gnu.org; Fri, 09 May 2008 13:32:54 -0400 Received: from [199.232.76.173] (port=49244 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1JuWSj-0002yj-44 for grub-devel@gnu.org; Fri, 09 May 2008 13:32:53 -0400 Received: from mailout05.t-online.de ([194.25.134.82]:52398) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1JuWSi-0005E0-NH for grub-devel@gnu.org; Fri, 09 May 2008 13:32:53 -0400 Received: from fwd25.aul.t-online.de by mailout05.sul.t-online.de with smtp id 1JuWSb-0006gl-02; Fri, 09 May 2008 19:32:45 +0200 Received: from [10.3.2.2] (X6JVdmZGrhYPAatWG2fU3CWNp6Sme8bjsqjewhKttyU7AVhn8ZRTb4hM1UiUts3Zne@[217.235.228.23]) by fwd25.aul.t-online.de with esmtp id 1JuWSW-1HVblI0; Fri, 9 May 2008 19:32:40 +0200 Message-ID: <48248ABA.5010704@t-online.de> Date: Fri, 09 May 2008 19:32:42 +0200 From: Christian Franke User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071128 SeaMonkey/1.1.7 MIME-Version: 1.0 To: The development of GRUB 2 References: <47473879.1020004@t-online.de> <4822142F.1040505@t-online.de> <20080509124506.GA3705@thorin> In-Reply-To: <20080509124506.GA3705@thorin> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-ID: X6JVdmZGrhYPAatWG2fU3CWNp6Sme8bjsqjewhKttyU7AVhn8ZRTb4hM1UiUts3Zne X-TOI-MSGID: 9e886d56-a7f3-4d8c-ab29-8d5fedb2186c X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 3) Subject: Re: [PATCH] biosdisk, getroot for Cygwin 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: Fri, 09 May 2008 17:32:55 -0000 Robert Millan wrote: > On Wed, May 07, 2008 at 10:42:23PM +0200, Christian Franke wrote: > >> + >> + /* Check signature. */ >> + if (!(buf[0x1fe] == 0x55 && buf[0x1ff] == 0xaa)) >> + return ~0; >> + >> + /* Serial number offset depends on boot sector type. */ >> + if (mbr) >> + n = 0x1b8; >> + else if (memcmp (buf + 0x03, "NTFS", 4) == 0) >> + n = 0x048; >> + else if (memcmp (buf + 0x52, "FAT32", 5) == 0) >> + n = 0x043; >> + else if (memcmp (buf + 0x36, "FAT", 3) == 0) >> + n = 0x027; >> > > Did you consider using a struct here? > > There are 4 boot sector types, a clean declaration would require a union of 4 structs. IMO not worth the effort. >> + /* Cygwin returns the partition serial number in stat.st_dev. >> + This is never identical to the device number of the emulated >> + /dev/sdXN device, so above find_root_device () does not work. >> + Search the partion with the same serial in boot sector instead. */ >> + char devpath[sizeof ("/dev/sda15") + 13]; >> > > Where does this 13 come from? Would be nice to make it explicit (e.g. > sizeof(something) or so). > > 13 "paranoia" bytes added to the required size :-) >> +#ifndef __CYGWIN__ >> #ifdef __linux__ >> /* We first try to find the device in the /dev/mapper directory. If >> we don't do this, we get useless device names like /dev/dm-0 for >> @@ -242,12 +373,19 @@ grub_guess_root_device (const char *dir) >> os_dev = find_root_device ("/dev", st.st_dev); >> } >> >> +#else >> + /* Cygwin specific function. */ >> + os_dev = find_cygwin_root_device (dir, st.st_dev); >> + >> +#endif /* __CYGWIN__ */ >> > > The logic here seems a bit over-complicated. Surely whenever you have __linux__ > you don't have __CYGWIN__. Are you sure it can't be simplified? > > An alternative would be to move the #ifndef __CYGWIN__ behind the #ifdef __linux__ block: #ifdef __linux__ ... os_dev = find_root_device ("/dev/mapper", st.st_dev); ... if (! os_dev) #endif #ifndef __CYGWIN__ { /* This might be truly slow, but is there any better way? */ os_dev = find_root_device ("/dev", st.st_dev); } #else /* __CYGWIN__ */ /* Cygwin specific function. */ os_dev = find_cygwin_root_device (dir, st.st_dev); #endif /* __CYGWIN__ */ I'm not sure whether this would be more readable. >> +#ifndef __CYGWIN__ >> /* Check for LVM. */ >> if (!strncmp (os_dev, "/dev/mapper/", 12)) >> return GRUB_DEV_ABSTRACTION_LVM; >> @@ -255,6 +393,7 @@ grub_util_get_dev_abstraction (const char *os_dev) >> /* Check for RAID. */ >> if (!strncmp (os_dev, "/dev/md", 7)) >> return GRUB_DEV_ABSTRACTION_RAID; >> +#endif /* !__CYGWIN__ */ >> > > I think what we're missing here is "ifdef __linux__" since both LVM and > dmRAID are Linux-specific. > > Yes. Christian