[PATCH 1/4] Handle errors in rmmod

[PATCH 1/4] Handle errors in rmmod

[Pavel Roskin]

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;
 }

[PATCH 2/4] Fix insmod not to increase refcount above 1

[Pavel Roskin]

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;

[PATCH 3/4] Change grub_file_seek() to return grub_err_t

[Pavel Roskin]

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 offset)
 {
-  grub_off_t old;
-
   if (offset > file->size)
     {
-      grub_error (GRUB_ERR_OUT_OF_RANGE,
-		  "attempt to seek outside of the file");
-      return -1;
+      return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			 "attempt to seek outside of the file");
     }
 
-  old = file->offset;
   file->offset = offset;
-  return old;
+  return GRUB_ERR_NONE;
 }
diff --git a/loader/aout.c b/loader/aout.c
index 0254b6a..a700771 100644
--- a/loader/aout.c
+++ b/loader/aout.c
@@ -43,7 +43,7 @@ grub_aout_load (grub_file_t file, int offset,
 		int load_size,
                 grub_addr_t bss_end_addr)
 {
-  if ((grub_file_seek (file, offset)) == (grub_off_t) - 1)
+  if (grub_file_seek (file, offset) != GRUB_ERR_NONE)
     return grub_errno;
 
   if (!load_size)
diff --git a/loader/i386/bsd.c b/loader/i386/bsd.c
index 468e6d0..f592f81 100644
--- a/loader/i386/bsd.c
+++ b/loader/i386/bsd.c
@@ -610,7 +610,7 @@ grub_bsd_load_aout (grub_file_t file)
   int ofs, align_page;
   union grub_aout_header ah;
 
-  if ((grub_file_seek (file, 0)) == (grub_off_t) - 1)
+  if ((grub_file_seek (file, 0)) != GRUB_ERR_NONE)
     return grub_errno;
 
   if (grub_file_read (file, &ah, sizeof (ah)) != sizeof (ah))
diff --git a/loader/i386/bsdXX.c b/loader/i386/bsdXX.c
index 3f15579..2ae542e 100644
--- a/loader/i386/bsdXX.c
+++ b/loader/i386/bsdXX.c
@@ -13,7 +13,7 @@ load (grub_file_t file, void *where, grub_off_t off, grub_size_t size)
   if (PTR_TO_UINT32 (where) + size > grub_os_area_addr + grub_os_area_size)
     return grub_error (GRUB_ERR_OUT_OF_RANGE,
 		       "Not enough memory for the module");
-  if (grub_file_seek (file, off) == (grub_off_t) -1)
+  if (grub_file_seek (file, off) != GRUB_ERR_NONE)
     return grub_errno;
   if (grub_file_read (file, where, size)
       != (grub_ssize_t) size)
@@ -29,7 +29,7 @@ load (grub_file_t file, void *where, grub_off_t off, grub_size_t size)
 static inline grub_err_t
 read_headers (grub_file_t file, Elf_Ehdr *e, char **shdr)
 {
- if (grub_file_seek (file, 0) == (grub_off_t) -1)
+ if (grub_file_seek (file, 0) != GRUB_ERR_NONE)
     return grub_errno;
 
   if (grub_file_read (file, (char *) e, sizeof (*e)) != sizeof (*e))
@@ -55,7 +55,7 @@ read_headers (grub_file_t file, Elf_Ehdr *e, char **shdr)
   if (! *shdr)
     return grub_errno;
 
-  if (grub_file_seek (file, e->e_shoff) == (grub_off_t) -1)
+  if (grub_file_seek (file, e->e_shoff) != GRUB_ERR_NONE)
     return grub_errno;
 
   if (grub_file_read (file, *shdr, e->e_shnum * e->e_shentsize)
@@ -264,7 +264,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (grub_file_t file, grub_addr_t *kern_end)
   symstart = curload = ALIGN_UP (*kern_end, sizeof (grub_freebsd_addr_t));
   *((grub_freebsd_addr_t *) UINT_TO_PTR (curload)) = symsize;
   curload += sizeof (grub_freebsd_addr_t);
-  if (grub_file_seek (file, symoff) == (grub_off_t) -1)
+  if (grub_file_seek (file, symoff) != GRUB_ERR_NONE)
     return grub_errno;
   sym = (Elf_Sym *) UINT_TO_PTR (curload);
   if (grub_file_read (file, UINT_TO_PTR (curload), symsize) !=
@@ -278,7 +278,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (grub_file_t file, grub_addr_t *kern_end)
 
   *((grub_freebsd_addr_t *) UINT_TO_PTR (curload)) = strsize;
   curload += sizeof (grub_freebsd_addr_t);
-  if (grub_file_seek (file, stroff) == (grub_off_t) -1)
+  if (grub_file_seek (file, stroff) != GRUB_ERR_NONE)
     return grub_errno;
   str = (char *) UINT_TO_PTR (curload);
   if (grub_file_read (file, UINT_TO_PTR (curload), strsize)
diff --git a/loader/i386/multiboot.c b/loader/i386/multiboot.c
index 87ffcae..fc42cb1 100644
--- a/loader/i386/multiboot.c
+++ b/loader/i386/multiboot.c
@@ -293,7 +293,7 @@ grub_multiboot (int argc, char *argv[])
 
       grub_multiboot_payload_orig = (long) playground + RELOCATOR_SIZEOF(forward);
 
-      if ((grub_file_seek (file, offset)) == (grub_off_t) - 1)
+      if ((grub_file_seek (file, offset)) != GRUB_ERR_NONE)
 	goto fail;
 
       grub_file_read (file, (void *) grub_multiboot_payload_orig, load_size);
diff --git a/loader/i386/multiboot_elfxx.c b/loader/i386/multiboot_elfxx.c
index 77c4711..f1b7233 100644
--- a/loader/i386/multiboot_elfxx.c
+++ b/loader/i386/multiboot_elfxx.c
@@ -115,8 +115,8 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, void *buffer)
 	  grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
 			i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
 
-	  if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset)
-	      == (grub_off_t) -1)
+	  if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset) !=
+	      GRUB_ERR_NONE)
 	    return grub_error (GRUB_ERR_BAD_OS,
 			       "invalid offset in program header");
 
