[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: bugfix, hostfs
From: |
Marco Gerards |
Subject: |
Re: bugfix, hostfs |
Date: |
Sun, 08 Aug 2004 20:00:53 +0200 |
User-agent: |
Gnus/5.1006 (Gnus v5.10.6) Emacs/21.3 (gnu/linux) |
address@hidden (Tomas Ebenlendr) writes:
> I'm thinkig about module loading in grub-emu (now I'm exploring dl.c, ld
> and so on) and as side efect I wrote access to host filesystem for grub.
> This also discovered a bug in dl.c which occur when 'normal.mod' exists,
> but cannot be loaded.
Nice! Thanks for your patches.
Why do you need hostfs for this? I think hostfs can be really useful
to me for debugging etc., but is it required for module loading?
Anyway, it would be really useful for me when I am writing filesystem
implementations.
Here is a quick review of your patches. As I do not have much time
now I hope someone else can provide better comments.
Most comments are still about the GCS. If I will commit the patch (I
can do so if Okuji agrees with the patches), I will fix the GCS
related problems as well. Hopefully you will read the GCS comments to
see our coding style.
> mod = grub_dl_load_core (core, size);
> - mod->ref_count = 0;
> + if (mod) mod->ref_count = 0;
AFAIK you should split this up according to the GCS. Like this:
if (mod)
mod->ref_count = 0;
> file conf/i386-pc.mk should be generated before commiting,
> file conf/powerpc-ieee1275.rmk should be patched same way as i386-pc.rmk
Ok.
> +#ifndef GRUB_UTIL
> +#error cannot live outside host fs
> +#endif
I think there is no need to do this.
> + if ((signed) device->disk->id != -2) {
What is -2?
> +static grub_err_t
> +grub_host_dir (grub_device_t device, const char *path,
> + int (*hook) (const char *filename, int dir))
> +{
> + DIR * dir;
> + struct dirent * dent;
Use: DIR *dir;
(no space)
> + struct stat stent;
> + static char pathbuf[/*FIXME*/2048 + NAME_MAX];
> + char * ename=pathbuf;
> +
> + if (host_mount(device)) return grub_errno;
A space between the function name and the "(".
> + grub_strncpy(pathbuf,path,/*FIXME*/2048 - 1);
Why do you use pathbuf? Can't you just use path directly? If that is
not possible use MAX_PATH_LEN here when it is defined and dynamic
memory allocation otherwise (when there is no limit).
> + while (*ename) ename++;/* let ename point after 'path' */
Please write this like this:
/* Let ename point after PATH. */
while (*ename)
ename++;
> + ename[NAME_MAX]=0;
Please add a space before and after the '='.
Thanks,
Marco
- bugfix, hostfs, Tomas Ebenlendr, 2004/08/08
- Re: bugfix, hostfs,
Marco Gerards <=
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/08
- Re: bugfix, hostfs, Marco Gerards, 2004/08/08
- Re: bugfix, hostfs, Yoshinori K. Okuji, 2004/08/08
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/12
- Re: bugfix, hostfs, Marco Gerards, 2004/08/13
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/14
- Re: bugfix, hostfs, Marco Gerards, 2004/08/14
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/14
- Re: bugfix, hostfs, Yoshinori K. Okuji, 2004/08/21