iso9660 support

iso9660 support

[Marco Gerards]

Hi,

Here is a patch to add iso9660 support to GRUB 2.  There is support
for rockridge (including stuff like symlinks), but there is no joliet
support.  I've also fixed a few filesystems so they do not fail when
the block read is out of range.

I hope someone can have a close look at iso9660.c, there could be some
ugly things, but I stared too long at the code to see them. :)

If no one replies before Tuesday I will assume it is ok and check it
in.

Next on my list is hacking xfs and reiser[34] support.  But I think I
will fix the VGA driver first and perhaps add splashscreen support
while I am doing that.

Thanks,
Marco


2004-10-15  Marco Gerards  <metgerards@student.han.nl>

	Add support for iso9660 (including rockrigde).
	
	* conf/i386-pc.rmk (grub_emu_SOURCES): Add fs/iso9660.c.
	(iso9660_mod_SOURCES): New variable.
	(iso9660_mod_CFLAGS): Likewise.
	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Add fs/iso9660.c.
	* include/grub/fs.h (grub_iso9660_init): New prototype.
	* util/grub-emu.c (main): Call `grub_iso9660_init'.
	* fs/iso9660.c: New file.

	* include/grub/misc.h (grub_strncat): New prototype.
	* kern/misc.c (grub_strncat): New function.
	
	* fs/hfs.c (grub_hfs_mount): Translate the error
	`GRUB_ERR_OUT_OF_RANGE' to `GRUB_ERR_BAD_FS'.
	* fs/jfs.c (grub_jfs_mount): Likewise.
	* fs/ufs.c (grub_ufs_mount): Likewise.



Index: conf/i386-pc.rmk
===================================================================
RCS file: /cvsroot/grub/grub2/conf/i386-pc.rmk,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 i386-pc.rmk
--- conf/i386-pc.rmk	17 Sep 2004 09:36:52 -0000	1.21
+++ conf/i386-pc.rmk	14 Oct 2004 22:13:12 -0000
@@ -69,7 +69,7 @@ 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 fs/hfs.c fs/jfs.c\
+	util/i386/pc/biosdisk.c fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c fs/hfs.c fs/jfs.c fs/iso9660.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
@@ -118,6 +118,10 @@ hfs_mod_CFLAGS = $(COMMON_CFLAGS)
 jfs_mod_SOURCES = fs/jfs.c
 jfs_mod_CFLAGS = $(COMMON_CFLAGS)
 
+# For iso9660.mod.
+iso9660_mod_SOURCES = fs/iso9660.c
+iso9660_mod_CFLAGS = $(COMMON_CFLAGS)
+
 # For _linux.mod.
 _linux_mod_SOURCES = loader/i386/pc/linux.c
 _linux_mod_CFLAGS = $(COMMON_CFLAGS)
Index: conf/powerpc-ieee1275.rmk
===================================================================
RCS file: /cvsroot/grub/grub2/conf/powerpc-ieee1275.rmk,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 powerpc-ieee1275.rmk
--- conf/powerpc-ieee1275.rmk	22 Sep 2004 18:26:16 -0000	1.10
+++ conf/powerpc-ieee1275.rmk	14 Oct 2004 22:13:12 -0000
@@ -26,7 +26,8 @@ grub_emu_SOURCES = kern/main.c kern/devi
         kern/misc.c kern/loader.c kern/rescue.c kern/term.c		\
 	disk/powerpc/ieee1275/partition.c fs/fshelp.c			\
 	util/i386/pc/biosdisk.c fs/fat.c fs/ext2.c fs/ufs.c fs/minix.c fs/hfs.c	\
-	fs/jfs.c normal/cmdline.c normal/command.c normal/main.c normal/menu.c	\
+	fs/jfs.c fs/iso9660.c kern/fshelp.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 \
 	kern/env.c commands/ls.c		\
Index: fs/hfs.c
===================================================================
RCS file: /cvsroot/grub/grub2/fs/hfs.c,v
retrieving revision 1.2
diff -u -p -u -p -r1.2 hfs.c
--- fs/hfs.c	24 Aug 2004 20:32:47 -0000	1.2
+++ fs/hfs.c	14 Oct 2004 22:13:12 -0000
@@ -391,14 +391,14 @@ grub_hfs_mount (grub_disk_t disk)
 
   data->rootdir = grub_be_to_cpu32 (dir.dirid);
   
- fail:
-  if (grub_errno)
-    {
-      grub_free (data);
-      data = 0;
-    }
-
   return data;
+ fail:
+  grub_free (data);
+  
+  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
+    grub_error (GRUB_ERR_BAD_FS, "not a hfs filesystem");
+  
+  return 0;
 }
 
 
