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 /.)
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
