idea: adjust conf file layout

[Bean]

Hi,

Currently, the organization of conf files are not optimal. There are a
lot of duplication in *.rmk files. For example, i386-efi.rmk and
x86_64-efi.rmk are basically the same, and grub-emu appears in every
rmk, but there is only minor difference between each one. Also,
related item are scattered around, which make it more likely for
casual mistakes, like forgeting to add files in grub_emu_SOURCES, etc.
Here are some of my thoughts:

1, Fine tune rmk layout
rmk are split into different blocks accordding to function, such as
common, commands, fs and partmap. Each block consists of generic and
platform dependent parts. For example, for i386-efi, the full set of
makefiles to be included is:

common-i386-efi.rmk
common-efi.rmk
common-i386.rmk
common.rmk
commands-i386-efi.rmk
commands-efi.rmk
commands-i386.rmk
commands.rmk
fs-i386-efi.rmk
fs-efi.rmk
fs-i386.rmk
fs.rmk
partmap-i386-efi.rmk
partmap-efi.rmk
partmap-i386.rmk
partmap.rmk

Of course, not all of them needs to be present, we can use -include to
add them to the main makefile.

2, Group related items together

For example, for the fat module, we could write it like this:

# For fat.mod.
fat_mod_SOURCES = fs/fat.c
fat_mod_CFLAGS = $(COMMON_CFLAGS)
fat_mod_LDFLAGS = $(COMMON_LDFLAGS)
pkglib_MODULES += fat.mod
grub_probe_SOURCES += fs/fat.c
grub_fstest_SOURCES += fs/fat.c
grub_emu_SOURCES += fs/fat.c

-- 
Bean


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

Re: Commands not executed in the else branch

[Pavel Roskin]

Quoting Bean :


Hi,

It's caused by the propagation of grub_errno value. This patch should fix it.

