[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: bugfix, hostfs
From: |
Yoshinori K. Okuji |
Subject: |
Re: bugfix, hostfs |
Date: |
Sun, 8 Aug 2004 21:17:56 +0200 |
User-agent: |
KMail/1.6.1 |
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
- bugfix, hostfs, Tomas Ebenlendr, 2004/08/08
- Re: bugfix, hostfs, Marco Gerards, 2004/08/08
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/08
- Re: bugfix, hostfs, Marco Gerards, 2004/08/08
- Re: bugfix, hostfs,
Yoshinori K. Okuji <=
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/12
- Re: bugfix, hostfs, Marco Gerards, 2004/08/13
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/14
- Re: bugfix, hostfs, Marco Gerards, 2004/08/14
- Re: bugfix, hostfs, Tomas Ebenlendr, 2004/08/14
- Re: bugfix, hostfs, Yoshinori K. Okuji, 2004/08/21