bugfix, hostfs

bugfix, hostfs

[Tomas Ebenlendr]

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.

Here is the bugfix:
##############################################################
Changelog:
2004-08-08  Tomas Ebenlendr  <ebik@ucw.cz>

	* kern/dl.c (grub_dl_load_file): Fixed bug: When
	grub_dl_load_core fails, mod shouldn't be derefered.

diff -rupN -x CVS grub2_x/kern/dl.c grub2_wodiff/kern/dl.c
--- grub2_x/kern/dl.c	2004-06-05 00:20:18.000000000 +0200
+++ grub2_wodiff/kern/dl.c	2004-08-08 19:20:04.000000000 +0200
@@ -548,7 +548,7 @@ 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;
 
  failed:
   grub_file_close (file);
###############################################################


And here is the hostfs patch. It adds device (host) to list of devices.
Directory (host)/ is root directory of the host filesystem.

###############################################################
file conf/i386-pc.mk should be generated before commiting,
file conf/powerpc-ieee1275.rmk should be patched same way as i386-pc.rmk

Changelog:
2004-08-08  Tomas Ebenlendr  <ebik@ucw.cz>

	Added support for accessing files via host filesystem to
	grub-emu.

	* conf/i386-pc.rmk (grub_emu_SOURCES): Added util/hostdisk.c
	and fs/hostfs.c
	* fs/hostfs.c: New file. Connects grub fs interface and libc file
	interface.
	* include/grub/hostdisk.h: New file, prototypes for inicialization
	and destruction of device "host".
	* util/hostdisk.c: New file, implements device "host" (i.e., tells
	that device "host" is disk with filesystem hostfs).
	* util/grub-emu.c (main): Added inicialzation and destruction
	of hostfs and hostdisk.

diff -rupN -x CVS grub2_x/conf/i386-pc.rmk grub2_wodiff/conf/i386-pc.rmk
--- grub2_x/conf/i386-pc.rmk	2004-08-08 14:18:18.000000000 +0200
+++ grub2_wodiff/conf/i386-pc.rmk	2004-08-08 15:41:50.000000000 +0200
@@ -64,7 +64,8 @@ grub_emu_SOURCES = kern/main.c kern/devi
         kern/misc.c kern/loader.c kern/rescue.c kern/term.c		\
 	disk/i386/pc/partition.c kern/env.c commands/ls.c		\
 	commands/terminal.c commands/boot.c commands/cmp.c commands/cat.c		\
-	util/i386/pc/biosdisk.c fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c			\
+	util/i386/pc/biosdisk.c util/hostdisk.c				\
+	fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c fs/hostfs.c		\
 	normal/cmdline.c normal/command.c normal/main.c normal/menu.c normal/arg.c	\
 	util/console.c util/grub-emu.c util/misc.c util/i386/pc/getroot.c
 grub_emu_LDFLAGS = -lncurses
diff -rupN -x CVS grub2_x/fs/hostfs.c grub2_wodiff/fs/hostfs.c
--- grub2_x/fs/hostfs.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2_wodiff/fs/hostfs.c	2004-08-08 16:00:20.000000000 +0200
@@ -0,0 +1,170 @@
+/* hostfs.c - host filesystem for grub-emu */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <grub/err.h>
+#include <grub/file.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/dl.h>
+#include <grub/types.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+#include <dirent.h>
+
+#ifndef GRUB_UTIL
+#error cannot live outside host fs
+#endif
+\f
+
+static grub_err_t
+host_mount(grub_device_t device)
+{
+  if ((signed) device->disk->id != -2) {
+    grub_error(GRUB_ERR_BAD_FS, "not a host filesystem");
+    return grub_errno;
+  }
+  return 0;
+}
+
+static grub_err_t
+errno2err(void)
+{
+  grub_err_t r;
+  switch (errno)
+    {
+      case EMFILE:
+      case ENFILE:
+      case ENOMEM: r = GRUB_ERR_OUT_OF_MEMORY; break;
+      case ENOTDIR:
+      case ENOENT: r = GRUB_ERR_FILE_NOT_FOUND; break;
+      case ELOOP: r =  GRUB_ERR_SYMLINK_LOOP; break;
+      case EIO: r = GRUB_ERR_FILE_READ_ERROR; break;
+      default: r = GRUB_ERR_BAD_ARGUMENT; break;
+    }
+  grub_error(r, strerror(errno));
+  return r;
+}
+
+/* Open a file named NAME and initialize FILE.  */
+static grub_err_t
+grub_host_open (struct grub_file *file, const char *name)
+{
+  struct stat statbuf;
+
+  if (host_mount(file->device)) return grub_errno;
+
+  file->data = (void *) open(name, O_RDONLY);
+  if (file->data == (void *) -1)
+    return errno2err();
+  fstat((int) file->data, &statbuf);
+  file->size = statbuf.st_size;
+  file->offset = 0;
+    
+  return 0;
+}
+
+static grub_err_t
+grub_host_close (grub_file_t file)
+{
+  close ((int) file->data);
+  file->data = (void *) -1;
+  return 0;
+}
+
+/* Read LEN bytes data from FILE into BUF.  */
+static grub_ssize_t
+grub_host_read (grub_file_t file, char *buf, grub_ssize_t len)
+{
+  int rv;
+
+  rv = read((int) file->data, buf, len);
+  if (rv == -1)
+    return errno2err();
+  return rv;
+}
+
+
+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;
+  struct stat stent;
+  static char pathbuf[/*FIXME*/2048 + NAME_MAX];
+  char * ename=pathbuf;
+
+  if (host_mount(device)) return grub_errno;
+  grub_strncpy(pathbuf,path,/*FIXME*/2048 - 1);
+  pathbuf[/*FIXME*/2048 - 1]=0;
+  while (*ename) ename++;/* let ename point after 'path' */
+  ename[NAME_MAX]=0;
+  
+  dir = opendir(pathbuf);
+  if (dir == 0)
+    return errno2err();
+
+  while ((dent = readdir (dir)) != 0)
+    {
+      grub_strncpy(ename, dent->d_name, NAME_MAX);
+      lstat(pathbuf,&stent);
+      hook (dent->d_name, (stent.st_mode & S_IFMT) == S_IFDIR);
+    }
+  return 0;
+}
+
+static grub_err_t
+grub_host_label (grub_device_t device, char **label)
+{
+  if (host_mount(device)) return grub_errno;
+  *label = 0;
+  return 0;
+}
+
+\f
+static struct grub_fs grub_host_fs =
+  {
+    .name = "host",
+    .dir = grub_host_dir,
+    .open = grub_host_open,
+    .read = grub_host_read,
+    .close = grub_host_close,
+    .label = grub_host_label,
+    .next = 0
+  };
+
+void
+grub_hostfs_init (void)
+{
+  grub_fs_register (&grub_host_fs);
+}
+
+void
+grub_hostfs_fini (void)
+{
+  grub_fs_unregister (&grub_host_fs);
+}
+
diff -rupN -x CVS grub2_x/include/grub/fs.h grub2_wodiff/include/grub/fs.h
--- grub2_x/include/grub/fs.h	2004-08-08 14:18:18.000000000 +0200
+++ grub2_wodiff/include/grub/fs.h	2004-08-08 14:27:38.000000000 +0200
@@ -74,6 +74,8 @@ void grub_ufs_init (void);
 void grub_ufs_fini (void);
 void grub_minix_init (void);
 void grub_minix_fini (void);
