Re: [PATCH] Move assembly code out of the kernel

2008-07-31 Thread Bean
On Thu, Jul 31, 2008 at 5:20 AM, Bean <[EMAIL PROTECTED]> wrote:
> Hi,
>
> Currently, most assembly code are in startup.S. This is normally used
> to ensure that the function address are below 1m, which is required if
> it would switch to real mode and call bios services. However, this
> make the kernel larger. For example, the biosdisk functions are only
> used by biodisk module, they should not be placed inside the kernel.
>
> This patch support splitting such code from startup.S. For example, we
> create a new module biosdisk_stub.mod for assembly code of biosdisk.
> Instead of call prot_to_real and real_to_prot, we call
> grub_call_real_stub to enter real mode. grub_call_real_stub would copy
> the code to real mode and do the mode switch.
>
> To avoid unnecessary memory transfer, grub_call_real_stub would not
> erase the real mode stub when it's done, so that it can be used
> directly next time. When the stub area is full, it zero it out and
> start anew. The area uses a simple verification method so that the old
> mapping is invalidated, the code would need be copied again on their
> next use.
>
> The patch shows how to do it for the biosdisk module, here is the new
> grub_biosdisk_rw_int13_extensions function.
>
> REAL_STUB_START(grub_biosdisk_rw_int13_extensions)
>movb%dh, %ah
>movw%cx, %ds
>int $0x13   /* do the operation */
>movb%ah, %dl/* save return value */
>lret
> REAL_STUB_END(grub_biosdisk_rw_int13_extensions)
>
> FUNCTION(grub_biosdisk_rw_int13_extensions)
>pushl   %ebp
>pushl   %esi
>
>/* compute the address of disk_address_packet */
>movw%cx, %si
>xorw%cx, %cx
>shrl$4, %ecx/* save the segment to cx */
>
>/* ah */
>movb%al, %dh
>
>lealgrub_biosdisk_rw_int13_extensions_stub, %eax
>callEXT_C(grub_call_real_stub)
>
>movb%dl, %al/* return value in %eax */
>
>popl%esi
>popl%ebp
>
>ret
>
> Real mode code is enclosed between REAL_STUB_START and REAL_STUB_END,
> no need to use .code16 and .code32 as it's handled by the macro. In
> the main function, use
>
>lealgrub_biosdisk_rw_int13_extensions_stub, %eax
>callEXT_C(grub_call_real_stub)
>
> to invoke grub_call_real_stub. grub_biosdisk_rw_int13_extensions_stub
> is defined in the REAL_STUB_START macro.
>
> This same method can be applied to loaders, vbe, etc. In fact, almost
> all function behind grub_call_real_stub can be moved out of startup.S.

Hi,

I make some adjustment for the patch. First, I don't try to remap
blocks anymore, it's not so reliable. and anyway, the real stub area
is 0x8000 long, which is more than enough for the assembly code. If
the area would ever overflow, I just halt grub2. This makes the code
much simpler, and also, there is no need for an extra field to store
the mapped address, we just replace the original address with the
mapped one.

I also add a grub_alloc_real_stub function, which can be used to
allocate memory from the stub area that can be used to to  communicate
with real mode service. Please note that once allocated, the space
can't be released, so remember to save the pointer for later use.

Some real mode service need some kind of alignment for the input data,
so I add two more function grub_call_real_stub_align and
grub_alloc_real_stub_align, which have a alignment parameter. 1 means
no alignment, 2 word alignment, 4 dword alignment, etc. Don't use 0,
it will cause problem for the function.

2008-07-31  Bean  <[EMAIL PROTECTED]>

* conf/i386-pc.rmk (biosdisk_mod_SOURCES): Add
disk/i386/pc/biosdisk_stub.S.

* disk/i386/pc/biosdisk_stub.S: New file.

* include/grub/i386/pc/biosdisk.h (grub_biosdisk_rw_int13_extensions):
Remove EXPORT_FUNC, as we have moved it out of the kernel.
(grub_biosdisk_rw_standard): Likewise.
(grub_biosdisk_check_int13_extensions): Likewise.
(grub_biosdisk_get_diskinfo_int13_extensions): Likewise.
(grub_biosdisk_get_cdinfo_int13_extensions): Likewise.
(grub_biosdisk_get_diskinfo_standard): Likewise.
(grub_biosdisk_get_num_floppies): Likewise.

* include/grub/i386/pc/kernel.h (grub_real_stub): New structure.
(grub_alloc_real_stub): New function.
(grub_alloc_real_stub_align): Likewise.
(grub_call_real_stub): Likewise.
(grub_call_real_stub_align): Likewise.

* include/grub/i386/pc/memory.h (GRUB_MEMORY_MACHINE_REAL_STUB_START):
New macro.
(GRUB_MEMORY_MACHINE_REAL_STUB_SIZE): Likewise.
(GRUB_MEMORY_MACHINE_REAL_STUB_END): Likewise.
(GRUB_MEMORY_MACHINE_RESERVED_END): Change value.

* include/symbol.h (REAL_STUB_START): New macro.
(REAL_STUB_END): Likewise.

* kern/i386/pc/startup.S (real_stub_addr): New variable.
(grub_alloc_real_stub): New

Re: [PATCH] Move assembly code out of the kernel

2008-07-31 Thread Robert Millan
On Thu, Jul 31, 2008 at 03:46:47PM +0800, Bean wrote:
> diff --git a/include/grub/symbol.h b/include/grub/symbol.h
> index e951490..eef966f 100644
> --- a/include/grub/symbol.h
> +++ b/include/grub/symbol.h
> @@ -43,4 +43,13 @@
>  # define EXPORT_VAR(x)   x
>  #endif /* ! GRUB_SYMBOL_GENERATOR */
>  
> +#define REAL_STUB_START(x)   x ## _stub: ; \
> + .long   x ## _start ; \
> + .long   x ## _end - x ## _start ; \
> +x ## _start: ; \
> + .code16
> +
> +#define REAL_STUB_END(x) .code32 ; \
> +x ## _end:

Shouldn't this be in an i386/pc/ subdir?

> + * On entry, %eax should points to a structure that specifies the code/data 
> to
> + * be copied:
> + *
> + *   offset 0: address of code/data
> + *   offset 4: size of code/data
> + *
> + * grub_copy_real_stub fetches the address from offset 0, if it's not below 
> 1M or
> + * properly aligned, it copies it to a reserved area and updates the address 
> at
> + * offset 0, so that it don't need to do it again the next time.
> + *
> + * Currently, the reserved area for stub code/data is 0x8 - 0x88000. It
> + * should be big enough for assembly code.

I wonder if this could be made simpler.  For example, by having a function
that just calls "int" in real mode.  Then callers could use inline assembly
to put parameters in appropiate registers and reading results from appropiate
registers.  Would this be faster than copiing hooks?

-- 
Robert Millan

  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."


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] Move assembly code out of the kernel

2008-07-31 Thread chaac
Bean [EMAIL PROTECTED] kirjoitti: 

Hi,

Currently, most assembly code are in startup.S. This is normally used
to ensure that the function address are below 1m, which is required if
it would switch to real mode and call bios services. However, this
make the kernel larger. For example, the biosdisk functions are only
used by biodisk module, they should not be placed inside the kernel.

This patch support splitting such code from startup.S. For example, we
create a new module biosdisk_stub.mod for assembly code of biosdisk.
Instead of call prot_to_real and real_to_prot, we call
grub_call_real_stub to enter real mode. grub_call_real_stub would copy
the code to real mode and do the mode switch.

To avoid unnecessary memory transfer, grub_call_real_stub would not
erase the real mode stub when it's done, so that it can be used
directly next time. When the stub area is full, it zero it out and
start anew. The area uses a simple verification method so that the old
mapping is invalidated, the code would need be copied again on their
next use.

The patch shows how to do it for the biosdisk module, here is the new
grub_biosdisk_rw_int13_extensions function.

REAL_STUB_START(grub_biosdisk_rw_int13_extensions)
movb%dh, %ah
movw%cx, %ds
int $0x13   /* do the operation */
movb%ah, %dl/* save return value */
lret
REAL_STUB_END(grub_biosdisk_rw_int13_extensions)

FUNCTION(grub_biosdisk_rw_int13_extensions)
pushl   %ebp
pushl   %esi

/* compute the address of disk_address_packet */
movw%cx, %si
xorw%cx, %cx
shrl$4, %ecx/* save the segment to cx */

/* ah */
movb%al, %dh

lealgrub_biosdisk_rw_int13_extensions_stub, %eax
callEXT_C(grub_call_real_stub)

movb%dl, %al/* return value in %eax */

popl%esi
popl%ebp

ret

Real mode code is enclosed between REAL_STUB_START and REAL_STUB_END,
no need to use .code16 and .code32 as it's handled by the macro. In
the main function, use

lealgrub_biosdisk_rw_int13_extensions_stub, %eax
callEXT_C(grub_call_real_stub)

to invoke grub_call_real_stub. grub_biosdisk_rw_int13_extensions_stub
is defined in the REAL_STUB_START macro.