Index: fs/iso9660.c
===================================================================
RCS file: fs/iso9660.c
diff -N fs/iso9660.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ fs/iso9660.c	14 Oct 2004 22:13:12 -0000
@@ -0,0 +1,841 @@
+/* iso9660.c - iso9660 implementation with extensions:
+   SUSP, Rock Ridge.  */
+/*
+ *  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/fshelp.h>
+
+#define GRUB_ISO9660_FSTYPE_DIR		0040000
+#define GRUB_ISO9660_FSTYPE_REG		0100000
+#define GRUB_ISO9660_FSTYPE_SYMLINK	0120000
+#define GRUB_ISO9660_FSTYPE_MASK	0170000
+
+#define GRUB_ISO9660_LOG2_BLKSZ		2
+#define GRUB_ISO9660_BLKSZ		2048
+
+#define GRUB_ISO9660_RR_DOT		2
+#define GRUB_ISO9660_RR_DOTDOT		4
+
+/* The head of a volume descriptor.  */
+struct grub_iso9660_voldesc
+{
+  grub_uint8_t type;
+  grub_uint8_t magic[5];
+  grub_uint8_t version;
+} __attribute__ ((packed));
+
+/* A directory entry.  */
+struct grub_iso9660_dir
+{
+  grub_uint8_t len;
+  grub_uint8_t ext_sectors;
+  grub_uint32_t first_sector;
+  grub_uint32_t first_sector_be;
+  grub_uint32_t size;
+  grub_uint32_t size_be;
+  grub_uint8_t unused1[7];
+  grub_uint8_t flags;
+  grub_uint8_t unused2[6];
+  grub_uint8_t namelen;
+} __attribute__ ((packed));
+
+/* The primary volume descriptor.  Only little endian is used.  */
+struct grub_iso9660_primary_voldesc
+{
+  struct grub_iso9660_voldesc voldesc;
+  grub_uint8_t unused1[33];
+  grub_uint8_t volname[32];
+  grub_uint8_t unused2[60];
+  grub_uint32_t path_table_size;
+  grub_uint8_t unused3[4];
+  grub_uint32_t path_table;
+  grub_uint8_t unused4[12];
+  struct grub_iso9660_dir rootdir;
+} __attribute__ ((packed));
+
+/* A single entry in the path table.  */
+struct grub_iso9660_path
+{
+  grub_uint8_t len;
+  grub_uint8_t sectors;
+  grub_uint32_t first_sector;
+  grub_uint16_t parentdir;
+  grub_uint8_t name[0];
+} __attribute__ ((packed));
+
+/* An entry in the System Usage area of the directory entry.  */
+struct grub_iso9660_susp_entry
+{
+  grub_uint8_t sig[2];
+  grub_uint8_t len;
+  grub_uint8_t version;
+  grub_uint8_t data[0];
+} __attribute__ ((packed));
+
+/* The CE entry.  This is used to describe the next block where data
+   can be found.  */
+struct grub_iso9660_susp_ce
+{
+  struct grub_iso9660_susp_entry entry;
+  grub_uint32_t blk;
+  grub_uint32_t blk_be;
+  grub_uint32_t off;
+  grub_uint32_t off_be;
+  grub_uint32_t len;
+  grub_uint32_t len_be;
+} __attribute__ ((packed));
+
+struct grub_iso9660_data
+{
+  struct grub_iso9660_primary_voldesc voldesc;
+  grub_disk_t disk;
+  unsigned int first_sector;
+  unsigned int length;
+  int rockridge;
+  int susp_skip;
+};
+
+struct grub_fshelp_node
+{
+  struct grub_iso9660_data *data;
+  unsigned int size;
+  unsigned int blk;
+  unsigned int dir_blk;
+  unsigned int dir_off;
+};
+
+#ifndef GRUB_UTIL
+static grub_dl_t my_mod;
+#endif
+\f
+
+/* Iterate over the susp entries, starting with block SUA_BLOCK on the
+   offset SUA_POS with a size of SUA_SIZE bytes.  Hook is called for
+   every entry.  */
+static grub_err_t
+grub_iso9660_susp_iterate (struct grub_iso9660_data *data,
+			   int sua_block, int sua_pos, int sua_size,
+			   grub_err_t (*hook)
+			   (struct grub_iso9660_susp_entry *entry))
+{
+  char *sua;
+  struct grub_iso9660_susp_entry *entry;
+  
+  auto grub_err_t load_sua (void);
+  
+  /* Load a part of the System Usage Area.  */
+  grub_err_t load_sua (void)
+    {
+      sua = grub_malloc (sua_size);
+      if (!sua)
+	return grub_errno;
+      
+      if (grub_disk_read (data->disk, sua_block, sua_pos,
+			  sua_size, sua))
+	return grub_errno;
+      
+      entry = (struct grub_iso9660_susp_entry *) sua;
+      return 0;
+    }
+  
+  if (load_sua ())
+    return grub_errno;
+  
+  for (; (char *) entry < (char *) sua + sua_size - 1;
+       entry = (struct grub_iso9660_susp_entry *)
+	 ((char *) entry + entry->len))
+    {
+      /* The last entry.  */
+      if (!grub_strncmp (entry->sig, "ST", 2))
+	break;
+      
+      /* Additional entries are stored elsewhere.  */
+      if (!grub_strncmp (entry->sig, "CE", 2))
+	{
+	  struct grub_iso9660_susp_ce *ce;
+	  
+	  ce = (struct grub_iso9660_susp_ce *) entry;
+	  sua_size = grub_le_to_cpu32 (ce->len);
+	  sua_pos = grub_le_to_cpu32 (ce->off);
+	  sua_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
+	  
+	  grub_free (sua);
+	  if (load_sua ())
+	    return grub_errno;
+	}
+      
+      if (hook (entry))
+	{
+	  grub_free (sua);
+	  return 0;
+	}
+    }
+  
+  grub_free (sua);
+  return 0;
+}
+
+
+static struct grub_iso9660_data *
+grub_iso9660_mount (grub_disk_t disk)
+{
+  struct grub_iso9660_data *data = 0;
+  struct grub_iso9660_dir rootdir;
+  int sua_pos;
+  int sua_size;
+  char *sua;
+  struct grub_iso9660_susp_entry *entry;
+  
+  auto grub_err_t susp_iterate (struct grub_iso9660_susp_entry *);
+  
+  grub_err_t susp_iterate (struct grub_iso9660_susp_entry *susp_entry)
+    {
+      /* The "ER" entry is used to detect extensions.  The
+	 `IEEE_P1285' extension means Rock ridge.  */
+      if (!grub_strncmp (susp_entry->sig, "ER", 2))
+	{
+	  data->rockridge = 1;
+	  return 1;
+	}
+      return 0;
+    }
+  
+  data = grub_malloc (sizeof (struct grub_iso9660_data));
+  if (!data)
+    return 0;
+  
+  /* Read the superblock.  */
+  if (grub_disk_read (disk, 16 << GRUB_ISO9660_LOG2_BLKSZ, 0,
+		      sizeof (struct grub_iso9660_primary_voldesc),
+		      (char *) &data->voldesc))
+    {
+      grub_error (GRUB_ERR_BAD_FS, "not a iso9660 filesystem");
+      goto fail;
+    }
+
+  if (grub_strncmp (data->voldesc.voldesc.magic, "CD001", 5))
+    {
+      grub_error (GRUB_ERR_BAD_FS, "not a iso9660 filesystem");
+      goto fail;
+    }
+  
+  data->disk = disk;
+  data->rockridge = 0;
+  
+  /* Read the system use area and test it to see if SUSP is
+     supported.  */
+  if (grub_disk_read (disk, (grub_le_to_cpu32 (data->voldesc.rootdir.first_sector)
+			     << GRUB_ISO9660_LOG2_BLKSZ), 0,
+		      sizeof (rootdir), (char *) &rootdir))
+    {
+      grub_error (GRUB_ERR_BAD_FS, "not a iso9660 filesystem");
+      goto fail;
+    }
+  
+  if (grub_disk_read (disk, (grub_le_to_cpu32 (data->voldesc.rootdir.first_sector)
+			     << GRUB_ISO9660_LOG2_BLKSZ), 0,
+		      sizeof (rootdir), (char *) &rootdir))
+    {
+      grub_error (GRUB_ERR_BAD_FS, "not a iso9660 filesystem");
+      goto fail;
+    }
+  
+  sua_pos = (sizeof (rootdir) + rootdir.namelen
+	     + (rootdir.namelen % 2) - 1);
+  sua_size = rootdir.len - sua_pos;
+
+  sua = grub_malloc (sua_size);
+  if (!sua)
+    goto fail;
+  
+  if (grub_disk_read (disk, (grub_le_to_cpu32 (data->voldesc.rootdir.first_sector)
+			     << GRUB_ISO9660_LOG2_BLKSZ), sua_pos,
+		      sua_size, sua))
+    {
+      grub_error (GRUB_ERR_BAD_FS, "not a iso9660 filesystem");
+      goto fail;
+    }
+  
+  entry = (struct grub_iso9660_susp_entry *) sua;
+  
+  /* Test if the SUSP protocol is used on this filesystem.  */
+  if (!grub_strncmp (entry->sig, "SP", 2))
+    {
+      /* The 2nd data byte stored how many bytes are skipped everytime
+	 to get to the SUA (System Usage Area).  */
+      data->susp_skip = entry->data[2];
+      entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
+      
+      /* Iterate over the entries in the SUA area to detect
+	 entensions.  */
+      if (grub_iso9660_susp_iterate (data,
+				     (grub_le_to_cpu32 (data->voldesc.rootdir.first_sector)
+				      << GRUB_ISO9660_LOG2_BLKSZ),
+				     sua_pos, sua_size, susp_iterate))
+	goto fail;
+    }
+  
+  return data;
+    
+ fail:
+  grub_free (data);
+  return 0;
+}
+
+
+#if 0
+/* XXX: This code is used to lookup directories using the path tables.
+   This code is currently disabled, but should be enabled again to
+   speed up the lookup of nested files.  */
+static grub_err_t
+grub_iso9660_finddir (struct grub_iso9660_data *data, int parent,
+		      const char *name, int *dirid, int *first_sector)
+{
+  unsigned int block = (grub_le_to_cpu32 (data->voldesc.path_table)
+			<< GRUB_ISO9660_LOG2_BLKSZ);
+  unsigned int offset = 0;
+  struct grub_iso9660_path path;
+  char namer[32];
+  int fileid = 1;
+  
+  while (offset < grub_le_to_cpu32 (data->voldesc.path_table_size))
+    {
+      if (grub_disk_read (data->disk,
+			  block + offset / GRUB_DISK_SECTOR_SIZE,
+			  offset % GRUB_DISK_SECTOR_SIZE,
+			  sizeof (struct grub_iso9660_path),
+			  (char *) &path))
+	goto fail;
+      offset += sizeof (path);
+      
+      if (grub_disk_read (data->disk,
+			  block
+			  + offset / GRUB_DISK_SECTOR_SIZE,
+			  offset % GRUB_DISK_SECTOR_SIZE,
+			  path.len, (char *) namer))
+	goto fail;
+      namer[path.len] = '\0';
+
+      offset += path.len + path.len % 2;
+      
+      if (parent == path.parentdir && !grub_strcmp (name, namer))
+	{
+	  *first_sector = grub_le_to_cpu32 (path.first_sector);
+	  *dirid = fileid;
+	  return 0;
+	}
+
+      fileid++;
+    }
+  
+ fail:
+  return grub_error (GRUB_ERR_FILE_NOT_FOUND, "file not found");
+}
+#endif /* 0 */
+
+
+static char *
+grub_iso9660_read_symlink (grub_fshelp_node_t node)
+{
+  struct grub_iso9660_dir dirent;
+  int sua_off;
+  int sua_size;
+  char *symlink = 0;
+  int addslash = 0;
+  
+  auto void add_part (const char *part, int len);
+  auto grub_err_t susp_iterate_sl (struct grub_iso9660_susp_entry *);
+
+  /* Extend the symlink.  */
+  void add_part (const char *part, int len)
+    {
+      int size = grub_strlen (symlink);
+      
+      symlink = grub_realloc (symlink, size + len + 1);
+      if (!symlink)
+	return;
+      grub_strncat (symlink, part, len);
+
+      return;
+    }
+    
+  /* Read in a symlink.  */
+  grub_err_t susp_iterate_sl (struct grub_iso9660_susp_entry *entry)
+    {
+      if (!grub_strncmp ("SL", entry->sig, 2))
+	{
+	  unsigned int pos = 1;
+
+	  /* The symlink is not stored as a POSIX symlink, translate it.  */
+	  while (pos < grub_le_to_cpu32 (entry->len))
+	    {
+	      if (addslash)
+		{
+		  add_part ("/", 1);
+		  addslash = 0;
+		}
+	      
+	      /* The current position is the `Component Flag'.  */
+	      switch (entry->data[pos] & 30)
+		{
+		case 0:
+		  {
+		    /* The data on pos + 2 is the actual data, pos + 1
+		       is the length.  Both are part of the `Component
+		       Record'.  */
+		    add_part (&entry->data[pos + 2],
+			      entry->data[pos + 1]);
+		    if ((entry->data[pos] & 1))
+		      addslash = 1;
+
+		    break;
+		  }
+		
+		case 2:
+		  add_part ("./", 2);
+		  break;
+		
+		case 4:
+		  add_part ("../", 3);
+		  break;
+		  
+		case 8:
+		  add_part ("/", 1);
+		  break;
+		}
+	      /* In pos + 1 the length of the `Component Record' is
+		 stored.  */
+	      pos += entry->data[pos + 1] + 2;
+	    }
+	  
+	  /* Check if `grub_realloc' failed.  */
+	  if (grub_errno)
+	    return grub_errno;
+	}
+      
+      return 0;
+    }
+  
+  if (grub_disk_read (node->data->disk, node->dir_blk, node->dir_off,
+		      sizeof (dirent), (char *) &dirent))
+    return 0;
+  
+  sua_off = (sizeof (dirent) + dirent.namelen + 1 - (dirent.namelen % 2)
+	     + node->data->susp_skip);
+  sua_size = dirent.len - sua_off;
+  
+  symlink = grub_malloc (1);
+  if (!symlink)
+    return 0;
+  
+  *symlink = '\0';
+  
+  if (grub_iso9660_susp_iterate (node->data, node->dir_blk,
+				 node->dir_off + sua_off,
+				 sua_size, susp_iterate_sl))
+    {
+      grub_free (symlink);
+      return 0;
+    }
+  
+  return symlink;
+}
+
+
+static int
+grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
+			  int NESTED_FUNC_ATTR
+			  (*hook) (const char *filename,
+				   enum grub_fshelp_filetype filetype,
+				   grub_fshelp_node_t node))
+{
+  struct grub_iso9660_dir dirent;
+  unsigned int offset = 0;
+  char *filename;
+  int filename_alloc = 0;
+  enum grub_fshelp_filetype type;
+  
+  auto grub_err_t susp_iterate_dir (struct grub_iso9660_susp_entry *);
+
+  grub_err_t susp_iterate_dir (struct grub_iso9660_susp_entry *entry)
+    {
+      /* The filename in the rock ridge entry.  */
+      if (!grub_strncmp ("NM", entry->sig, 2))
+	{
+	  /* The flags are stored at the data position 0, here the
+	     filename type is stored.  */
+	  if (entry->data[0] & GRUB_ISO9660_RR_DOT)
+	    filename = ".";
+	  else if (entry->data[0] & GRUB_ISO9660_RR_DOTDOT)
+	    filename = "..";
+	  else
+	    {
+	      int size = 1;
+	      if (filename)
+		{
+		  size += grub_strlen (filename);
+		  grub_realloc (filename,
+				grub_strlen (filename)
+				+ entry->len);
+		}
+	      else
+		{
+		  size = entry->len - 5;
+		  filename = grub_malloc (size + 1);
+		  filename[0] = '\0';
+		}
+	      filename_alloc = 1;
+	      grub_strncpy (filename, &entry->data[1], size);
+	      filename [size] = '\0';
+	    }
+	}
+      /* The mode information (st_mode).  */
+      else if (!grub_strncmp (entry->sig, "PX", 2))
+	{
+	  /* At position 0 of the PX record the st_mode information is
+	     stored.  */
+	  grub_uint32_t mode = ((*(grub_uint32_t *) &entry->data[0])
+				& GRUB_ISO9660_FSTYPE_MASK);
+
+	  switch (mode)
+	    {
+	    case GRUB_ISO9660_FSTYPE_DIR:
+	      type = GRUB_FSHELP_DIR;
+	      break;
+	    case GRUB_ISO9660_FSTYPE_REG:
+	      type = GRUB_FSHELP_REG;
+	      break;
+	    case GRUB_ISO9660_FSTYPE_SYMLINK:
+	      type = GRUB_FSHELP_SYMLINK;
+	      break;
+	    default:
+	      type = GRUB_FSHELP_UNKNOWN;
+	    }
+	}
+      
+      return 0;
+    }
+
+  while (offset < dir->size)
+    {
+      if (grub_disk_read (dir->data->disk,
+			  (dir->blk << GRUB_ISO9660_LOG2_BLKSZ)
+			  + offset / GRUB_DISK_SECTOR_SIZE,
+			  offset % GRUB_DISK_SECTOR_SIZE,
+			  sizeof (dirent), (char *) &dirent))
+	return 0;
+      
+      /* The end of the block, skip to the next one.  */
+      if (!dirent.len)
+	{
+	  offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
+	  continue;
+	}
+      
+      {
+	char name[dirent.namelen + 1];
+	int nameoffset = offset + sizeof (dirent);
+	struct grub_fshelp_node *node;
+	int sua_off = (sizeof (dirent) + dirent.namelen + 1
+		       - (dirent.namelen % 2));;
+	int sua_size = dirent.len - sua_off;
+	
+	sua_off += offset + dir->data->susp_skip;
+	
+	filename = 0;
+	filename_alloc = 0;
+	type = GRUB_FSHELP_UNKNOWN;
+
+	if (dir->data->rockridge
+	    && grub_iso9660_susp_iterate (dir->data,
+					  (dir->blk << GRUB_ISO9660_LOG2_BLKSZ)
+					  + (sua_off
+					     / GRUB_DISK_SECTOR_SIZE),
+					  sua_off % GRUB_DISK_SECTOR_SIZE,
+					  sua_size, susp_iterate_dir))
+	  return 0;
+	
+	/* Read the name.  */
+	if (grub_disk_read (dir->data->disk,
+			    (dir->blk << GRUB_ISO9660_LOG2_BLKSZ)
+			    + nameoffset / GRUB_DISK_SECTOR_SIZE,
+			    nameoffset % GRUB_DISK_SECTOR_SIZE,
+			    dirent.namelen, (char *) name))
+	  return 0;
+	
+	node = grub_malloc (sizeof (struct grub_fshelp_node));
+	if (!node)
+	  return 0;
+	
+	/* Setup a new node.  */
+	node->data = dir->data;
+	node->size = grub_le_to_cpu32 (dirent.size);
+	node->blk = grub_le_to_cpu32 (dirent.first_sector);
+	node->dir_blk = ((dir->blk << GRUB_ISO9660_LOG2_BLKSZ)
+			 + offset / GRUB_DISK_SECTOR_SIZE);
+	node->dir_off = offset % GRUB_DISK_SECTOR_SIZE;
+	
+	/* If the filetype was not stored using rockridge, use
+	   whatever is stored in the iso9660 filesystem.  */
+	if (type == GRUB_FSHELP_UNKNOWN)
+	  {
+	    if ((dirent.flags & 3) == 2)
+	      type = GRUB_FSHELP_DIR;
+	    else
+	      type = GRUB_FSHELP_REG;
+	  }
+	
+	/* The filename was not stored in a rock ridge entry.  Read it
+	   from the iso9660 filesystem.  */
+	if (!filename)
+	  {
+	    name[dirent.namelen] = '\0';
+	    filename = grub_strrchr (name, ';');
+	    if (filename)
+	      *filename = '\0';
+	    
+	    if (dirent.namelen == 1 && name[0] == 0)
+	      filename = ".";
+	    else if (dirent.namelen == 1 && name[0] == 1)
+	      filename = "..";
+	    else
+	      filename = name;
+	  }
+	
+	if (hook (filename, type, node))
+	  {
+	    if (filename_alloc)
+	      grub_free (filename);
+	    return 1;
+	  }
+	if (filename_alloc)
+	  grub_free (filename);
+      }
+      
+      offset += dirent.len;
+    }
+  
+  return 0;
+}
+
+
+\f
+static grub_err_t
+grub_iso9660_dir (grub_device_t device, const char *path, 
+		  int (*hook) (const char *filename, int dir))
+{
+  struct grub_iso9660_data *data = 0;
+  struct grub_fshelp_node rootnode;
+  struct grub_fshelp_node *foundnode;
+  
+  auto int NESTED_FUNC_ATTR iterate (const char *filename,
+				     enum grub_fshelp_filetype filetype,
+				     grub_fshelp_node_t node);
+
+  int NESTED_FUNC_ATTR iterate (const char *filename,
+				enum grub_fshelp_filetype filetype,
+				grub_fshelp_node_t node)
+    {
+      grub_free (node);
+      
+      if (filetype == GRUB_FSHELP_DIR)
+	return hook (filename, 1);
+      else 
+	return hook (filename, 0);
+      
+      return 0;
+    }
+
+#ifndef GRUB_UTIL
+  grub_dl_ref (my_mod);
+#endif
+
+  data = grub_iso9660_mount (device->disk);
+  if (!data)
+    goto fail;
+  
+  rootnode.data = data;
+  rootnode.blk = grub_le_to_cpu32 (data->voldesc.rootdir.first_sector);
+  rootnode.size = grub_le_to_cpu32 (data->voldesc.rootdir.size);
+  
+  /* Use the fshelp function to traverse the path.  */
+  if (grub_fshelp_find_file (path, &rootnode,
+			     &foundnode,
+			     grub_iso9660_iterate_dir,
+			     grub_iso9660_read_symlink,
+			     GRUB_FSHELP_DIR))
+    goto fail;
+  
+  /* List the files in the directory.  */
+  grub_iso9660_iterate_dir (foundnode, iterate);
+  
+  if (foundnode != &rootnode)
+    grub_free (foundnode);
+  
+ fail:
+  grub_free (data);
+
+#ifndef GRUB_UTIL
+  grub_dl_unref (my_mod);
+#endif
+
+  return grub_errno;
+}
+
+
+/* Open a file named NAME and initialize FILE.  */
+static grub_err_t
+grub_iso9660_open (struct grub_file *file, const char *name)
+{
+  struct grub_iso9660_data *data;
+  struct grub_fshelp_node rootnode;
+  struct grub_fshelp_node *foundnode;
+  
+#ifndef GRUB_UTIL
+  grub_dl_ref (my_mod);
+#endif
+
+  data = grub_iso9660_mount (file->device->disk);
+  if (!data)
+    goto fail;
+  
+  rootnode.data = data;
+  rootnode.blk = grub_le_to_cpu32 (data->voldesc.rootdir.first_sector);
+  rootnode.size = grub_le_to_cpu32 (data->voldesc.rootdir.size);
+  
+  /* Use the fshelp function to traverse the path.  */
+  if (grub_fshelp_find_file (name, &rootnode,
+			     &foundnode,
+			     grub_iso9660_iterate_dir,
+			     grub_iso9660_read_symlink,
+			     GRUB_FSHELP_REG))
+    goto fail;
+  
+  data->first_sector = foundnode->blk;
+  data->length = foundnode->size;
+  
+  file->data = data;
+  file->size = foundnode->size;
+  file->offset = 0;
+  
+  return 0;
+  
+ fail:
+#ifndef GRUB_UTIL
+  grub_dl_unref (my_mod);
+#endif
+  
+  grub_free (data);
+  
+  return grub_errno;;
+}
+
+
+static grub_ssize_t
+grub_iso9660_read (grub_file_t file, char *buf, grub_ssize_t len)
+{
+  struct grub_iso9660_data *data = 
+    (struct grub_iso9660_data *) file->data;
+  
+  /* XXX: The file is stored in as a single extent.  */
+  data->disk->read_hook = file->read_hook;	  
+  grub_disk_read (data->disk,
+		  data->first_sector << GRUB_ISO9660_LOG2_BLKSZ,
+		  file->offset,
+		  len, buf);
+  data->disk->read_hook = 0;
+  
+  return len;
+}
+
+
+static grub_err_t
+grub_iso9660_close (grub_file_t file)
+{
+  grub_free (file->data);
+  
+#ifndef GRUB_UTIL
+  grub_dl_unref (my_mod);
+#endif
+  
+  return GRUB_ERR_NONE;
+}
+
+
+static grub_err_t
+grub_iso9660_label (grub_device_t device, char **label)
+{
+  struct grub_iso9660_data *data;
+  data = grub_iso9660_mount (device->disk);
+  
+  if (data)
+    {
+      *label = grub_strndup (data->voldesc.volname, 32);
+      grub_free (data);
+    }
+  else
+    *label = 0;
+
+  return grub_errno;
+}
+
+\f
+
+static struct grub_fs grub_iso9660_fs =
+  {
+    .name = "iso9660",
+    .dir = grub_iso9660_dir,
+    .open = grub_iso9660_open,
+    .read = grub_iso9660_read,
+    .close = grub_iso9660_close,
+    .label = grub_iso9660_label,
+    .next = 0
+  };
+
+#ifdef GRUB_UTIL
+void
+grub_iso9660_init (void)
+{
+  grub_fs_register (&grub_iso9660_fs);
+}
+
+void
+grub_iso9660_fini (void)
+{
+  grub_fs_unregister (&grub_iso9660_fs);
+}
+#else /* ! GRUB_UTIL */
+GRUB_MOD_INIT
+{
+  grub_fs_register (&grub_iso9660_fs);
+  my_mod = mod;
+}
+
+GRUB_MOD_FINI
+{
+  grub_fs_unregister (&grub_iso9660_fs);
+}
+#endif /* ! GRUB_UTIL */
Index: fs/jfs.c
===================================================================
RCS file: /cvsroot/grub/grub2/fs/jfs.c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 jfs.c
--- fs/jfs.c	28 Aug 2004 13:14:29 -0000	1.1
+++ fs/jfs.c	14 Oct 2004 22:13:12 -0000
@@ -361,6 +361,10 @@ grub_jfs_mount (grub_disk_t disk)
   
  fail:
   grub_free (data);