+void grub_hostfs_init (void);
+void grub_hostfs_fini (void);
 #endif /* GRUB_UTIL */
 
 #endif /* ! GRUB_FS_HEADER */
diff -rupN -x CVS grub2_x/include/grub/hostdisk.h grub2_wodiff/include/grub/hostdisk.h
--- grub2_x/include/grub/hostdisk.h	1970-01-01 01:00:00.000000000 +0100
+++ grub2_wodiff/include/grub/hostdisk.h	2004-08-08 14:27:38.000000000 +0200
@@ -0,0 +1,26 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef GRUB_HOSTDISK_HEADER
+#define GRUB_HOSTDISK_HEADER	1
+
+void grub_util_hostdisk_init (void);
+void grub_util_hostdisk_fini (void);
+
+#endif /* ! GRUB_HOSTDISK_HEADER */
diff -rupN -x CVS grub2_x/util/grub-emu.c grub2_wodiff/util/grub-emu.c
--- grub2_x/util/grub-emu.c	2004-08-08 14:18:18.000000000 +0200
+++ grub2_wodiff/util/grub-emu.c	2004-08-08 14:29:16.000000000 +0200
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2003  Free Software Foundation, Inc.
+ *  Copyright (C) 2003,2004  Free Software Foundation, Inc.
  *
  *  GRUB is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -35,6 +35,8 @@
 #include <grub/util/getroot.h>
 #include <grub/env.h>
 
+#include <grub/hostdisk.h>
+
 #ifdef __NetBSD__
 /* NetBSD uses /boot for its boot block.  */
 # define DEFAULT_DIRECTORY	"/grub"
@@ -154,7 +156,9 @@ main (int argc, char *argv[])
   
   /* XXX: This is a bit unportable.  */
   grub_util_biosdisk_init (args.dev_map);
+  grub_util_hostdisk_init ();
 
+  grub_hostfs_init ();
   /* Initialize the default modules.  */
   grub_fat_init ();
   grub_ext2_init ();
@@ -172,6 +176,7 @@ main (int argc, char *argv[])
   /* Start GRUB!  */
   grub_main ();
 
+  grub_util_hostdisk_fini ();
   grub_util_biosdisk_fini ();
   grub_normal_fini ();
   grub_ufs_fini ();
@@ -182,6 +187,7 @@ main (int argc, char *argv[])
   grub_cmp_fini ();
   grub_cat_fini ();
   grub_terminal_fini ();
+  grub_hostfs_fini ();
 
   return 0;
 }