This same method can be applied to loaders, vbe, etc. In fact, almost
all function behind grub_call_real_stub can be moved out of startup.S.


I have made generic function that does basically the same thing for bios 
service 0x10 (video). In that modification you prepare registers structure that 
will be configured during real mode switching. I am yet to commit it for 
review, but I think it would be more generic way to do this. When I come back 
from my holiday I will commit the code for review.

So please wait a bit before committing this :)

Thanks,
Vesa Jääskeläinen


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] Move assembly code out of the kernel

2008-07-31 Thread Bean
On Thu, Jul 31, 2008 at 7:22 PM, Robert Millan <[EMAIL PROTECTED]> wrote:
> On Thu, Jul 31, 2008 at 03:46:47PM +0800, Bean wrote:
>> diff --git a/include/grub/symbol.h b/include/grub/symbol.h
>> index e951490..eef966f 100644
>> --- a/include/grub/symbol.h
>> +++ b/include/grub/symbol.h
>> @@ -43,4 +43,13 @@
>>  # define EXPORT_VAR(x)   x
>>  #endif /* ! GRUB_SYMBOL_GENERATOR */
>>
>> +#define REAL_STUB_START(x)   x ## _stub: ; \
>> + .long   x ## _start ; \
>> + .long   x ## _end - x ## _start ; \
>> +x ## _start: ; \
>> + .code16
>> +
>> +#define REAL_STUB_END(x) .code32 ; \
>> +x ## _end:
>
> Shouldn't this be in an i386/pc/ subdir?

Oh, perhaps I can move them to include/i386/pc/kernel.h

>
>> + * On entry, %eax should points to a structure that specifies the code/data 
>> to
>> + * be copied:
>> + *
>> + *   offset 0: address of code/data
>> + *   offset 4: size of code/data
>> + *
>> + * grub_copy_real_stub fetches the address from offset 0, if it's not below 
>> 1M or
>> + * properly aligned, it copies it to a reserved area and updates the 
>> address at
>> + * offset 0, so that it don't need to do it again the next time.
>> + *
>> + * Currently, the reserved area for stub code/data is 0x8 - 0x88000. It
>> + * should be big enough for assembly code.
>
> I wonder if this could be made simpler.  For example, by having a function
> that just calls "int" in real mode.  Then callers could use inline assembly
> to put parameters in appropiate registers and reading results from appropiate
> registers.  Would this be faster than copiing hooks?

INT sounds good, but it can't cover all cases. For example, it can't
pass parameter using stack, or call a far pointer. Besides, we still
need the stub area to store data.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


[PATCH] add support for GRUB_GFXMODE variable

2008-07-31 Thread Felix Zielcke
Here's a patch to add a GRUB_GFXMODE variable for update-grub
to set the gfxmode. 


2008-07-31  Felix Zielcke  <[EMAIL PROTECTED]>

* util/update-grub.in (GRUB_GFXMODE): Export variable.
* util/grub.d/00_header.in: Allow the administrator to change default 
gfxmode via ${GRUB_GFXMODE}.

Index: util/grub.d/00_header.in
===
--- util/grub.d/00_header.in	(Revision 1754)
+++ util/grub.d/00_header.in	(Arbeitskopie)
@@ -31,6 +31,7 @@
 
 if [ "x${GRUB_DEFAULT}" = "x" ] ; then GRUB_DEFAULT=0 ; fi
 if [ "x${GRUB_TIMEOUT}" = "x" ] ; then GRUB_TIMEOUT=5 ; fi
+if [ "x${GRUB_GFXMODE}" = "x" ] ; then GRUB_GFXMODE=640x480 ; fi
 
 cat << EOF
 set default=${GRUB_DEFAULT}
@@ -43,7 +44,7 @@
 prepare_grub_to_access_device `${grub_probe} --target=device ${GRUB_FONT_PATH}`
 cat << EOF
 if font `make_system_path_relative_to_its_root ${GRUB_FONT_PATH}` ; then
-  set gfxmode=640x480
+  set gfxmode=${GRUB_GFXMODE}
   insmod gfxterm
   insmod vbe
   terminal gfxterm
Index: util/update-grub.in
===
--- util/update-grub.in	(Revision 1754)
+++ util/update-grub.in	(Arbeitskopie)
@@ -164,7 +164,7 @@
 export GRUB_DEVICE GRUB_DEVICE_UUID GRUB_DEVICE_BOOT GRUB_DEVICE_BOOT_UUID GRUB_FS GRUB_FONT_PATH GRUB_PRELOAD_MODULES
 
 # These are optional, user-defined variables.
-export GRUB_DEFAULT GRUB_TIMEOUT GRUB_DISTRIBUTOR GRUB_CMDLINE_LINUX GRUB_CMDLINE_LINUX_DEFAULT GRUB_TERMINAL GRUB_SERIAL_COMMAND GRUB_DISABLE_LINUX_UUID
+export GRUB_DEFAULT GRUB_TIMEOUT GRUB_DISTRIBUTOR GRUB_CMDLINE_LINUX GRUB_CMDLINE_LINUX_DEFAULT GRUB_TERMINAL GRUB_SERIAL_COMMAND GRUB_DISABLE_LINUX_UUID GRUB_GFXMODE
 
 rm -f ${grub_cfg}.new
 exec > ${grub_cfg}.new
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] make memdisk portable

2008-07-31 Thread Robert Millan
On Wed, Jul 30, 2008 at 08:51:57PM +0200, Robert Millan wrote:
> 
> This patch makes memdisk much less dependant on i386-pc specific structures
> and therefore easier to port.

Here we go again, after some suggestions from Bean on IRC.

It does now stop copiing the module section to low memory.  Instead, we just
make sure memdisk grabs its own copy of the data (as is done with ELF modules),
and use the module section at GRUB_MEMORY_MACHINE_DECOMPRESSION_ADDR for the
whole dance.

Ah, and it saves 40 bytes in kernel, and 48 bytes in memdisk :-)

-- 
Robert Millan

  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."