+  
+  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
+    grub_error (GRUB_ERR_BAD_FS, "not a jfs filesystem");
+  
   return 0;
 }
 
Index: fs/ufs.c
===================================================================
RCS file: /cvsroot/grub/grub2/fs/ufs.c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 ufs.c
--- fs/ufs.c	24 May 2004 21:32:21 -0000	1.1
+++ fs/ufs.c	14 Oct 2004 22:13:12 -0000
@@ -522,6 +522,10 @@ grub_ufs_mount (grub_disk_t disk)
 
  fail:
   grub_free (data);
+  
+  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
+    grub_error (GRUB_ERR_BAD_FS, "not a ufs filesystem");
+  
   return 0;
 }
 
Index: include/grub/fs.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/fs.h,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 fs.h
--- include/grub/fs.h	28 Aug 2004 13:14:29 -0000	1.9
+++ include/grub/fs.h	14 Oct 2004 22:13:12 -0000
@@ -78,6 +78,7 @@ void grub_hfs_init (void);
 void grub_hfs_fini (void);
 void grub_jfs_init (void);
 void grub_jfs_fini (void);
+void grub_iso9660_init (void);
 #endif /* GRUB_UTIL */
 
 #endif /* ! GRUB_FS_HEADER */
Index: include/grub/misc.h
===================================================================
RCS file: /cvsroot/grub/grub2/include/grub/misc.h,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 misc.h
--- include/grub/misc.h	28 Aug 2004 13:14:29 -0000	1.10
+++ include/grub/misc.h	14 Oct 2004 22:13:12 -0000
@@ -34,6 +34,7 @@ char *EXPORT_FUNC(grub_strcpy) (char *de
 char *EXPORT_FUNC(grub_strncpy) (char *dest, const char *src, int c);
 char *EXPORT_FUNC(grub_stpcpy) (char *dest, const char *src);
 char *EXPORT_FUNC(grub_strcat) (char *dest, const char *src);
+char *EXPORT_FUNC(grub_strncat) (char *dest, const char *src, int c);
 
 
 int EXPORT_FUNC(grub_memcmp) (const void *s1, const void *s2, grub_size_t n);
Index: kern/misc.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/misc.c,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 misc.c
--- kern/misc.c	14 Sep 2004 08:56:51 -0000	1.15
+++ kern/misc.c	14 Oct 2004 22:13:12 -0000
@@ -100,6 +100,21 @@ grub_strcat (char *dest, const char *src
   return dest;
 }
 
+char *
+grub_strncat (char *dest, const char *src, int c)
+{
+  char *p = dest;
+
+  while (*p)
+    p++;
+
+  while ((*p++ = *src++) != '\0' && --c)
+    ;
+  *(--p) = '\0';
+  
+  return dest;
+}
+
 int
 grub_printf (const char *fmt, ...)
 {
Index: util/grub-emu.c
===================================================================
RCS file: /cvsroot/grub/grub2/util/grub-emu.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 grub-emu.c
--- util/grub-emu.c	28 Aug 2004 13:14:29 -0000	1.7
+++ util/grub-emu.c	14 Oct 2004 22:13:12 -0000
@@ -156,6 +156,7 @@ main (int argc, char *argv[])
   grub_util_biosdisk_init (args.dev_map);
 
   /* Initialize the default modules.  */
+  grub_iso9660_init ();
   grub_fat_init ();
   grub_ext2_init ();
   grub_ufs_init ();

Re: iso9660 support

[Johan Rydberg]

Marco Gerards <metgerards@student.han.nl> writes:

> Here is a patch to add iso9660 support to GRUB 2.  There is support
> for rockridge (including stuff like symlinks), but there is no joliet
> support.  I've also fixed a few filesystems so they do not fail when
> the block read is out of range.

This might be a stupid request, but could you split the patch into
two?  One with iso9660 support, and one with the small fixes.  Just
for the sake of making clean change sets.  That would simplify, for
example, the process of reversing one of the patches if they turn out
to be buggy and ugly. :)

A few comments:

> +#if 0
> +/* XXX: This code is used to lookup directories using the path tables.
> +   This code is currently disabled, but should be enabled again to
> +   speed up the lookup of nested files.  */
> +static grub_err_t
> +grub_iso9660_finddir (struct grub_iso9660_data *data, int parent,
> +		      const char *name, int *dirid, int *first_sector)
> +{
> [...]
> +#endif /* 0 */

Why include dead code in the patch?  If you want to keep it around,
store it on your local disk instead of the repository.

I know it's easy to include half-done future enhancements.  But instead
write a (lengthy) comment on how things could be tuned for performance.

Though, I don't think that you should remove code that is disabled just
temporary, that used to be "alive." 

Just my 2 cents.

> +	  /* The symlink is not stored as a POSIX symlink, translate it.  */
> +	  while (pos < grub_le_to_cpu32 (entry->len))
> +	    {

Are grub_le_to_cpu32 and friends implemented as macros?  If not, break
out the grub_le_to_cpu32 (entry->len) expr into a variable and use
that for comparison.

> +static grub_err_t
> +grub_iso9660_dir (grub_device_t device, const char *path, 
> +		  int (*hook) (const char *filename, int dir))
> +{
> +  struct grub_iso9660_data *data = 0;
> +  struct grub_fshelp_node rootnode;
> +  struct grub_fshelp_node *foundnode;
> +  
> +  auto int NESTED_FUNC_ATTR iterate (const char *filename,
> +				     enum grub_fshelp_filetype filetype,
> +				     grub_fshelp_node_t node);
> +
> +  int NESTED_FUNC_ATTR iterate (const char *filename,
> +				enum grub_fshelp_filetype filetype,
> +				grub_fshelp_node_t node)
> +    {
> +      grub_free (node);
> +      

I do not know what NESTED_FUNC_ATTR do or is good for, but I guess it
is needed for all nested functions.  I could of course be wrong.  If
I'm right, I think I saw another function (that currently is somewhere
in my kill-ring) without the attribute (lua_read or something.)

> +static grub_ssize_t
> +grub_iso9660_read (grub_file_t file, char *buf, grub_ssize_t len)
> +{
> +  struct grub_iso9660_data *data = 
> +    (struct grub_iso9660_data *) file->data;
> +  
> +  /* XXX: The file is stored in as a single extent.  */
> +  data->disk->read_hook = file->read_hook;	  
> +  grub_disk_read (data->disk,
> +		  data->first_sector << GRUB_ISO9660_LOG2_BLKSZ,
> +		  file->offset,
> +		  len, buf);
> +  data->disk->read_hook = 0;

I know it's easy to add "XXX" or "FIXME" comments.  But try to write a
real comment instead.  In this case, why not just remove the "XXX:"

So, now I'm finished playing police.  Hope you have had as much fun as
I have.

~j

NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Tomas Ebenlendr]

> I do not know what NESTED_FUNC_ATTR do or is good for, but I guess it
> is needed for all nested functions.  I could of course be wrong.  If
> I'm right, I think I saw another function (that currently is somewhere
> in my kill-ring) without the attribute (lua_read or something.)

The NESTED_FUNC_ATTR is need for buggy versions of gcc, because we
compile i386 grub (bootime) with mregparm=3. And there are versions of
gcc that don't save content of one of registers when calling such
function or something similar.

My opinion on mregparm=3 is not good, because this is only obstacle (and
it is BIG obstacle) to use same modules at boottime and in posix-userland.

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

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Johan Rydberg]

Tomas Ebenlendr <ebik@artax.karlin.mff.cuni.cz> writes:

> My opinion on mregparm=3 is not good, because this is only obstacle (and
> it is BIG obstacle) to use same modules at boottime and in posix-userland.

So why not just stop using -mregparm=3?  I'm pretty sure it isn't
needed in GRUB, since a boot loader doesn't have very high performance
constrains.

~j

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Yoshinori K. Okuji]

On Friday 15 October 2004 03:16, Johan Rydberg wrote:
> So why not just stop using -mregparm=3?  I'm pretty sure it isn't
> needed in GRUB, since a boot loader doesn't have very high
> performance constrains.

It is necessary for the size constraint. Note that we don't need to use 
the same binary between the real GRUB and the emulated one. And, the 
emulation is only useful for debugging. So if grub-emu is difficult to 
maintain, I vote for just dropping it.

Okuji

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Tomas Ebenlendr]

> On Friday 15 October 2004 03:16, Johan Rydberg wrote:
> > So why not just stop using -mregparm=3?  I'm pretty sure it isn't
> > needed in GRUB, since a boot loader doesn't have very high
> > performance constrains.
> 
> It is necessary for the size constraint. Note that we don't need to use 
> the same binary between the real GRUB and the emulated one. And, the 
> emulation is only useful for debugging. So if grub-emu is difficult to 
> maintain, I vote for just dropping it.
> 
> Okuji

No, it isn't. I think grub-emu is important for example for saving
default menu entry mechanism or so. I also thought that grub-setup will
be replaced by install mechanism which will be in grub (and grub-emu).
And I think, that in such case will be less confusing, when there will
be one binary (module) for both grub-emu and grub (boottime).

So, what is the size constraint? Is it for machines with small stack?
If so, do we recurse so deep somewhere?
-- 
                                 Tomas 'ebi' Ebenlendr
                                 http://get.to/ebik
                                 PF 2004.7946652373

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Marco Gerards]

Tomas Ebenlendr <ebik@artax.karlin.mff.cuni.cz> writes:

>> On Friday 15 October 2004 03:16, Johan Rydberg wrote:
>> > So why not just stop using -mregparm=3?  I'm pretty sure it isn't
>> > needed in GRUB, since a boot loader doesn't have very high
>> > performance constrains.
>> 
>> It is necessary for the size constraint. Note that we don't need to use 
>> the same binary between the real GRUB and the emulated one. And, the 
>> emulation is only useful for debugging. So if grub-emu is difficult to 
>> maintain, I vote for just dropping it.
>> 
>> Okuji
>
> No, it isn't. I think grub-emu is important for example for saving
> default menu entry mechanism or so. I also thought that grub-setup will
> be replaced by install mechanism which will be in grub (and grub-emu).
> And I think, that in such case will be less confusing, when there will
> be one binary (module) for both grub-emu and grub (boottime).

The main reason why I made grub-emu is because it is convenient when
I am debugging things.  So please do not drop it.

As for module support, it is really important for me to have in
grub-emu.  But I do not care too much about compatibility between the
normal modules and those of grub-emu.

Perhaps it would be even best if grub-emu is disabled when GRUB is
built by normal users.  And it is the best that a debugging tool like
this does not have any impact of GRUB itself.  Therefore I agree with
Okuji.

I really hope install mechanisms will not be added to grub-emu in a
way people will all start using it.  But that is just a matter of
personal taste.

Thanks,
Marco

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Tomas Ebenlendr]

> I really hope install mechanisms will not be added to grub-emu in a
> way people will all start using it.  But that is just a matter of
> personal taste.
> 

Hmm, so you want to have grub-setup, and install mechanism in grub
(because of some grub rescue floppy, or so). I thought there will be one
mechanism common for both cases. (This is grub legacy feature, isn't
it?)

> Thanks,
> Marco

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

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Marco Gerards]

Tomas Ebenlendr <ebik@artax.karlin.mff.cuni.cz> writes:

>> I really hope install mechanisms will not be added to grub-emu in a
>> way people will all start using it.  But that is just a matter of
>> personal taste.
>> 
>
> Hmm, so you want to have grub-setup, and install mechanism in grub
> (because of some grub rescue floppy, or so). I thought there will be one
> mechanism common for both cases. (This is grub legacy feature, isn't
> it?)

Personally I have not thought a lot about how to install it.  AFAIK
grub-setup can handle any case.  Which cases are you thinking about
and how do you want to see those implemented?

Thanks,
Marco

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Yoshinori K. Okuji]

On Sunday 17 October 2004 22:31, Tomas Ebenlendr wrote:
> Hmm, so you want to have grub-setup, and install mechanism in grub
> (because of some grub rescue floppy, or so). I thought there will be
> one mechanism common for both cases. (This is grub legacy feature,
> isn't it?)

In GRUB 2, I don't intend to have an install mechanism in GRUB itself.

This feature is rarely useful nowadays, because it is necessary to put 
GRUB files in an appropriate place, reboot your machine then install 
it. After I wrote the grub shell, almost all people install GRUB under 
an OS but not from a boot floppy, since you don't need to reboot your 
machine any longer.

Okuji

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Yoshinori K. Okuji]

On Sunday 17 October 2004 21:41, Marco Gerards wrote:
> I really hope install mechanisms will not be added to grub-emu in a
> way people will all start using it.  But that is just a matter of
> personal taste.

I agree. You can use noinst_UTILITIES if you don't want to install it.

Okuji

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Yoshinori K. Okuji]

On Sunday 17 October 2004 20:49, Tomas Ebenlendr wrote:
> No, it isn't. I think grub-emu is important for example for saving
> default menu entry mechanism or so.

I don't agree. Please look at GRUB legacy. It does not use the grub 
shell to save a defaule entry.

> And I think, that in such case will be less confusing, 
> when there will be one binary (module) for both grub-emu and grub
> (boottime).

I don't think so, because grub-emu is useful only for developers. 
Ordinary users don't care.

> So, what is the size constraint? Is it for machines with small stack?

No. This is the same as GRUB legacy. We want to use free space right 
after the MBR, but this region is quite small (normally, 31KB). I 
reduced the binary size very seriously to make core.img fit in this 
region, and I don't want to give it up.

Okuji

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

>> So, what is the size constraint? Is it for machines with small stack?
>
> No. This is the same as GRUB legacy. We want to use free space right 
> after the MBR, but this region is quite small (normally, 31KB). I 
> reduced the binary size very seriously to make core.img fit in this 
> region, and I don't want to give it up.

Do you remember how much is gained by doing this?  I do agree with
you, I am just asking out of curiosity.

Thanks,
Marco

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Yoshinori K. Okuji]

On Sunday 17 October 2004 22:48, Marco Gerards wrote:
> Do you remember how much is gained by doing this?

I don't remember well, but my paper in Japanese says that it was about 
2KB. Since the kernel has grown a bit more since that time, I guess the 
gain could be about 3KB in the current implementation. -mregparm=3 has 
a significant effect on functions with a few arguments.

Okuji

Re: NESTED_FUNC_ATTR (was: Re: iso9660 support)

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Sunday 17 October 2004 22:48, Marco Gerards wrote:
>> Do you remember how much is gained by doing this?
>
> I don't remember well, but my paper in Japanese says that it was about 
> 2KB. Since the kernel has grown a bit more since that time, I guess the 
> gain could be about 3KB in the current implementation. -mregparm=3 has 
> a significant effect on functions with a few arguments.

Nice.  Is there more interesting information like this in the Japanese
paper you wrote?  I know this is a really vague question. :/

Thanks,
Marco

Re: iso9660 support

[Marco Gerards]

Johan Rydberg <jrydberg@night.trouble.net> writes:

> This might be a stupid request, but could you split the patch into
> two?  One with iso9660 support, and one with the small fixes.  Just
> for the sake of making clean change sets.  That would simplify, for
> example, the process of reversing one of the patches if they turn out
> to be buggy and ugly. :)

The other parts of the patch are kind of related.  It fixes some bugs
in the probe of filesystems which result in problems with some iso9660
images I used.  Therefore I have included them.

> A few comments:
>
>> +#if 0
>> +/* XXX: This code is used to lookup directories using the path tables.
>> +   This code is currently disabled, but should be enabled again to
>> +   speed up the lookup of nested files.  */
>> +static grub_err_t
>> +grub_iso9660_finddir (struct grub_iso9660_data *data, int parent,
>> +		      const char *name, int *dirid, int *first_sector)
>> +{
>> [...]
>> +#endif /* 0 */
>
> Why include dead code in the patch?  If you want to keep it around,
> store it on your local disk instead of the repository.
>
> I know it's easy to include half-done future enhancements.  But instead
> write a (lengthy) comment on how things could be tuned for performance.
>
> Though, I don't think that you should remove code that is disabled just
> temporary, that used to be "alive." 
>
> Just my 2 cents.

I could remove that.  Personally I like to keep this around so it is
there when people need it.

>> +	  /* The symlink is not stored as a POSIX symlink, translate it.  */
>> +	  while (pos < grub_le_to_cpu32 (entry->len))
>> +	    {
>
> Are grub_le_to_cpu32 and friends implemented as macros?  If not, break
> out the grub_le_to_cpu32 (entry->len) expr into a variable and use
> that for comparison.

They are.

>> +static grub_err_t
>> +grub_iso9660_dir (grub_device_t device, const char *path, 
>> +		  int (*hook) (const char *filename, int dir))
>> +{
>> +  struct grub_iso9660_data *data = 0;
>> +  struct grub_fshelp_node rootnode;
>> +  struct grub_fshelp_node *foundnode;
>> +  
>> +  auto int NESTED_FUNC_ATTR iterate (const char *filename,
>> +				     enum grub_fshelp_filetype filetype,
>> +				     grub_fshelp_node_t node);
>> +
>> +  int NESTED_FUNC_ATTR iterate (const char *filename,
>> +				enum grub_fshelp_filetype filetype,
>> +				grub_fshelp_node_t node)
>> +    {
>> +      grub_free (node);
>> +      
>
> I do not know what NESTED_FUNC_ATTR do or is good for, but I guess it
> is needed for all nested functions.  I could of course be wrong.  If
> I'm right, I think I saw another function (that currently is somewhere
> in my kill-ring) without the attribute (lua_read or something.)

NESTED_FUNC_ATTR is there to work around a bug in gcc.  It is only
triggered when 3 or ore arguments are used.  Because of that it is
used here and not in the other function you mentioned.

>> +static grub_ssize_t
>> +grub_iso9660_read (grub_file_t file, char *buf, grub_ssize_t len)
>> +{
>> +  struct grub_iso9660_data *data = 
>> +    (struct grub_iso9660_data *) file->data;
>> +  
>> +  /* XXX: The file is stored in as a single extent.  */
>> +  data->disk->read_hook = file->read_hook;	  
>> +  grub_disk_read (data->disk,
>> +		  data->first_sector << GRUB_ISO9660_LOG2_BLKSZ,
>> +		  file->offset,
>> +		  len, buf);
>> +  data->disk->read_hook = 0;
>
> I know it's easy to add "XXX" or "FIXME" comments.  But try to write a
> real comment instead.  In this case, why not just remove the "XXX:"

I'm used to Hurd hacking and this is the style that is used in the
Hurd.  Comments like XXX and FIXME are useful because it is the first
thing you want to look at in case of problems.  This is such case.  I
am not really convinced yet that it should be removed.

Thanks,
Marco