diff -rupN -x CVS grub2_x/util/hostdisk.c grub2_wodiff/util/hostdisk.c
--- grub2_x/util/hostdisk.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2_wodiff/util/hostdisk.c	2004-08-08 15:59:33.000000000 +0200
@@ -0,0 +1,92 @@
+/* hostdisk.c - device for hostfs */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <grub/disk.h>
+#include <grub/types.h>
+#include <grub/err.h>
+#include <grub/util/misc.h>
+#include <grub/misc.h>
+
+static int
+grub_util_hostdisk_iterate (int (*hook) (const char *name))
+{
+  if (hook("host")) return 1;
+  else return 0;
+}
+
+/*FIXME*/
+static grub_err_t
+grub_util_hostdisk_open (const char *name, grub_disk_t disk)
+{
+  if (grub_strcmp(name,"host") != 0)
+      return grub_error(GRUB_ERR_UNKNOWN_DEVICE, "not a host fake-disk");
+  disk->has_partitions = 0;
+  disk->id = -2;
+  disk->total_sectors = 1234;
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_util_hostdisk_read (grub_disk_t disk __attribute__ ((unused)),
+			 unsigned long sector __attribute__ ((unused)),
+			 unsigned long size __attribute__ ((unused)),
+			 char *buf __attribute__ ((unused)))
+{
+  /* BAD_FS is here for filesystem probe to work */
+  grub_error (GRUB_ERR_BAD_FS, "cannot read from fake-device host");
+  return grub_errno;
+}
+
+
+static grub_err_t
+grub_util_hostdisk_write (grub_disk_t disk __attribute__ ((unused)),
+			  unsigned long sector __attribute__ ((unused)),
+			  unsigned long size __attribute__ ((unused)),
+			  const char *buf __attribute__ ((unused)))
+{
+  grub_error (GRUB_ERR_BAD_FS, "cannot write to fake-device'host");
+  return grub_errno;
+}
+
+
+static struct grub_disk_dev grub_util_hostdisk_dev =
+  {
+    .name = "hostdisk",
+    .iterate = grub_util_hostdisk_iterate,
+    .open = grub_util_hostdisk_open,
+    .close = 0,
+    .read = grub_util_hostdisk_read,
+    .write = grub_util_hostdisk_write,
+    .next = 0
+  };
+
+
+
+void
+grub_util_hostdisk_init (void)
+{
+  grub_disk_dev_register (&grub_util_hostdisk_dev);
+}
+
+void
+grub_util_hostdisk_fini (void)
+{
+  grub_disk_dev_unregister (&grub_util_hostdisk_dev);
+}
###############################################################
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.60328080728

Re: bugfix, hostfs

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (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

Re: bugfix, hostfs

[Tomas Ebenlendr]

> ebik@artax.karlin.mff.cuni.cz (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.

I don't *need* it, it is just comfortable way to access files from
grub-emu. I also don't *need* to load modules in grub-emu, but if it
will be easy and not user-confusing, it will make easier solving some
problems.

> Most comments are still about the GCS.

Hmm, I wrote the patch for myself, and then I forgot to read it
carefully. So I'm sory for inconvenient code.

> > +#ifndef GRUB_UTIL
> > +#error cannot live outside host fs
> > +#endif
> 
> I think there is no need to do this.
> 

Maybe. I'm just used to write in files that shouldn't be ported to other
"parts" of software that they can't be ported.

> > +  if ((signed) device->disk->id != -2) {
> 
> What is -2?
>

Oh, sorry. Just a magic constant. Probably there should be better
identification (e.g. magic device->disk->data (e.g. device->disk ==
device->disk->data)). This identification is used in hostfs_mount()

> 
> > +  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).
> 

I concatenate path of directory and its entries to stat them. I will use
the MAX_PATH_LEN. I only forgot that I forgot the name of this constant.
(Uh).


Thanks for comments. I will read the patch more carefuly and I will try
to make the code 'nice'.
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.60337551862

Re: bugfix, hostfs

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> I don't *need* it, it is just comfortable way to access files from
> grub-emu. I also don't *need* to load modules in grub-emu, but if it
> will be easy and not user-confusing, it will make easier solving some
> problems.

Ok, I just asked out of interest.

>> Most comments are still about the GCS.
>
> Hmm, I wrote the patch for myself, and then I forgot to read it
> carefully. So I'm sory for inconvenient code.

np.  I am happy you took the time to do this work. :)

>> > +#ifndef GRUB_UTIL
>> > +#error cannot live outside host fs
>> > +#endif
>> 
>> I think there is no need to do this.
>> 
>
> Maybe. I'm just used to write in files that shouldn't be ported to other
> "parts" of software that they can't be ported.

If someone would compile this code for usage outside of grub-emu it
won't compile anyway. :)

>> > +  if ((signed) device->disk->id != -2) {
>> 
>> What is -2?
>>
>
> Oh, sorry. Just a magic constant. Probably there should be better
> identification (e.g. magic device->disk->data (e.g. device->disk ==
> device->disk->data)). This identification is used in hostfs_mount()

Ok.  If you can't do it that way it is better to use a macro like
GRUB_HOSTFS_DISK_ID or so.

>> 
>> > +  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).
>> 
>
> I concatenate path of directory and its entries to stat them. I will use
> the MAX_PATH_LEN. I only forgot that I forgot the name of this constant.
> (Uh).

Oh, I thought it wasn't written to.  I must have missed something
then.  Please be careful with MAX_PATH_LEN, GNU/Hurd does not define
it because there is no limit at all.  In that case better use dynamic
allocation.

Thanks,
Marco

Re: bugfix, hostfs

[Yoshinori K. Okuji]

On Sunday 08 August 2004 20:17, Tomas Ebenlendr wrote:
> > > +#ifndef GRUB_UTIL
> > > +#error cannot live outside host fs
> > > +#endif
> >
> > I think there is no need to do this.
>
> Maybe. I'm just used to write in files that shouldn't be ported to
> other "parts" of software that they can't be ported.

It is a good idea. But the indentation is not appropriate. It should be:

#ifndef GRUB_UTIL
# error cannot live outside host fs
#endif

> > > +  if ((signed) device->disk->id != -2) {
> >
> > What is -2?
>
> Oh, sorry. Just a magic constant. Probably there should be better
> identification (e.g. magic device->disk->data (e.g. device->disk ==
> device->disk->data)). This identification is used in hostfs_mount()

Probably the id is a problem on Open Firmware potentially, since Marco 
used phandlers for disk ids and phandlers can be anything in theory.

I bet that I made a mistake in the design of struct grub_disk. I thought 
it would be easy to find an identifier because the size of an id is 
4-byte. But this assumption breaks on Open Firmware. So perhaps we 
should add one more id for the type of a disk, so that we can write 
this kind of code:

(include/grub/disk.h)
enum grub_disk_class_id
  {
    GRUB_DISK_CLASS_IEEE1275_ID,
    GRUB_DISK_CLASS_PCBIOS_ID,
    GRUB_DISK_CLASS_HOST_ID,
  };

(disk/powerpc/ieee1275/ofdisk.c)
disk->class_id = GRUB_DISK_CLASS_IEEE1275_ID;
disk->id = phandler;

Then you can use this:

disk->class_id = GRUB_DISK_CLASS_HOST_ID;
disk->id = 0;

This requires some changes in the cache manager, but not difficult. What 
do you think?

> > > +  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).
>
> I concatenate path of directory and its entries to stat them. I will
> use the MAX_PATH_LEN. I only forgot that I forgot the name of this
> constant. (Uh).

You should not use MAX_PATH_LEN if not defined, otherwise you code won't 
work well on GNU/Hurd. The right way is to allocate memory dynamically 
for PATHBUF:

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;
  struct stat stent;
  char *pathbuf;
  unsigned dir_len;
  unsigned entry_max_len;
  char *ename;

  if (host_mount(device)) return grub_errno;
  
  dir = opendir(path);
  if (dir == 0)
    return errno2err();

  dir_len = grub_strlen (path)
  entry_max_len = 32; /* Sifficient as the initial value.  */
  pathbuf = grub_malloc (dir_len + entry_max_len + 1);
  if (! pathbuf)
    goto fail;
  grub_strcpy(pathbuf, path);
  if (pathbuf[dir_len - 1] != '/')
    {
      pathbuf[dir_len - 1] = '/';
      pathbuf[dir_len] = '\0';
      dir_len++;
      entry_max_len--;
    }
  ename = pathbuf + dir_len;

  while ((dent = readdir (dir)) != 0)
    {
      if (grub_strlen (dent->d_name) > entry_max_len)
        {
          entry_max_len = grub_strlen (dent->d_name);
          pathbuf = grub_realloc (pathbuf, dir_len + entry_max_len + 1);
          if (! pathbuf)
            goto fail;
        }
      grub_strcpy(ename, dent->d_name);
      lstat(pathbuf, &stent);
      hook (dent->d_name, (stent.st_mode & S_IFMT) == S_IFDIR);
    }

 fail:
  grub_free (pathbuf);
  closedir (dir);
  return grub_errno;
}

BTW, do you have an account on Savannah? If you have, let me know. I can 
give you a write permission for the CVS.

Regards,
Okuji

Re: bugfix, hostfs

[Tomas Ebenlendr]

At the end of the mail are new versions of patches. Hope, there are few
(or no) GCS-errors.


> > > > +  if ((signed) device->disk->id != -2) {
> > >
> > > What is -2?
> >
> > Oh, sorry. Just a magic constant. Probably there should be better
> > identification (e.g. magic device->disk->data (e.g. device->disk ==
> > device->disk->data)). This identification is used in hostfs_mount()
> 
> Probably the id is a problem on Open Firmware potentially, since Marco 
> used phandlers for disk ids and phandlers can be anything in theory.
> 
> I bet that I made a mistake in the design of struct grub_disk. I thought 
> it would be easy to find an identifier because the size of an id is 
> 4-byte. But this assumption breaks on Open Firmware. So perhaps we 
> should add one more id for the type of a disk, so that we can write 
> this kind of code:
> 
> (include/grub/disk.h)
> enum grub_disk_class_id
>   {
>     GRUB_DISK_CLASS_IEEE1275_ID,
>     GRUB_DISK_CLASS_PCBIOS_ID,
>     GRUB_DISK_CLASS_HOST_ID,
>   };
> 
> (disk/powerpc/ieee1275/ofdisk.c)
> disk->class_id = GRUB_DISK_CLASS_IEEE1275_ID;
> disk->id = phandler;
> 
> Then you can use this:
> 
> disk->class_id = GRUB_DISK_CLASS_HOST_ID;
> disk->id = 0;
> 
> This requires some changes in the cache manager, but not difficult. What 
> do you think?

Hmm, leaving this change for anybody who understand disks in grub.
Instead of it I use disk->name :-) .

> > > > +  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).
> >
> > I concatenate path of directory and its entries to stat them. I will
> > use the MAX_PATH_LEN. I only forgot that I forgot the name of this
> > constant. (Uh).
> 
> You should not use MAX_PATH_LEN if not defined, otherwise you code won't 
> work well on GNU/Hurd. The right way is to allocate memory dynamically 
> for PATHBUF:

I don't use MAX_PATH_LEN even if defined now: Less #ifdef means less
bugs.


I will commit both following patches if you agree. I tested both on
i386,pc,linux-2.4.

PS.: I'm changing conf/powerpc-ieee1275.rmk and I didn't test
  compilability of my code on ppc. But it should be OS-dependent, but
  not architecture dependent.

##########################################################################
Changelog:
2004-08-08  Tomas Ebenlendr  <ebik@ucw.cz>

	* kern/dl.c (grub_dl_load_file): Fixed bug: When
	grub_dl_load_core fails, mod shouldn't be derefered.

Index: kern/dl.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/dl.c,v
retrieving revision 1.7
diff -p -u -r1.7 dl.c
--- kern/dl.c	4 Apr 2004 13:46:01 -0000	1.7
+++ kern/dl.c	12 Aug 2004 21:37:53 -0000
@@ -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;
 
  failed:
   grub_file_close (file);
##########################################################################

Changelog:
2004-08-08  Tomas Ebenlendr  <ebik@ucw.cz>

	Added support for accessing files via host filesystem to
	grub-emu.

	* conf/i386-pc.rmk (grub_emu_SOURCES): Added util/hostdisk.c
	and fs/hostfs.c
	* fs/hostfs.c: New file. Connects grub fs interface and libc file
	interface.
	* include/grub/fakedisk.h: New file, prototypes for inicialization
	and destruction of fakedisk device.
	* util/fakedisk.c: New file, implements device "host" (i.e., tells
	that device "host" is fake-disk with filesystem hostfs).
	* util/grub-emu.c (main): Added inicialzation and destruction
	of hostfs and fakedisk.