2008-07-30  Robert Millan  <[EMAIL PROTECTED]>

	* disk/memdisk.c (memdisk_size): Don't initialize.
	(GRUB_MOD_INIT(memdisk)): Find memdisk using grub_module_iterate().

	* include/grub/i386/pc/kernel.h
	(GRUB_KERNEL_MACHINE_MEMDISK_IMAGE_SIZE): Remove macro.
	(GRUB_KERNEL_MACHINE_PREFIX, GRUB_KERNEL_MACHINE_DATA_END): Shift.
	(grub_memdisk_image_size, grub_arch_memdisk_addr)
	(grub_arch_memdisk_size): Remove.

	* include/grub/kernel.h (struct grub_module_header): Remove `offset'
	field (was only used to transfer a constant).  Add `type' field to
	support multiple module types.
	(grub_module_iterate): New function.

	* kern/device.c (grub_device_open): Do not hide error messages
	when grub_disk_open() fails.  Use grub_print_error() instead.

	* kern/i386/pc/init.c (grub_arch_modules_addr)
	(grub_arch_memdisk_size): Remove functions.
	(grub_arch_modules_addr): Return the module address in high memory
	(now that it isn't copied anymore).

	* kern/i386/pc/startup.S (grub_memdisk_image_size): Remove variable.
	(codestart): Don't add grub_memdisk_image_size to %ecx in LZMA
	decompression routine (grub_total_module_size already includes that
	now).  Don't copy modules back to low memory.

	* kern/main.c: Include `'.
	(grub_load_modules): Split out (and use) ...
	(grub_module_iterate): ... this function, which iterates through
	module objects and runs a hook.
	Comment out grub_mm_init_region() call, as it would cause non-ELF
	modules to be overwritten.

	* util/i386/pc/grub-mkimage.c (generate_image): Instead of appending
	the memdisk image in its own region, make it part of the module list.

Index: disk/memdisk.c
===
--- disk/memdisk.c	(revision 1754)
+++ disk/memdisk.c	(working copy)
@@ -82,21 +82,30 @@
 
 GRUB_MOD_INIT(memdisk)
 {
-  char *memdisk_orig_addr;
+  auto int hook (struct grub_module_header *);
+  int hook (struct grub_module_header *header)
+{
+  if (header->type == OBJ_TYPE_MEMDISK)
+	{
+	  char *memdisk_orig_addr;
+	  memdisk_orig_addr = (char *) header + sizeof (struct grub_module_header);
 
-  memdisk_size = grub_arch_memdisk_size ();
-  if (! memdisk_size)
-return;
+	  grub_dprintf ("memdisk", "Found memdisk image at %p\n", memdisk_orig_addr);
 
-  memdisk_orig_addr = (char *) grub_arch_memdisk_addr ();
-  grub_dprintf ("memdisk", "Found memdisk image at %p\n", memdisk_orig_addr);
+	  memdisk_size = header->size - sizeof (struct grub_module_header);
+	  memdisk_addr = grub_malloc (memdisk_size);
 
-  memdisk_addr = grub_malloc (memdisk_size);
+	  grub_dprintf ("memdisk", "Copying memdisk image to dynamic memory\n");
+	  grub_memmove (memdisk_addr, memdisk_orig_addr, memdisk_size);
 
-  grub_dprintf ("memdisk", "Copying memdisk image to dynamic memory\n");
-  grub_memmove (memdisk_addr, memdisk_orig_addr, memdisk_size);
+	  grub_disk_dev_register (&grub_memdisk_dev);
+	  return 1;
+	}
 
-  grub_disk_dev_register (&grub_memdisk_dev);
+  return 0;
+}
+  
+  grub_module_iterate (hook);
 }
 
 GRUB_MOD_FINI(memdisk)
Index: kern/device.c
===
--- kern/device.c	(revision 1754)
+++ kern/device.c	(working copy)
@@ -50,7 +50,8 @@
   disk = grub_disk_open (name);
   if (! disk)
 {
-  grub_error (GRUB_ERR_BAD_DEVICE, "unknown device %s", name);
+  grub_print_error ();
+  grub_errno = GRUB_ERR_NONE;
   goto fail;
 }
 
Index: kern/i386/pc/startup.S
===
--- kern/i386/pc/startup.S	(revision 1754)
+++ kern/i386/pc/startup.S	(working copy)
@@ -96,8 +96,6 @@
 	.long	0x
 VARIABLE(grub_install_bsd_part)
 	.long	0x
-VARIABLE(grub_memdisk_image_size)
-	.long	0
 VARIABLE(grub_prefix)
 	/* to be filled by grub-mkimage */
 
@@ -211,7 +209,6 @@
 	call	lzo1x_decompress
 	addl	$12, %esp
 
-	/* copy back the decompressed part */
 	movl	%eax, %ecx
 	cld
 #elif defined(ENABLE_LZMA)
@@ -221,19 +218,22 @@
 	pushl	%esi
 	movl	EXT_C(grub_kernel_image_size), %ecx
 	addl	EXT_C(grub_total_module_size), %ecx
-	addl	EXT_C(grub_memdisk_image_size), %ecx
 	subl	$GRUB_KERNEL_MACHINE_RAW_SIZE, %ecx
 	pushl	%ecx
 	leal	(%

Re: [PATCH] Drivemap module

2008-07-31 Thread Marco Gerards
Javier Martín <[EMAIL PROTECTED]> writes:

> El dom, 20-07-2008 a las 21:40 +0200, Marco Gerards escribió:
>> Did you use code from other people or projects?
> No, as far as I can control my own mind: the assembly int13h handler is
> loosely based on that of GRUB Legacy, but heavily rewritten. All other
> code was written from scratch, even the crappy linked lists all over the
> place.

Okay :-)

Sorry I kept you waiting... again...

>> > For newcomers, full info on the patch is available on the list archives
>> > - it was proposed on June and its discussion deferred for "two or three
>> > weeks" because the developers were busy.
>> 
>> 
>> I have copied the changelog entry from your other e-mail:
>> 
>>  * commands/i386/pc/drivemap.c : New file, main part of the new
>>  drivemap module allowing BIOS drive remapping not unlike the
>>  legacy "map" command. This allows to boot OSes with boot-time
>>  dependencies on the particular ordering of BIOS drives or
>>  trusting their own to be 0x80, like Windows XP, with
>>  non-standard boot configurations.
>> 
>> "New file." would be sufficient
>> 
>>  * commands/i386/pc/drivemap_int13h.S : New file, INT 13h handler
>>  for the drivemap module. Installed as a TSR routine by
>>  drivemap.c, performs the actual redirection of BIOS drives.
>> 
>> Same here.
> Hmm... isn't the ChangeLog too spartan? I thought it should be a bit
> informative - what about a single sentence per file?
>   * commands/i386/pc/drivemap.c : New file - drivemap command and
>   int13h installer
>   * commands/i386/pc/drivemap_int13h.S : New file, resident real
>   mode BIOS int13 handler

No, please just stick to "New file.".  The Changelog is used for
changes only.  If you add more information, it will be noise and it
will make my job harder.

>> * conf/i386-pc.rmk : Added the new module
>> Please say which variables you added in this file.  You can find some
>> examples on how to do this in the ChangeLog file.
>   * conf/i386-pc.rmk (pkglib_MODULES) : Added drivemap.mod
>   (drivemap_mod_SOURCES) : New variable
>   (drivemap_mod_ASFLAGS) : Likewise
>   (drivemap_mod_CFLAGS) : Likewise
>   (drivemap_mod_LDFLAGS) : Likewise
> And now we're being uselessly verbose. IMHO, ChangeLog should be more
> about semantic changes in the code and less about literal changes - we
> have `svn diff' for those.

You are wrong.  It is your opinion that this should go into a
changelog entry and I can understand that.  However, this is simply
how we do it ;)

>>  * include/grub/loader.h : Added a "just-before-boot" callback
>>  infrastructure used by drivemap.mod to install the INT13 handler
>>  only when the "boot" command has been issued.
>> 
>> Please describe changes, not effects.  So which prototypes and macros
>> did you add?
>> 
>   * include/grub/loader.h (grub_loader_register_preboot) : New
>   function (proto). Register a new pre-boot handler
>   (grub_loader_unregister_preboot) : Likewise. Unregister handler
>   (grub_preboot_hookid) : New typedef. Registered hook "handle"
>>  * kern/loader.c : Implement the preboot-hook described
>> 
>> Which functions did you change and how?  Please describe actual changes.
>   * kern/loader.c (grub_loader_register_preboot) : New function.
>   (grub_loader_unregister_preboot) : Likewise.
>   (preboot_hooks) : New variable. Linked list of preboot hooks
>   (grub_loader_boot) : Call the list of preboot-hooks before the
>   actual loader
>> 
>> The header is missing, please include it.  Also newlines between the
>> files make it easier to read.
> What header? The drivemap module itself has no .h files. The only header
> I touch is loader.h, and is both in the ChangeLog entry and the patch.

With header I meant the first line of the changelog entry that
includes your name and e-mail address.
 
>> Here follows a review.  Sorry I kept you waiting for this long, this
>> feature and your work is really appreciated!  Perhaps I can spot some
>> more problems after you fixed it and supplied an updated changelog
>> entry.  There are quite some comments, but please do not let this
>> demotivate you, it is mainly coding style related :-)
> Well, thanks to all the time I had free, I have nearly finished Final
> Fantasy XII, so the wait was not soo bad ^^

:-)

>> (...)
>> > +
>> > +/* Uncomment the following line to enable debugging output */
>> > +/* #define DRIVEMAP_DEBUG */
>> > +
>> > +#ifdef DRIVEMAP_DEBUG
>> > +# define DBG_PRINTF(...) grub_printf(__VA_ARGS__)
>> > +#else
>> > +# define DBG_PRINTF(...)
>> > +#endif
>> 
>> Please use the grub_dprintf infrastructure.
> Done. I didn't even know it existed :S

:-)
 
