Re: hostfs patch

[Tomas Ebenlendr]

> I don't see this change in the changelog.  Please put every change you
> make in the changelog and please make sure everything you describe in
> the changelog entry can be found in that patch.

Ok.

> > diff -rupN -x CVS -x '*.mk' grub2_savannah/kern/dl.c grub2_wodiff/kern/dl.c
> > --- grub2_savannah/kern/dl.c        2004-08-12 23:39:00.000000000 +0200
> > +++ grub2_wodiff/kern/dl.c  2004-08-12 23:48:50.000000000 +0200
> > @@ -548,7 +548,8 @@ grub_dl_load_file (const char *filename)
> >      goto failed;
> >  
> >    mod = grub_dl_load_core (core, size);
> > -  mod->ref_count = 0;
> > +  if (mod)
> > +    mod->ref_count = 0;
> 
> Perhaps an error code should be returned as well, no?
>

I think that error code and return value is correct, there was only
segfault in error case (in grub-emu).

> > +      if (hook (d->name))
> > +   return 1;
> 
> This looks funny... Is my mailclient or brain screwing things up or is
> this a tab?
> 
Yes there is ascii 9 character, should there be spaces?

> > +  disk->total_sectors = 1234;
> 
> Why 1234?  Can't it be set to 0?  If I fill in a bogus value, I
> usually use -1 because it is so obvious it is bogus...
> 
Hmm, I probably forgot comment there. Some other fs considers disk bad,
if it is too small and cannot read some sectors at beginning of it. This
will stop fs autodetection too early to probe hostfs. This may be a bug,
but that you will know better than me. (If other error than
GRUB_ERR_BAD_FS is returned by fs, autodetection stops, So i implemented
grub_util_fakedisk_read, to return GRUB_ERR_BAD_FS. grub_disk_read() or
how is the correct name, will first look if sector is in range and then
runs grub_<disk_driver>_read(), otherwise returns some error different
form BAD_FS and filesystem will only propagate it.)


> > +/* FIXME grub consider the fs in utf-8 => use only ascii (0-127) chars.  */
> 
> GRUB uses UTF-8 for filesystems.  So in that case everything can be
> used (0-255), no?
> 
One must have utf-8 locale, or something similar set. Otherwise (as in my case)
file names are in iso-8859-2, or other charset. That is very
incompatible with utf-8, because 128-255 means that it is not one-byte
character in utf-8. So such character can mangle next charactes as well.
I didn't tried to fix this, because grub is not considered to access
files with non ascii characters.


> > +  /* Maybe loop to get as most as possible should be here, may be not.  */
> > +  rv = read ((int) file->data, buf, len);
> 
> I think you should loop when using read because it can return with not
> all requested data read.  But I am not a POSIX expert so I can be
> wrong here.
> 
Okay, I will fix it. This was just first implementation, and it worked.
(I guess that every file is regular file on local filesystem, then this
works perfectly. I will probably choose to return error on EAGAIN,
because we don't need to support this. (anyway, POSIX should not return
EAGAIN if O_NONBLOCK is not set).

> > +  while ((dent = readdir (dir)) != 0)
> 
> [...]
> 
> > +      lstat (pathbuf, &stent);
> 
> Can these functions fail?
Don't know. I will read manpages, and try to figure it out.

I agree all other comments, and I will fix the patch in that way.

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