Re: [PATCH] romfs support for Grub2

[Bean]

Hi,

On Sun, Aug 17, 2008 at 10:43 AM, y.volta <address@hidden> wrote:
> Hi,
>
>    I made a small patch for grub2 using  a romfs file ( zip compressing 
> support ). there are 3 files added:
>
> -/commands/i386/pc/romfst.c                                 command to mount 
> a romfs image file.
> -/fs/i386/pc/romfs.c                                                 fs and 
> disk layer for romfs.
> -/include/grub/i386/pc/romfs.h                                 header file.
>
> usage:
> grub>romfst /romfs.gz
> grub>cat (rd)/readme.txt
>
> info
> ------------------------------------------------------------
> i make this patch have two resources: the grub legancy romfs patch and the 
> bean's pxe patch.
> i get to know the pxe mod merged the disk&fs into a fs source file. this will 
> make mod more simple to implement.
> the romfst command will load the specify file to a new memory block. when 
> user access (rd), the new files are ready for him.

I think romfs support is not in grub legacy, do you mean some local
patch of distro ? Also, code from grub legacy is not always compatible
with grub2. To avoid legal issue, just don't import code from other
source, write them from scratch.

There is no need to add a command to mount the image, you can use
loopback. Also, don't merge disk&fs, romfs should be a fs module, just
like cpio.

BTW, I notice that you define le32, but there is already a function to
do the job: grub_le_to_cpu32.

Also, you use global buffer blk_buf, this is very grub-legacy. In
grub2, you should avoid it as much as possible, because it would have
problem when accessing multiple file, try to place the buffer in local
structure like grub_romfs_data.

> limitations
> ------------------------------------------------------------
> * this patch is for file only: if you want to use it on a disk/partition, we 
> should improve it.
> * grub_romfs_data *data, has not set to file->data - i used several global 
> varibles to store releated stuffs.
>
>
> well, this is just a work-able patch, not too professional. ;-)
>
> btw, i think the grub_dprintf should output to non-display device, this will 
> make debuging more easier and comfort . i think the low-cost device is a 
> com-port. for, now time virtual machine software has this legancy device full 
> supported.
> the freeldr coming with ReactOS have this kind of function (only display a 
> string, but can do kernal debug also )

Please don't define new debug mechanism. grub_dprintf may not be the
best, but it's already used through grub2, just keep it that way.

Also, I notice that indentation is not right, grub2 use gnu style. If
you're too lazy to reformat the code, you can run it through indent.

-- 
Bean