diff --git a/loader/macho.c b/loader/macho.c
index bd460b8..7fddaf0 100644
--- a/loader/macho.c
+++ b/loader/macho.c
@@ -50,7 +50,7 @@ grub_macho_parse32 (grub_macho_t macho)
     return;
 
   /* Read header and check magic*/
-  if (grub_file_seek (macho->file, macho->offset32) == (grub_off_t) -1
+  if (grub_file_seek (macho->file, macho->offset32) != GRUB_ERR_NONE
       || grub_file_read (macho->file, &head, sizeof (head))
       != sizeof(head))
     {
@@ -123,7 +123,7 @@ grub_macho32_readfile (grub_macho_t macho, void *dest)
     return grub_error (GRUB_ERR_BAD_OS,
 		       "Couldn't read architecture-specific part");
 
-  if (grub_file_seek (macho->file, macho->offset32) == (grub_off_t) -1)
+  if (grub_file_seek (macho->file, macho->offset32) != GRUB_ERR_NONE)
     {
       grub_error_push ();
       return grub_error (GRUB_ERR_BAD_OS,
@@ -206,8 +206,8 @@ grub_macho32_load (grub_macho_t macho, char *offset, int flags)
     if (! hdr->vmsize)
       return 0;
 
-    if (grub_file_seek (_macho->file, hdr->fileoff
-			+ _macho->offset32) == (grub_off_t) -1)
+    if (grub_file_seek (_macho->file, hdr->fileoff + _macho->offset32) !=
+	GRUB_ERR_NONE)
       {
 	grub_error_push ();
 	grub_error (GRUB_ERR_BAD_OS,
@@ -297,7 +297,7 @@ grub_macho_file (grub_file_t file)
   macho->cmds32 = 0;
   macho->cmds64 = 0;
 
-  if (grub_file_seek (macho->file, 0) == (grub_off_t) -1)
+  if (grub_file_seek (macho->file, 0) != GRUB_ERR_NONE)
     goto fail;
 
   if (grub_file_read (macho->file, &filestart, sizeof (filestart))
@@ -316,8 +316,8 @@ grub_macho_file (grub_file_t file)
 
       /* Load architecture description. */
       narchs = grub_be_to_cpu32 (filestart.fat.nfat_arch);
-      if (grub_file_seek (macho->file, sizeof (struct grub_macho_fat_header))
-	  == (grub_off_t) -1)
+      if (grub_file_seek (macho->file, sizeof (struct grub_macho_fat_header)) !=
+	  GRUB_ERR_NONE)
 	goto fail;
       archs = grub_malloc (sizeof (struct grub_macho_fat_arch) * narchs);
       if (!archs)
diff --git a/loader/xnu_resume.c b/loader/xnu_resume.c
index 77f6887..998e61e 100644
--- a/loader/xnu_resume.c
+++ b/loader/xnu_resume.c
@@ -103,7 +103,7 @@ grub_xnu_resume (char *imagename)
     }
 
   /* Read image. */
-  if (grub_file_seek (file, 0) == (grub_off_t)-1
+  if (grub_file_seek (file, 0) != GRUB_ERR_NONE
       || grub_file_read (file, buf, hibhead.image_size)
       != (grub_ssize_t) hibhead.image_size)
     {

[PATCH 4/4] Fix ALIGN_UP cutting upper bits

[Pavel Roskin]

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)

Re: [PATCH 3/4] Change grub_file_seek() to return grub_err_t

[Vladimir 'phcoder' Serbinenko]

On Wed, Jul 22, 2009 at 5:16 AM, Pavel Roskin<proski@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);

> ---
>  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 offset)
>  {
> -  grub_off_t old;
> -
>   if (offset > file->size)
>     {
> -      grub_error (GRUB_ERR_OUT_OF_RANGE,
> -                 "attempt to seek outside of the file");
> -      return -1;
> +      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +                        "attempt to seek outside of the file");
>     }
>
> -  old = file->offset;
>   file->offset = offset;
> -  return old;
> +  return GRUB_ERR_NONE;
>  }
> diff --git a/loader/aout.c b/loader/aout.c
> index 0254b6a..a700771 100644
> --- a/loader/aout.c
> +++ b/loader/aout.c
> @@ -43,7 +43,7 @@ grub_aout_load (grub_file_t file, int offset,
>                int load_size,
>                 grub_addr_t bss_end_addr)
>  {
> -  if ((grub_file_seek (file, offset)) == (grub_off_t) - 1)
> +  if (grub_file_seek (file, offset) != GRUB_ERR_NONE)
>     return grub_errno;
>
>   if (!load_size)
> diff --git a/loader/i386/bsd.c b/loader/i386/bsd.c
> index 468e6d0..f592f81 100644
> --- a/loader/i386/bsd.c
> +++ b/loader/i386/bsd.c
> @@ -610,7 +610,7 @@ grub_bsd_load_aout (grub_file_t file)
>   int ofs, align_page;
>   union grub_aout_header ah;
>
> -  if ((grub_file_seek (file, 0)) == (grub_off_t) - 1)
> +  if ((grub_file_seek (file, 0)) != GRUB_ERR_NONE)
>     return grub_errno;
>
>   if (grub_file_read (file, &ah, sizeof (ah)) != sizeof (ah))
> diff --git a/loader/i386/bsdXX.c b/loader/i386/bsdXX.c
> index 3f15579..2ae542e 100644
> --- a/loader/i386/bsdXX.c
> +++ b/loader/i386/bsdXX.c
> @@ -13,7 +13,7 @@ load (grub_file_t file, void *where, grub_off_t off, grub_size_t size)
>   if (PTR_TO_UINT32 (where) + size > grub_os_area_addr + grub_os_area_size)
>     return grub_error (GRUB_ERR_OUT_OF_RANGE,
>                       "Not enough memory for the module");
> -  if (grub_file_seek (file, off) == (grub_off_t) -1)
> +  if (grub_file_seek (file, off) != GRUB_ERR_NONE)
>     return grub_errno;
>   if (grub_file_read (file, where, size)
>       != (grub_ssize_t) size)
> @@ -29,7 +29,7 @@ load (grub_file_t file, void *where, grub_off_t off, grub_size_t size)
>  static inline grub_err_t
>  read_headers (grub_file_t file, Elf_Ehdr *e, char **shdr)
>  {
> - if (grub_file_seek (file, 0) == (grub_off_t) -1)
> + if (grub_file_seek (file, 0) != GRUB_ERR_NONE)
>     return grub_errno;
>
>   if (grub_file_read (file, (char *) e, sizeof (*e)) != sizeof (*e))
> @@ -55,7 +55,7 @@ read_headers (grub_file_t file, Elf_Ehdr *e, char **shdr)
>   if (! *shdr)
>     return grub_errno;
>
> -  if (grub_file_seek (file, e->e_shoff) == (grub_off_t) -1)
> +  if (grub_file_seek (file, e->e_shoff) != GRUB_ERR_NONE)
>     return grub_errno;
>
>   if (grub_file_read (file, *shdr, e->e_shnum * e->e_shentsize)
> @@ -264,7 +264,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (grub_file_t file, grub_addr_t *kern_end)
>   symstart = curload = ALIGN_UP (*kern_end, sizeof (grub_freebsd_addr_t));
>   *((grub_freebsd_addr_t *) UINT_TO_PTR (curload)) = symsize;
>   curload += sizeof (grub_freebsd_addr_t);
> -  if (grub_file_seek (file, symoff) == (grub_off_t) -1)
> +  if (grub_file_seek (file, symoff) != GRUB_ERR_NONE)
>     return grub_errno;
>   sym = (Elf_Sym *) UINT_TO_PTR (curload);
>   if (grub_file_read (file, UINT_TO_PTR (curload), symsize) !=
> @@ -278,7 +278,7 @@ SUFFIX (grub_freebsd_load_elf_meta) (grub_file_t file, grub_addr_t *kern_end)
>
>   *((grub_freebsd_addr_t *) UINT_TO_PTR (curload)) = strsize;
>   curload += sizeof (grub_freebsd_addr_t);
> -  if (grub_file_seek (file, stroff) == (grub_off_t) -1)
> +  if (grub_file_seek (file, stroff) != GRUB_ERR_NONE)
>     return grub_errno;
>   str = (char *) UINT_TO_PTR (curload);
>   if (grub_file_read (file, UINT_TO_PTR (curload), strsize)
> diff --git a/loader/i386/multiboot.c b/loader/i386/multiboot.c
> index 87ffcae..fc42cb1 100644
> --- a/loader/i386/multiboot.c
> +++ b/loader/i386/multiboot.c
> @@ -293,7 +293,7 @@ grub_multiboot (int argc, char *argv[])
>
>       grub_multiboot_payload_orig = (long) playground + RELOCATOR_SIZEOF(forward);
>
> -      if ((grub_file_seek (file, offset)) == (grub_off_t) - 1)
> +      if ((grub_file_seek (file, offset)) != GRUB_ERR_NONE)
>        goto fail;
>
>       grub_file_read (file, (void *) grub_multiboot_payload_orig, load_size);
> diff --git a/loader/i386/multiboot_elfxx.c b/loader/i386/multiboot_elfxx.c
> index 77c4711..f1b7233 100644
> --- a/loader/i386/multiboot_elfxx.c
> +++ b/loader/i386/multiboot_elfxx.c
> @@ -115,8 +115,8 @@ CONCAT(grub_multiboot_load_elf, XX) (grub_file_t file, void *buffer)
>          grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
>                        i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
>
> -         if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset)
> -             == (grub_off_t) -1)
> +         if (grub_file_seek (file, (grub_off_t) phdr(i)->p_offset) !=
> +             GRUB_ERR_NONE)
>            return grub_error (GRUB_ERR_BAD_OS,
>                               "invalid offset in program header");
>
> diff --git a/loader/macho.c b/loader/macho.c
> index bd460b8..7fddaf0 100644
> --- a/loader/macho.c
> +++ b/loader/macho.c
> @@ -50,7 +50,7 @@ grub_macho_parse32 (grub_macho_t macho)
>     return;
>
>   /* Read header and check magic*/
> -  if (grub_file_seek (macho->file, macho->offset32) == (grub_off_t) -1
> +  if (grub_file_seek (macho->file, macho->offset32) != GRUB_ERR_NONE
>       || grub_file_read (macho->file, &head, sizeof (head))
>       != sizeof(head))
>     {
> @@ -123,7 +123,7 @@ grub_macho32_readfile (grub_macho_t macho, void *dest)
>     return grub_error (GRUB_ERR_BAD_OS,
>                       "Couldn't read architecture-specific part");
>
> -  if (grub_file_seek (macho->file, macho->offset32) == (grub_off_t) -1)
> +  if (grub_file_seek (macho->file, macho->offset32) != GRUB_ERR_NONE)
>     {
>       grub_error_push ();
>       return grub_error (GRUB_ERR_BAD_OS,
> @@ -206,8 +206,8 @@ grub_macho32_load (grub_macho_t macho, char *offset, int flags)
>     if (! hdr->vmsize)
>       return 0;
>
> -    if (grub_file_seek (_macho->file, hdr->fileoff
> -                       + _macho->offset32) == (grub_off_t) -1)
> +    if (grub_file_seek (_macho->file, hdr->fileoff + _macho->offset32) !=
> +       GRUB_ERR_NONE)
>       {
>        grub_error_push ();
>        grub_error (GRUB_ERR_BAD_OS,
> @@ -297,7 +297,7 @@ grub_macho_file (grub_file_t file)
>   macho->cmds32 = 0;
>   macho->cmds64 = 0;
>
> -  if (grub_file_seek (macho->file, 0) == (grub_off_t) -1)
> +  if (grub_file_seek (macho->file, 0) != GRUB_ERR_NONE)
>     goto fail;
>
>   if (grub_file_read (macho->file, &filestart, sizeof (filestart))
> @@ -316,8 +316,8 @@ grub_macho_file (grub_file_t file)
>
>       /* Load architecture description. */
>       narchs = grub_be_to_cpu32 (filestart.fat.nfat_arch);
> -      if (grub_file_seek (macho->file, sizeof (struct grub_macho_fat_header))
> -         == (grub_off_t) -1)
> +      if (grub_file_seek (macho->file, sizeof (struct grub_macho_fat_header)) !=
> +         GRUB_ERR_NONE)
>        goto fail;
>       archs = grub_malloc (sizeof (struct grub_macho_fat_arch) * narchs);
>       if (!archs)
> diff --git a/loader/xnu_resume.c b/loader/xnu_resume.c
> index 77f6887..998e61e 100644
> --- a/loader/xnu_resume.c
> +++ b/loader/xnu_resume.c
> @@ -103,7 +103,7 @@ grub_xnu_resume (char *imagename)
>     }
>
>   /* Read image. */
> -  if (grub_file_seek (file, 0) == (grub_off_t)-1
> +  if (grub_file_seek (file, 0) != GRUB_ERR_NONE
>       || grub_file_read (file, buf, hibhead.image_size)
>       != (grub_ssize_t) hibhead.image_size)
>     {
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH 3/4] Change grub_file_seek() to return grub_err_t

[Pavel Roskin]

On Thu, 2009-07-23 at 10:23 +0200, Vladimir 'phcoder' Serbinenko wrote:
> On Wed, Jul 22, 2009 at 5:16 AM, Pavel Roskin<proski@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