>> > +/* realmode far ptr = 2 * 16b */
>> > +extern grub_uint32_t EXPORT_VAR (grub_drivemap_int13_oldhandler);
>> > +/* Size of the section to be copied */
>> > +extern grub_uint16_t EXPORT_VAR (grub_drivemap_int13_size);
>> > +
>> > +/*

Re: [PATCH] High resolution time/TSC patch v3

2008-07-31 Thread Marco Gerards
Colin D Bennett <[EMAIL PROTECTED]> writes:

> Another updated TSC patch.  Now it detects TSC support for x86 CPUs at
> runtime and selects either the TSC or RTC time source.  This way 386
> and 486 CPUs without RDTSC instruction support are supported.
>
> Robert Millan was interested in getting this patch merged for the
> Coreboot port, so I decided to take another crack at it.
>
> Comments are welcome!

Great!  Did you have a look at Roberts comment that it does not work
in Qemu?  This is the most important testing ground for many of us...

> === modified file 'ChangeLog'
> --- ChangeLog 2008-07-27 19:57:43 +
> +++ ChangeLog 2008-07-28 16:51:30 +
> @@ -1,3 +1,68 @@
> +2008-07-28  Colin D Bennett <[EMAIL PROTECTED]>
> +
> + High resolution timer support.  Implemented for x86 CPUs using TSC.
> + Extracted generic grub_millisleep() so it's linked in only as needed.
> + This requires a Pentium compatible CPU; if the RDTSC instruction is
> + not supported, then it falls back on the generic grub_get_time_ms()
> + implementation that uses the machine's RTC.
> +
> + * conf/i386-efi.rmk: Added new source files to kernel_elf_SOURCES.

Do you mean:

   * conf/i386-efi.rmk (kernel_elf_SOURCES): Add <...>.

Please replace <...> by your source files :-)

> + * conf/i386-pc.rmk: Likewise.
> +
> + * conf/sparc64-ieee1275.rmk: Likewise.
> +
> + * conf/powerpc-ieee1275.rmk: Likewise.
> +
> + * conf/i386-coreboot.rmk: Likewise.
> +
> + * kern/generic/rtc_get_time_ms.c: New file.
> +
> + * kern/generic/millisleep.c: New file.
> +
> + * kern/misc.c (grub_millisleep_generic): Removed.
> +
> + * commands/sleep.c (grub_interruptible_millisleep): Uses
> + grub_get_time_ms() instead of grub_get_rtc() to stay in sync with
> + grub_millisleep() from kern/generic/millisleep.c.
> +
> + * include/grub/i386/tsc.h (grub_get_tsc): New file.  New inline
> + function.

Huh?

* include/grub/i386/tsc.h: New file.

Would be correct, right?

> + (grub_cpu_is_cpuid_supported): New inline function.
> + (grub_cpu_is_tsc_supported): New inline function.
> + (grub_tsc_init): New function prototype.
> + (grub_tsc_get_time_ms): New function prototype.

No need to describe the contents of a new file, simply stating the
file is new is sufficient.

> + * kern/i386/tsc.c (grub_get_time_ms): New file.  New function.
> + (calibrate_tsc): New static function.
> + (grub_tsc_init): New function.

Same here.

> + * include/grub/time.h (grub_millisleep_generic): Removed.
> + (grub_get_time_ms): New function.
> + (grub_install_get_time_ms): New function.
> +
> + * kern/time.c (grub_get_time_ms): New function.
> + (grub_install_get_time_ms): New function.
> +
> + * kern/i386/efi/init.c (grub_millisleep): Removed.
> + (grub_machine_init): Call grub_tsc_init.
> +
> + * kern/i386/linuxbios/init.c (grub_machine_init): Install the RTC
> + get_time_ms() implementation.
> +
> + * kern/sparc64/ieee1275/init.c (grub_millisleep): Removed.
> + (ieee1275_get_time_ms): New function.
> + (grub_machine_init): Install get_time_ms() implementation.
> +
> + * kern/i386/pc/init.c (grub_machine_init): Call grub_tsc_init().
> + (grub_millisleep): Removed.
> +
> + * kern/ieee1275/init.c (grub_millisleep): Removed.
> + (grub_machine_init): Install ieee1275_get_time_ms() implementation.
> + (ieee1275_get_time_ms): New static function.
> + (grub_get_rtc): Now calls ieee1275_get_time_ms(), which does the real
> + work.
> +
>  2008-07-27  Robert Millan  <[EMAIL PROTECTED]>
>  
>   * disk/ata.c (grub_ata_dumpinfo): Use grub_dprintf() for debugging
>
> === modified file 'commands/sleep.c'
> --- commands/sleep.c  2008-05-16 20:55:29 +
> +++ commands/sleep.c  2008-07-04 16:55:48 +
> @@ -43,15 +43,15 @@
>grub_printf ("%d", n);
>  }
>  
> -/* Based on grub_millisleep() from kern/misc.c.  */
> +/* Based on grub_millisleep() from kern/generic/millisleep.c.  */
>  static int
>  grub_interruptible_millisleep (grub_uint32_t ms)
>  {
> -  grub_uint32_t end_at;
> -  
> -  end_at = grub_get_rtc () + grub_div_roundup (ms * GRUB_TICKS_PER_SECOND, 
> 1000);
> -  
> -  while (grub_get_rtc () < end_at)
> +  grub_uint64_t start;
> +
> +  start = grub_get_time_ms ();
> +
> +  while (grub_get_time_ms () - start < ms)
>  if (grub_checkkey () >= 0 &&
>   GRUB_TERM_ASCII_CHAR (grub_getkey ()) == GRUB_TERM_ESC)
>return 1;
>
> === modified file 'conf/i386-coreboot.rmk'
> --- conf/i386-coreboot.rmk2008-07-27 12:51:30 +
> +++ conf/i386-coreboot.rmk2008-07-28 16:23:46 +
> @@ -16,6 +16,7 @@
>   kern/main.c kern/device.c \
>   kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
>   kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
> + kern/time.c \
>   kern/i386/dl.c kern/parser.c kern/partition.c \
>   kern/env.c \
>   term/i386/pc/co

Re: [PATCH] buffered file read

2008-07-31 Thread Marco Gerards
Hi Bean,

Bean <[EMAIL PROTECTED]> writes:

> I have added a buffer length parameter to grub_buffile_open:
>
> grub_file_t grub_buffile_open (const char *name, int size);
>
> size < 0: Load the whole file.
> size = 0: Use default buffer size (8192 bytes)
> size > 0: Custom buffer size = size
>
> The upper limit is 1m, if size > 1m, we set it to 1m.
>
> 2008-07-28  Bean  <[EMAIL PROTECTED]>
>
>   * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>   (bufio_mod_SOURCES): New macro.
>   (bufio_mod_CFLAGS): Likewise.
>   (bufio_mod_LDFLAGS): Likewise.
>
>   * include/grub/bufio.h: New file.
>
>   * io/bufio.c: Likewise.
>
>   * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>   file.
>
>   * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>
>   * video/tga.c (grub_video_reader_tga): Likewise.
>
>   * font/manager.c (add_font): Likewise.

Please mention it when you include .  The same
for when you remove .

Otherwise it seems good for inclusion :-)

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] buffered file read

2008-07-31 Thread Bean
On Fri, Aug 1, 2008 at 3:18 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Hi Bean,
>
> Bean <[EMAIL PROTECTED]> writes:
>
>> I have added a buffer length parameter to grub_buffile_open:
>>
>> grub_file_t grub_buffile_open (const char *name, int size);
>>
>> size < 0: Load the whole file.
>> size = 0: Use default buffer size (8192 bytes)
>> size > 0: Custom buffer size = size
>>
>> The upper limit is 1m, if size > 1m, we set it to 1m.
>>
>> 2008-07-28  Bean  <[EMAIL PROTECTED]>
>>
>>   * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>>   (bufio_mod_SOURCES): New macro.
>>   (bufio_mod_CFLAGS): Likewise.
>>   (bufio_mod_LDFLAGS): Likewise.
>>
>>   * include/grub/bufio.h: New file.
>>
>>   * io/bufio.c: Likewise.
>>
>>   * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>>   file.
>>
>>   * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>>
>>   * video/tga.c (grub_video_reader_tga): Likewise.
>>
>>   * font/manager.c (add_font): Likewise.
>
> Please mention it when you include .  The same
> for when you remove .
>
> Otherwise it seems good for inclusion :-)

Hi,

Oh, how the changelog would be like ?

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] High resolution time/TSC patch v3

2008-07-31 Thread Robert Millan
On Thu, Jul 31, 2008 at 09:08:31PM +0200, Marco Gerards wrote:
> Colin D Bennett <[EMAIL PROTECTED]> writes:
> 
> > Another updated TSC patch.  Now it detects TSC support for x86 CPUs at
> > runtime and selects either the TSC or RTC time source.  This way 386
> > and 486 CPUs without RDTSC instruction support are supported.
> >
> > Robert Millan was interested in getting this patch merged for the
> > Coreboot port, so I decided to take another crack at it.
> >
> > Comments are welcome!
> 
> Great!  Did you have a look at Roberts comment that it does not work
> in Qemu?  This is the most important testing ground for many of us...

No no, I said it doesn't work on Coreboot.

-- 
Robert Millan

  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."


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] make memdisk portable

2008-07-31 Thread Robert Millan

New patch.  Also enable it for ELF platforms (coreboot, ieee1275).

-- 
Robert Millan

  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."
