Re: hostfs patch

[Marco Gerards]

Tomas Ebenlendr <address@hidden> writes:

> So here is the promised hosfs patch. If no comments will occur, I
> commit it cca next friday.
> I rewrote hostdisk to fakedisk, so any such special filesystem can
> register a fakedisk with some name. Now it still resides in util/
> directory. If you know better place, please tell me.
> If you will see any formating (indenting) error, please tell me too.

Great!


>       * util/grub-emu.c (main): Added inicialzation and destruction
>       of hostfs and fakedisk.

A typo.

>       * kern/dl.c: Fixed bug. Nil pointer was derefered.

In which function?  You don't have to mention you fixed a bug.  Just
use something like:

* kern/dl.c (foo): Don't dereference null pointer.

> diff -rupN -x CVS -x '*.mk' grub2_savannah/include/grub/disk.h 
> grub2_wodiff/include/grub/disk.h
> --- grub2_savannah/include/grub/disk.h        2004-08-24 23:38:50.000000000 
> +0200
> +++ grub2_wodiff/include/grub/disk.h  2004-08-24 23:51:29.000000000 +0200
> @@ -30,6 +30,7 @@
>  enum grub_disk_dev_id
>    {
>      GRUB_DISK_DEVICE_BIOSDISK_ID,
> +    GRUB_DISK_DEVICE_FAKEDISK_ID,
>      GRUB_DISK_DEVICE_OFDISK_ID,
>    };

I don't see this change in the changelog.  Please put every change you
make in the changelog and please make sure everything you describe in
the changelog entry can be found in that patch.

> diff -rupN -x CVS -x '*.mk' grub2_savannah/kern/dl.c grub2_wodiff/kern/dl.c
> --- grub2_savannah/kern/dl.c  2004-08-12 23:39:00.000000000 +0200
> +++ grub2_wodiff/kern/dl.c    2004-08-12 23:48:50.000000000 +0200
> @@ -548,7 +548,8 @@ grub_dl_load_file (const char *filename)
>      goto failed;
>  
>    mod = grub_dl_load_core (core, size);
> -  mod->ref_count = 0;
> +  if (mod)
> +    mod->ref_count = 0;

Perhaps an error code should be returned as well, no?

> +struct grub_fakedisk_entry {

Can you put the opening brace on the next line?

> +      if (hook (d->name))
> +     return 1;

This looks funny... Is my mailclient or brain screwing things up or is
this a tab?

> +      d = d->next;

This looks like a classic for loop construction:

for (d = fakedisks; d; d = d->next)
  {
...

> +  grub_fakedisk_entry_t d=fakedisks;

d = fakedisks...

> +  while (d && (grub_strcmp (name,d->name) != 0))

name, d ...

> +  /* This shoud be much greater than zero. Some fs claims that disk is
> +   * totaly bad while fs-probe if total_sectors to low.  */

I don't like multiline comments with multiple *'s that
much... Normally it is not used.

> +  disk->total_sectors = 1234;

Why 1234?  Can't it be set to 0?  If I fill in a bogus value, I
usually use -1 because it is so obvious it is bogus...

> +/* FIXME grub consider the fs in utf-8 => use only ascii (0-127) chars.  */

GRUB uses UTF-8 for filesystems.  So in that case everything can be
used (0-255), no?

> +/* Open a file named NAME and initialize FILE.  */
> +/* File->data will contain retyped filedescriptor.  */

Same here about those *'s.

> +  /* Maybe loop to get as most as possible should be here, may be not.  */
> +  rv = read ((int) file->data, buf, len);

I think you should loop when using read because it can return with not
all requested data read.  But I am not a POSIX expert so I can be
wrong here.

> +  char *pathbuf, *p;

Please use multiple lines for multiple declarations:

char *pathbuf;
char *p;

> +  while ((dent = readdir (dir)) != 0)

[...]

> +      lstat (pathbuf, &stent);

Can these functions fail?



Thanks again for this patch.  I am really looking forwards to using
it :).

Thanks,
Marco