Re: [update3] [PATCH] init: bzip2 or lzma -compressed kernels and initrds

[Alain Knaff <alain@knaff.lu>]

H. Peter Anvin wrote:
> Looks a lot better now.  See the following, though:

Thanks :)

> 
>> -#define OF(args)    args
>>  #define STATIC        static
>> +#define NEW_CODE
> 
> Should be deleted, right?

Is already deleted in update4, posted yesterday.

>> -    phdrs = malloc(sizeof(*phdrs) * ehdr.e_phnum);
>> +    phdrs = (void *) malloc(sizeof(*phdrs) * ehdr.e_phnum);
>>      if (!phdrs)
> 
> malloc() returns a void pointer already...

ok, I'll consider this for update5

>> +
>> +#ifdef CONFIG_RD_BZIP2
>> +    /*
>> +     * If it matches the bzip magic numbers, return -1
>> +     */
>> +    if (buf[0] == 0x42 && (buf[1] == 0x5a)) {
>> +        printk(KERN_NOTICE
>> +               "RAMDISK: Bzipped image found at block %d\n",
>> +               start_block);
>> +        *uncompressor = bunzip2;
>>          nblocks = 0;
>>          goto done;
>>      }
>> +#endif
>> +
>> +#ifdef CONFIG_RD_LZMA
>> +    /*
>> +     * If it matches the bzip magic numbers, return -1
>                              ^^^^
>                              ????
>> +     */
>> +    if (buf[0] == 0x5d && (buf[1] == 0x00)) {
>> +        printk(KERN_NOTICE
>> +               "RAMDISK: Lzma image found at block %d\n",
>> +               start_block);

noted, will be fixed in update5

>> -#define OF(args)  args
>> +#ifdef CONFIG_RD_BZIP2
>> +#include <linux/decompress_bunzip2.h>
>> +#endif
>> +#ifdef CONFIG_RD_LZMA
>> +#include <linux/decompress_unlzma.h>
>>  #endif
> 
> There is no reason to include header files conditionally.
> 
> It might be worthwhile to create a proper subdirectory rather than using
> an underscored namespace (also, in general, dashes are preferred in
> filenames.)

I hate to contradict you, but in that case, lots of other filenames
would need fixing too:

> ls include/linux/*_*.h
include/linux/8250_pci.h             include/linux/linux_logo.h
include/linux/ac97_codec.h           include/linux/lm_interface.h
include/linux/acpi_pmtmr.h           include/linux/memory_hotplug.h
include/linux/adfs_fs.h              include/linux/minix_fs.h
include/linux/adfs_fs_i.h            include/linux/mm_inline.h
include/linux/adfs_fs_sb.h           include/linux/mm_types.h
include/linux/affs_hardblocks.h      include/linux/mmu_notifier.h
include/linux/agp_backend.h          include/linux/mnt_namespace.h
include/linux/aio_abi.h              include/linux/mod_devicetable.h
include/linux/anon_inodes.h          include/linux/msdos_fs.h
....

And dashes are not even in the majority:

> ls include/linux/*-*.h | wc
     67      67    1867
> ls include/linux/*_*.h | wc
    282     282    7598

Any other people want to chime in about this before I make update5? I'm
all for adopting common coding standards, but only if they are actually
common...

>> +#ifndef always_inline
>> +#  if defined(__GNUC__) && (__GNUC__ > 3 || __GNUC__ == 3 &&
>> __GNUC_MINOR__ > 0)
>> +#    define always_inline __attribute__((always_inline)) inline
>> +#  else
>> +#    define always_inline inline
>> +#  endif
>> +#endif
> 
> Any reason to not just use <linux/compiler.h> here?

As you may have understood, the decompressors themselves are samples
and/or reference implementations downloaded from elsewhere, and thus
might not be up to the coding standards of the rest of the kernel.

... but the same can be said of inflate.c (the existing gzip
decompressor), which would not pass muster either (and indeed, the only
scripts/checkpatch.pl WARNINGS of my patch are about funny indentation
of inflate.c that happen in the context of my changes...)

There's a fine line to walk here between "bringing up" these codes to
standard (at the risk of breaking them...) or touching the decompressing
engines as little as possible.

But, I've taken note for update5...

>> +#ifdef CONFIG_FEATURE_LZMA_FAST
>> +#  define speed_inline always_inline
>> +#else
>> +#  define speed_inline
>> +#endif
> 
> Should this be always_inline or just plain inline?
> 
>     -hpa

Same remark: this is 3rd party code which I preferred to touch as little
as possible...

Alain