diff -rupN -x CVS grub2_x/conf/i386-pc.rmk grub2_wodiff/conf/i386-pc.rmk
--- grub2_x/conf/i386-pc.rmk	2004-08-08 19:32:20.000000000 +0200
+++ grub2_wodiff/conf/i386-pc.rmk	2004-08-10 14:13:32.000000000 +0200
@@ -69,7 +69,8 @@ grub_emu_SOURCES = kern/main.c kern/devi
         kern/misc.c kern/loader.c kern/rescue.c kern/term.c		\
 	disk/i386/pc/partition.c kern/env.c commands/ls.c		\
 	commands/terminal.c commands/boot.c commands/cmp.c commands/cat.c		\
-	util/i386/pc/biosdisk.c fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c			\
+	util/i386/pc/biosdisk.c util/fakedisk.c				\
+	fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c fs/hostfs.c		\
 	normal/cmdline.c normal/command.c normal/main.c normal/menu.c normal/arg.c	\
 	util/console.c util/grub-emu.c util/misc.c util/i386/pc/getroot.c
 grub_emu_LDFLAGS = -lncurses
diff -rupN -x CVS grub2_x/conf/powerpc-ieee1275.rmk grub2_wodiff/conf/powerpc-ieee1275.rmk
--- grub2_x/conf/powerpc-ieee1275.rmk	2004-08-08 19:32:20.000000000 +0200
+++ grub2_wodiff/conf/powerpc-ieee1275.rmk	2004-08-12 23:59:07.000000000 +0200
@@ -25,7 +25,8 @@ grub_emu_SOURCES = kern/main.c kern/devi
 	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c		\
         kern/misc.c kern/loader.c kern/rescue.c kern/term.c		\
 	disk/powerpc/ieee1275/partition.c 					\
-	util/i386/pc/biosdisk.c fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c	\
+	util/i386/pc/biosdisk.c util/fakedisk.c				\
+	fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c fs/hostfs.c		\
 	normal/cmdline.c normal/command.c normal/main.c normal/menu.c	\
 	normal/arg.c	\
 	util/console.c util/grub-emu.c util/misc.c util/i386/pc/getroot.c \