diff --git a/normal/execute.c b/normal/execute.c
index 8bf6d17..aec4589 100644
--- a/normal/execute.c
+++ b/normal/execute.c
@@ -177,6 +177,7 @@ grub_script_execute_cmdif (struct grub_script_cmd *cmd)
  read from the env variable `?'.  */
   grub_script_execute_cmd (cmdif->exec_to_evaluate);
   result = grub_env_get ("?");
+  grub_errno = 0;

   /* Execute the `if' or the `else' part depending on the value of
  `?'.  */


Thanks, it's working fine!

--
Regards,
Pavel Roskin


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

Re: [PATCH] Preboot support

[Vladimir 'phcoder' Serbinenko]

Commited

On Sun, Apr 26, 2009 at 4:38 PM, Vladimir 'phcoder' Serbinenko <
phco...@gmail.com> wrote:

> Warning fixed in the patch. If nobody objects I commit it tomorrow
>
>
> On Wed, Apr 15, 2009 at 10:40 AM, John Stanley wrote:
>
>> The way it looks to me is that preboot_func is the function to be executed
>>  a preboot time, whereas preboot_rest_func is a cleanup function to be
>> called to "restore" things to the pre-preboot_func state. Something like
>> this anyway.. its not yet real clear to me either.
>>
>> phcoder wrote:
>>
>>> Yoshinori K. Okuji wrote:
>>>
>>>  - I don't understand how preboot_func and preboot_rest_func are
 different. At least, not obvious. Can you elaborate on them?

>>> preboot_rest_func is a function which should undo any action taken by
>>> preboot_func. It's used if either loader aborts due to an error or returns
>>> (which is possible on some platforms)
>>>

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

>>>
>>>
>>> 
>>>
>>> ___
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>>
>>
>>
>> ___
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>



-- 
Regards
Vladimir 'phcoder' Serbinenko
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: idea: adjust conf file layout

[Vladimir 'phcoder' Serbinenko]

On Mon, Apr 27, 2009 at 9:09 AM, Bean  wrote:

> Hi,
>
> Currently, the organization of conf files are not optimal. There are a
> lot of duplication in *.rmk files. For example, i386-efi.rmk and
> x86_64-efi.rmk are basically the same, and grub-emu appears in every
> rmk, but there is only minor difference between each one. Also,
> related item are scattered around, which make it more likely for
> casual mistakes, like forgeting to add files in grub_emu_SOURCES, etc.

Basically I agree that it should be reorganized but some remarks follow

>
> Here are some of my thoughts:
>
> 1, Fine tune rmk layout
> rmk are split into different blocks accordding to function, such as
> common, commands, fs and partmap. Each block consists of generic and
> platform dependent parts. For example, for i386-efi, the full set of
> makefiles to be included is:
>
> common-i386-efi.rmk
> common-efi.rmk
> common-i386.rmk
> common.rmk
> commands-i386-efi.rmk
> commands-efi.rmk
> commands-i386.rmk
> commands.rmk
> fs-i386-efi.rmk
> fs-efi.rmk
> fs-i386.rmk
> fs.rmk
> partmap-i386-efi.rmk
> partmap-efi.rmk
> partmap-i386.rmk
> partmap.rmk
>
> Of course, not all of them needs to be present, we can use -include to
> add them to the main makefile.
>
I'm not sure that fine-graining is really necessary. Basically the most
important point is to eliminate duplication and group related things
together
However it's a good thing to have a separate efi.rmk and perhaps same
applies to ieee1275.rmk

>
> 2, Group related items together
>
> For example, for the fat module, we could write it like this:
>
> # For fat.mod.
> fat_mod_SOURCES = fs/fat.c
> fat_mod_CFLAGS = $(COMMON_CFLAGS)
> fat_mod_LDFLAGS = $(COMMON_LDFLAGS)
> pkglib_MODULES += fat.mod
> grub_probe_SOURCES += fs/fat.c
> grub_fstest_SOURCES += fs/fat.c
> grub_emu_SOURCES += fs/fat.c

It's better to unify the last three into a variable FS_SOURCES which will be
added to tools sources in corresponding files. This is useful because
different platforms provide different set of tools

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



-- 
Regards
Vladimir 'phcoder' Serbinenko
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Use symbol database to maintain module dependence

[Vladimir 'phcoder' Serbinenko]

symdb code seems to duplicate your list code. Perhaps you could reuse the
file from kernel thus making maintaining easier
I liked the file modsym.lst: it gives clear view of symbols.
This patch brek grub-install because moddep.lst isn't copied correctly
Also check that
Also in some places I saw
+  while (n)
+{
+  modified += remove_string (&u->cur_mods, (char *) n->name);
Here perhaps modified = remove_string (..) || modified; is more appropriate
+  n = n->next;
+}

+  remove MODULES  remove modules from the symbol databsae\n\
Typo

On Mon, Apr 27, 2009 at 8:33 AM, Bean  wrote:

> Hi,
>
> ping ?
>
> On Tue, Apr 21, 2009 at 5:11 AM, Bean  wrote:
> > Hi,
> >
> > This patch add a new tool grub-symdb to manage symbol database and
> > update module dependence as required. The build process is greatly
> > simplified, moddep.lst, def-*.lst, und-*.lst are not generated
> > anymore. grub-symdb will read the module files and update modsym.lst
> > and moddep.lst automatically.
> >
> > --
> > Bean
> >
>
>
>
> --
> Bean
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Split #2: parser/reader separation

[Vladimir 'phcoder' Serbinenko]

On Mon, Apr 27, 2009 at 7:51 AM, Bean  wrote:

> On Sun, Apr 26, 2009 at 11:50 PM, Vladimir 'phcoder' Serbinenko
>  wrote:
> > Hello this patch breaks  grub-emu. Also some files include grub/rescue.h
> and
> > this has to be fixed.
>
> Hi,
>
> For large patch, I normally add only the necessary changes to make it
> compile in i386-pc. As commits are quite frequent, this would minimize
> the effort to sync with svn head. I'd fix those small issue before
> final commitment.
>
Ok. However a Changlog would simplify the review because then it would be
easy to see which parts are newly written and which is just code moving
around

>
> > Also there is a bug is that in case of syntax error reader continues to
> ask
> > for more lines. E.g
> > grub> if
> >>;
> > syntax error
> >>
> > Then it's impossible to exit from this "bug mode". It's not your fault
> but
> > since you touch this code could you fix this?
>
> I believe a proper way to solve this is to add an eol character, for
> example ctrl-D. Then we have a way to break from console input. But
> this fix is not trivial, perhaps it should be in a separate patch.
>
Ok

>
> > This patch increases the size of kernel by 504 bytes and the size of
> > core.img by 224 bytes. Is there a way to do the same thing in a more
> compact
> > way?
>
> Oh, I think it is quite compact already, have you got any suggestion
> to reduce its size ?
>
I'll have a look at it again today or tomorrow

>
> > Also it would be good to write the current parser in "grub>" prompt (not
> > necessary for rescue prompt)
>
> Good point.
>
> --
> Bean
>
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] ACPI spoofing

[Vladimir 'phcoder' Serbinenko]

Should work on EFI now

On Sat, Apr 11, 2009 at 11:06 PM, phcoder  wrote:

> Hello, here is the patch to spoof ACPI tables. It's useful for developement
> and debugging but also for the end-users. Unfortunately many manufacturers
> ship ACPI tables that work well only with win. Fortunately now linux has an
> interpreter which workarounds most known bugs and has an ability to put dsdt
> on ramdisk. But it's not the case for all OS. Hence this patch
> --
>
> Regards
> Vladimir 'phcoder' Serbinenko
>



-- 
Regards
Vladimir 'phcoder' Serbinenko
diff --git a/ChangeLog b/ChangeLog
index b0943bc..a68b970 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,17 @@
 2009-04-26  Vladimir Serbinenko 
 
+	ACPI spoofing
+
+	* commands/acpi.c: new file
+	* commands/i386/pc/acpi.c: likewise
+	* include/grub/acpi.h: likewise
+	* conf/i386-pc.rmk (pkglib_MODULES): added acpi.mod
+	(acpi_mod_SOURCES): new variable
+	(acpi_mod_CFLAGS): likewise
+	(acpi_mod_LDFLAGS): likewise
+
+2009-04-11  Vladimir Serbinenko 
+
 	Mmap services
 
 	* loader/i386/efi/linux.c (grub_linux_boot): use grub_mmap_iterate
diff --git a/commands/acpi.c b/commands/acpi.c
new file mode 100644
index 000..05dfefa
--- /dev/null
+++ b/commands/acpi.c
@@ -0,0 +1,770 @@
+/* acpi.c - modify acpi tables. */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  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 .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#ifdef GRUB_MACHINE_EFI
+#include 
+#include 
+#endif
+
+static const struct grub_arg_option options[] = {
+  {"exclude", 'x', 0, 
+   "Don't load host tables specified by comma-separated list", 
+   0, ARG_TYPE_STRING},
+  {"load-only", 'n', 0, 
+   "Load only tables specified by comma-separated list", 0, ARG_TYPE_STRING},
+  {"v1", '1', 0, "Expose v1 tables", 0, ARG_TYPE_NONE},
+  {"v2", '2', 0, "Expose v2 and v3 tables", 0, ARG_TYPE_NONE},
+  {"oemid", 'o', 0, "Set OEMID of RSDP, XSDT and RSDT", 0, ARG_TYPE_STRING},
+  {"oemtable", 't', 0, 
+   "Set OEMTABLE ID of RSDP, XSDT and RSDT", 0, ARG_TYPE_STRING},  
+  {"oemtablerev", 'r', 0, 
+   "Set OEMTABLE revision of RSDP, XSDT and RSDT", 0, ARG_TYPE_INT},  
+  {"oemtablecreator", 'c', 0, 
+   "Set creator field of RSDP, XSDT and RSDT", 0, ARG_TYPE_STRING},  
+  {"oemtablecreatorrev", 'd', 0, 
+   "Set creator revision of RSDP, XSDT and RSDT", 0, ARG_TYPE_INT},  
+  {"no-ebda", 'e', 0, "Don't update EBDA. May fix failures or hangs on some"
+   " BIOSes but makes it uneffective with OS not recieving RSDP fro GRUB", 
+   0, ARG_TYPE_NONE},
+  {0, 0, 0, 0, 0, 0}
+};
+
+/* Simple checksum by summing all bytes. Used by ACPI and SMBIOS. */
+grub_uint8_t 
+grub_byte_checksum (void *base, grub_size_t size)
+{
+  grub_uint8_t *ptr;
+  grub_uint8_t ret = 0;
+  for (ptr = (grub_uint8_t *) base; ptr < ((grub_uint8_t *) base) + size;
+   ptr++)
+ret += *ptr;
+  return ret;
+}
+
+/* rev1 is 1 if ACPIv1 is to be generated, 0 otherwise. 
+   rev2 contains the revision of ACPIv2+ to generate or 0 if none. */
+static int rev1, rev2;
+static grub_dl_t my_mod;
+/* OEMID of RSDP, RSDT and XSDT. */
+static char root_oemid[6];
+/* OEMTABLE of the same tables. */
+static char root_oemtable[8];
+/* OEMREVISION of the same tables. */
+static grub_uint32_t root_oemrev;
+/* CreatorID of the same tables. */
+static char root_creator_id[4];
+/* CreatorRevision of the same tables. */
+static grub_uint32_t root_creator_rev;
+static struct grub_acpi_rsdp_v10 *rsdpv1_new = 0;
+static struct grub_acpi_rsdp_v20 *rsdpv2_new = 0;
+static char *playground = 0, *playground_ptr = 0;
+static int playground_size = 0;
+
+/* Linked list of ACPI tables. */
+struct efiemu_acpi_table
+{
+  void *addr;
+  grub_size_t size;
+  struct efiemu_acpi_table *next;
+};
+static struct efiemu_acpi_table *acpi_tables = 0;
+
+/* DSDT isn't in RSDT. So treat it specially. */
+static void *table_dsdt = 0;
+/* Pointer to recreated RSDT. */
+static void *rsdt_addr = 0;
+
+/* Allocation handles for different tables. */
+static grub_size_t dsdt_size = 0;
+
+/* Address of original FACS. */
+static grub_uint32_t facs_addr = 0;
+
+struct grub_acpi_rsdp_v20 *
+grub_acpi_get_rsdpv2 (void)
+{
+  if (rsdpv2_new)
+return rsdpv2_new;
+  if (rsdpv1_new)
+return 0;
+  return grub_machine_acpi_get_rsdpv2 (

Re: [PATCH] Use symbol database to maintain module dependence

[Bean]

On Tue, Apr 28, 2009 at 2:13 AM, Vladimir 'phcoder' Serbinenko
 wrote:
> symdb code seems to duplicate your list code. Perhaps you could reuse the
> file from kernel thus making maintaining easier

Hi,

I'm planning to use kern/list.c at first, but there is some issue.
list.c uses grub_strcmp, which means we need to add kern/misc.c as
well. But kern/misc.c uses environment and terminal output, etc. I end
up adding a lot of unrelated stuff just to satisfy the dependence. So
perhaps it'd be better to keep it simple for now.

A better solution is to move the string function to a separate source
file, such as kern/string.c. But this is the subject of another patch.

> I liked the file modsym.lst: it gives clear view of symbols.
> This patch brek grub-install because moddep.lst isn't copied correctly
> Also check that
> Also in some places I saw
> +  while (n)
> +    {
> +  modified += remove_string (&u->cur_mods, (char *) n->name);
> Here perhaps modified = remove_string (..) || modified; is more appropriate
> +  n = n->next;
> +    }
>

Right, the += is a quick trick, || would be more meaningful.

> +  remove MODULES  remove modules from the symbol databsae\n\
> Typo
>

Oh, thanks for noticing this.

> On Mon, Apr 27, 2009 at 8:33 AM, Bean  wrote:
>>
>> Hi,
>>
>> ping ?
>>
>> On Tue, Apr 21, 2009 at 5:11 AM, Bean  wrote:
>> > Hi,
>> >
>> > This patch add a new tool grub-symdb to manage symbol database and
>> > update module dependence as required. The build process is greatly
>> > simplified, moddep.lst, def-*.lst, und-*.lst are not generated
>> > anymore. grub-symdb will read the module files and update modsym.lst
>> > and moddep.lst automatically.
>> >
>> > --
>> > Bean
>> >
>>
>>
>>
>> --
>> Bean
>>
>>
>> ___
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Bean


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

Re: Revision 2136 breaks two-disk configuarion

[Pavel Roskin]

On Sun, 2009-04-26 at 22:57 -0700, David Miller wrote:

> grub_disk_open() isn't used, but a grub_device_open() does occur
> during the iterator that has us find the FS_UUID device.  This
> happens search_fs_uuid().
> 
> If we don't do this, we leave a device reference open and dangling.

Then we should be using grub_device_close().  I've made a patch that
keeps dev, not disk in the data field.  Unfortunately, the problem
persists.  I could easily reproduce it on another machine by using a
flash drive and qemu.

It's entirely possible that the problem is elsewhere.  But I have no
experience debugging memory problems in GRUB, so it will take time
before I find out.

Here's the patch.  It doesn't make the memory problem go away, but it
makes the code nicer by using the same abstraction to open and close the
disk.

diff --git a/disk/fs_uuid.c b/disk/fs_uuid.c
index 9d83bb8..f590ad2 100644
--- a/disk/fs_uuid.c
+++ b/disk/fs_uuid.c
@@ -89,7 +89,7 @@ grub_fs_uuid_open (const char *name, grub_disk_t disk)
   disk->total_sectors = dev->disk->total_sectors;
   disk->has_partitions = 0;
   disk->partition = dev->disk->partition;
-  disk->data = dev->disk;
+  disk->data = dev;
 
   return GRUB_ERR_NONE;
 }
@@ -97,24 +97,24 @@ grub_fs_uuid_open (const char *name, grub_disk_t disk)
 static void
 grub_fs_uuid_close (grub_disk_t disk __attribute((unused)))
 {
-  grub_disk_t parent = disk->data;
-  grub_disk_close (parent);
+  grub_device_t parent = disk->data;
+  grub_device_close (parent);
 }
 
 static grub_err_t
 grub_fs_uuid_read (grub_disk_t disk, grub_disk_addr_t sector,
   grub_size_t size, char *buf)
 {
-  grub_disk_t parent = disk->data;
-  return parent->dev->read (parent, sector, size, buf);
+  grub_device_t parent = disk->data;
+  return parent->disk->dev->read (parent->disk, sector, size, buf);
 }
 
 static grub_err_t
 grub_fs_uuid_write (grub_disk_t disk, grub_disk_addr_t sector,
grub_size_t size, const char *buf)
 {
-  grub_disk_t parent = disk->data;
-  return parent->dev->write (parent, sector, size, buf);
+  grub_device_t parent = disk->data;
+  return parent->disk->dev->write (parent->disk, sector, size, buf);
 }
 
 static struct grub_disk_dev grub_fs_uuid_dev =

-- 
Regards,
Pavel Roskin


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

[PATCH] Re: Revision 2136 breaks two-disk configuarion

[Pavel Roskin]

On Mon, 2009-04-27 at 18:26 -0400, Pavel Roskin wrote:

> It's entirely possible that the problem is elsewhere.  But I have no
> experience debugging memory problems in GRUB, so it will take time
> before I find out.

Done!  disk->partition should not be copied by reference.  This patch
fixes the broken magic problem.  The issue with mixing device and disk
functions could be addressed separately.

ChangeLog:
disk/fs_uuid.c (grub_fs_uuid_open): Allocate memory to copy
parent's partition, don't copy it by reference, as it gets freed
on close.
---

 disk/fs_uuid.c |   12 +++-
 1 files changed, 11 insertions(+), 1 deletions(-)


diff --git a/disk/fs_uuid.c b/disk/fs_uuid.c
index 9d83bb8..9636ce9 100644
--- a/disk/fs_uuid.c
+++ b/disk/fs_uuid.c
@@ -25,6 +25,7 @@
 #include 
 
 #include 
+#include 
 
 static grub_device_t
 search_fs_uuid (const char *key, unsigned long *count)
@@ -88,7 +89,16 @@ grub_fs_uuid_open (const char *name, grub_disk_t disk)
 
   disk->total_sectors = dev->disk->total_sectors;
   disk->has_partitions = 0;
-  disk->partition = dev->disk->partition;
+  if (dev->disk->partition)
+{
+  disk->partition = grub_malloc (sizeof (*disk->partition));
+  if (disk->partition)
+   grub_memcpy (disk->partition, dev->disk->partition,
+sizeof (*disk->partition));
+}
+  else
+disk->partition = NULL;
+
   disk->data = dev->disk;
 
   return GRUB_ERR_NONE;

-- 
Regards,
Pavel Roskin


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

Re: [PATCH] Re: Revision 2136 breaks two-disk configuarion

[David Miller]

From: Pavel Roskin 
Date: Mon, 27 Apr 2009 21:37:52 -0400

> On Mon, 2009-04-27 at 18:26 -0400, Pavel Roskin wrote:
> 
>> It's entirely possible that the problem is elsewhere.  But I have no
>> experience debugging memory problems in GRUB, so it will take time
>> before I find out.
> 
> Done!  disk->partition should not be copied by reference.  This patch
> fixes the broken magic problem.  The issue with mixing device and disk
> functions could be addressed separately.
> 
> ChangeLog:
>   disk/fs_uuid.c (grub_fs_uuid_open): Allocate memory to copy
>   parent's partition, don't copy it by reference, as it gets freed
>   on close.

Thanks for fixing this Pavel.  I suspect this bug is why the close was
left as a NOP function all of this time.

Please commit this as it seems you haven't already :-)

I'll take a look at your other patch too.


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

Re: [PATCH] Re: Revision 2136 breaks two-disk configuarion

[Pavel Roskin]

On Mon, 2009-04-27 at 18:45 -0700, David Miller wrote:

> Thanks for fixing this Pavel.  I suspect this bug is why the close was
> left as a NOP function all of this time.

Maybe.

> Please commit this as it seems you haven't already :-)

I was about to commit it when I realized that the same could be done
using a reference counter in struct grub_partition.  It may be an
overkill for this purpose, but maybe there will be more users of the
partition table in the future.  Besides, this patch could set an
example.  Please have a cursory look to make sure it's a good example.

ChangeLog:
* grub/partition.h (struct grub_partition): Add refcount.
(grub_partition_ref): New inline function.
(grub_partition_unref): Likewise.
* kern/disk.c (grub_disk_close): Use grub_partition_unref().
* disk/fs_uuid.c (grub_fs_uuid_open): Use grub_partition_ref().

diff --git a/disk/fs_uuid.c b/disk/fs_uuid.c
index 9d83bb8..12086b2 100644
--- a/disk/fs_uuid.c
+++ b/disk/fs_uuid.c
@@ -25,6 +25,7 @@
 #include 
 
 #include 
+#include 
 
 static grub_device_t
 search_fs_uuid (const char *key, unsigned long *count)
@@ -88,7 +89,7 @@ grub_fs_uuid_open (const char *name, grub_disk_t disk)
 
   disk->total_sectors = dev->disk->total_sectors;
   disk->has_partitions = 0;
-  disk->partition = dev->disk->partition;
+  disk->partition = grub_partition_ref (dev->disk->partition);
   disk->data = dev->disk;
 
   return GRUB_ERR_NONE;
diff --git a/include/grub/partition.h b/include/grub/partition.h
index 6e74cd5..c3e4d15 100644
--- a/include/grub/partition.h
+++ b/include/grub/partition.h
@@ -62,6 +62,9 @@ struct grub_partition
 
   /* The index of this partition in the partition table.  */
   int index;
+
+  /* Reference count.  */
+  int refcount;
   
   /* Partition map type specific data.  */
   void *data;
@@ -110,4 +113,23 @@ grub_partition_get_len (const grub_partition_t p)
   return p->len;
 }
 
+/* Return a copy by reference of the original partition.  */
+static inline grub_partition_t
+grub_partition_ref (grub_partition_t p)
+{
+  if (p)
+p->refcount++;
+  return p;
+}
+
+/* Return partition to be freed if it can be freed.  */
+static inline grub_partition_t
+grub_partition_unref (const grub_partition_t p)
+{
+  if (p && p->refcount-- <= 0)
+return p;
+  else
+return 0;
+}
+
 #endif /* ! GRUB_PART_HEADER */
diff --git a/kern/disk.c b/kern/disk.c
index 8a92989..070cab3 100644
--- a/kern/disk.c
+++ b/kern/disk.c
@@ -324,7 +324,7 @@ grub_disk_close (grub_disk_t disk)
   /* Reset the timer.  */
   grub_last_time = grub_get_time_ms ();
 
-  grub_free (disk->partition);
+  grub_free (grub_partition_unref (disk->partition));
   grub_free ((void *) disk->name);
   grub_free (disk);
 }

-- 
Regards,
Pavel Roskin


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

Crash in usb_keyboard

[Pavel Roskin]

Hello!

Running "terminal_input.usb_keyboard" crashed GRUB (or qemu it's running
in).  It turns out we never check if usbdev in term/usb_keyboard.c is
not NULL.  It is NULL if no USB host controllers have been detected.

I'm not sure if we even want to call grub_term_register_input() if no
USB keyboard is present.

-- 
Regards,
Pavel Roskin


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

Re: grub-mkconfig for combined serial and console

[Pavel Roskin]

On Fri, 2009-04-24 at 12:09 +1200, Daniel Reurich wrote:
> Hi
> 
> I'd like to be able to define terminal --timeout=5 serial console in a
> way that grub-mkconfig will automatically set that when updating the
> grub.cfg file.
> 
> Is there an easy way to do this (and disable the use of gfxterm).

See 00_header file (it's installed to /usr/local/etc/grub.d by default).
Try variables found in that file, such as GRUB_TERMINAL_INPUT,
GRUB_TIMEOUT, GRUB_SERIAL_COMMAND and others.

-- 
Regards,
Pavel Roskin


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