2008-07-31  Robert Millan  <[EMAIL PROTECTED]>

	* disk/memdisk.c (memdisk_size): Don't initialize.
	(GRUB_MOD_INIT(memdisk)): Find memdisk using grub_module_iterate().

	* include/grub/i386/pc/kernel.h
	(GRUB_KERNEL_MACHINE_MEMDISK_IMAGE_SIZE): Remove macro.
	(GRUB_KERNEL_MACHINE_PREFIX, GRUB_KERNEL_MACHINE_DATA_END): Shift.
	(grub_memdisk_image_size, grub_arch_memdisk_addr)
	(grub_arch_memdisk_size): Remove.

	* include/grub/kernel.h (struct grub_module_header): Remove `offset'
	field (was only used to transfer a constant).  Add `type' field to
	support multiple module types.
	(grub_module_iterate): New function.

	* kern/device.c (grub_device_open): Do not hide error messages
	when grub_disk_open() fails.  Use grub_print_error() instead.

	* kern/i386/pc/init.c (grub_arch_modules_addr)
	(grub_arch_memdisk_size): Remove functions.
	(grub_arch_modules_addr): Return the module address in high memory
	(now that it isn't copied anymore).

	* kern/i386/pc/startup.S (grub_memdisk_image_size): Remove variable.
	(codestart): Don't add grub_memdisk_image_size to %ecx in LZMA
	decompression routine (grub_total_module_size already includes that
	now).  Don't copy modules back to low memory.

	* kern/main.c: Include `'.
	(grub_load_modules): Split out (and use) ...
	(grub_module_iterate): ... this function, which iterates through
	module objects and runs a hook.
	Comment out grub_mm_init_region() call, as it would cause non-ELF
	modules to be overwritten.

	* util/i386/pc/grub-mkimage.c (generate_image): Instead of appending
	the memdisk image in its own region, make it part of the module list.
	* util/elf/grub-mkimage.c (options): Add "memdisk"|'m' option.
	(main): Parse --memdisk|-m option, and pass user-provided path as
	parameter to generate_image().
	(add_segments): Pass `memdisk_path' down to load_modules().
	(load_modules): Embed memdisk image in module section when requested.

	* conf/powerpc-ieee1275.rmk (pkglib_MODULES): Add `memdisk.mod'.
	(memdisk_mod_SOURCES, memdisk_mod_CFLAGS)
	(memdisk_mod_LDFLAGS): New variables.
	* conf/i386-coreboot.rmk: Likewise.
	* conf/i386-ieee1275.rmk: Likewise.

Index: conf/i386-coreboot.rmk
===
--- conf/i386-coreboot.rmk	(revision 1754)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -94,7 +94,7 @@
 # Modules.
 pkglib_MODULES = _linux.mod linux.mod normal.mod	\
 	_multiboot.mod multiboot.mod aout.mod		\
-	play.mod cpuid.mod serial.mod ata.mod
+	play.mod cpuid.mod serial.mod ata.mod memdisk.mod
 
 # For _linux.mod.
 _linux_mod_SOURCES = loader/i386/pc/linux.c
@@ -154,4 +154,9 @@
 ata_mod_CFLAGS = $(COMMON_CFLAGS)
 ata_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
+# For memdisk.mod.
+memdisk_mod_SOURCES = disk/memdisk.c
+memdisk_mod_CFLAGS = $(COMMON_CFLAGS)
+memdisk_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 include $(srcdir)/conf/common.mk
Index: conf/powerpc-ieee1275.rmk
===
--- conf/powerpc-ieee1275.rmk	(revision 1754)
+++ conf/powerpc-ieee1275.rmk	(working copy)
@@ -110,7 +110,8 @@
 	reboot.mod \
 	suspend.mod \
 _multiboot.mod \
-multiboot.mod
+multiboot.mod \
+	memdisk.mod
 
 # For _linux.mod.
 _linux_mod_SOURCES = loader/powerpc/ieee1275/linux.c
@@ -159,6 +160,10 @@
 multiboot_mod_CFLAGS = $(COMMON_CFLAGS)
 multiboot_mod_LDFLAGS = $(COMMON_LDFLAGS) 
 
+# For memdisk.mod.
+memdisk_mod_SOURCES = disk/memdisk.c
+memdisk_mod_CFLAGS = $(COMMON_CFLAGS)
+memdisk_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 include $(srcdir)/conf/common.mk
 
Index: conf/i386-ieee1275.rmk
===
--- conf/i386-ieee1275.rmk	(revision 1754)
+++ conf/i386-ieee1275.rmk	(working copy)
@@ -102,7 +102,7 @@
 # Modules.
 pkglib_MODULES = normal.mod halt.mod reboot.mod suspend.mod cpuid.mod	\
 	multiboot.mod _multiboot.mod aout.mod serial.mod linux.mod	\
-	_linux.mod nand.mod
+	_linux.mod nand.mod memdisk.mod
 
 # For normal.mod.
 normal_mod_SOURCES = normal/arg.c normal/cmdline.c normal/command.c	\
@@ -171,4 +171,9 @@
 nand_mod_CFLAGS = $(COMMON_CFLAGS)
 nand_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
+# For memdisk.mod.
+memdisk_mod_SOURCES = disk/memdisk.c
+memdisk_mod_CFLAGS = $(COMMON_CFLAGS)
+memdisk_mod_LDFLAGS = $(COMMON_LDFLAGS)
+
 include $(srcdir)/conf/common.mk
Index: disk/memdisk.c
===
--- disk/memdisk.c	(revision 1754)
+++ disk/memdisk.c	(working copy)
@@ -82,21 +82,30 @@
 
 GRUB_MOD_INIT(memdisk)
 {
-  char *memdisk_orig_addr;
+  auto int hook (struct grub_module_header *);
+  int hook (struct grub_module_header *header)
+{
+  if (header->type == OBJ_

Re: [PATCH] buffered file read

2008-07-31 Thread Marco Gerards
Bean <[EMAIL PROTECTED]> writes:

> On Fri, Aug 1, 2008 at 3:18 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
>> Hi Bean,
>>
>> Bean <[EMAIL PROTECTED]> writes:
>>
>>> I have added a buffer length parameter to grub_buffile_open:
>>>
>>> grub_file_t grub_buffile_open (const char *name, int size);
>>>
>>> size < 0: Load the whole file.
>>> size = 0: Use default buffer size (8192 bytes)
>>> size > 0: Custom buffer size = size
>>>
>>> The upper limit is 1m, if size > 1m, we set it to 1m.
>>>
>>> 2008-07-28  Bean  <[EMAIL PROTECTED]>
>>>
>>>   * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
>>>   (bufio_mod_SOURCES): New macro.
>>>   (bufio_mod_CFLAGS): Likewise.
>>>   (bufio_mod_LDFLAGS): Likewise.
>>>
>>>   * include/grub/bufio.h: New file.
>>>
>>>   * io/bufio.c: Likewise.
>>>
>>>   * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
>>>   file.
>>>
>>>   * video/jpeg.c (grub_video_reader_jpeg): Likewise.
>>>
>>>   * video/tga.c (grub_video_reader_tga): Likewise.
>>>
>>>   * font/manager.c (add_font): Likewise.
>>
>> Please mention it when you include .  The same
>> for when you remove .
>>
>> Otherwise it seems good for inclusion :-)
>
> Hi,
>
> Oh, how the changelog would be like ?

Specifically or generally?  Generally something like:

 * fs/foo.c: Include .
 (baz): New function.


--
Marco




___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] High resolution time/TSC patch v3

2008-07-31 Thread Marco Gerards
Robert Millan <[EMAIL PROTECTED]> writes:

> On Thu, Jul 31, 2008 at 09:08:31PM +0200, Marco Gerards wrote:
>> Colin D Bennett <[EMAIL PROTECTED]> writes:
>> 
>> > Another updated TSC patch.  Now it detects TSC support for x86 CPUs at
>> > runtime and selects either the TSC or RTC time source.  This way 386
>> > and 486 CPUs without RDTSC instruction support are supported.
>> >
>> > Robert Millan was interested in getting this patch merged for the
>> > Coreboot port, so I decided to take another crack at it.
>> >
>> > Comments are welcome!
>> 
>> Great!  Did you have a look at Roberts comment that it does not work
>> in Qemu?  This is the most important testing ground for many of us...
>
> No no, I said it doesn't work on Coreboot.

Coreboot in Qemu?

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: Remove conf/*.mk from svn (was: Re: Linking)

2008-07-31 Thread Yoshinori K. Okuji
On Friday 25 July 2008 22:43:10 Robert Millan wrote:
> On Fri, Jul 25, 2008 at 08:18:27AM -0700, Colin D Bennett wrote:
> > On Thu, 24 Jul 2008 22:17:06 -0700 (PDT)
> >
> > Viswesh S <[EMAIL PROTECTED]> wrote:
> > > I have modified the conf/common.mk accordingly as shown below.
> >
> > GRUB developers,
> >
> > I think we should remove conf/*.mk from the Subversion repository.  If
> > people are going to be developing on GRUB and checking out svn
> > branches, then I think it's fine to require them to have Ruby.  For
> > released tarballs that we expect non-developers to use, we just need to
> > generate the *.mk files and include them in the tarball.
> >
> > What is the benefit of having conf/*.mk files in svn?  They cause lots
> > of merge conflicts: sure, the conflicts are easy to resolve by simply
> > choosing to take one of the revisions, but then that merge really has
> > not meaning, since it won't be in sync with your .rmk file until it's
> > once again regenerated and checked in.
> >
> > Removing the autogenerated .mk files would also eliminate problems new
> > developers like Viswesh and I have encountered -- we won't be tempted
> > to modify a non-versioned file like this.
>
> I'd check with Okuji first.  IIRC he didn't like that idea.

I don't like. You can check the archive for my comments on this.

Regards,
Okuji


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] High resolution time/TSC patch v3

2008-07-31 Thread Robert Millan
On Thu, Jul 31, 2008 at 10:07:46PM +0200, Marco Gerards wrote:
> Robert Millan <[EMAIL PROTECTED]> writes:
> 
> > On Thu, Jul 31, 2008 at 09:08:31PM +0200, Marco Gerards wrote:
> >> Colin D Bennett <[EMAIL PROTECTED]> writes:
> >> 
> >> > Another updated TSC patch.  Now it detects TSC support for x86 CPUs at
> >> > runtime and selects either the TSC or RTC time source.  This way 386
> >> > and 486 CPUs without RDTSC instruction support are supported.
> >> >
> >> > Robert Millan was interested in getting this patch merged for the
> >> > Coreboot port, so I decided to take another crack at it.
> >> >
> >> > Comments are welcome!
> >> 
> >> Great!  Did you have a look at Roberts comment that it does not work
> >> in Qemu?  This is the most important testing ground for many of us...
> >
> > No no, I said it doesn't work on Coreboot.
> 
> Coreboot in Qemu?

Yep

-- 
Robert Millan

  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."


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] access phdr header entries like an array

2008-07-31 Thread Robert Millan

Committed.

On Sun, Oct 14, 2007 at 03:48:18PM +0200, Robert Millan wrote:
> 
> This patch makes it easier and more intuitive to access entries in the phdr
> header as if it were a C array.  It has otherwise no effect on the current
> code (other than saving some space for an awkward reason), but is needed to
> implement the ability to load segments at an arbitrary address, distinguishing
> the relative offset rather than their absolute requested address.
> 
> I have the code for all the dance, including the ability to relocate the
> payload later on, but I've chosen to split it up for revision tracking
> purposes (besides, the rest needs quite a bit of cleanup yet ;-)).
> 
> I've checked there are no regressions (at least with invaders).  If there are
> no objections in a few days I'll check it in.
> 
> -- 
> Robert Millan
> 
>  I know my rights; I want my phone call!
>  What use is a phone call, if you are unable to speak?
> (as seen on /.)

> 2007-10-14  Robert Millan  <[EMAIL PROTECTED]>
> 
>   * loader/i386/pc/multiboot.c (grub_multiboot_load_elf32): When loading
>   ELF segments, use a macro for arbitrarily accessing any of them instead
>   of preparing a pointer that allows access to one at a time.
>   (grub_multiboot_load_elf64): Likewise.
> 
> diff -ur grub2/loader/i386/pc/multiboot.c 
> grub2.phdr_array/loader/i386/pc/multiboot.c
> --- grub2/loader/i386/pc/multiboot.c  2007-07-25 21:29:24.0 +0200
> +++ grub2.phdr_array/loader/i386/pc/multiboot.c   2007-10-14 
> 15:32:37.0 +0200
> @@ -94,7 +94,7 @@
>  grub_multiboot_load_elf32 (grub_file_t file, void *buffer)
>  {
>Elf32_Ehdr *ehdr = (Elf32_Ehdr *) buffer;
> -  Elf32_Phdr *phdr;
> +  void *phdr_base;
>int i;
>  
>if (ehdr->e_ident[EI_CLASS] != ELFCLASS32)
> @@ -112,35 +112,38 @@
>
>entry = ehdr->e_entry;
>
> +  phdr_base = (void *) buffer + ehdr->e_phoff;
> +#define phdr(i)  ((Elf32_Phdr *) (phdr_base + (i) * 
> ehdr->e_phentsize))
> +
>/* Load every loadable segment in memory.  */
>for (i = 0; i < ehdr->e_phnum; i++)
>  {
> -  phdr = (Elf32_Phdr *) ((char *) buffer + ehdr->e_phoff
> -  + i * ehdr->e_phentsize);
> -  if (phdr->p_type == PT_LOAD)
> +  if (phdr(i)->p_type == PT_LOAD)
>  {
>/* The segment should fit in the area reserved for the OS.  */
> -  if ((phdr->p_paddr < grub_os_area_addr)
> -  || (phdr->p_paddr + phdr->p_memsz
> +  if ((phdr(i)->p_paddr < grub_os_area_addr)
> +  || (phdr(i)->p_paddr + phdr(i)->p_memsz
> > grub_os_area_addr + grub_os_area_size))
>   return grub_error (GRUB_ERR_BAD_OS,
>  "segment doesn't fit in memory reserved for the 
> OS");
>  
> -  if (grub_file_seek (file, (grub_off_t) phdr->p_offset)
> +  if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset)
> == (grub_off_t) -1)
>   return grub_error (GRUB_ERR_BAD_OS,
>  "invalid offset in program header");
> 
> -  if (grub_file_read (file, (void *) phdr->p_paddr, phdr->p_filesz)
> -  != (grub_ssize_t) phdr->p_filesz)
> +  if (grub_file_read (file, (void *) phdr(i)->p_paddr, 
> phdr(i)->p_filesz)
> +  != (grub_ssize_t) phdr(i)->p_filesz)
>   return grub_error (GRUB_ERR_BAD_OS,
>  "couldn't read segment from file");
>  
> -  if (phdr->p_filesz < phdr->p_memsz)
> -grub_memset ((char *) phdr->p_paddr + phdr->p_filesz, 0,
> -  phdr->p_memsz - phdr->p_filesz);
> +  if (phdr(i)->p_filesz < phdr(i)->p_memsz)
> +grub_memset ((char *) phdr(i)->p_paddr + phdr(i)->p_filesz, 0,
> +  phdr(i)->p_memsz - phdr(i)->p_filesz);
>  }
>  }
> +
> +#undef phdr
>
>return grub_errno;
>  }
> @@ -158,7 +161,7 @@
>  grub_multiboot_load_elf64 (grub_file_t file, void *buffer)
>  {
>Elf64_Ehdr *ehdr = (Elf64_Ehdr *) buffer;
> -  Elf64_Phdr *phdr;
> +  void *phdr_base;
>int i;
>  
>if (ehdr->e_ident[EI_CLASS] != ELFCLASS64)
> @@ -186,39 +189,42 @@
>  
>entry = ehdr->e_entry;
>  
> +  phdr_base = (void *) buffer + ehdr->e_phoff;
> +#define phdr(i)  ((Elf64_Phdr *) (phdr_base + (i) * 
> ehdr->e_phentsize))
> +
>/* Load every loadable segment in memory.  */
>for (i = 0; i < ehdr->e_phnum; i++)
>  {
> -  phdr = (Elf64_Phdr *) ((char *) buffer + ehdr->e_phoff
> -  + i * ehdr->e_phentsize);
> -  if (phdr->p_type == PT_LOAD)
> +  if (phdr(i)->p_type == PT_LOAD)
>  {
>/* The segment should fit in the area reserved for the OS.  */
> -  if ((phdr->p_paddr < (grub_uint64_t) grub_os_area_addr)
> -  || (phdr->p_paddr + phdr->p_memsz
> +  if ((phdr(i)->p_paddr < (grub_uint64_t) grub_os_area_addr)
> +  || (

Re: [PATCH] update-grub for Cygwin

2008-07-31 Thread Christian Franke

Robert Millan wrote:

On Thu, Jul 24, 2008 at 10:39:04PM +0200, Christian Franke wrote:
  

+
+d="`${grub_probe} -t drive "$p" 2>/dev/null`" || exit 0



Please avoid reliing on '-t drive'.  It's based on device.map which just
contains guesswork.

prepare_grub_to_access_device() is a much better option.

  


Yes. Thanks for the info. New version below.

Christian

2008-07-31  Christian Franke  <[EMAIL PROTECTED]>

   * conf/common.rmk: Add `10_cygwin' to `update-grub_SCRIPTS'.
   * util/grub.d/10_cygwin.in: New file.


diff --git a/conf/common.rmk b/conf/common.rmk
index 7db0b2a..0f74f1a 100644
--- a/conf/common.rmk
+++ b/conf/common.rmk
@@ -120,7 +120,7 @@ CLEANFILES += update-grub_lib
 %: util/grub.d/%.in config.status
 	./config.status --file=$@:$<
 	chmod +x $@
-update-grub_SCRIPTS = 00_header 10_linux 10_hurd 30_os-prober 40_custom
+update-grub_SCRIPTS = 00_header 10_cygwin 10_linux 10_hurd 30_os-prober 40_custom
 CLEANFILES += $(update-grub_SCRIPTS)
 
 update-grub_DATA += util/grub.d/README
diff --git a/util/grub.d/10_cygwin.in b/util/grub.d/10_cygwin.in
new file mode 100644
index 000..7dee6c1
--- /dev/null
+++ b/util/grub.d/10_cygwin.in
@@ -0,0 +1,52 @@
+#! /bin/sh -e
+
+# update-grub helper script.
+# Copyright (C) 2008  Free Software Foundation, Inc.
+#
+# GRUB is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# GRUB is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GRUB.  If not, see .
+
[EMAIL PROTECTED]@
[EMAIL PROTECTED]@
[EMAIL PROTECTED]@
+. ${libdir}/grub/update-grub_lib
+
+case "`uname 2>/dev/null`" in
+  CYGWIN_NT-5.0) OS="Windows 2000" ;;
+  CYGWIN_NT-5.1) OS="Windows XP"   ;;
+  CYGWIN_NT-5.2) OS="Windows 2003" ;;
+  CYGWIN*)   OS="Windows"  ;;
+  *) exit 0 ;;
+esac
+
+case "$SYSTEMDRIVE" in
+  [A-Za-z]:) ;;
+  *) exit 0  ;;
+esac
+
+test -f "$SYSTEMDRIVE"/ntldr || exit 0
+
+sysdev=`${grub_probe} -t device "$SYSTEMDRIVE"/ 2>/dev/null` || exit 0
+
+echo "Found $OS on $SYSTEMDRIVE/ ($sysdev)" >&2
+cat << EOF
+menuentry "$OS" {
+EOF
+
+prepare_grub_to_access_device "$sysdev" | sed 's,^,\t,'
+
+cat << EOF
+	chainloader +1
+}
+EOF
+
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