diff -rupN -x CVS grub2_x/fs/hostfs.c grub2_wodiff/fs/hostfs.c
--- grub2_x/fs/hostfs.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2_wodiff/fs/hostfs.c	2004-08-13 00:06:36.000000000 +0200
@@ -0,0 +1,203 @@
+/* hostfs.c - host filesystem for grub-emu */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <grub/err.h>
+#include <grub/file.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+#include <grub/disk.h>
+#include <grub/dl.h>
+#include <grub/types.h>
+#include <grub/fakedisk.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <fcntl.h>
+#include <dirent.h>
+
+#ifndef GRUB_UTIL
+# error Cannot live without underlying OS.
+#endif
+\f
+
+/* FIXME grub consider the fs in utf-8 => use only ascii (0-127) chars.  */
+
+static grub_err_t
+host_mount (grub_device_t device)
+{
+  if (! GRUB_IS_HOST_FAKEDISK(device->disk))
+    return grub_error (GRUB_ERR_BAD_FS, "not a host filesystem");
+  return 0;
+}
+
+/* Maybe not a exact mapping, but at least sufficient.  */
+static grub_err_t
+errno2err (void)
+{
+  grub_err_t r;
+  switch (errno)
+    {
+      case EMFILE:
+      case ENFILE:
+      case ENOMEM: r = GRUB_ERR_OUT_OF_MEMORY; break;
+      case ENOTDIR:
+      case ENOENT: r = GRUB_ERR_FILE_NOT_FOUND; break;
+      case ELOOP: r = GRUB_ERR_SYMLINK_LOOP; break;
+      case EIO: r = GRUB_ERR_FILE_READ_ERROR; break;
+      default: r = GRUB_ERR_BAD_ARGUMENT; break;
+    }
+  return grub_error (r, strerror (errno));
+}
+
+/* Open a file named NAME and initialize FILE.  */
+/* File->data will contain retyped filedescriptor.  */
+static grub_err_t
+grub_host_open (struct grub_file *file, const char *name)
+{
+  struct stat statbuf;
+
+  if (host_mount (file->device))
+    return grub_errno;
+
+  file->data = (void *) open(name, O_RDONLY);
+  if (file->data == (void *) -1)
+    return errno2err();
+  fstat((int) file->data, &statbuf);
+  file->size = statbuf.st_size;
+  file->offset = 0;
+    
+  return 0;
+}
+
+static grub_err_t
+grub_host_close (grub_file_t file)
+{
+  close ((int) file->data);
+  file->data = (void *) -1;
+  return 0;
+}
+
+/* Read LEN bytes data from FILE into BUF.  */
+static grub_ssize_t
+grub_host_read (grub_file_t file, char *buf, grub_ssize_t len)
+{
+  int rv;
+
+  /* Maybe loop to get as most as possible should be here, may be not.  */
+  rv = read ((int) file->data, buf, len);
+  if (rv == -1)
+    return errno2err();
+  return rv;
+}
+
+
+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;
+  struct stat stent;
+  char *pathbuf, *p;
+  unsigned pathlen;
+  unsigned maxname = 32;
+
+  if (host_mount (device))
+    return grub_errno;
+
+  pathlen = grub_strlen (path) + 1;
+  pathbuf = grub_malloc (pathlen + maxname);
+  if (pathbuf == 0)
+    goto mem_fail;
+  
+  grub_strcpy (pathbuf, path);
+  pathbuf [pathlen - 1] = '/';
+  pathbuf [pathlen] = 0;
+  
+  dir = opendir (pathbuf);
+  if (dir == 0)
+    goto errno_fail;
+
+  while ((dent = readdir (dir)) != 0)
+    {
+      if (grub_strlen (dent->d_name) >= maxname)
+	{
+	  maxname = grub_strlen (dent->d_name);
+	  p = grub_realloc (pathbuf, pathlen + maxname);
+	  if (p == 0)
+	    goto mem_fail;
+	  pathbuf = p;
+	}
+      grub_strcpy (pathbuf + pathlen, dent->d_name);
+      lstat (pathbuf, &stent);
+      hook (dent->d_name, (stent.st_mode & S_IFMT) == S_IFDIR);
+    }
+
+  grub_free (pathbuf);
+  return 0;
+
+mem_fail:
+  grub_error (GRUB_ERR_OUT_OF_MEMORY, "Not enough memory.");
+  goto fail;
+
+errno_fail:
+  errno2err ();
+
+fail:
+  grub_free (pathbuf);
+  return grub_errno;
+}
+
+static grub_err_t
+grub_host_label (grub_device_t device, char **label)
+{
+  if (host_mount (device))
+    return grub_errno;
+  *label = 0;
+  return 0;
+}
+
+\f
+static struct grub_fs grub_host_fs =
+  {
+    .name = "host",
+    .dir = grub_host_dir,
+    .open = grub_host_open,
+    .read = grub_host_read,
+    .close = grub_host_close,
+    .label = grub_host_label,
+    .next = 0
+  };
+
+void
+grub_hostfs_init (void)
+{
+  grub_fs_register (&grub_host_fs);
+}
+
+void
+grub_hostfs_fini (void)
+{
+  grub_fs_unregister (&grub_host_fs);
+}
+
diff -rupN -x CVS grub2_x/include/grub/fakedisk.h grub2_wodiff/include/grub/fakedisk.h
--- grub2_x/include/grub/fakedisk.h	1970-01-01 01:00:00.000000000 +0100
+++ grub2_wodiff/include/grub/fakedisk.h	2004-08-13 00:08:40.000000000 +0200
@@ -0,0 +1,32 @@
+/* fakedisk.h - fake disk device */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef GRUB_FAKEDISK_HEADER
+#define GRUB_FAKEDISK_HEADER	1
+
+void grub_util_fakedisk_init (void);
+void grub_util_fakedisk_fini (void);
+
+#define GRUB_FAKEDISK_HOST_STRING "host"
+
+/* For hostfs to detect, that this is hostdisk.  */
+#define GRUB_IS_HOST_FAKEDISK(x) (grub_strcmp((x)->name, GRUB_FAKEDISK_HOST_STRING) == 0)
+
+#endif /* ! GRUB_FAKEDISK_HEADER */
diff -rupN -x CVS grub2_x/include/grub/fs.h grub2_wodiff/include/grub/fs.h
--- grub2_x/include/grub/fs.h	2004-08-08 14:18:18.000000000 +0200
+++ grub2_wodiff/include/grub/fs.h	2004-08-08 19:33:28.000000000 +0200
@@ -74,6 +74,8 @@ void grub_ufs_init (void);
 void grub_ufs_fini (void);
 void grub_minix_init (void);
 void grub_minix_fini (void);
