Re: iso9660 support

[Marco Gerards]

Johan Rydberg <address@hidden> 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