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