+void grub_hostfs_init (void);
+void grub_hostfs_fini (void);
 #endif /* GRUB_UTIL */
 
 #endif /* ! GRUB_FS_HEADER */
diff -rupN -x CVS grub2_x/util/fakedisk.c grub2_wodiff/util/fakedisk.c
--- grub2_x/util/fakedisk.c	1970-01-01 01:00:00.000000000 +0100
+++ grub2_wodiff/util/fakedisk.c	2004-08-13 00:08:05.000000000 +0200
@@ -0,0 +1,96 @@
+/* fakedisk.c - fake disk device */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2004  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <grub/disk.h>
+#include <grub/types.h>
+#include <grub/err.h>
+#include <grub/util/misc.h>
+#include <grub/misc.h>
+
+#include <grub/fakedisk.h>
+
+static int
+grub_util_fakedisk_iterate (int (*hook) (const char *name))
+{
+  if (hook (GRUB_FAKEDISK_HOST_STRING))
+    return 1;
+  else
+    return 0;
+}
+
+static grub_err_t
+grub_util_fakedisk_open (const char *name, grub_disk_t disk)
+{
+  if (grub_strcmp (name,GRUB_FAKEDISK_HOST_STRING) != 0)
+    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "unknown disk %s", name);
+  disk->has_partitions = 0;
+  /* Id is irrelevant.  */
+  disk->id = -1;
+  /* This shoud be much greater than zero. Some fs claims that disk is
+   * totaly bad while fs-probe if total_sectors to low.  */
+  disk->total_sectors = 1234;
+  return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+grub_util_fakedisk_read (grub_disk_t disk __attribute__ ((unused)),
+			 unsigned long sector __attribute__ ((unused)),
+			 unsigned long size __attribute__ ((unused)),
+			 char *buf __attribute__ ((unused)))
+{
+  /* BAD_FS is here for filesystem probe to work.  */
+  grub_error (GRUB_ERR_BAD_FS, "cannot read from fakedisk %s", disk->name);
+  return grub_errno;
+}
+
+
+static grub_err_t
+grub_util_fakedisk_write (grub_disk_t disk __attribute__ ((unused)),
+			  unsigned long sector __attribute__ ((unused)),
+			  unsigned long size __attribute__ ((unused)),
+			  const char *buf __attribute__ ((unused)))
+{
+  grub_error (GRUB_ERR_BAD_FS, "cannot write to fakedisk %s", disk->name);
+  return grub_errno;
+}
+
+
+static struct grub_disk_dev grub_util_fakedisk_dev =
+  {
+    .name = "fakedisk",
+    .iterate = grub_util_fakedisk_iterate,
+    .open = grub_util_fakedisk_open,
+    .close = 0,
+    .read = grub_util_fakedisk_read,
+    .write = grub_util_fakedisk_write,
+    .next = 0
+  };
+
+void
+grub_util_fakedisk_init (void)
+{
+  grub_disk_dev_register (&grub_util_fakedisk_dev);
+}
+
+void
+grub_util_fakedisk_fini (void)
+{
+  grub_disk_dev_unregister (&grub_util_fakedisk_dev);
+}
diff -rupN -x CVS grub2_x/util/grub-emu.c grub2_wodiff/util/grub-emu.c
--- grub2_x/util/grub-emu.c	2004-08-08 14:18:18.000000000 +0200
+++ grub2_wodiff/util/grub-emu.c	2004-08-10 14:12:15.000000000 +0200
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2003  Free Software Foundation, Inc.
+ *  Copyright (C) 2003,2004  Free Software Foundation, Inc.
  *
  *  GRUB is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -35,6 +35,8 @@
 #include <grub/util/getroot.h>
 #include <grub/env.h>
 
+#include <grub/fakedisk.h>
+
 #ifdef __NetBSD__
 /* NetBSD uses /boot for its boot block.  */
 # define DEFAULT_DIRECTORY	"/grub"
@@ -154,7 +156,9 @@ main (int argc, char *argv[])
   
   /* XXX: This is a bit unportable.  */
   grub_util_biosdisk_init (args.dev_map);
+  grub_util_fakedisk_init ();
 
+  grub_hostfs_init ();
   /* Initialize the default modules.  */
   grub_fat_init ();
   grub_ext2_init ();
@@ -172,6 +176,7 @@ main (int argc, char *argv[])
   /* Start GRUB!  */
   grub_main ();
 
+  grub_util_fakedisk_fini ();
   grub_util_biosdisk_fini ();
   grub_normal_fini ();
   grub_ufs_fini ();
@@ -182,6 +187,7 @@ main (int argc, char *argv[])
   grub_cmp_fini ();
   grub_cat_fini ();
   grub_terminal_fini ();
+  grub_hostfs_fini ();
 
   return 0;
 }
##########################################################################

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.61477452692

Re: bugfix, hostfs

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

>> This requires some changes in the cache manager, but not difficult. What 
>> do you think?
>
> Hmm, leaving this change for anybody who understand disks in grub.
> Instead of it I use disk->name :-) .

