* [PATCH 1/4] Handle errors in rmmod
@ 2009-07-22 3:16 Pavel Roskin
2009-07-22 3:16 ` [PATCH 2/4] Fix insmod not to increase refcount above 1 Pavel Roskin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Pavel Roskin @ 2009-07-22 3:16 UTC (permalink / raw)
To: grub-devel
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/4] Fix insmod not to increase refcount above 1 2009-07-22 3:16 [PATCH 1/4] Handle errors in rmmod Pavel Roskin @ 2009-07-22 3:16 ` Pavel Roskin 2009-07-22 3:16 ` [PATCH 3/4] Change grub_file_seek() to return grub_err_t Pavel Roskin 2009-07-22 3:16 ` [PATCH 4/4] Fix ALIGN_UP cutting upper bits Pavel Roskin 2 siblings, 0 replies; 6+ messages in thread From: Pavel Roskin @ 2009-07-22 3:16 UTC (permalink / raw) To: grub-devel 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] Change grub_file_seek() to return grub_err_t 2009-07-22 3:16 [PATCH 1/4] Handle errors in rmmod Pavel Roskin 2009-07-22 3:16 ` [PATCH 2/4] Fix insmod not to increase refcount above 1 Pavel Roskin @ 2009-07-22 3:16 ` Pavel Roskin 2009-07-23 8:23 ` Vladimir 'phcoder' Serbinenko 2009-07-22 3:16 ` [PATCH 4/4] Fix ALIGN_UP cutting upper bits Pavel Roskin 2 siblings, 1 reply; 6+ messages in thread From: Pavel Roskin @ 2009-07-22 3:16 UTC (permalink / raw) To: grub-devel 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) { ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/4] Change grub_file_seek() to return grub_err_t 2009-07-22 3:16 ` [PATCH 3/4] Change grub_file_seek() to return grub_err_t Pavel Roskin @ 2009-07-23 8:23 ` Vladimir 'phcoder' Serbinenko 2009-07-23 19:14 ` Pavel Roskin 0 siblings, 1 reply; 6+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2009-07-23 8:23 UTC (permalink / raw) To: The development of GRUB 2 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/4] Change grub_file_seek() to return grub_err_t 2009-07-23 8:23 ` Vladimir 'phcoder' Serbinenko @ 2009-07-23 19:14 ` Pavel Roskin 0 siblings, 0 replies; 6+ messages in thread From: Pavel Roskin @ 2009-07-23 19:14 UTC (permalink / raw) To: The development of GRUB 2 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 4/4] Fix ALIGN_UP cutting upper bits 2009-07-22 3:16 [PATCH 1/4] Handle errors in rmmod Pavel Roskin 2009-07-22 3:16 ` [PATCH 2/4] Fix insmod not to increase refcount above 1 Pavel Roskin 2009-07-22 3:16 ` [PATCH 3/4] Change grub_file_seek() to return grub_err_t Pavel Roskin @ 2009-07-22 3:16 ` Pavel Roskin 2 siblings, 0 replies; 6+ messages in thread From: Pavel Roskin @ 2009-07-22 3:16 UTC (permalink / raw) To: grub-devel 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) ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-07-23 19:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-22 3:16 [PATCH 1/4] Handle errors in rmmod Pavel Roskin 2009-07-22 3:16 ` [PATCH 2/4] Fix insmod not to increase refcount above 1 Pavel Roskin 2009-07-22 3:16 ` [PATCH 3/4] Change grub_file_seek() to return grub_err_t Pavel Roskin 2009-07-23 8:23 ` Vladimir 'phcoder' Serbinenko 2009-07-23 19:14 ` Pavel Roskin 2009-07-22 3:16 ` [PATCH 4/4] Fix ALIGN_UP cutting upper bits Pavel Roskin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.