two bugs in configfile parser

2008-07-31 Thread Patrick Georgi

Hi,

given the following grub.cfg (simplified test case):

menuentry "test" {
if test "a" = "a" ; then
  echo foo
fi
}

I have some issues. To reproduce, load grub with that file, enter the 
editor on the menu item, run it with ctrl-x.
Two things can happen (I have some local patches which lead to one 
happening slightly more often than the other, so it seems to be quite 
sensitive to $whatever):


 1. it crashes on malloc magic problems. It seems to be related to the 
leading spaces on "echo foo". If I remove them, it works. I guess, 
they're skipped at some place, and after that, the string should be 
grub_free()d. grub's mm doesn't support that.


 2. it corrupts the text once it finishes. After removing the leading 
spaces, it runs correctly and returns to the editor. Unfortunately, 
starting with "  echo foo", the text is corrupted. Another run (with all 
those garbage strings) ends in malloc magic error.


That code runs fine if executed directly from the menu.


Regards,
Patrick Georgi



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: loadee relocation (Re: loader modules jumping back to kernel)

2008-07-31 Thread Robert Millan
On Wed, Jul 30, 2008 at 09:15:10PM +0200, Robert Millan wrote:
> 
> Let's try to revive this discussion.  Here's a patch I made a while ago that
> implements support for loading at any address.  It works by having a "special"
> version of malloc() that is told to allocate a chunk of memory that does
> _not_ overlap with a specific region.  It does so by iteratively reserving
> memory (and keeping track of what was reserved, of course).
> 
> Then we use this function to allocate the asm relocator code in heap, asking
> it to garantee that it won't overlap with our final destination.  Finally,
> our loadee can be put anywhere (e.g. in heap), and we just jump to the
> relocator which will jump to the loadee.
> 
> But I really find the approach really ugly.  What do you think?  If we agree
> that this is the way to go, I can do some cleanup & update it to current svn
> for a merge.

