[PATCH] Support Fedora 13 initrd filenames
Hello!
The initial ramdisk on Fedora 13 is called initramfs-${version}.img
unlike older versions of Fedora, which used initrd-${version}.img
The patch has been tested on Fedora 13.
ChangeLog:
* util/grub.d/10_linux.in: Add support for initrd images on Fedora 13.
--- util/grub.d/10_linux.in 2010-06-09 18:43:25 +
+++ util/grub.d/10_linux.in 2010-06-18 21:09:00 +
@@ -114,8 +114,9 @@
initrd=
for i in initrd.img-${version} initrd-${version}.img \
- initrd-${version} initrd.img-${alt_version} \
- initrd-${alt_version}.img initrd-${alt_version}; do
+ initrd-${version} initramfs-${version}.img \
+ initrd.img-${alt_version} initrd-${alt_version}.img \
+ initrd-${alt_version} initramfs-${alt_version}.img; do
if test -e ${dirname}/${i} ; then
initrd=$i
break
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: powerpc/sparc problems
Quoting David Miller da...@davemloft.net: From: Pavel Roskin pro...@gnu.org Date: Thu, 15 Oct 2009 18:41:41 -0400 This makes me think that checks for __bswapsi2 and __bswapdi2 will fail on Sparc64, even if those functions are present and even if --disable-werror is used. They worked perfectly fine for me on a real system with a real compiler and glibc. I don't think we can rely on testing done months ago. Now that we use -Werror by default, the checks would fail even natively. It they don't, I'd like to see the relevant excerpt from your config.log. I'm going to test the native PowerPC build later today. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: powerpc/sparc problems
On Fri, 2009-10-16 at 05:44 -0700, David Miller wrote: From: Pavel Roskin pro...@gnu.org Date: Thu, 15 Oct 2009 18:41:41 -0400 This makes me think that checks for __bswapsi2 and __bswapdi2 will fail on Sparc64, even if those functions are present and even if --disable-werror is used. They worked perfectly fine for me on a real system with a real compiler and glibc. If you're going to use cross compilation to test, use a full cross toolset and glibc build not some hacked up uclibc thing. I have tested the current GRUB on PowerPC. It's Fedora 11 with a real glibc. I added __ashldi3 to the arguments of AC_CHECK_FUNCS. The check fails. Yet __ashldi3 is present in libgcc and is exported unconditionally. The reason is that -nostdlib is added to CFLAGS immediately above AC_CHECK_FUNCS. -nostdlib disables linking against libgcc. I believe the checks for __bswapsi2 __bswapdi2 would fail on sparc64 for the same reason. Also, I believe the effect of -Werror on the test will be seen on sparc64. Adding -Werror should be after all tests and there should be a big warning in configure.ac telling not to add tests after that point. I also believe that even if it still fails for you, native building is more important to work than cross building situations. It is a native build and the current code. The whole reason I removed the checks is because they stopped working correctly when the target libc requirement was eliminated. Restoring the checks without removing -nostdlib not going to help. I'm surprised that my code is being reverted immediately before the release and the result is not tested. It's one thing to revert the code that has just been committed, and it's entirely different when the code has been in the repository for months. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: powerpc/sparc problems
On Thu, 2009-10-15 at 13:58 +0200, Vladimir 'phcoder' Serbinenko wrote: The methods discussed in this thread are good but aren't for release. So I just reverted Pavel's commit My cross-build for sparc64 fails now: __bswapsi2 in fat is not defined This can be traced to the following part of config.log: configure:7314: checking for __bswapsi2 configure:7370: sparc64-linux-uclibc-gcc -o conftest -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes -Wundef -Wstrict-prototypes -g -Os -m64 -fno-stack-protector -Werror -nostdlib -Wl,--defsym,___main=0x8100 -m64 conftest.c 5 In file included from /opt/sparc/usr/lib/gcc/sparc64-linux-uclibc/4.3.3/include-fixed/syslimits.h:7, from /opt/sparc/usr/lib/gcc/sparc64-linux-uclibc/4.3.3/include-fixed/limits.h:11, from conftest.c:38: /opt/sparc/usr/lib/gcc/sparc64-linux-uclibc/4.3.3/include-fixed/limits.h:122:61: error: no include path in which to search for limits.h cc1: warnings being treated as errors conftest.c:51: error: function declaration isn't a prototype configure:7377: $? = 1 The reason limits.h is missing is because I failed to compile uClibc for sparc64 using buildroot. But even if I create an empty /opt/sparc/usr/include/limits.h, the test fails: configure:7314: checking for __bswapsi2 configure:7370: gcc -o conftest -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes -Wundef -Wstrict-prototypes -g -Os -fno-dwarf2-cfi-asm -m64 -fno-stack-protector -mno-stack-arg-probe -Werror -nostdlib -Wl,--defsym,___main=0x8100 -m64 conftest.c 5 cc1: warnings being treated as errors conftest.c:51: error: function declaration isn't a prototype configure:7377: $? = 1 I'm afraid it's a real bug that would affect native compilation. We should move adding -Werror to TARGET_CFLAGS after all checks. Even after I do that, I still get: configure:7305: checking for __bswapsi2 configure:7361: gcc -o conftest -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes -Wundef -Wstrict-prototypes -g -Os -fno-dwarf2-cfi-asm -m64 -fno-stack-protector -mno-stack-arg-probe -nostdlib -Wl,--defsym,___main=0x8100 -m64 conftest.c 5 conftest.c:51: warning: function declaration isn't a prototype /usr/bin/ld: warning: cannot find entry symbol _start; defaulting to 00400144 /tmp/ccoCvQMe.o: In function `main': /home/proski/src/grub2.git/build-ieee1275-sparc64-linux-uclibc/conftest.c:62: undefined reference to `__bswapsi2' collect2: ld returned 1 exit status configure:7368: $? = 1 That may or may not be due to the lack of the libc. I tried checking for __ashldi3, which is exported unconditionally on PowerPC, and the check fails, even though I have libc for PowerPC. That's also a cross-compiler, but I can test it on a PowerMac if necessary. This makes me think that checks for __bswapsi2 and __bswapdi2 will fail on Sparc64, even if those functions are present and even if --disable-werror is used. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: powerpc/sparc problems
On Mon, 2009-10-12 at 03:28 -0700, David Miller wrote: Do you think we should just revert it? Probably. The purpose of the patch was to remove the requirement that the target libc development package is present. That's a common situation for x86_64 systems that may have a 32-bit capable compiler, and maybe the 32-bit libc installed as a dependency of a 32-bit package (e.g. wine), but no the files necessary to link against the 32-bit libc. I don't know why the checks need to be reinstated, but if it's really needed to be done, we could use the trick described in the gcc manual: `-print-libgcc-file-name' Same as `-print-file-name=libgcc.a'. This is useful when you use `-nostdlib' or `-nodefaultlibs' but you do want to link with `libgcc.a'. You can do gcc -nostdlib FILES... `gcc -print-libgcc-file-name` This way, it should be possible to check if the functions are in libgcc without requiring libc. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: powerpc/sparc problems
On Mon, 2009-10-12 at 17:45 +0200, Vladimir 'phcoder' Serbinenko wrote: On sparc64, ppc and mips we compile using -nostdlib -lgcc -static-libgcc. I would prefer to use the same method in check and actual compile. Ideally, we should use those flags on all architectures. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Resignation
Quoting Robert Millan r...@aybabtu.com: I'm sorry to hear that. I'd like to ask you to reconsider, and giving it a while before making it final. Would you do that? I'm sorry, but my decision is final. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Resignation
Hello! I have very little time for GNU GRUB, and I don't want to exercise any authority in the project without being able to track the mailing list and participate in the discussions. I would like to resign as comaintainer of GNU GRUB starting immediately. I'll try to participate if I have time. However, I cannot promise I'll reply to any pending e-mail. If there are any issues waiting for my response, please bring them to the list again. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: About firmware facilities
On Tue, 2009-09-15 at 04:27 +0930, Brendan Trotter wrote: Hi, On Tue, Sep 15, 2009 at 1:02 AM, Robert Millan r...@aybabtu.com wrote: Well, you have the freedom to disagree with anything we do and bring your customized GRUB to a different direction :-) Anyhow, my priority for GRUB is strong driver-based support. We could recruit someone to develop the framework in next year's GSoC (unless somebody steps in, of course). Why stop there? If proprietory ethernet ROMs aren't good enough, then what about proprietory SCSI ROMs, and proprietory firmware/BIOS? We are already doing it. There is functional ATA support, USB support is under development. GRUB can serve as BIOS together with Coreboot. Surely a boot manager wouldn't be a good manager if it didn't include it's own replacements for all of these things; and perhaps it should also include it's own replacement for proprietory OS's too... Why are you worrying about such silly things when the multi-boot specification (which actually is relevant) is still severely borked? Patches are welcome. The DRM opt-in fallacy: Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all. Sigh. I think I understand now - lack of logical thinking leads to lack of rational behavior. Ad hominem arguments are not welcome here. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] save_env variable_name=value
On Sat, 2009-09-12 at 09:54 -0500, richardvo...@gmail.com wrote: On Fri, Sep 11, 2009 at 4:43 PM, Pavel Roskin pro...@gnu.org wrote: I don't see how grub-mkconfig could compensate for a missing feature in save_env. Perhaps I'm missing the context here. AFAICT, it's grub.cfg that has to work around using two commands instead of one. Yes, if we want to keep backward compatibility with older modules. Therefore grub-mkconfig has to generate a longer grub.cfg (not sure how this makes grub-mkconfig uglier), but that's an incomplete assessment. There's also a different burden placed on user-edited configs and usage of the grub console, correct? Correct. I don't think that the suggestion was meant to save a few bytes in grub-mkconfig. I think it was suggesting a nicer interface for users working in the console. Yes. Also, grub-mkconfig could use it once we decide to break backward compatibility with the last version that didn't support save_env with a name. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] save_env variable_name=value
On Thu, 2009-09-03 at 17:08 +0200, Robert Millan wrote: Pavel, please comment on this when you can. It seems to me that doing it in grub-mkconfig would require less ad-hoc code in loadenv.mod and make it more efficient. I don't see how grub-mkconfig could compensate for a missing feature in save_env. Perhaps I'm missing the context here. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Some ideas about new features of grub
On Sun, 2009-08-23 at 14:56 +0200, Robert Millan wrote: On Thu, Jul 02, 2009 at 04:48:56PM +0800, Bean wrote: LUA integration. LUA is quite powerful, it's more suitable to do complicated task than sh script. For example, we can use it to detect os at runtime, implement simple commands, or draw the graphic menu. I feel similarly about LUA as I do about writable filesystems. In fact I'm considering a configure flag so that it's only enabled only when user requests it. But I know both Marco and Pavel feel strongly about this. Please can you comment? Ideally, I would prefer GRUB to have one scripting language. I just didn't have time and desire to argue against LUA inclusion. Having more code means more work for maintainers. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fix an infinite loop with a corrupted pc partition table
On Sun, 2009-08-23 at 17:40 +0200, Vladimir 'phcoder' Serbinenko wrote: I would prefer something simpler, but if that's not possible, this is better than hardcoding a number IMO. Unless Pavel has any objection, I think it's ok. Any problems with this one? No objections. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-mkconfig fails on every non i386-pc because of gfxterm/vbe
On Sun, 2009-08-23 at 13:03 +0200, Robert Millan wrote: Pavel, if you could confirm that you're ok with checking for module existance, maybe Felix' patch can be made simpler. Generally, I would prefer not to rely on the existence of modules, as it's a poor substitute for the knowledge whether they are actually functional on the target hardware. But if that's the simplest approach and it solves the original problem with all architectures other than i386-pc, then I'm fine with it. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: install-sh and docs/mdate-sh
On Wed, 2009-09-09 at 16:06 +0200, Felix Zielcke wrote: Am Mittwoch, den 09.09.2009, 15:57 +0200 schrieb Felix Zielcke: Is there any reason why we have install-sh and docs/mdate-sh? We don't use them anywhere and they're shipped with automake, which Okuji doestn't like anyway IIRC We can still use rules in the build system similar to those that Automake would generate. Oh I didn't notice configure even checks for install-sh. But what about mdate-sh? mdate-sh was copied from gnulib together with texinfo.tex. It looks like the build system doesn't handle the documentation yet. It would be more useful to implement building of the documentation and then check is mdate-sh is still unused. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [Fwd: Re: Bug#545460: segfault during update of grub-pc package]
On Tue, 2009-09-08 at 14:58 +0200, Felix Zielcke wrote: Program received signal SIGSEGV, Segmentation fault. 0x080490a8 in probe_raid_level (disk=0x0) at /home/work/src/grub2/util/grub-probe.c:94 94if (disk-dev-id != GRUB_DISK_DEVICE_RAID_ID) (gdb) bt #0 0x080490a8 in probe_raid_level (disk=0x0) at /home/work/src/grub2/util/grub-probe.c:94 #1 0x08049293 in probe (path=0x0, device_name=0xbea0 /dev/mapper/rhenvar-lvol0) at /home/work/src/grub2/util/grub-probe.c:167 #2 0x0804982f in main (argc=4, argv=0xbd74) at /home/work/src/grub2/util/grub-probe.c:417 Should we just check if disk or disk-dev is NULL or is this a more serious problem where something else needs a change? Let's not do it blindly. probe_raid_level is called with the NULL argument when going over the list. We need to understand how we get such an element in the list. Then we can think how to fix it. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: pc.mod - part_msdos.mod (etc)
On Tue, 2009-08-18 at 19:57 +0200, Robert Millan wrote: Hi, As was discussed in http://www.mail-archive.com/grub-devel@gnu.org/msg06210.html I intend to prefix all partmap modules with part_ and rename pc to part_msdos. If anyone has an objection, please say it now. No objection. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Split big and little endian BeFS and AtheFS
On Wed, 2009-08-19 at 12:05 +0200, Vladimir 'phcoder' Serbinenko wrote: I'm fine with the split, but please use something more descriptive than U16, U32 and U64. Maybe fs_to_cpu16() etc. U* was here before me but here is a patch w/o them Sorry, I didn't realize that. Then maybe it would be better to apply that part separately after the split. Please define macros with arguments. It makes the code more readable. Please don't introduce excessively long lines in *.rmk files. Newlines are cheap :-) I don't think we should rename byte_order to unused. Just because we doesn't use it now to determine the endianess, it doesn't mean that the field becomes meaningless. We may want to check it in the future. There are other fields in the superblock that we don't use, such as flags, yet we don't rename them to unusedN. Apart from that, I'm fine with the patch. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-mkconfig fails on every non i386-pc because of gfxterm/vbe
On Wed, 2009-08-19 at 17:18 +0200, Robert Millan wrote: The eye candy is nice but not so important. For me, gfxterm should be default on platforms where it's available, because it implements UTF-8, which is necessary to support l10n. Fine with me. Just please don't rely on existence of modules. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-mkconfig fails on every non i386-pc because of gfxterm/vbe
On Wed, 2009-07-22 at 13:04 +0200, Felix Zielcke wrote: Am Donnerstag, den 11.06.2009, 18:09 +0200 schrieb Vladimir 'phcoder' Serbinenko: The correct solution would be to check the presence of graphical backend and base the default on it. Or even better is to make the default depend on platform. It would solve the situations in which video backend is available but known to cause problems Pavel already wanted that gfxterm isn't anymore the default but didn't replied yet to my other mail about it. I'm sorry, I'm very busy these days. I don't know what message you are talking about, but I still have a lot of mail to read. I don't insist on changing the default, as long as GRUB behaves correctly. I have verified that setting GRUB_TERMINAL_OUTPUT=console in /usr/local/etc/default/grub would disable gfxterm, and that should be OK. The only problem I see right now is that it's not documented in the textinfo documentation, so that users won't be able to figure it out without reading the code. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-mkconfig fails on every non i386-pc because of gfxterm/vbe
On Fri, 2009-08-14 at 08:44 +0200, Felix Zielcke wrote: Am Donnerstag, den 11.06.2009, 21:09 -0400 schrieb Pavel Roskin: Quoting Felix Zielcke fziel...@z-51.de: So you would prefer something like the attached patch? Though then we'll need to tell people to explicit enable gfxterm. Pavel? Seems like you missed my mail. No, it's still in my INBOX. The patch looks reasonable, but I didn't have a chance to test it yet. So what do we do know with this? If we're going to make gfxterm not the default then I think it would be good to have this changed before 1.97 is released. You are right. I have applied your patch, as there were no objections. Sorry that it took so long. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] prevent duplicated entries due to symlinks
On Sun, 2009-05-10 at 00:20 +0200, Andreas wrote: ping? any problems still? I don't think anyone is really interested in that change. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Use common linker script for all i386-pc systems
On Tue, 2009-05-19 at 17:15 -0400, Pavel Roskin wrote: I appreciate a lot of work you put into the Apple CC support, but your patch complicates the code a lot. All features have a maintenance cost. Somebody will need to fix your code when it breaks. Considering that it can only be tested on Darwin, I'm afraid it will be vulnerable to bitrot. My patch simplifies things and allows further simplifications, some of which could be beneficial to your effort. Perhaps it would be better if you split uncontroversial or simple parts of your patch and submit them. Also, maybe it would be better to limit Apple support to the EFI platforms? That would make your patch less intrusive. The original of this message can be found here: http://lists.gnu.org/archive/html/grub-devel/2009-05/msg00238.html There was no reply to that message. It means that the Apple support was committed despite my objections. I wasn't the maintainer at the time, but neither was Vladimir. We should not do it again. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-mkconfig fails on every non i386-pc because of gfxterm/vbe
On Tue, 2009-08-18 at 19:53 +0200, Vladimir 'phcoder' Serbinenko wrote: I'm sorry, I'm very busy these days. I don't know what message you are talking about, but I still have a lot of mail to read. I don't insist on changing the default, as long as GRUB behaves correctly. I have verified that setting GRUB_TERMINAL_OUTPUT=console in /usr/local/etc/default/grub would disable gfxterm, and that should be OK. Why not to change default to use console? I understand that distros may want background image or other beatiful stuff but IMO mainstream should remain simple and reliable and in this case using gfxterm at least on some console is less reliable than old and proven console approach. I'm ok with providing an option to choose gfxterm but I don't see why it would be default in mainstream Done. Sorry, I posted my reply before I found the message Felix was referring to. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Patch to compile boot.img properly on OSX
On Thu, 2009-08-13 at 02:24 -0400, Pavel Roskin wrote: Since Yves is interested perhaps he could do the dirty job of applying repetitive fixes and testing different images? And submit a patch then? Fine with me. I have fixed the *.img files other than kernel.img, but there is still a lot of APPLE_CC in other *.S files, so there is enough work for everyone interested. Fixings lnxboot.S wasn't exactly trivial. By the way, it should be possible to remove -static from TARGET_IMG_CFLAGS if some data labels in lnxboot.S are also marked local. That's a good thing since my Darwin cross-compiler fails silently with -static trying to link kernel.exec. But I'd like to understand it better first. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Split big and little endian BeFS and AtheFS
On Fri, 2009-08-14 at 22:53 +0200, Vladimir 'phcoder' Serbinenko wrote: Hello. Currently afs and befs modules handle both big and little endian variants. Here is a patch to split into 2 modules. Unfortunately I wasn't able to find easily-available big-endian images to test but it shouldn't be a huge problem since we currently don't boot corresponding OS on big-endian machines. 24643 coreafsbe.img 24423 coreafsle.img 25168 coreafs.img 24548 corebefsbe.img 24353 corebefsle.img 25039 corebefs.img I'm fine with the split, but please use something more descriptive than U16, U32 and U64. Maybe fs_to_cpu16() etc. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] RFT: Rename local labels with a macro from boot/i386/pc/boot.S
On Mon, 2009-08-10 at 13:29 +0200, Robert Millan wrote: On Sat, Aug 08, 2009 at 04:48:32PM +0200, Yves Blusseau wrote: diff --git a/include/grub/symbol.h b/include/grub/symbol.h index 68d9f00..5fba549 100644 --- a/include/grub/symbol.h +++ b/include/grub/symbol.h @@ -21,6 +21,8 @@ #include config.h +#define LOCAL(X) L_##X + /* Add an underscore to a C symbol in assembler code if needed. */ #ifdef HAVE_ASM_USCORE # define EXT_C(sym)_ ## sym I changed LOCAL to be more similar to EXT_C (macro arguments don't normally use capital letters). I also added a comment, as there is no other way to know why we are adding L_ to the labels. Assuming Pavel is OK with it, I have no objection. Committed. Thank you! -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Patch to compile boot.img properly on OSX
On Thu, 2009-07-30 at 11:57 +0200, Vladimir 'phcoder' Serbinenko wrote: This wasn't specific for the patch. Neither was it a requirement, but more an idea. It's your work and you can choose to do it the way you like. OK then. The patches have been committed. Since Yves is interested perhaps he could do the dirty job of applying repetitive fixes and testing different images? And submit a patch then? Fine with me. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub.cfg and core.img getting out of sync
On Wed, 2009-08-12 at 16:25 +0100, Colin Watson wrote: I'm having persistent problems with grub.cfg and core.img getting out of sync. The usual pattern is: * Some shiny new feature appears in core.img * We extend grub-mkconfig to use it * User runs update-grub but not grub-install (historically a perfectly sane thing to do, and indeed basically standard practice) * grub either falls over at boot or does something odd The assumption is that grub.cfg should be able to deal with it. That's why it sets root before searching. This just bit me again today, and if it bites me it's going to bite the users of the packages I upload too. By our today's standards, it's a bug, so it should be reported in a detailed way. Is there any way we can do better? One thing I was thinking of is that Ubuntu's carried a patch to GRUB Legacy for some time that stores the package version at the point when grub-install was last run in /boot/grub/installed-version. If we did something like that, then grub-mkconfig could carry a bit of conditional code that says only use this feature if the installed version of GRUB is at least foo. grub-mkconfig would end up carrying a bit of bloat, but at least the bloat is in /usr, and I'm assuming that at this point things are stable enough that we won't be talking about *too* many changes. What do people think about this general approach? There is already a version hardcoded into the uncompressed part of core.img. See kern/i386/pc/startup.S. Unfortunately, there is no policy when the version should be updated. But I'm fine with a separate file in /boot/grub if we decide that the existing version in core.img cannot be used for that and we cannot keep grub.cfg backward compatible. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 6/18] define asm constant for absolute memory access
On Tue, 2009-06-16 at 12:49 +0200, Vladimir 'phcoder' Serbinenko wrote: +#ifdef APPLE_CC + movl $(mmaphook_mmap_rel), %esi +#else movw $(DS(mmaphook_mmap)), %si +#endif That's not an equivalent replacement. The argument width is different. I know. Sometimes because of the quirks I had to replace some code with a less logical alternative which should however work in the flow Please don't commit such changes without an explicit permissions form the maintainers. It's very time consuming to verify such code after it has been committed. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] RFT: Eliminate Apple specific code from boot/i386/pc/boot.S
On Thu, 2009-07-16 at 18:31 +0200, Vladimir 'phcoder' Serbinenko wrote: On Thu, Jul 16, 2009 at 5:47 AM, Pavel Roskinpro...@gnu.org wrote: ChangeLog: * boot/i386/pc/boot.S: Remove all code dependent on APPLE_CC. Use local labels starting with L_ so that Apple assembler would know they are local. You have really a lot of patches. I don't think so. It's undoubtly a good thing but since I was on vacation I lost a track a bit. For casual viewers I suppose it's even worse. Perhaps you could create a git repository which would hold all patches you haven't committed yet, one per branch? I'm sorry, I don't have time to create any private repositories. I wish I could do it, but I couldn't find time from that for almost a month, so chances are it won't happen any time soon. It will make a much better overview. Bean created a mirror on github. Perhaps we can use it as a tool to have an easily-viewable list of all unmerged patches and prevent patches from get lost. I know it's really unfortunate that I come up with a proposition of using such system for a relatively small project like grub. Alternatively we may want to formulate rules which would prevent future developpement deadlocks. There are ways to apply patches from e-mail. STGit can do it. My concern is that we would make the submitters responsible for something other than the quality of the patch if we require them to create private repositories. Normally, a patch is good as long as it applies. With a private repository, it will need to be kept up-to-date with the master repository, otherwise testing might find already fixed problems. As for the deadlocks, I don't think it's a technical issue. In any case, I find myself submitting too many patches, I'm ready to reconsider, but right now I cannot create any private repositories. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] RFT: Remove ABS macro from boot/i386/pc/boot.S
On Fri, 2009-08-07 at 19:15 +0200, Vladimir 'phcoder' Serbinenko wrote: gcc2 even the one updated by haiku has bugs. I don't think we should go long way just to support it. Hence I withdraw my request for holding this patch back because of it Both patches have been committed. Mirroring to git://repo.or.cz/grub2.git is currently not working. Apparently they have run out of space: error: file write error (No space left on device) fatal: unable to write sha1 file error: unpack failed: unpacker exited with error code To git+ssh://repo.or.cz/srv/git/grub2.git ! [remote rejected] master - master (n/a (unpacker error)) error: failed to push some refs to 'git+ssh://repo.or.cz/srv/git/grub2.git' -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2
On Sat, 2009-08-08 at 08:04 +0200, Felix Zielcke wrote: I think it would be more natural to let the user specify the full path to the file. Setting LANG=C seems unneeded in this case. After all, it's the user's choice, and we cannot examine the font file to check which characters it has. Ok here's a new one. Looks good to me. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] Relocator framework
On Fri, 2009-08-07 at 14:01 +0200, Vladimir 'phcoder' Serbinenko wrote: Apple's compiler is based GCC but binutils aren't and they pose the most of problems. Actualy the most problematic bit was that I didn't know that unless you prefix variable with L_ apple's assembler treats it as global. Adding L_ in this code solves problem. Yves proposed to add a LOCAL(x) macro to symbol.h this way the assembler code is also more readable. I also did further adjustments in this patch to decrease the number of ifdefs but had no time to test it and so couldn't submit it I would prefer if you don't commit any patches that introduce any more preprocessor conditionals for the Apple compiler or assembler. It would make it hard to debug problems reported by others, as different compilers would produce different binaries. If we are using some feature that is just nice but not needed (such as binary in lnxboot.S to set the image size to 1024), it would be better to avoid using that feature for all compilers rather than for the compilers that don't support it. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Solving the grub-pe2elf problem
On Fri, 2009-08-07 at 22:39 +0200, Robert Millan wrote: I don't understand this. Why is a requirement to install GNU binutils unreasonable, whereas a requirement to install objconv is not? Installing binutils can break the existing toolchain. Installing objconv should not. It's a good idea, as it would unify Apple and Cygwin support to some degree. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Solving the grub-pe2elf problem
On Fri, 2009-08-07 at 15:46 +0200, Robert Millan wrote: Other maintainers, is it burdensome to any of you to include these binaries in official builds? I suppose it's not, since mingw32 packages are widely available, but it doesn't hurt to ask :-) I would prefer not to do a binary build at this stage. Especially Windows users are at risk of messing things up if the entry barrier is lowered too much. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2
On Fri, 2009-08-07 at 14:57 +0200, Felix Zielcke wrote: I commited it now with an ack from Robert on IRC. Sorry, I'm commenting after it has been committed. Anyway, please note that having an approval doesn't absolve you from testing the code on your own. Reviews are not testing. There was a warning introduced by your change, and there was a syntax error after ascii. Also, the formatting of the moved code should have been changed to use the same indentation as the target file. I have fixed all that. Could you please explain what I should do to keep using ascii.pf2? I checked the script, but don't see any variable controlling that. make install would install both unicode.pf2 and ascii.pf2, so unicode.pf2 would always be preferred. If changing the default, it's a good style to provide an easy way for users to keep the old setting, and I just don't see it, short or removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so that they are never reinstalled or detected by GRUB. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] make 10_linux' test_gt() a bit more generic
On Fri, 2009-08-07 at 14:05 +0200, Robert Millan wrote: Committed. Also moved those functions to grub-mkconfig_lib.in. When comparing vmlinuz-2.6.31-rc4-wl and vmlinuz-2.6.31-rc5-wl, version_test_gt() ends up comparing wl and wl. Using the g modifier leads to the replacement being applied continuously until the last minus is consumed. The use of g wasn't warranted before, but it didn't have such effect. I'm committing the fix. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] move functionality of font_path() directly to util/grub-mkconfig.in and prefer unicode.pf2 over ascii.pf2
On Sat, 2009-08-08 at 07:36 +0200, Felix Zielcke wrote: Could you please explain what I should do to keep using ascii.pf2? I checked the script, but don't see any variable controlling that. make install would install both unicode.pf2 and ascii.pf2, so unicode.pf2 would always be preferred. With the old code ascii.pf2 would be always preferred. There wasn't either a way to specify it. I see. Maybe that's what we should have fixed first. If changing the default, it's a good style to provide an easy way for users to keep the old setting, and I just don't see it, short or removing /usr/src/unifont.bdf and /usr/local/share/grub/unicode.pf2 so that they are never reinstalled or detected by GRUB. Here's now a patch which allows users to specifiy the used font with GRUB_FONT=ascii First of all, I hope that the patch you will actually commit will use sane formatting. Diffs that ignore spacing changes are OK for review, but not for applying as is. I think it would be more natural to let the user specify the full path to the file. Setting LANG=C seems unneeded in this case. After all, it's the user's choice, and we cannot examine the font file to check which characters it has. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-pe2elf
On Mon, 2009-08-03 at 11:53 +0200, Vladimir 'phcoder' Serbinenko wrote: I personally think this is a hassle for nothing: we already have a working cygwin toolchain. While I acknowledge it's not perfect it works. And I propose not to touch it until it gives any maintainance or technical problem. When it does then we may consider dropping or limiting cygwin support. Until then this discussion is a waste of time (IMO) I agree that there is no urgent need to remove grub-pe2elf (although the issue might come up essentially). However, it's not so much a technical problem as an issue of trust. Committing code against the wish of maintainers should not be permitted. Those who do it should lose commit access. They are welcome to contribute, but their patches will need to be committed by others. I haven't seen any apology or explanation in this thread. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe, but in reverse?
On Fri, 2009-07-31 at 11:36 +0100, Colin Watson wrote: They're entered by users choosing where to install GRUB, whom we can hardly expect to enter UUIDs by hand. Likewise, we shouldn't expect them to enter drives in the GRUB notation, such as (hd0). Perhaps we can figure out how to give them a select list of available choices, which could then include UUIDs behind the scenes ... Yes, something like that. Users should be able to choose the disk by its size, maker, Linux device name. Maybe GRUB could support disk UUID eventually, but in the meantime, a UUID of one of the partitions would do, as long as it's indeed unique. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Should the website still say that GRUB 2 is currently under development?
On Thu, 2009-07-30 at 15:05 +0200, adrian15 wrote: Felix Zielcke escribió: I think the currently under development part can be now removed. Or would it be better to wait for the 1.97 release? (which hopefully comes before December) That would be nice because Debian freezes at December. I don't see anything wrong with being under development. I hope that GRUB 2 will be under development even after the 1.97. I don't see how Debian can have any problem with that. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [BUGFIX] Incorrect count of argument with rescue parser
On Fri, 2009-07-31 at 00:46 +0200, Vladimir 'phcoder' Serbinenko wrote:
This patch fixes the parsing of two strings like following ones:
echo 1 was parsed into echo, 1,
echo $root was parsed into echo (variable just disappeared)
It would be helpful if you explain how to see the difference without
tracing grub_parser_split_cmdline() in the debugger.
Also, it would be great if you explain the change. A comment for the
newly added code would help understand it. Otherwise it looks like the
previous comment still applies (A special case for when the last
character was part of a variable).
Since you looked at the problem, perhaps you know why argc is
decremented before the exit. I think it needs a comment.
Also, grub_malloc() appears to allocate two extra pointers for argv (if
we consider that argc is decremented). argv is not supposed to be null
terminated. I'd rather allocate just enough memory so that we could
catch abusers by running grub-emu in valgrind.
Anyway, the patch doesn't pass even minimal testing. Pressing Tab in
grub-emu crashes it at normal/completion.c:424
(gdb) where
#0 0x004179d1 in grub_normal_do_completion (buf=0x7fff5d5a29d0 ,
restore=0x7fff5d5a304c, hook=0x41568e print_completion) at
normal/completion.c:424
#1 0x004159d1 in grub_cmdline_get (prompt=0x7fff5d5a30e0 sh:grub ,
cmdline=0x662fa0 , readline=1) at normal/cmdline.c:329
#2 0x00418813 in grub_normal_read_line (line=0x7fff5d5a3160, cont=0)
at normal/main.c:504
#3 0x004141b3 in grub_reader_loop (getline=0) at kern/reader.c:43
#4 0x004117f4 in grub_main () at kern/main.c:176
#5 0x004397ab in main (argc=3, argv=0x7fff5d5a32c8) at
util/grub-emu.c:236
(gdb) l
419 {
420 /* Complete a command. */
421 if (grub_command_iterate (iterate_command))
422 goto fail;
423 }
424 else if (*current_word == '-')
425 {
426 if (complete_arguments (buf))
427 goto fail;
428 }
(gdb) p current_word
$1 = 0x21 Address 0x21 out of bounds
(gdb)
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: menuentrys with special or non-ASCII chars aren't displayed
On Wed, 2009-07-29 at 21:57 +0200, Felix Zielcke wrote: When a menuentry contains any character different from [-a-zA-Z] the menuentry isn't shown at all, even when gfxterm + unicode.pf2 is used. Is there any reason why this is done? On Launchpad there is a bug report open that grub2 can't anymore by default show chinese characters (because of unicode.pff - ascii.pff switch) and now another one about the not shown menuentrys. So for them it seems it is somehow important. I cannot reproduce the problem. A menu entry with Russian, Chinese, Vietnamese and German characters is displayed correctly. The same applies to a menu item consisting only of Chinese characters. Maybe your menu title has incorrect UTF-8 sequences? -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: grub-probe, but in reverse?
On Wed, 2009-07-29 at 19:35 +0100, Colin Watson wrote: $ sudo grub-probe -d /dev/sda1 -t drive (hd0,1) I have a reason to want to do the reverse of this: I have a libparted-based program that ensures that at least one partition on a disk is marked active (needed for some BIOSes), and would like to call it on the disk selected for installation of GRUB in d-i. Of course libparted is only going to understand OS device names. If possible I'd rather avoid reading device.map by hand to figure out how to map (hd0,1) back to /dev/sda1. Is there any way to do this with the code as it stands, and if not would it make sense to make it possible to pass GRUB device names to grub-probe? I think device.map is fundamentally unreliable and should be obsoleted. I don't know where you are getting the GRUB device names, but I suggest that you use UUID instead. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] gfxterm clear_char
On Tue, 2009-07-28 at 22:15 -0700, Joe Auricchio wrote:
Eine kleine gfxterm.c cleanup
Applied with formatting changes.
static void
+clear_char(struct grub_colored_char *c)
Space is needed before the parenthesis.
+{
+c-code = ' ';
Indentation should be 2 spaces, not 4.
for(i = 0; i virtual_screen.columns * virtual_screen.rows; i++)
{
This brace is no longer needed.
Index: src/ChangeLog
It would be more convenient if the ChangeLog entry is posted inline
rather than inside the patch.
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] SDL support
On Sat, 2009-07-25 at 18:26 +0200, Robert Millan wrote: On Thu, Jul 23, 2009 at 08:19:24PM -0400, Pavel Roskin wrote: Also, when compiling on PowerPC, configure reports that SDL is enabled, but it is not. The same applies to USB support. Shouldn't it be enabled in all architectures? Unfortunately, the grub-emu definition is arch-specific, but other than this the rest is portable. Yes, we should try to keep grub-emu the same on all architectures. That's a necessary step to the build system reorganization. This would be the first time gfxterm works on powerpc. Yes, it would be great to be able to compile that code on a big-endian system. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fix an infinite loop with a corrupted pc partition table
On Sat, 2009-07-25 at 18:56 +0200, Vladimir 'phcoder' Serbinenko wrote: On Sat, Jul 25, 2009 at 6:36 PM, Robert Millanr...@aybabtu.com wrote: On Fri, Jul 24, 2009 at 09:24:31PM +0200, Felix Zielcke wrote: + loop = 0; + while (loop 10) [...] + if (loop == 10) +return grub_error (GRUB_ERR_BAD_PART_TABLE, Corrupted partition table found.); What does this 10 represent? It looks like heuristic, but I'm missing some context (I'm not familiar with extended partition layout). It is a heuristic.It's a number bigger than highest number of sane partitions but an amount of iteration which a computer can handle easily. I know that it's an arbitrary limit butusing a correct algorithm (e.g. record all visited extended partitions offsets) would take place almost uselessly and other solutions risk to have other problems (as invisible partitions) Let's try to see the complete picture. By the time we get 10 partitions we are already in trouble, especially if running ls on a slow terminal. Reaching 10 means we were wrong all along. Links backwards between extended partition entries are more likely to be due to data corruption than due to buggy partitoning tools. OK, if you want, let's support up to 10 backward links. That's more than enough. I would hate to get caught in another discussion about minor issues when we have a real problem in that code. GRUB only follows a single chain of extended partitions rather than a tree of links. I think the whole point in having the Linux extended partition type is to allow it to coexist with an MS-DOS extended partition. That's a realistic scenario, unlike backward links. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fix an infinite loop with a corrupted pc partition table
On Fri, 2009-07-24 at 18:58 +0200, Felix Zielcke wrote:
With this [0] partition table grub-probe currently loops forever:
kern/disk.c:389: Reading `hd1'...
partmap/pc.c:142: partition 0: flag 0x0, type 0x5, start 0x0, len
0x11177330
That's so evil!
This patch fixes it, but probable there's a better fix.
We could require that all references to extended partitions are only
considered if they lead to a sector after the one currently being
processed.
Actually, no partition table should point to any partition (extended or
not) in an earlier sector, but it's enough to exclude backward links
between extended partitions to break the loop.
ChangeLog:
* partmap/pc.c (pc_partition_map_iterate): Only allow references
to subsequent sectors in extended partition entries.
---
partmap/pc.c |8 +++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/partmap/pc.c b/partmap/pc.c
index 6f68ecf..cd119c0 100644
--- a/partmap/pc.c
+++ b/partmap/pc.c
@@ -208,7 +208,13 @@ pc_partition_map_iterate (grub_disk_t disk,
if (grub_pc_partition_is_extended (e-type))
{
- p.offset = pcdata.ext_offset + grub_le_to_cpu32 (e-start);
+ grub_disk_addr_t new_offset;
+
+ /* Only allow references subsequent sectors */
+ new_offset = pcdata.ext_offset + grub_le_to_cpu32 (e-start);
+ if (new_offset = p.offset)
+ continue;
+
if (! pcdata.ext_offset)
pcdata.ext_offset = p.offset;
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]
On Fri, 2009-07-24 at 20:46 +0200, Felix Zielcke wrote: And another bug forward Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp here: grub_partition_iterate (dest_dev-disk, (strcmp (dest_partmap, pc_partition_map) ? find_usable_region_gpt : find_usable_region_msdos)); dest_partmap is only assigned a value in identify_partmap. If grub_partition_iterate() doesn't find any partitions, dest_partmap remains a random pointer. The fix would be probably to initialize dest_partmap with NULL. If it becomes pc_partition_map, iterate with find_usable_region_msdos, if it becomes gpt_partition_map, iterate with find_usable_region_gpt. If it's NULL or another string, exit with a warning. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fix an infinite loop with a corrupted pc partition table
On Fri, 2009-07-24 at 21:24 +0200, Felix Zielcke wrote: Here's a new one after comments from Vladimir on IRC loop count is increased to 100'000 and partitions with a starting sector of 0 are ignored. I don't see why it's better. Do you think the links backwards should be allowed? -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]
On Fri, 2009-07-24 at 16:09 -0400, Pavel Roskin wrote:
On Fri, 2009-07-24 at 20:46 +0200, Felix Zielcke wrote:
And another bug forward
Anyone has an idea why a dm-crypt/lvm leads to a segfault in the strcmp
here:
grub_partition_iterate (dest_dev-disk, (strcmp (dest_partmap,
pc_partition_map) ?
find_usable_region_gpt :
find_usable_region_msdos));
dest_partmap is only assigned a value in identify_partmap. If
grub_partition_iterate() doesn't find any partitions, dest_partmap
remains a random pointer.
The fix would be probably to initialize dest_partmap with NULL. If it
becomes pc_partition_map, iterate with find_usable_region_msdos, if it
becomes gpt_partition_map, iterate with find_usable_region_gpt. If
it's NULL or another string, exit with a warning.
How about this?
Require positive identification of PC or GPT partition for embedding
ChangeLog:
* util/i386/pc/grub-setup.c (setup): Initialize dest_partmap
before iteration. Don't allow embedding unless dest_partmap is
pc_partition_map or gpt_partition_map.
---
util/i386/pc/grub-setup.c | 37 ++---
1 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/util/i386/pc/grub-setup.c b/util/i386/pc/grub-setup.c
index 5a51964..7ac5ace 100644
--- a/util/i386/pc/grub-setup.c
+++ b/util/i386/pc/grub-setup.c
@@ -329,16 +329,39 @@ setup (const char *dir,
dest_partmap = p-partmap-name;
return 1;
}
+
+ dest_partmap = NULL;
grub_partition_iterate (dest_dev-disk, identify_partmap);
- grub_partition_iterate (dest_dev-disk, (strcmp (dest_partmap,
pc_partition_map) ?
- find_usable_region_gpt :
find_usable_region_msdos));
- if (embed_region.end == embed_region.start)
+ if (! dest_partmap)
{
- if (! strcmp (dest_partmap, pc_partition_map))
- grub_util_warn (This msdos-style partition label has no post-MBR gap;
embedding won't be possible!);
- else
- grub_util_warn (This GPT partition label has no BIOS Boot Partition;
embedding won't be possible!);
+ grub_util_warn (Cannot identify partition map.);
+ goto unable_to_embed;
+}
+ else if (strcmp (dest_partmap, pc_partition_map) == 0)
+{
+ grub_partition_iterate (dest_dev-disk, find_usable_region_msdos);
+ if (embed_region.end == embed_region.start)
+ {
+ grub_util_warn (This msdos-style partition label has no post-MBR
+ gap; embedding won't be possible!);
+ goto unable_to_embed;
+ }
+}
+ else if (strcmp (dest_partmap, gpt_partition_map) == 0)
+{
+ grub_partition_iterate (dest_dev-disk, find_usable_region_gpt);
+ if (embed_region.end == embed_region.start)
+ {
+ grub_util_warn (This GPT partition label has no BIOS Boot
+ Partition; embedding won't be possible!);
+ goto unable_to_embed;
+ }
+}
+ else
+{
+ grub_util_warn (Embedding on partition type %s is unsupported,
+ dest_partmap);
goto unable_to_embed;
}
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [Fwd: Re: Bug#495949: grub-common: grub-probe segfaults]
On Fri, 2009-07-24 at 22:37 +0200, Vladimir 'phcoder' Serbinenko wrote: Actualy I already fixed this in my patch for installing on partitionless devices. Since there were no oppositions I'll commit it OK, go ahead. I just realized that your patch is a subset of my patch, so it would be fair if you apply your part under your name. Just please use a better description in the ChangeLog. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] fix an infinite loop with a corrupted pc partition table
On Fri, 2009-07-24 at 22:35 +0200, Vladimir 'phcoder' Serbinenko wrote: Hello This patch fixes it, but probable there's a better fix. We could require that all references to extended partitions are only considered if they lead to a sector after the one currently being processed. I already thought about this and spoke about it on IRC. Unfortunately backward pointers do exist. I had it few years ago when I used diskdrake. It put the logical partition at the end of the chain even if partition itself was in the middle of the disk. I don't know if it still has this behaviour but partition schemes have sometimes tendency to stay a long time. I don't like they idea of user not being able to boot his mandriva or some partitions not being accessible. This would still work. This is still allowed: MBR Ext Logical -/ This is what I want to forbid: MBR Ext Ext / \-- Logical I don't think any tool would create that insanity. A big problem with pc partitions is that AFAIK there is no normative standard for it, only descriptive documents Yes, it's a typical industry standard :-( -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems
On Fri, 2009-07-24 at 23:02 +0200, Christian Franke wrote: A correct performance-aware solution would look like: #ifdef DT_DIR if (de-d_type == DT_DIR) info.dir = 1; else if (de-type == DT_FILE) There in no DT_FILE in glibc, but there is DT_REG. DT_UNKNOWN is present. Perhaps the above line should be else if (de-type != DT_UNKNOWN) We only care if it's a directory or not. All other objects can be treated like files. I'm fine either way, whether we fix the high-performance code or remove it, as long as we don't have to add more checks. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/4] Change grub_file_seek() to return grub_err_t
On Thu, 2009-07-23 at 10:23 +0200, Vladimir 'phcoder' Serbinenko wrote: On Wed, Jul 22, 2009 at 5:16 AM, Pavel Roskinpro...@gnu.org wrote: No callers need the previous offset. Returning -1 with implicit cast to grub_off_t required a cast just to check for errors. This also makes grub_file_seek() more similar to fseek(). ChangeLog: * kern/file.c (grub_file_seek): Return grub_err_t. Adjust all callers that check the return value. Good idea but you forgot to update the following entry: ./script/lua/grub_lib.c:270: offset = grub_file_seek (file, offset); Nice catch, thank you! And by the way, it's a reminder for all of us how massive changes can introduce subtle errors. It means that the lua interface was actually the only user on the old API. Changing it would change the user interface in the lua implementation. The standard lua seek is completely different: http://www.lua.org/manual/5.1/manual.html#pdf-file:seek I guess we are not trying to emulate that behavior. osdetect.lua doesn't use grub.file_seek(). So let's patch lua first, and then this patch can be applied. The lua patch will be sent separately. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] lua: change grub.file_seek() to return error code, not the old offset
This makes it easier to check if the call has succeeded. The old offset is rarely needed. ChangeLog: * script/lua/grub_lib.c (grub_lua_file_seek): Return error code, not the old offset. --- script/lua/grub_lib.c |7 ++- 1 files changed, 2 insertions(+), 5 deletions(-) diff --git a/script/lua/grub_lib.c b/script/lua/grub_lib.c index d26f193..42f370a 100644 --- a/script/lua/grub_lib.c +++ b/script/lua/grub_lib.c @@ -266,12 +266,9 @@ grub_lua_file_seek (lua_State *state) luaL_checktype (state, 1, LUA_TLIGHTUSERDATA); file = lua_touserdata (state, 1); offset = luaL_checkinteger (state, 2); + grub_file_seek (file, offset); - offset = grub_file_seek (file, offset); - save_errno (state); - - lua_pushinteger (state, offset); - return 1; + return push_result (state); } static int ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
On Thu, 2009-07-23 at 21:51 +0200, Vladimir 'phcoder' Serbinenko wrote: 2009/7/23 Javier Martín lordhab...@gmail.com: Here is a new version which also incorporates the C99 integer constant macros. To avoid excess verbosity, all macros have now names like Gx, where x is the standard name. Thus, PRIx64, UINT64_C(a) -- GPRIx64, GUINT64_C(a). Please respect current convention of using GRUB_ as prefix for macros Are the macros *_C really useful? Anyway it's to be discussed separately. Could you not do increase previous patch bt make a new one to separate what is already in discussion from new things. @Pavel: do you have any further comments? I believe it's not worth the trouble at this point. There are many patches that I make and never send or never commit because I'm not sure that the change is valuable enough. Every change comes with a risk of breaking something. For instance, I planned to remove cli in the bootloader, but decided that it's not worth the risk. I wanted to remove support for module unloading, but decided that there is a downside for users, and there are better ways to reduce the size of core.img. I wanted to simplify awk invocation, but decided not to pursue it at this time, as it would conflict with other efforts to reorganize the build system, and the benefits would be mostly theoretical. I think a good developer should be able to drop changes that are not exactly useful and move on. We have other issues. Javier mentioned -Wconversion. It makes the compiler very noisy, but some issues it found are real, such as the problem with ALIGN_UP. It's more important than pushing the same patch. One day we may find a nice solution to the issue of file formats. Maybe a new C standard appears with new format specifications. Maybe gcc will add some checks. But as it stands now, we won't benefit from it enough to justify less readable code. Let's move on and work on the issues where we can make the difference now. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] SDL support
On Thu, 2009-07-23 at 11:49 +0200, Vladimir 'phcoder' Serbinenko wrote: Hello. Here is already-discussed SDL support The patch refers to video/fb/video_fb.c, which is not included. Also, please check the changes I made to your changes to configure.ac. Please check the spelling of explicitly. There is no need for verbose excuses, it's enough to say need SDL library or need SDL headers. build and install the `grub-emu' debugging utility with SDL support is also too long and not quite correct. --enable-grub-emu-sdl decides whether SDL support will be in grub-emu. Since it's the default, maybe it's better to describe --disable-grub-emu-sdl. Obviously, it applies to grub-emu-usb as well, I just didn't have a chance to clean it up. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Check for the appropriate condition in types.h
On Thu, 2009-07-23 at 22:30 +0200, Vladimir 'phcoder' Serbinenko wrote: [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ... [GRUB_CPU_SIZEOF_LONG == 8]: ... this. Ok, let's adopt this form instead. The proposed ChangeLog would now be: From the GNU Coding Standards: C programs often contain compile-time `#if' conditionals. Many changes are conditional; sometimes you add a new definition which is entirely contained in a conditional. It is very useful to indicate in the change log the conditions for which the change applies. Our convention for indicating conditional changes is to use square brackets around the name of the condition. It means that the square brackets are used if the changes only affect the code under the condition specified in brackets. This is not what is happening here. This is exactly what happens here: we change only what happens in conditionals [GRUB_CPU_SIZEOF_VOID_P == 8] and [GRUB_CPU_SIZEOF_LONG == 8] I'm not interested to discuss the right interpretation of the coding standards. Frankly, I'm not a fan of keeping ChangeLog, as it's too formal and it's a point of contention for parallel development. I prefer the Linux kernel style of descriptions. But since we are updating ChangeLog, let's keep it readable. (UINT_TO_PTR): move outside wordsize conditionals (PTR_TO_UINT): new macro We should remove PTR_TO_UINT32 and PTR_TO_UINT64 with PTR_TO_UINT everywhere. I've checked that it doesn't introduce any warnings on any platform. Sometimes PTR_TOUINT32 with precision loss is exactly what the coder wants. A typical example is booting 32-bit OS on 64-bit platform. This is the case of booting linux on efi64. Then the code has to ensure that the target is below 4GiB (see my mm propositions for more on how to ensure it). But I'm ok with requirement of additional explicit cast in such cases as (grub_uint32_t) PTR_TO_UINT (x) That would be much better that PTR_TO_UINT32, as it makes the cast explicit in the code that should ensure that the cast is valid. We can find such cases by compiling with -Wconversion and finding the newly appearing warnings after PTR_TO_UINT32 and PTR_TO_UINT64 are replaced with PTR_TO_UINT. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] zero-fill entry before asking BIOS for memory map
On Thu, 2009-07-23 at 11:38 +0200, Vladimir 'phcoder' Serbinenko wrote: Hello. According to xen some BIOSes update only lower 32-bit in mmap entries. To workaround this and not get high values in memory map zero-fill before calling BIOS I think the fix belongs to grub_get_mmap_entry(), not to all of its callers. Something like this: diff --git a/kern/i386/pc/startup.S b/kern/i386/pc/startup.S index be258fb..5468ba8 100644 --- a/kern/i386/pc/startup.S +++ b/kern/i386/pc/startup.S @@ -997,6 +997,14 @@ FUNCTION(grub_get_mmap_entry) /* push ADDR */ pushl %eax + /* clear the request area, buggy BIOSes may not clear it */ + xor %edi, %edi + movl%edi, 4(%eax) + movl%edi, 8(%eax) + movl%edi, 12(%eax) + movl%edi, 16(%eax) + movl%edi, 20(%eax) + /* place address (+4) in ES:DI */ addl$4, %eax movl%eax, %edi I don't know whether it would interfere with the Apple CC workaround. It would be great if we get rid of it, perhaps by using labels starting with L_. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC, PATCH] C99 format specifiers for fixed-length integer types
On Thu, 2009-07-23 at 23:25 +0200, Javier Martín wrote: With the reduced version of the patch I'm putting forward, such a (hypothetical, indeed) change will only impact types.h, while otherwise many source files will need to be modified in a hunt for %lls and their variations. We can consider lower types safe since the autopromotion rules will keep the compiler happy even if int becomes 64-bit. OK, I'm fine with this change. It would only address the problem we have now (long vs .long long for 64-bit types) without trying to anticipate what other platforms we may support. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [BUGFIX] Don't use DT_DIR: It doesn't work on non-ext* filesystems
On Thu, 2009-07-23 at 11:29 +0200, Vladimir 'phcoder' Serbinenko wrote: -#ifdef DT_DIR - info.dir = (de-d_type == DT_DIR); -#else info.dir = !! is_dir (path, de-d_name); -#endif Fine with me. Finally a patch that reduces the number of preprocessor directives :-) -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] Alias cls as clear
On Wed, 2009-07-22 at 22:08 -0700, Joe Auricchio wrote: * commands/minicmd.c: Add clear as alias for cls. My background is in Unix, so 'clear' comes much more naturally to my fingers than 'cls'. But it's bad to clutter grub with too many commands. So I leave this to the maintainers: do we add both commands, only one, or neither? Let's have clear only, as it's more clear :-) Please include the ChangeLog entry. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: Fwd: [PATCH 1/2] Framebuffer split
On Thu, 2009-07-23 at 11:52 +0200, Vladimir 'phcoder' Serbinenko wrote: Hello. Here is a framebuffer split which has already been discussed. This patch contains some code by Collin D Bennett in addition to my code. Sorry for compression but maillist server doesn't accept it otherwise Please include an uncompressed ChangeLog entry. Even though the issue was discussed, it would help if you include a description of the changes to simplify reviewing. You can copy a previously posted description. grub_video_vbe_set_viewport and grub_video_vbe_get_info_and_fini need to be declared static to avoid compiler warnings. Apart from that, no warnings are introduced. Please don't add trailing whitespace. Since you are using git, you can easily check it with STGit by running stg edit -d. grub_video_vbe_get_info_and_fini strikes as a weird name. The comment says: /* Get information about active video mode. */ Likewise, all occurrences of get_info_and_fini should probably be replaced with something more descriptive. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] SDL support
On Thu, 2009-07-23 at 22:44 +0200, Vladimir 'phcoder' Serbinenko wrote: On Thu, Jul 23, 2009 at 10:32 PM, Pavel Roskinpro...@gnu.org wrote: On Thu, 2009-07-23 at 11:49 +0200, Vladimir 'phcoder' Serbinenko wrote: Hello. Here is already-discussed SDL support The patch refers to video/fb/video_fb.c, which is not included. Well it's PATCH 2/2 and PATCH 1/2 is framebuffer split which is the pre-requisite for this patch. I had some problems with sending but now I checked the archives and see that the patch 1 successfully arrived Sorry, I missed that. When testing the patches together, there is a warning in grub_video_sdl_enable_double_buffering(). The enable argument should be marked as unused. Also, when compiling on PowerPC, configure reports that SDL is enabled, but it is not. The same applies to USB support. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Build system improvement
On Wed, 2009-07-22 at 14:41 +0200, Felix Zielcke wrote: [0] http://lists.gnu.org/archive/html/grub-devel/2009-05/msg9.html So what's now with it? Marco can you at least say something about it? It would be very nice if the warnings would be more visible by default. Probable we can't ever use -Werror because different compilers produce different warnings etc. For example gentoo's gcc with his trampoline patch. I don't see any warnings in any configuration, unless more warning flags are added (e.g. -Wconversion, which is very noisy). We can use -Werror by default, but make it very easy to disable. For instance, we could have a configure option --disable-strict-warnings. GRUB is a bootloader; we don't want anyone's system to become unbootable because something was miscompiled. Requiring an extra step to disable warnings would tell users to be more careful. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Fix when installing on pationless but partionable medium
On Wed, 2009-07-22 at 19:16 +0200, Robert Millan wrote: As a bootloader, most of the decisions GRUB takes have a critical effect. We can't make GRUB take those decisions based on heuristic. Agreed. Sorry for being late in joining this discussion. It's not enough to make sure that there is some partitioning on the hard driver. boot.img has holes for FAT and PC MBR. That's two configurations we support. No other partition tables or filesystems are supported. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Fix when installing on pationless but partionable medium
On Wed, 2009-07-22 at 19:22 +0200, Robert Millan wrote: On Sat, Jul 18, 2009 at 11:28:58PM +0200, Vladimir 'phcoder' Serbinenko wrote: I don't understand what you mean here. Let's take a common example of cdrom. Most of the users and developers are accustomed to a cdrom holding one filesystem. On macs however cds are partitioned and not being able to access all the partitions is a problem for end user. Such situations are probably common. If we ditch has_partitions altogether the only negative side effect will be that in some weird configurations unpartitioned media may appear to have partitions but whole media is still accessible. Additionally it simplifies and makes kernel smaller I'm not sure they're so weird. But we could still do it. Pavel, what do you think? We don't use grub-setup to install GRUB on CDROM. I don't know if we can eliminate has_partitions. If we can, I'm fine. I suggest that we start with known safe cases, such are PC partition table and FAT filesystem. If we can positively identify the place where we install the bootsector as either of that, we can install without --force. We can extend the rules as we check if other filesystems allow overwriting the bootsector. I don't think dumping floppy support would be right at this point. Maybe five years from now. I haven't looked at the source code, but what he said is we can determine if an MBR is valid by checking the bootable flag, and this is not always so. I don't see any problem. He said: checking that bootable flags of all partitions are either set (0x80) or unset (0x0) and not another value Oh, that's different. I think it's fine provided that: - None of the commonly used free partitioning tools uses an illegal value. - We fail gracefully and let the user know why. I'm fine with extra checks. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Fix when installing on pationless but partionable medium
On Wed, 2009-07-22 at 19:43 +0200, Vladimir 'phcoder' Serbinenko wrote: On Wed, Jul 22, 2009 at 7:36 PM, Pavel Roskinpro...@gnu.org wrote: On Wed, 2009-07-22 at 19:16 +0200, Robert Millan wrote: boot.img has holes for FAT and PC MBR. That's two configurations we support. No other partition tables or filesystems are supported. Many filesystems leave first block free. These are supported too Fine with me if we can identify them. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] UUID support for UFS
On Wed, 2009-07-22 at 19:40 +0200, Robert Millan wrote: On Tue, Jul 21, 2009 at 03:03:34PM +0200, Vladimir 'phcoder' Serbinenko wrote: + grub_uint32_t uuidhi; + grub_uint32_t uuidlow; [...] + grub_sprintf (*uuid, %08lx%08lx, + (unsigned long) grub_le_to_cpu32 (data-sblock.uuidhi), + (unsigned long) grub_le_to_cpu32 (data-sblock.uuidlow)); Is this really being interpreted as mixed endian by other programs? I also wondered about it. Yes, it's mixed endian in the dumpfs output and in the kernel log. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Call a module's init function only after the module is successfully added
On Wed, 2009-07-22 at 19:36 +0200, Robert Millan wrote: On Tue, Jul 21, 2009 at 04:02:48PM -0400, Pavel Roskin wrote: By the way, kern/dl.c have some unused functions (grub_dl_unload_all). grub_machine_fini() used to rely on this, but at some point we concluded it wasn't useful to change machine state when loading an OS, other than things specific to each particular loader. As I said, I missed calls in *.S files. grub_linux16_boot() and grub_chainloader_real_boot() still use it. Perhaps they shouldn't. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Fix chainloding + Chainloading into logical partitions
On Thu, 2009-07-23 at 00:18 +0200, Vladimir 'phcoder' Serbinenko wrote: There is strictly no need to do this restructuration. The real bug is different fix would be setting dev-disk-partition to 0 before calling grub_disk_read and restoring it afterwards. This part of code is changed anyway with my nested partition patch and I was hoping it could be applied quickly. Could you test nestpart branch of my repository? I'm not comfortable to approve a 60k long patch that reorganizes the code and fixed numerous bugs in the same time. I'm sure bugs can be fixed separately. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types
On Wed, 2009-07-22 at 21:10 +0200, Javier Martín wrote:
This patch modifies the global types.h header to define a number of
macros used for the formatted output of fixed-length integers like
grub_uint64_t. Currently we use the traditional %llu format
specifiers, adding casts as required to assuage the loud GCC.
However, casts to shut the compiler up are generally a bad idea, and the
fact that this kind of output occurs mostly in debug code (printing an
inode number, for example) which is dispersed about the GRUB codebase
makes it very likely that the eventual breakage of one of these
assumptions will result in either a stream of warnings/errors or, given
that we are using casts to shut the compiler up, runtime weirdness.
I doubt about runtime weirdness. gcc is good at catching such
problems at the compile time.
Using these C99-like format specifiers will allow us to have the
relevant assumptions centralized in a single file, namely
grub/types.h. Thus, if and when one is broken - for example, when
sizeof(void*)!=sizeof(long), as it happens in mingw64 - the fix will be
way simpler to develop and deploy.
It would be a good idea to use standard modifiers if they were not so
hard on the eye.
Also, the strictness it not increased by such change. It's still
possible to use wrong modifiers and see nothing unless compiling for an
architecture where the size of the argument is different from the
expected size.
For example, using a fictional MyFS:
typedef struct {
grub_uint64_t ino_num;
grub_uint16_t perm_flags;
} myfs_ino_t;
myfs_ino_t *cur = ...
The patch will turn something like this:
grub_dprintf(myfs, Inode number %llu has permissions %03o,
(unsigned long) grub_be_to_cpu64(cur-ino_num),
grub_be_to_cpu16(cur-perm_flags));
Into this:
grub_dprintf(myfs, Inode number %GRUB_PRI64u has permissions
%03GRUB_PRI16o, grub_be_to_cpu64(cur-ino_num),
grub_be_to_cpu16(cur-perm_flags));
That's a biased example where the size of the arguments is obvious. And
even then, it's hard to read. And if you put GRUB_PRI32o instead of
GRUB_PRI16o there, the compiler won't tell you anything.
The patch includes the easy part, namely adding the macros. But I
doesn't think it would be so pretty with the ugly part, that is,
fixing every almost every *printf call. It will be very hard to
review.
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
This belongs to a separate patch.
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC,PATCH] C99 format specifiers for fixed-length integer types
On Thu, 2009-07-23 at 04:03 +0200, Javier Martín wrote: El mié, 22-07-2009 a las 21:08 -0400, Pavel Roskin escribió: I doubt about runtime weirdness. gcc is good at catching such problems at the compile time. Yes, in naked expressions. However, once we start using casts the C compiler tends to be quite silent about the whole nitty-gritty process. For example, the following code generates no warnings (with -ansi -pedantic -Wall -Wconversion): unsigned short a = (unsigned short) 123456789; uint16_t b = (uint16_t) UINT32_C(123456789); That is, casts shut the compiler up (without the casts gcc just shows a warning about the truncation being performed). The runtime value of both a and b is 0xcd15 in my x86_64 Linux box. That's totally expected. But it doesn't represent a full scenario when something can go wrong when porting to another platform. It would be a good idea to use standard modifiers if they were not so hard on the eye. Well, I suggested GRUB_x names for the macros, where x is the C99 standard name, but we could use them directly as PRIx64 and the like. About verbosity, though... Have you programmed in Java? Some appearances of GRUB_PRId32 are nothing compared to System.out.println or java.util.ArrayListInteger, yet I write the last two religiously. We are not writing GRUB in Java. I guess Java also offers way to simplify code and to check its correctness that we don't have in C. That's a biased example where the size of the arguments is obvious. And even then, it's hard to read. And if you put GRUB_PRI32o instead of GRUB_PRI16o there, the compiler won't tell you anything. Hey, I'm the one proposing the patch. Surely you wouldn't expect me to be unbiased ;) Nevertheless, the issue you pointed happens because variadic arguments are automatically promoted with the following rules [1][2] from the ancient days of C: - Integral types narrower than int - int - Floating point types narrower than double - double So, you could argue, we could do without at the very least PRI?8 (and, here in grub, PRI?16 too). However, given how obscure this feature is (see [1]), I'd go for keeping all of them: orthogonality means less surprises for the future coders. That's OK. The patch includes the easy part, namely adding the macros. But I doesn't think it would be so pretty with the ugly part, that is, fixing every almost every *printf call. It will be very hard to review. Well, of course. This patch only adds the infrastructure, or else it would be too invasive. Once it is in, a small number of people can start checking most source files and replace the old specifiers when appropriate. And that's what I don't want to see. We sacrifice readability and go through massive code changes. We only gain potential fixes for ports to hypothetical platforms. In my opinion, it's not worth the trouble. I'm not going to spend any more time on this. If you convince any maintainers, fine. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] UUID support for UFS
On Tue, 2009-07-21 at 22:01 +0200, Vladimir 'phcoder' Serbinenko wrote: On Tue, Jul 21, 2009 at 7:14 PM, Pavel Roskinpro...@gnu.org wrote: On Tue, 2009-07-21 at 15:03 +0200, Vladimir 'phcoder' Serbinenko wrote: + grub_sprintf (*uuid, %08lx%08lx, + (unsigned long) grub_le_to_cpu32 (data-sblock.uuidhi), + (unsigned long) grub_le_to_cpu32 (data-sblock.uuidlow)); unsigned long is 64-bit on x86_64. unsigned int would do just fine here. Ok I would add a dash between the numbers to make it more readable unless there is a precedent where the dash is not used. If you look into /dev/ufsid/ then you'll see that it has no dash. This is also a syntax used in set FreeBSD.vfs.root.mountfrom=ufs:ufsid/id So making it with dash would need additional conversion to pass it to FreeBSD OK then. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Call a module's init function only after the module is successfully added
On Tue, 2009-07-21 at 16:02 -0400, Pavel Roskin wrote: By the way, kern/dl.c have some unused functions (grub_dl_unload_all). I was wrong, I didn't check assembler files. Also, it looks like the memory for the module information (grub_dl_t) is allocated twice - first in grub_dl_load_core(), then in grub_dl_add() as part of grub_dl_list_t. I was wrong here too. I'm too accustomed to Linux style where typedefs are discouraged. The list keeps a pointer, not a copy. I'll apply your patch if it passes testing. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] UUID support for UFS
On Tue, 2009-07-21 at 23:45 +0200, Vladimir 'phcoder' Serbinenko wrote: This change would allow to produce a code which is cleaner, easier to read and understand. However I'm opposed to modifying printf function for it. I think Javier misspoke or didn't realize that *printf doesn't need to be modified. Instead we could just define somewhere: GRUB_PRIx32 %x Better yet, x, as in libc. That would allow for %08x that we need for UUID. But I would prefer that we work on fixing bugs rather than non-bugs. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] UUID support for UFS
On Tue, 2009-07-21 at 23:34 +0200, Javier Martín wrote: If int and int32_t are different types, gcc will warn about it, at least for implicit conversion with data loss. Oh, yes... with the current build system and without -Werror, warnings are _very_ visible. /sarcasm I just use make /dev/null Besides, do we really have -Wconversion enabled? No. Have you actually tried it? It finds some interesting stuff. For instance, the return value of grub_file_seek(). We can clean whatever -Wconversion finds. That may find some real bugs and it will prepare us to supporting new architectures. I don't know, but gcc tends to be quite silent when it comes to type conversion, mainly due to the laxitude it's used in *nix C programs. The cast in that code was all but implicit, and explicit casts tend to shut the compiler up. However, we are not adding support for architectures with non-32-bit int type right now. Things may improve by the time we start that effort. New format specifiers can appear to deal with 32-bit numbers. It's more likely that bugs will be introduced by that change, not fixed. Besides, the code will be harder to read. You say it'd be harder to read because the macros are newly-introduced (C99) and thus not widely know. However, they are pretty clear and self-explanatory once you google them the first time... and at the very least they call attention to themselves: an unknowing programmer would wonder what they are. Using a normal print specifier and a type cast does not. OK, if you can do it if you want. It would be great if you fix some real bugs in process. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/3] Don't initialize module before grub_dl_add() succeeds
ChangeLog:
* kern/dl.c (grub_dl_load_core): Call grub_dl_call_init() only
after grub_dl_add() succeeds. Set mod-ref_count to 1 later to
allow grub_dl_unload() to work.
Original patch by Joe Auricchio jauricc...@gmail.com
---
kern/dl.c | 14 +-
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/kern/dl.c b/kern/dl.c
index 78ebc1e..ebde547 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -539,14 +539,13 @@ grub_dl_load_core (void *addr, grub_size_t size)
if (! mod)
return 0;
- mod-ref_count = 1;
-
grub_dprintf (modules, relocating to %p\n, mod);
if (grub_dl_resolve_name (mod, e)
|| grub_dl_resolve_dependencies (mod, e)
|| grub_dl_load_segments (mod, e)
|| grub_dl_resolve_symbols (mod, e)
- || grub_arch_dl_relocate_symbols (mod, e))
+ || grub_arch_dl_relocate_symbols (mod, e)
+ || grub_dl_add (mod))
{
mod-fini = 0;
grub_dl_unload (mod);
@@ -557,13 +556,10 @@ grub_dl_load_core (void *addr, grub_size_t size)
grub_dprintf (modules, module name: %s\n, mod-name);
grub_dprintf (modules, init function: %p\n, mod-init);
- grub_dl_call_init (mod);
- if (grub_dl_add (mod))
-{
- grub_dl_unload (mod);
- return 0;
-}
+ mod-ref_count = 1;
+
+ grub_dl_call_init (mod);
return mod;
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/3] Eliminate grub_dl_call_init()
It's just two lines long and there is only one caller. Besides, there
is no equivalent for mod-fini.
ChangeLog:
* kern/dl.c (grub_dl_call_init): Remove.
(grub_dl_load_core): Call mod-init directly.
---
kern/dl.c | 10 ++
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/kern/dl.c b/kern/dl.c
index ebde547..e2382d6 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -393,13 +393,6 @@ grub_dl_resolve_symbols (grub_dl_t mod, Elf_Ehdr *e)
return GRUB_ERR_NONE;
}
-static void
-grub_dl_call_init (grub_dl_t mod)
-{
- if (mod-init)
-(mod-init) (mod);
-}
-
static grub_err_t
grub_dl_resolve_name (grub_dl_t mod, Elf_Ehdr *e)
{
@@ -559,7 +552,8 @@ grub_dl_load_core (void *addr, grub_size_t size)
mod-ref_count = 1;
- grub_dl_call_init (mod);
+ if (mod-init)
+(mod-init) (mod);
return mod;
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/3] Merge struct grub_dl_list into struct grub_dl
There is no need to keep a separate list of the modules. It requires
additional memory allocation and additional error handling. Now
grub_dl_add() can only fail if the module is already loaded.
ChangeLog:
* include/grub/dl.h (struct grub_dl): Add next field.
* kern/dl.c: Remove struct grub_dl_list. Use mod-next to
iterate over modules.
---
include/grub/dl.h |1 +
kern/dl.c | 55 +++--
2 files changed, 21 insertions(+), 35 deletions(-)
diff --git a/include/grub/dl.h b/include/grub/dl.h
index 3f8b328..55cc6cc 100644
--- a/include/grub/dl.h
+++ b/include/grub/dl.h
@@ -82,6 +82,7 @@ struct grub_dl
Elf_Sym *symtab;
void (*init) (struct grub_dl *mod);
void (*fini) (void);
+ struct grub_dl *next;
};
typedef struct grub_dl *grub_dl_t;
diff --git a/kern/dl.c b/kern/dl.c
index e2382d6..81c211e 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -40,31 +40,17 @@
-struct grub_dl_list
-{
- struct grub_dl_list *next;
- grub_dl_t mod;
-};
-typedef struct grub_dl_list *grub_dl_list_t;
-
-static grub_dl_list_t grub_dl_head;
+static grub_dl_t grub_dl_head;
static grub_err_t
grub_dl_add (grub_dl_t mod)
{
- grub_dl_list_t l;
-
if (grub_dl_get (mod-name))
return grub_error (GRUB_ERR_BAD_MODULE,
`%s' is already loaded, mod-name);
- l = (grub_dl_list_t) grub_malloc (sizeof (*l));
- if (! l)
-return grub_errno;
-
- l-mod = mod;
- l-next = grub_dl_head;
- grub_dl_head = l;
+ mod-next = grub_dl_head;
+ grub_dl_head = mod;
return GRUB_ERR_NONE;
}
@@ -72,13 +58,12 @@ grub_dl_add (grub_dl_t mod)
static void
grub_dl_remove (grub_dl_t mod)
{
- grub_dl_list_t *p, q;
+ grub_dl_t *p, q;
for (p = grub_dl_head, q = *p; q; p = q-next, q = *p)
-if (q-mod == mod)
+if (q == mod)
{
*p = q-next;
- grub_free (q);
return;
}
}
@@ -86,11 +71,11 @@ grub_dl_remove (grub_dl_t mod)
grub_dl_t
grub_dl_get (const char *name)
{
- grub_dl_list_t l;
+ grub_dl_t mod;
- for (l = grub_dl_head; l; l = l-next)
-if (grub_strcmp (name, l-mod-name) == 0)
- return l-mod;
+ for (mod = grub_dl_head; mod; mod = mod-next)
+if (grub_strcmp (name, mod-name) == 0)
+ return mod;
return 0;
}
@@ -98,10 +83,10 @@ grub_dl_get (const char *name)
void
grub_dl_iterate (int (*hook) (grub_dl_t mod))
{
- grub_dl_list_t l;
+ grub_dl_t mod;
- for (l = grub_dl_head; l; l = l-next)
-if (hook (l-mod))
+ for (mod = grub_dl_head; mod; mod = mod-next)
+if (hook (mod))
break;
}
@@ -684,17 +669,17 @@ grub_dl_unload_unneeded (void)
{
/* Because grub_dl_remove modifies the list of modules, this
implementation is tricky. */
- grub_dl_list_t p = grub_dl_head;
+ grub_dl_t mod = grub_dl_head;
- while (p)
+ while (mod)
{
- if (grub_dl_unload (p-mod))
+ if (grub_dl_unload (mod))
{
- p = grub_dl_head;
+ mod = grub_dl_head;
continue;
}
- p = p-next;
+ mod = mod-next;
}
}
@@ -704,13 +689,13 @@ grub_dl_unload_all (void)
{
while (grub_dl_head)
{
- grub_dl_list_t p;
+ grub_dl_t mod;
grub_dl_unload_unneeded ();
/* Force to decrement the ref count. This will purge pre-loaded
modules and manually inserted modules. */
- for (p = grub_dl_head; p; p = p-next)
- p-mod-ref_count--;
+ for (mod = grub_dl_head; mod; mod = mod-next)
+ mod-ref_count--;
}
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 1/4] Handle errors in rmmod
ChangeLog:
* commands/minicmd.c (grub_mini_cmd_rmmod): Check the result of
grub_dl_unload(), but not of grub_dl_unref(). On failure,
restore reference count and report error.
---
commands/minicmd.c |8 ++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/commands/minicmd.c b/commands/minicmd.c
index b314388..1f5abae 100644
--- a/commands/minicmd.c
+++ b/commands/minicmd.c
@@ -288,8 +288,12 @@ grub_mini_cmd_rmmod (struct grub_command *cmd
__attribute__ ((unused)),
if (! mod)
return grub_error (GRUB_ERR_BAD_ARGUMENT, no such module);
- if (grub_dl_unref (mod) = 0)
-grub_dl_unload (mod);
+ grub_dl_unref (mod);
+ if (grub_dl_unload (mod) == 0)
+{
+ grub_dl_ref (mod);
+ return grub_error (GRUB_ERR_BAD_MODULE, `%s' is in use, mod-name);
+}
return 0;
}
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 2/4] Fix insmod not to increase refcount above 1
grub_dl_get() can return non-NULL for an already loaded module. insmod should not increase it refcount. Only increase refcount if the module is newly loaded or if the module is unused and autoloaded, so that it won't get unloaded automatically. It's not perfect, but close enough. insmod won't lock autoloaded modules with dependencies. But it can be fixed by running insmod on the dependent module. ChangeLog: * kern/corecmd.c (grub_core_cmd_insmod): Exit without increasing refcount if the module is already loaded. --- kern/corecmd.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/kern/corecmd.c b/kern/corecmd.c index 03944f2..078b33e 100644 --- a/kern/corecmd.c +++ b/kern/corecmd.c @@ -102,7 +102,8 @@ grub_core_cmd_insmod (struct grub_command *cmd __attribute__ ((unused)), else mod = grub_dl_load_file (argv[0]); - if (mod) + /* Pin module in memory unless already pinned */ + if (mod mod-ref_count == 0) grub_dl_ref (mod); return 0; ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH 3/4] Change grub_file_seek() to return grub_err_t
No callers need the previous offset. Returning -1 with implicit cast to
grub_off_t required a cast just to check for errors. This also makes
grub_file_seek() more similar to fseek().
ChangeLog:
* kern/file.c (grub_file_seek): Return grub_err_t. Adjust all
callers that check the return value.
---
commands/minicmd.c|4 ++--
font/font.c |2 +-
include/grub/file.h |2 +-
kern/elf.c| 10 +-
kern/file.c | 12
loader/aout.c |2 +-
loader/i386/bsd.c |2 +-
loader/i386/bsdXX.c | 10 +-
loader/i386/multiboot.c |2 +-
loader/i386/multiboot_elfxx.c |4 ++--
loader/macho.c| 14 +++---
loader/xnu_resume.c |2 +-
12 files changed, 31 insertions(+), 35 deletions(-)
diff --git a/commands/minicmd.c b/commands/minicmd.c
index 1f5abae..bc6458d 100644
--- a/commands/minicmd.c
+++ b/commands/minicmd.c
@@ -188,7 +188,7 @@ grub_rescue_cmd_testload (int argc, char *argv[])
/* Read sequentially again. */
grub_printf (Reading %s sequentially again, argv[0]);
- if (grub_file_seek (file, 0) 0)
+ if (grub_file_seek (file, 0) != GRUB_ERR_NONE)
goto fail;
for (pos = 0; pos size; pos += GRUB_DISK_SECTOR_SIZE)
@@ -216,7 +216,7 @@ grub_rescue_cmd_testload (int argc, char *argv[])
pos -= GRUB_DISK_SECTOR_SIZE;
- if (grub_file_seek (file, pos) 0)
+ if (grub_file_seek (file, pos) != GRUB_ERR_NONE)
goto fail;
if (grub_file_read (file, sector, GRUB_DISK_SECTOR_SIZE)
diff --git a/font/font.c b/font/font.c
index a812919..67b97d2 100644
--- a/font/font.c
+++ b/font/font.c
@@ -530,7 +530,7 @@ grub_font_load (const char *filename)
grub_printf(Unhandled section type, skipping.\n);
#endif
grub_off_t section_end = grub_file_tell (file) + section.length;
- if ((int) grub_file_seek (file, section_end) == -1)
+ if (grub_file_seek (file, section_end) != GRUB_ERR_NONE)
goto fail;
}
}
diff --git a/include/grub/file.h b/include/grub/file.h
index 2aacf93..88fa088 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -54,7 +54,7 @@ char *EXPORT_FUNC(grub_file_get_device_name) (const char
*name);
grub_file_t EXPORT_FUNC(grub_file_open) (const char *name);
grub_ssize_t EXPORT_FUNC(grub_file_read) (grub_file_t file, void *buf,
grub_size_t len);
-grub_off_t EXPORT_FUNC(grub_file_seek) (grub_file_t file, grub_off_t offset);
+grub_err_t EXPORT_FUNC(grub_file_seek) (grub_file_t file, grub_off_t offset);
grub_err_t EXPORT_FUNC(grub_file_close) (grub_file_t file);
static inline grub_off_t
diff --git a/kern/elf.c b/kern/elf.c
index f141610..fdb7d94 100644
--- a/kern/elf.c
+++ b/kern/elf.c
@@ -67,7 +67,7 @@ grub_elf_file (grub_file_t file)
elf-file = file;
- if (grub_file_seek (elf-file, 0) == (grub_off_t) -1)
+ if (grub_file_seek (elf-file, 0) != GRUB_ERR_NONE)
goto fail;
if (grub_file_read (elf-file, elf-ehdr, sizeof (elf-ehdr))
@@ -130,7 +130,7 @@ grub_elf32_load_phdrs (grub_elf_t elf)
if (! elf-phdrs)
return grub_errno;
- if ((grub_file_seek (elf-file, elf-ehdr.ehdr32.e_phoff) == (grub_off_t) -1)
+ if ((grub_file_seek (elf-file, elf-ehdr.ehdr32.e_phoff) != GRUB_ERR_NONE)
|| (grub_file_read (elf-file, elf-phdrs, phdrs_size) != phdrs_size))
{
grub_error_push ();
@@ -243,7 +243,7 @@ grub_elf32_load (grub_elf_t _elf, grub_elf32_load_hook_t
_load_hook,
(unsigned long long) load_addr,
(unsigned long long) phdr-p_memsz);
-if (grub_file_seek (elf-file, phdr-p_offset) == (grub_off_t) -1)
+if (grub_file_seek (elf-file, phdr-p_offset) != GRUB_ERR_NONE)
{
grub_error_push ();
return grub_error (GRUB_ERR_BAD_OS,
@@ -309,7 +309,7 @@ grub_elf64_load_phdrs (grub_elf_t elf)
if (! elf-phdrs)
return grub_errno;
- if ((grub_file_seek (elf-file, elf-ehdr.ehdr64.e_phoff) == (grub_off_t) -1)
+ if ((grub_file_seek (elf-file, elf-ehdr.ehdr64.e_phoff) != GRUB_ERR_NONE)
|| (grub_file_read (elf-file, elf-phdrs, phdrs_size) != phdrs_size))
{
grub_error_push ();
@@ -423,7 +423,7 @@ grub_elf64_load (grub_elf_t _elf, grub_elf64_load_hook_t
_load_hook,
(unsigned long long) load_addr,
(unsigned long long) phdr-p_memsz);
-if (grub_file_seek (elf-file, phdr-p_offset) == (grub_off_t) -1)
+if (grub_file_seek (elf-file, phdr-p_offset) != GRUB_ERR_NONE)
{
grub_error_push ();
return grub_error (GRUB_ERR_BAD_OS,
diff --git a/kern/file.c b/kern/file.c
index 9b56b88..647fb8c 100644
--- a/kern/file.c
+++ b/kern/file.c
@@ -141,19 +141,15 @@ grub_file_close (grub_file_t file)
return grub_errno;
}
-grub_off_t
+grub_err_t
grub_file_seek (grub_file_t file, grub_off_t
[PATCH 4/4] Fix ALIGN_UP cutting upper bits
If align is unsigned int, ~(align - 1) will also be unsigned int and will cut addr to 32 bits. Cast align to the type of addr. This also avoid 64-bit calculations if addr is 32-bit. ChangeLog: * include/grub/misc.h (ALIGN_UP): Cast align to the type of addr to avoid loss of upper bits if align is unsigned and shorter than addr. --- include/grub/misc.h |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/include/grub/misc.h b/include/grub/misc.h index e229062..769ec5c 100644 --- a/include/grub/misc.h +++ b/include/grub/misc.h @@ -25,7 +25,8 @@ #include grub/symbol.h #include grub/err.h -#define ALIGN_UP(addr, align) (((grub_uint64_t)addr + align - 1) ~(align - 1)) +#define ALIGN_UP(addr, align) \ + ((addr + (typeof (addr)) align - 1) ~((typeof (addr)) align - 1)) #define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0])) #define grub_dprintf(condition, fmt, args...) grub_real_dprintf(__FILE__, __LINE__, condition, fmt, ## args) ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/2] Disable lzo compression, lzma is doing its job just fine
On Sat, 2009-07-18 at 20:18 +0200, Robert Millan wrote: It doesn't break things, since it's barely modified, and doesn't interact with the rest of the code, but simply having more code means an added work to maintain it when we restructure things, etc. It needs to pay off in some way. Exactly. Actually, my intention was to improve test coverage. To test lzma and lzo, GRUB needs to be compiled twice. If we want to support MacOS compilation properly (I mean local labels), we'll need to adjust both lzma and lzo. Extra code means more development time wasted on mostly unused code. There then there is an issue of user choice. I think we are offering too many choices without explaining what's behind it. I could say lzo is old and proven and lzma is new and more effective, but since lzo is disabled by default and there are no complaints about it, keeping lzo becomes pointless. I don't think we are going to see compression much more effecting than lzma to justify keeping the infrastructure for more than one compression algorithm. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] AFS fixes and improvements
On Sun, 2009-07-19 at 17:34 +0200, Vladimir 'phcoder' Serbinenko wrote: It's better to split fixes from the new features. Attached patches I don't have afs images around. It would be great if you test all new functionality with valgrind. It's very good at finding mistakes in the code. I will do. I use the publically-available image from http://web.syllable.org/pages/get-Syllable.html#emulate I could not use those images directly, but I installed Syllable on a virtual hard drive under qemu. I could successfully read the image with the patches. I tried grub-fstest under valgrind, and it was fine. Thanks for the patches! -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] AFS fixes and improvements
On Sun, 2009-07-19 at 15:20 +0200, Vladimir 'phcoder' Serbinenko wrote:
- if ((! dir-inode.stream.size) ||
+ if ((dir-inode.stream.size == 0) ||
The later is marginally better, but it would be easier to review your
patches if you don't include such changes.
It's not a stylistic improvement. dir-inode.stream.size is 64-bit
quantity which will be truncated on 32-bit platform
No, values are not truncated like that. No implicit conversions shorten
the value. That worst thing you can get is conversion from signed to
unsigned or vice versa, but you will get a warning about it with -Wall.
Just try compiling this for 32-bit:
#include stdio.h
unsigned long long size = 0x1ULL;
int main()
{
if (! size)
printf(test1: size is 0\n);
else
printf(test1: size is not 0\n);
if (size == 0)
printf(test2: size is 0\n);
else
printf(test2: size is not 0\n);
}
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] change --enable-efiemu to --enable-efiemu64
On Sat, 2009-07-18 at 20:32 +0200, Robert Millan wrote: On Fri, Jul 17, 2009 at 06:15:57PM +0200, Vladimir 'phcoder' Serbinenko wrote: Like previously discussed efiemu32 can always be compiled hence this patch This does a few more things than just toggle compile options. Could you split the patch? I second that. Also, we need to look at things from the users' point of view. Users don't know what efiemu64 is and why it's so special that they are told about it at the of the configure output. Why it is more important that efiemu32? Why is efiemu64 only needed on the i386-pc platform? I believe efiemu64 is only more important from the implementation point of view, as it requires an additional test to be passed. But most users won't need efiemu64 at all. If we add efiemu compilation to the x86_64-efi support, efiemu32 will be special. So it would be better to report something like efiemu support - 32-bit and 64-bit if we care to report. I suggest that you google for The Paradox of Choice. Sure, there are difference between end users and those compiling GRUB from the sources, but you'll get the idea. Sometimes we should make the choice. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] enable buildable targets by default
On Sat, 2009-07-18 at 23:38 +0200, Vladimir 'phcoder' Serbinenko wrote: I fully agree. The idea with my proposal (we talked this on IRC, I think) was to simplify things, so I proposed that we enable everything so we don't have to provide flags, etc. The main pro was to avoid frequent breakage as we experienced with grub-emu. I just use a script to build grub for all supported platforms with all supported options. It's quite good for finding such errors. As it stands now, there are no errors and no warnings. But it took a while to fix everything. I think we should eventually revert to not building debug tools by default. I prefer if we got rid of the flags, or at least most of them (lumping them together with a flag to disable debug tools or so), I agree. grub-emu, grub-fstest and efiemu are debug tools. Neither is needed for normal operation on any platform. I'm undecided on this point. On one hand they can be useful if used correctly on the other hand if removing them would avoid users doing errors What kind of errors do you mean? and remove the summary message as well. However I really think summary message should remain. It avoids developpers to scroll through tons of messages to find out if the part they need is built and if it isn't why The summary message is OK with me, be we should keep it short and to the point. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] xfs bug fix
Quoting Bean bean12...@gmail.com:
The current xfs driver uses fixed inode size (256), this patch should fix it.
I'm afraid this patch doesn't take something into account. I tried
creating an image by
mkfs.xfs -i log=9 /root/tmp/xfs.img -f
Then I copied stgit sources on top of it (it's just a directory with
many files).
Then I ran
valgrind grub-fstest /root/tmp/xfs.img ls -al '/stgit'
It reported this problem:
==11029== Conditional jump or move depends on uninitialised value(s)
==11029==at 0x403CAE: grub_disk_adjust_range (disk.c:373)
==11029==by 0x403D6C: grub_disk_read (disk.c:392)
==11029==by 0x41CB8C: grub_xfs_read_inode (xfs.c:222)
==11029==by 0x41D217: call_hook.2830 (xfs.c:414)
==11029==by 0x41D49C: grub_xfs_iterate_dir (xfs.c:469)
==11029==by 0x41DA6B: grub_xfs_dir (xfs.c:655)
==11029==by 0x408E72: grub_ls_list_files (ls.c:195)
==11029==by 0x4093F8: grub_cmd_ls (ls.c:252)
==11029==by 0x407CB8: grub_extcmd_dispatcher (extcmd.c:48)
==11029==by 0x400EF1: execute_command (grub-fstest.c:74)
==11029==by 0x4016AC: fstest (grub-fstest.c:300)
==11029==by 0x401EBC: main (grub-fstest.c:550)
I tried to track it down, and it appears that line 465 in call_hook()
is responsible:
ino = *(grub_uint64_t *) inopos;
First inopos points to valid memory, but at some point it starts
pointing to invalid memory. I don't get this problem if I omit -i
log=9 from the mkfs.xfs arguments.
I got some interesting warnings from the RAID code too, which I
suppressed by this patch:
--- a/disk/raid.c
+++ b/disk/raid.c
@@ -613,6 +613,7 @@ grub_raid_scan_device (int head_only)
for (p = grub_raid_list; p; p = p-next)
{
+ grub_memset(array, 0, sizeof(array));
if (! p-detect (disk, array))
{
if (! insert_array (disk, array, p-name))
Let's fix errors first and then we can add improvements.
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] xfs bug fix
Quoting Pavel Roskin pro...@gnu.org: ==11029== Conditional jump or move depends on uninitialised value(s) ==11029==at 0x403CAE: grub_disk_adjust_range (disk.c:373) ==11029==by 0x403D6C: grub_disk_read (disk.c:392) ==11029==by 0x41CB8C: grub_xfs_read_inode (xfs.c:222) P.S. It looks like struct grub_xfs_inode hardcodes size 256, so every use of sizeof (struct grub_xfs_inode) is suspicious. Likewise, struct grub_fshelp_node and struct grub_xfs_data should not be used blindly with sizeof. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] AFS fixes and improvements
Quoting Vladimir 'phcoder' Serbinenko phco...@gmail.com: Update: added symlink support On Fri, Jul 17, 2009 at 9:30 PM, Vladimir 'phcoder' Serbinenkophco...@gmail.com wrote: Hello. Currently I'm coding BFS (filesystem of BeOS and Haiku) which is similar to AFS. So I took the later as codebase. I found some bugs and incompletenesses in it. Here is the fix. Tested using grub-fstest on virtual machine image downloaded from Syllable website and then successfully booted from it using grub-mkrescue image Many changes have no value at all, e.g.: -#defineGRUB_AFS_SBLOCK_MAGIC1 0x41465331 /* AFS1 */ +/* AFS1 in hexadecimal. */ +#defineGRUB_AFS_SBLOCK_MAGIC1 0x41465331 #defineGRUB_AFS_SBLOCK_MAGIC2 0xdd121031 #defineGRUB_AFS_SBLOCK_MAGIC3 0x15b6830e The original comment suggested that only the first value made it clear that only the first value is AFS1. Now it's unclear. There is no need to point out that AFS1 is in hexadecimal notation. It's obvious to anyone competent to read C code. Besides, there is no need to single out AFS1. - grub_uint32_t index_type;/* Key data-key only used for index files */ + /* Key data-key only used for index files. */ + grub_uint32_t index_type; grub_uint32_t inode_size; The same comment applies. - if ((! dir-inode.stream.size) || + if ((dir-inode.stream.size == 0) || The later is marginally better, but it would be easier to review your patches if you don't include such changes. Double semicolons can be removed in all files, and it doesn't need a review. It's better to split fixes from the new features. I don't have afs images around. It would be great if you test all new functionality with valgrind. It's very good at finding mistakes in the code. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: New developing branch of grub2
On Fri, 2009-07-17 at 17:52 +0800, Bean wrote: Hi, I've created a repository at GitHub to hold some developing patches, the main repos is at: http://github.com/grub/grub/ master is the developing branch, while svn is the mirror of grub2 svn. I also have a forked project at: http://github.com/bean123/grub/ The lib branch contains the new object format code. It's hard to see the unmerged changes in those repositories. I can tell from my experience with Linux kernel development that branches are not used for such efforts. Patches need to be posted, reviewed and discussed by others. Separate branches or repositories are used for subsystems that have their own maintainers. That is, there is a repository for wireless networking. Patches are still posted and reviewed in a separate list. Once they are good, they are committed to the subsystem repository. And then Linus pulls from it because he trusts the subsystem maintainers and knows that the patches have been reviewed. When patches are publishing for review, there is incentive for the patch author to make changes clear. When the patches are published as a branch, chances are that the development will go too far before other developers have a look at the code. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] enable buildable targets by default
On Fri, 2009-07-17 at 11:21 +0200, Vladimir 'phcoder' Serbinenko wrote: On Fri, Jul 17, 2009 at 2:37 AM, Pavel Roskinpro...@gnu.org wrote: Quoting Vladimir 'phcoder' Serbinenko phco...@gmail.com: comitted I wish I had time to review it :-( I have fixed the easy stuff (spelling, wrong variables, use of $no, Thanks use of -c in CFLAGS). This was done on purpose so that we don't need gcc-multilib for tests AC_COMPILE_IFELSE already means that the program is compiled but not linked. I tried removing glibc-devel.i586, and everything is still OK. The remaining problem is that efiemu only compiles on i386-pc, but configure not only checks for the necessary flags on other platforms, but also announces that it will be built. Actually the check is necessary only for efiemu64. Perhaps we should always build efiemu32 and make grub fallback to efiemu32 even on 64-bit platform if efiemu64 is unavailable. Then enable-efiemu would be renamed to enable-efiemu64 That's a separate issue. And then there is a problem that efiemu is undocumented, so users cannot decide whether they need it or not. Perhaps we can compile efiemu on i386 and x86_64, but certainly not on powerpc and sparc. Once everything is compiled by default, some of the configure options become less meaningful, so we can consider removing them. It's not so important to disable some feature as it is to enable it. IMO if user does some not so useful things it's his problems. It's not a reason to limit flexibility I think we should provide meaningful options. For example, an option to disable compiling debugging tools, or an option to add extra sanity checks. An option to disable efiemu64 is not meaningful for someone who doesn't know how to use efiemu64. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Support for OSX
On Thu, 2009-07-16 at 19:45 +0200, Yves Blusseau wrote:
Hi,
here's a patch so grub-setup can be compiled and work on MacOSX.
# if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
if (ioctl (fd, DIOCGMEDIASIZE, nr))
+# elif defined(__APPLE__)
+ if (ioctl (fd, DKIOCGETBLOCKCOUNT, nr))
# else
...
+#if defined (__APPLE__)
+ disk-total_sectors = nr;
+#else
disk-total_sectors = nr / 512;
This is misleading. The same variable nr is used for different values
that are obtained by different calls.
@@ -1047,11 +1063,10 @@ grub_util_biosdisk_get_grub_dev (const char
*os_dev)
if (strncmp (/dev/, os_dev, 5) == 0)
{
-const char *p;
-char *q;
+char *p, *q;
long int n;
-for (p = os_dev + 5; *p; ++p)
+for (p = (char *) os_dev + 5; *p; ++p)
The change to grub_util_biosdisk_get_grub_dev() is not described.
The only problem remaining is trampoline (nested functions), because
OSX don't want
to execute code in the heap.
I think we should start an effort to eliminate the nested functions one
at a time.
--
Regards,
Pavel Roskin
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] enable buildable targets by default
Quoting Vladimir 'phcoder' Serbinenko phco...@gmail.com: comitted I wish I had time to review it :-( I have fixed the easy stuff (spelling, wrong variables, use of $no, use of -c in CFLAGS). The remaining problem is that efiemu only compiles on i386-pc, but configure not only checks for the necessary flags on other platforms, but also announces that it will be built. Perhaps we can compile efiemu on i386 and x86_64, but certainly not on powerpc and sparc. Once everything is compiled by default, some of the configure options become less meaningful, so we can consider removing them. It's not so important to disable some feature as it is to enable it. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 7/7] RFC: Use correct addresses, eliminate manual relocations
On Tue, 2009-07-14 at 21:00 -0400, Pavel Roskin wrote: ChangeLog: * boot/i386/pc/boot.S: Declare 0x0-0x7c00 as a discardable .bss segment. Eliminate ABS, rely on the assembler knowing correct addresses. Eliminate .bss segment for the kernel, use direct jump to the kernel address. I managed to install a Darwin cross-compiler, and it turn out it won't accept .bss in assembler files. Using real addresses would be very, very nice, but we'll need to find a portable way. Maybe grub-macho2img should be taught to cut away sections filled with zeroes. So patches 5 and 7 are dropped for now. Patches 1-4 and 6 have been applied. Installation on FAT32 is safe now. The Hard Disk message has been preserved, so it can be shortened later if the real need arises. That said, using direct jump to 0x8000 would save 3 bytes, and then we can save 2 bytes by taking an unconditional jump from the disk check code and reverting the logic. And maybe we could save 1 byte by yanking cli as writing to %ss disables interrupts until the next instruction. While testing the patches in qemu, I've seen some error messages and found that they should be followed by a new line, as qemu adds its own error message. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] unused constants in i386/boot.h
On Wed, 2009-07-15 at 09:36 +0200, Yves Blusseau wrote: Cool, i have planed to do it but if you do the job it'll be great ! It's committed. I'm looking at the sanity checks in grub-setup now. By the way, please do something about your mailer. You messages come in HTML format, which makes it hard to quote them. -- Regards, Pavel Roskin ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