Better use an int.  It is easier/cheaper to check and IMHO cleaner.

> PS.: I'm changing conf/powerpc-ieee1275.rmk and I didn't test
>   compilability of my code on ppc. But it should be OS-dependent, but
>   not architecture dependent.

Don't worry about that.  This only changes stuff in util/, so no bad
things will happen.  BTW, I see you have added hostfs.c to fs/, I
think util/ is a better place for this.

> ##########################################################################
> Changelog:
> 2004-08-08  Tomas Ebenlendr  <ebik@ucw.cz>
>
> 	* kern/dl.c (grub_dl_load_file): Fixed bug: When
> 	grub_dl_load_core fails, mod shouldn't be derefered.

Just leave "Fixed bug: " away.  If Okuji says it is ok to commit,
please change the date to the commit date.

> +      case ENOMEM: r = GRUB_ERR_OUT_OF_MEMORY; break;
> +      case ENOTDIR:
> +      case ENOENT: r = GRUB_ERR_FILE_NOT_FOUND; break;
> +      case ELOOP: r = GRUB_ERR_SYMLINK_LOOP; break;
> +      case EIO: r = GRUB_ERR_FILE_READ_ERROR; break;
> +      default: r = GRUB_ERR_BAD_ARGUMENT; break;

I think I missed this last time.  Better use multiple lines for a case
statement.

> +  file->data = (void *) open(name, O_RDONLY);

open (

> +    return errno2err();

You missed a space here as well.

> +    return errno2err();

...

Thanks,
Marco

Re: bugfix, hostfs

[Tomas Ebenlendr]

> >> This requires some changes in the cache manager, but not difficult. What 
> >> do you think?
> >
> > Hmm, leaving this change for anybody who understand disks in grub.
> > Instead of it I use disk->name :-) .
> 
> Better use an int.  It is easier/cheaper to check and IMHO cleaner.
> 

Hmm, I check it via macro GRUB_FAKEDISK_IS_HOST(disk), and I want to
know it is 'host' fakedisk, because then it is not problem to add other
fakedisks. The best way is probably Okuji's proposal, but I want it as
single patch. (I'll probably do it next week.)

As worse problem I see utf8 from grub side and 8-bit encoding from libc side
(pathnames). This can be solved with some translation, but I'm not
decided how to tell grub the encoding.
One way is to have device.map entry style:
host	iso8859-2
This will also specify that 'host' device will be accessible only if
defined in device.map.

> > +      case ENOMEM: r = GRUB_ERR_OUT_OF_MEMORY; break;
> > +      case ENOTDIR:
> > +      case ENOENT: r = GRUB_ERR_FILE_NOT_FOUND; break;
> > +      case ELOOP: r = GRUB_ERR_SYMLINK_LOOP; break;
> > +      case EIO: r = GRUB_ERR_FILE_READ_ERROR; break;
> > +      default: r = GRUB_ERR_BAD_ARGUMENT; break;
> 
> I think I missed this last time.  Better use multiple lines for a case
> statement.
>

Thanks. That is relict from '"nice" coding standard' which we used for our
school project. ("nice" coding standard says: do it the "nicer" way.). I
will fix all GCS errors that you found.

Btw.: does anybody here use vim? Is there GCS-syntax file for vim?
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.61865285367

Re: bugfix, hostfs

[Marco Gerards]

ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:

> As worse problem I see utf8 from grub side and 8-bit encoding from libc side
> (pathnames). This can be solved with some translation, but I'm not
> decided how to tell grub the encoding.
> One way is to have device.map entry style:
> host	iso8859-2

And you will use iconv for conversion?

> This will also specify that 'host' device will be accessible only if
> defined in device.map.

Is that really required?

> Thanks. That is relict from '"nice" coding standard' which we used for our
> school project. ("nice" coding standard says: do it the "nicer" way.). I
> will fix all GCS errors that you found.

Same problem here.  Switching coding styles all the time sucks. :-/

> Btw.: does anybody here use vim? Is there GCS-syntax file for vim?

I do not know about vim, but how about "indent"?

--
Marco

Re: bugfix, hostfs

[Tomas Ebenlendr]

> ebik@artax.karlin.mff.cuni.cz (Tomas Ebenlendr) writes:
> 
> > As worse problem I see utf8 from grub side and 8-bit encoding from libc side
> > (pathnames). This can be solved with some translation, but I'm not
> > decided how to tell grub the encoding.
> > One way is to have device.map entry style:
> > host	iso8859-2
> 
> And you will use iconv for conversion?

Probably. I'm not decided yet, because if there will be some conversion
tool in grub itself, (for 8-bit encoding filesystems) it should be used
rather than external library. I want to commit hostfs without
internationalisation and solve this 'same way' as in other filesystems.

> > This will also specify that 'host' device will be accessible only if
> > defined in device.map.
> 
> Is that really required?
> 

No. It was just an idea.

I fixed all that you commented in previous mail but the disk detection
(mounting). The disk detection is in fakedisk.h and can be easily
changed.

-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.61889347425

Re: bugfix, hostfs

[Yoshinori K. Okuji]

I've added "device id". The id is defined in disk.h, and used to assign 
the id of a disk device. Normally, you can look up a device id in this 
way:

disk->dev->id

Tomas, can you update your patch for hostfs according to my change?

Okuji