Here's a new patch, with the following approach.  We put this in a single
heap area:



we pick the relocator we want depending on wether we want to copy to a lower
or higher address.  This garantees that the relocator itself isn't
overwritten.  Then we set cpu context appropiately, and jump.

I like this much better.  Also, it doesn't depend on the static OS load
area.

Some doubts:

  - What to do about physical_entry_addr now?  My patch currently discards
it, which I suppose is not what we want.

  - Should we do the same for the elf64 loader?  And how?  I don't know of
any example ELF64 images I could test with.

-- 
Robert Millan

  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."
Index: kern/i386/loader.S
===
--- kern/i386/loader.S	(revision 1755)
+++ kern/i386/loader.S	(working copy)
@@ -123,6 +123,13 @@
  * This starts the multiboot kernel.
  */
 
+VARIABLE(grub_multiboot_payload_size)
+	.long	0
+VARIABLE(grub_multiboot_payload_orig)
+	.long	0
+VARIABLE(grub_multiboot_payload_dest)
+	.long	0
+
 FUNCTION(grub_multiboot_real_boot)
 	/* Push the entry address on the stack.  */
 	pushl	%eax
@@ -138,9 +145,16 @@
 
 	/* Move the magic value into eax and jump to the kernel.  */
 	movl	$MULTIBOOT_MAGIC2,%eax
-	popl	%ecx
-	jmp 	*%ecx
 
+	/* Where do we copy what from.  */
+	movl	EXT_C(grub_multiboot_payload_size), %ecx
+	movl	EXT_C(grub_multiboot_payload_orig), %esi
+	movl	EXT_C(grub_multiboot_payload_dest), %edi
+			
+	/* Jump to the relocator.  */
+	popl	%edx
+	jmp 	*%edx
+	
 /*
  * This starts the multiboot 2 kernel.
  */
Index: include/grub/i386/pc/kernel.h
===
--- include/grub/i386/pc/kernel.h	(revision 1755)
+++ include/grub/i386/pc/kernel.h	(working copy)
@@ -86,6 +86,10 @@
 extern grub_addr_t EXPORT_FUNC(grub_arch_memdisk_addr) (void);
 extern grub_off_t EXPORT_FUNC(grub_arch_memdisk_size) (void);
 
+extern grub_addr_t EXPORT_VAR(grub_multiboot_payload_orig);
+extern grub_addr_t EXPORT_VAR(grub_multiboot_payload_dest);
+extern grub_size_t EXPORT_VAR(grub_multiboot_payload_size);
+
 #endif /* ! ASM_FILE */
 
 #endif /* ! KERNEL_MACHINE_HEADER */
Index: loader/i386/pc/multiboot.c
===
--- loader/i386/pc/multiboot.c	(revision 1756)
+++ loader/i386/pc/multiboot.c	(working copy)
@@ -50,6 +50,28 @@
 static struct grub_multiboot_info *mbi;
 static grub_addr_t entry;
 
+static char *playground = NULL;
+extern char *grub_multiboot_payload_orig, *grub_multiboot_payload_dest;
+extern grub_size_t grub_multiboot_payload_size;
+
+static grub_uint8_t forward_relocator[] =
+{
+  0xfc,		/* cld */
+  0x89, 0xf2,	/* movl %esi, %edx */
+  0xf3, 0xa4,	/* rep movsb */
+  0xff, 0xe2,	/* jmp *%edx */
+};
+
+static grub_uint8_t backward_relocator[] =
+{
+  0xfd,		/* std */
+  0x01, 0xce,	/* addl %ecx, %esi */
+  0x01, 0xcf,	/* addl %ecx, %edi */
+  0x41,		/* incl %ecx */
+  0xf3, 0xa4,	/* rep movsb */
+  0xff, 0xe7,	/* jmp *%edi */
+};
+
 static grub_err_t
 grub_multiboot_boot (void)
 {
@@ -118,35 +140,50 @@
 
   phdr_base = (char *) buffer + ehdr->e_phoff;
 #define phdr(i)			((Elf32_Phdr *) (phdr_base + (i) * ehdr->e_phentsize))
+  
+  {
+/* Find the highest address claimed by our payload.  */
+char *end_addr = NULL;
+for (i = 0; i < ehdr->e_phnum; i++)
+  {
+	grub_addr_t addr = (phdr(i)->p_paddr + phdr(i)->p_memsz);
+	if (addr > end_addr)
+	  end_addr = addr;
+  }
+grub_multiboot_payload_size = end_addr - phdr(0)->p_paddr;
+  }
 
+  if (playground)
+grub_free (playground);
+  playground = grub_malloc (sizeof (forward_relocator) + grub_multiboot_payload_size + sizeof (backward_relocator));
+  if (! playground)
+return grub_errno;
+
+  grub_multiboot_payload_orig = playground + sizeof (forward_relocator);
+  grub_multiboot_payload_dest = (char *) phdr(0)->

slight oddity in script parser

2008-07-31 Thread Patrick Georgi

Hi,

I forgot this, so sorry for the separate mail:
Currently, the parser expects comments to start at the beginning of the 
line, so "ls /foo # necessary because of baz" doesn't work properly


I think a good scenario would be to have comments start either on "#" at 
pos0, or on " #". This way, ls /foo#bar continues to work.


Also, currently the following is a single comment, which is an unusual 
feature:

# this comment starts here\
and continues on this line


Regards,
Patrick Georgi



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] PXE support for grub2

2008-07-31 Thread Bean
On Thu, Jul 31, 2008 at 12:50 AM, Bean <[EMAIL PROTECTED]> wrote:
> Hi,
>
> This patch add the (pxe) device that can be used to load files using
> the pxe service. It also add a user land command pxe that can be used
> to show pxe information as well as set some parameter.
>
> To create a pxe boot image:
>
> ./grub-mkimage -d . -o core.img pxe
> cat pxeboot.img core.img > g2pxe
>
> g2pxe is the pxe boot file, copy it to tftp server, you also need to
> copy *.mod, fs.lst, command.lst, moddep.lst and grub.cfg to /boot/grub
> directory in the tftp server.
>
> To test it in qemu, copy the files to directory such as /tftp, then:
>
> qemu -boot n -tftp /tftp -bootp /g2pxe
>
> usage for pxe command:
>
> pxe info
> Show information about pxe, like block size, client ip, etc.
>
> pxe blksize size
> Set block size. tftp transfer in trunks of bytes, the size can be
> configured. The minimum size is 512, which is also the default. The
> maximum size is 1432. Normally, you can increase download speed by
> setting larger block size, but some old tftp server may not support
> it. Also, qemu doesn't support size other than 512.
>
> pxe unload
> Unload the pxe runtime environment.
>
> Please note that this patch depends on my other patch bufio, you need
> to apply that first.
>
> 2008-07-30  Bean  <[EMAIL PROTECTED]>
>
>* boot/i386/pc/pxeboot.S: Use drive number 0x7F for pxe.
>
>* conf/i386-pc.rmk (kernel_img_HEADERS): Add machine/pxe.h.
>(pkglib_MODULES): Add pxe.mod and pxecmd.mod.
>(pxe_mod_SOURCES): New macro.
>(pxe_mod_CFLAGS): Likewise.
>(pxe_mod_LDFLAGS): Likewise.
>(pxecmd_mod_SOURCES): Likewise.
>(pxecmd_mod_CFLAGS): Likewise.
>(pxecmd_mod_LDFLAGS): Likewise.
>
>* kern/i386/pc/startup.S (grub_pxe_scan): New function.
>(grub_pxe_call): Likewise.
>
>* kern/i386/pc/init.c (make_install_device): Set root to (pxe) for pxe 
> boot.
>
>* include/grub/disk.h (grub_disk_dev_id): Add GRUB_DISK_DEVICE_PXE_ID.
>
>* commands/i386/pc/pxecmd.c: New file.
>
>* disk/i386/pc/pxe.c: Likewise.
>
>* include/grub/i386/pc/pxe.h: Likewise.

