Re: Strong Crypto Support for GRUB2

[Robert Millan]

On Sun, Sep 02, 2007 at 10:53:45PM +0200, Simon Peter wrote:
> > > +#ifndef GET_UINT32_BE
> > > +#define GET_UINT32_BE(n,b,i)                            \
> > > +{                                                       \
> > > +    (n) = ( (uint32) (b)[(i)    ] << 24 )        \
> > > +        | ( (uint32) (b)[(i) + 1] << 16 )        \
> > > +        | ( (uint32) (b)[(i) + 2] <<  8 )        \
> > > +        | ( (uint32) (b)[(i) + 3]       );       \
> > > +}
> > Doesn't follow GCS indentation style in a number of places.  I would
> > suggest using the indent(1) tool on it.
> 
> Any specific options that I shall use?

I use it without options.  But please note that I'm not the guy in charge
here, just providing advice on what I think the maintainers will like to see
(since Marco and Okuji are often busy).

> > > +GRUB_MOD_INIT(crypto)
> > > +{
> > > +  (void)mod;                     /* To stop warning. */
> > > +  grub_crypto_cipher_register(&grub_cipher_none);
> > > +  grub_crypto_cipher_register(&grub_hash_none);
> > > +}
> > Which warning was that?
> 
> Actually, I copied that line verbatim from hello.c, the GRUB hello
> world module. :) It seems that warning is long gone.

Oh.  It seems that lot of modules have this, and there are no gcc warnings
when removing them.  I would go and remove them all, unless we're missing
something; anyone knows about them?

> > 3) doesn't look GPL-compatible.  As for 1), note the author is
> > claiming ownership of any patents that might be covered by this
> > code.  GPL compatibility aside, I'm not sure what the consequences of
> > accepting the license would be (could it lead to someone
> > acknowledging K.U.Leuven as the owner of their own patents?), but it
> > looks dangerous.
> 
> Interesting, as RIPEMD is known to be one of the most open and
> unencumbered hash functions (see http://en.wikipedia.org/wiki/RIPEMD).
> There are no patents covering the code. :)

Ah, that is good (although on patents you can never be sure if one exists).
My point is that this particular wording is a bit slippery, and I don't
think it means what the author intended (IANAL, etc).

> > > +enum grub_cipher_type
> > > +  {
> > > +    GRUB_CIPHER_TYPE_NONE = 0,
> > > +    GRUB_CIPHER_TYPE_CIPHER = 1,
> > > +    GRUB_CIPHER_TYPE_HASH = 2
> > > +  };
> > Wasn't the point of using enum to avoid hardcoding these numbers? :-)
> 
> Woops. I thought you guys were doing the same and that's why I did it.
> I reverted that (leaving NONE = 0 intact).

Oh, didn't notice that.  Then I suppose you're better off keeping the numbers.

> I'm going to post another patch with your comments implemented, after I
> have your reply (I need to know what to pass to indent(1)).

Don't forget the ChangeLog entry ;-)

-- 
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 /.)