Re: iso9660 support

[Johan Rydberg]

Marco Gerards <address@hidden> 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