Hi,

Any comment ?

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] PXE support for grub2

2008-07-31 Thread Pavel Roskin
On Fri, 2008-08-01 at 11:46 +0800, Bean wrote:
> On Thu, Jul 31, 2008 at 12:50 AM, Bean <[EMAIL PROTECTED]> wrote:
> > Hi,
> >
> > This patch add the (pxe) device that can be used to load files using
> > the pxe service. It also add a user land command pxe that can be used
> > to show pxe information as well as set some parameter.
> >
> > To create a pxe boot image:
> >
> > ./grub-mkimage -d . -o core.img pxe
> > cat pxeboot.img core.img > g2pxe

Perhaps it should be a separate script?  Or maybe it should be an option
for grub-mkimage?  At least it should be documented.  Users who want to
use PXE should be able to figure it out easily.

-- 
Regards,
Pavel Roskin


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] buffered file read

2008-07-31 Thread Bean
On Fri, Aug 1, 2008 at 4:06 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Bean <[EMAIL PROTECTED]> writes:
>
>> On Fri, Aug 1, 2008 at 3:18 AM, Marco Gerards <[EMAIL PROTECTED]> wrote:
>>> Hi Bean,
>>>
>>> Bean <[EMAIL PROTECTED]> writes:
>>>
 I have added a buffer length parameter to grub_buffile_open:

 grub_file_t grub_buffile_open (const char *name, int size);

 size < 0: Load the whole file.
 size = 0: Use default buffer size (8192 bytes)
 size > 0: Custom buffer size = size

 The upper limit is 1m, if size > 1m, we set it to 1m.

 2008-07-28  Bean  <[EMAIL PROTECTED]>

   * conf/common.rmk (pkglib_MODULES): Add bufio.mod.
   (bufio_mod_SOURCES): New macro.
   (bufio_mod_CFLAGS): Likewise.
   (bufio_mod_LDFLAGS): Likewise.

   * include/grub/bufio.h: New file.

   * io/bufio.c: Likewise.

   * video/png.c (grub_video_reader_png): Use grub_buffile_open to open
   file.

   * video/jpeg.c (grub_video_reader_jpeg): Likewise.

   * video/tga.c (grub_video_reader_tga): Likewise.

   * font/manager.c (add_font): Likewise.
>>>
>>> Please mention it when you include .  The same
>>> for when you remove .
>>>
>>> Otherwise it seems good for inclusion :-)
>>
>> Hi,
>>
>> Oh, how the changelog would be like ?
>
> Specifically or generally?  Generally something like:
>
> * fs/foo.c: Include .
> (baz): New function.

Committed.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: slight oddity in script parser

2008-07-31 Thread Bean
On Fri, Aug 1, 2008 at 7:48 AM, Patrick Georgi <[EMAIL PROTECTED]> wrote:
> Hi,
>
> I forgot this, so sorry for the separate mail:
> Currently, the parser expects comments to start at the beginning of the
> line, so "ls /foo # necessary because of baz" doesn't work properly
>
> I think a good scenario would be to have comments start either on "#" at
> pos0, or on " #". This way, ls /foo#bar continues to work.
>
> Also, currently the following is a single comment, which is an unusual
> feature:
> # this comment starts here\
> and continues on this line

Hi,

Actually, # and \ is handled in the read line function, it can't
handle complicated situation.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: slight oddity in script parser

2008-07-31 Thread Stefan Reinauer
Bean wrote:
> On Fri, Aug 1, 2008 at 7:48 AM, Patrick Georgi <[EMAIL PROTECTED]> wrote:
>   
>> Hi,
>>
>> I forgot this, so sorry for the separate mail:
>> Currently, the parser expects comments to start at the beginning of the
>> line, so "ls /foo # necessary because of baz" doesn't work properly
>>
>> I think a good scenario would be to have comments start either on "#" at
>> pos0, or on " #". This way, ls /foo#bar continues to work.
>>
>> Also, currently the following is a single comment, which is an unusual
>> feature:
>> # this comment starts here\
>> and continues on this line
>> 
>
> Hi,
>
> Actually, # and \ is handled in the read line function, it can't
> handle complicated situation.
>   

Are you implying it is wrong there and should be moved to the parser?


-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
  Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: [EMAIL PROTECTED]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: slight oddity in script parser

2008-07-31 Thread Bean
On Fri, Aug 1, 2008 at 12:26 PM, Stefan Reinauer <[EMAIL PROTECTED]> wrote:
> Bean wrote:
>> On Fri, Aug 1, 2008 at 7:48 AM, Patrick Georgi <[EMAIL PROTECTED]> wrote:
>>
>>> Hi,
>>>
>>> I forgot this, so sorry for the separate mail:
>>> Currently, the parser expects comments to start at the beginning of the
>>> line, so "ls /foo # necessary because of baz" doesn't work properly
>>>
>>> I think a good scenario would be to have comments start either on "#" at
>>> pos0, or on " #". This way, ls /foo#bar continues to work.
>>>
>>> Also, currently the following is a single comment, which is an unusual
>>> feature:
>>> # this comment starts here\
>>> and continues on this line
>>>
>>
>> Hi,
>>
>> Actually, # and \ is handled in the read line function, it can't
>> handle complicated situation.
>>
>
> Are you implying it is wrong there and should be moved to the parser?

Hi,

In theory, it should be handled by the lexer. but actually, it's not
so easy. lexer already has some issue, adding new handling would not
help.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: slight oddity in script parser

2008-07-31 Thread Stefan Reinauer
Bean wrote:
> On Fri, Aug 1, 2008 at 12:26 PM, Stefan Reinauer <[EMAIL PROTECTED]> wrote:
>   
>> Bean wrote:
>> 
>>> On Fri, Aug 1, 2008 at 7:48 AM, Patrick Georgi <[EMAIL PROTECTED]> wrote:
>>>
>>>   
 Hi,

 I forgot this, so sorry for the separate mail:
 Currently, the parser expects comments to start at the beginning of the
 line, so "ls /foo # necessary because of baz" doesn't work properly

 I think a good scenario would be to have comments start either on "#" at
 pos0, or on " #". This way, ls /foo#bar continues to work.

 Also, currently the following is a single comment, which is an unusual
 feature:
 # this comment starts here\
 and continues on this line

 
>>> Hi,
>>>
>>> Actually, # and \ is handled in the read line function, it can't
>>> handle complicated situation.
>>>
>>>   
>> Are you implying it is wrong there and should be moved to the parser?
>> 
>
> Hi,
>
> In theory, it should be handled by the lexer. but actually, it's not
> so easy. lexer already has some issue, adding new handling would not
> help.
>
>   
So, what's the right way to fix it, then?

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
  Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: [EMAIL PROTECTED]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: slight oddity in script parser

2008-07-31 Thread Bean
On Fri, Aug 1, 2008 at 12:42 PM, Stefan Reinauer <[EMAIL PROTECTED]> wrote:
> Bean wrote:
>> On Fri, Aug 1, 2008 at 12:26 PM, Stefan Reinauer <[EMAIL PROTECTED]> wrote:
>>
>>> Bean wrote:
>>>
 On Fri, Aug 1, 2008 at 7:48 AM, Patrick Georgi <[EMAIL PROTECTED]> wrote:


> Hi,
>
> I forgot this, so sorry for the separate mail:
> Currently, the parser expects comments to start at the beginning of the
> line, so "ls /foo # necessary because of baz" doesn't work properly
>
> I think a good scenario would be to have comments start either on "#" at
> pos0, or on " #". This way, ls /foo#bar continues to work.
>
> Also, currently the following is a single comment, which is an unusual
> feature:
> # this comment starts here\
> and continues on this line
>
>
 Hi,

 Actually, # and \ is handled in the read line function, it can't
 handle complicated situation.


>>> Are you implying it is wrong there and should be moved to the parser?
>>>
>>
>> Hi,
>>
>> In theory, it should be handled by the lexer. but actually, it's not
>> so easy. lexer already has some issue, adding new handling would not
>> help.
>>
>>
> So, what's the right way to fix it, then?

Hi,

Perhaps we should use automatic tool to generate the lexer, like using
bison for the parser. But I recall that Marco encounter some issue
with flex, but I can't remember what it's now.

-- 
Bean


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel