Re: bugfix, hostfs

[Marco Gerards]

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