Re: [PATCH] cpio and tar filesystem

[Robert Millan <rmh@aybabtu.com>]

On Mon, Dec 24, 2007 at 04:04:31AM +0800, Bean wrote:
> 
> 	* conf/i386-pc.rmk (grub_emu_SOURCES): Add cpio.c.
> 
> 	* conf/i386-efi.rmk (grub_emu_SOURCES): Likewise.
> 
> 	* conf/i386-linuxbios.rmk (grub_emu_SOURCES): Likewise.
> 
> 	* conf/powerpc-ieee1275.rmk (grub_emu_SOURCES): Likewise.

Perhaps it'd be a good idea to move the arch-independant part of
grub_emu_SOURCES to common.rmk (as grub_emu_SOURCES += foo), to avoid
having to update all the files so often ...

> diff --git a/disk/loopback.c b/disk/loopback.c
> index 9d48def..31d8116 100644
> --- a/disk/loopback.c
> +++ b/disk/loopback.c
> @@ -214,7 +214,7 @@ grub_loopback_read (grub_disk_t disk,
> grub_disk_addr_t sector,
>    if (pos > file->size)
>      {
>        grub_size_t amount = pos - file->size;
> -      grub_memset (buf + size - amount, 0, amount);
> +      grub_memset (buf + (size << GRUB_DISK_SECTOR_BITS) - amount, 0, amount);

Ugh :-)  I check this in right now.

> +struct HEAD_USTAR
> +{
> +  char	name[100];
> +  char	mode[8];
> +  char	uid[8];
> +  char	gid[8];
> +  char	size[12];
> +  char	mtime[12];
> +  char	chksum[8];
> +  char	typeflag;
> +  char	linkname[100];
> +  char	magic[6];
> +  char	version[2];
> +  char	uname[32];
> +  char	gname[32];
> +  char	devmajor[8];
> +  char	devminor[8];
> +  char	prefix[155];

These tabs should be spaces.

> +      if (grub_disk_read (data->disk, 0, data->hofs, sizeof(hd), (char*)&hd))
> [...]
> +      if (grub_disk_read (data->disk, 0, data->hofs, sizeof(hd), (char*)&hd))

`(char *) &hd' here (same for all other casts).

Btw, this line seems to be the same on both cases.  It can be moved out of
`if (data->mode == MODE_BCPIO)' to save some space?

> +      if (hd.namesize & 1)
> +        hd.namesize++;
> [...]
> +      if (data->size & 1)
> +        (*ofs)++;

I find this confusing.  AFAICT `hd.namesize == 1' would archieve the same and
seems to be more consistent with your use of this variable as a counter.

> +      if ((*name = grub_malloc (hd.namesize))==NULL)
> [...]
> +      if ((*name = grub_strdup (hd.name))==NULL)

Please add some spaces: ` == NULL'

> +      if (grub_memcmp(hd.magic, MAGIC_USTAR, sizeof(MAGIC_USTAR) - 1))
> [...]
> +      data->size = grub_strtoul(hd.size, NULL, 8);

`grub_memcmp (', `grub_strtoul (', etc.  Same for other function calls.

> +  return (grub_disk_read (data->disk, 0, data->dofs + file->offset,
> +                          len, buf))?-1:len;

` ? -1 : len'

> +#ifndef GRUB_UTIL
> +  grub_dl_unref (my_mod);
> +#endif
> [...]
> +#ifndef GRUB_UTIL
> +  my_mod = mod;
> +#endif

Are you sure these are still needed?  We have a few modules that use them but
AFAIK are not necessary at this time.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)