Add dev_sign_file utility for developers to sign their install scripts.

Add dev_sign_file utility for developers to sign their install scripts.

BUG=chromium-os:5306

[Randall Spangler, 2010-08-10 18:08:52]

lgtm

[adlr, 2010-08-11 01:08:41]

LGTM in general. mostly nits w/ one potential 32/64 bit issue.

it would be good to read our c++ style guide and apply for readability.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

http://codereview.chromium.org/3151005/diff/1/2
File utility/Makefile (right):

http://codereview.chromium.org/3151005/diff/1/2#newcode29
utility/Makefile:29: vbutil_key \
alphabetize this list

http://codereview.chromium.org/3151005/diff/1/3
File utility/dev_sign_file.c (right):

http://codereview.chromium.org/3151005/diff/1/3#newcode50
utility/dev_sign_file.c:50: static int PrintHelp(char *progname) {
const char*?

http://codereview.chromium.org/3151005/diff/1/3#newcode89
utility/dev_sign_file.c:89: static int Sign(const char* filename, const char*
keyblock_file,
i think our style says all args on one line or one arg per line

http://codereview.chromium.org/3151005/diff/1/3#newcode91
utility/dev_sign_file.c:91: uint8_t *file_data;
put * w/ the type, not the variable

http://codereview.chromium.org/3151005/diff/1/3#newcode98
utility/dev_sign_file.c:98: FILE* f;
avoid 1-letter variables except for iterators (again, our style guide)

http://codereview.chromium.org/3151005/diff/1/3#newcode128
utility/dev_sign_file.c:128: preamble = CreateKernelPreamble(0UL, 0UL, 0UL, 0UL,
will you always compile this on 32 or 64-bit machines? if it could be either,
0UL could be 32 bits or 64 bits wide. You might consider something like
(uint64_t)0 instead if you need it to always be 64 bits wide. I don't have the
function header for CreateKernelPreamble, so this may be a bad idea if it takes
unsigned longs.

http://codereview.chromium.org/3151005/diff/1/3#newcode166
utility/dev_sign_file.c:166: uint8_t *file_data;
* by type

http://codereview.chromium.org/3151005/diff/1/3#newcode174
utility/dev_sign_file.c:174: uint64_t now = 0;
i'm not sure what 'now' means. usually it's a timestamp, but it doesn't seem to
be one here

http://codereview.chromium.org/3151005/diff/1/3#newcode218
utility/dev_sign_file.c:218: //HEY   printf("  Signature:           %s\n",
sign_key ? "valid" : "ignored");
delete this line?

http://codereview.chromium.org/3151005/diff/1/3#newcode234
utility/dev_sign_file.c:234: preamble, file_size, rsa)) {
seems like you can easily fit this on one line

http://codereview.chromium.org/3151005/diff/1/3#newcode252
utility/dev_sign_file.c:252: if (0 != VerifyData(file_data, file_size,
&preamble->body_signature,
all args on 1 line or 1 arg per line

http://codereview.chromium.org/3151005/diff/1/3#newcode259
utility/dev_sign_file.c:259: // HEY
delete?

http://codereview.chromium.org/3151005/diff/1/3#newcode271
utility/dev_sign_file.c:271: int i;
rename "option_index"?

http://codereview.chromium.org/3151005/diff/1/3#newcode341
utility/dev_sign_file.c:341: return 1;
// unreached?

[gauravsh, 2010-08-11 01:18:44]

Can you get C++ readability on C code?

On Aug 10, 2010 9:09 PM, <adlr@chromium.org> wrote:
> LGTM in general. mostly nits w/ one potential 32/64 bit issue.
>
> it would be good to read our c++ style guide and apply for readability.
>
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
>
>
> http://codereview.chromium.org/3151005/diff/1/2
> File utility/Makefile (right):
>
> http://codereview.chromium.org/3151005/diff/1/2#newcode29
> utility/Makefile:29: vbutil_key \
> alphabetize this list
>
> http://codereview.chromium.org/3151005/diff/1/3
> File utility/dev_sign_file.c (right):
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode50
> utility/dev_sign_file.c:50: static int PrintHelp(char *progname) {
> const char*?
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode89
> utility/dev_sign_file.c:89: static int Sign(const char* filename, const
> char* keyblock_file,
> i think our style says all args on one line or one arg per line
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode91
> utility/dev_sign_file.c:91: uint8_t *file_data;
> put * w/ the type, not the variable
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode98
> utility/dev_sign_file.c:98: FILE* f;
> avoid 1-letter variables except for iterators (again, our style guide)
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode128
> utility/dev_sign_file.c:128: preamble = CreateKernelPreamble(0UL, 0UL,
> 0UL, 0UL,
> will you always compile this on 32 or 64-bit machines? if it could be
> either, 0UL could be 32 bits or 64 bits wide. You might consider
> something like (uint64_t)0 instead if you need it to always be 64 bits
> wide. I don't have the function header for CreateKernelPreamble, so this
> may be a bad idea if it takes unsigned longs.
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode166
> utility/dev_sign_file.c:166: uint8_t *file_data;
> * by type
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode174
> utility/dev_sign_file.c:174: uint64_t now = 0;
> i'm not sure what 'now' means. usually it's a timestamp, but it doesn't
> seem to be one here
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode218
> utility/dev_sign_file.c:218: //HEY printf(" Signature:
> %s\n", sign_key ? "valid" : "ignored");
> delete this line?
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode234
> utility/dev_sign_file.c:234: preamble, file_size, rsa)) {
> seems like you can easily fit this on one line
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode252
> utility/dev_sign_file.c:252: if (0 != VerifyData(file_data, file_size,
> &preamble->body_signature,
> all args on 1 line or 1 arg per line
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode259
> utility/dev_sign_file.c:259: // HEY
> delete?
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode271
> utility/dev_sign_file.c:271: int i;
> rename "option_index"?
>
> http://codereview.chromium.org/3151005/diff/1/3#newcode341
> utility/dev_sign_file.c:341: return 1;
> // unreached?
>
> http://codereview.chromium.org/3151005/show

[adlr, 2010-08-11 03:41:18]

On Tue, Aug 10, 2010 at 6:18 PM, Gaurav Shah <gauravsh@chromium.org> wrote:

> Can you get C++ readability on C code?
>
Not sure, but it would be good to follow c++ readability where applicable,
and when one writes a decent sized C++ CL, apply for C++ readability.

-andrew

> On Aug 10, 2010 9:09 PM, <adlr@chromium.org> wrote:
> > LGTM in general. mostly nits w/ one potential 32/64 bit issue.
> >
> > it would be good to read our c++ style guide and apply for readability.
> >
> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
> >
> >
> > http://codereview.chromium.org/3151005/diff/1/2
> > File utility/Makefile (right):
> >
> > http://codereview.chromium.org/3151005/diff/1/2#newcode29
> > utility/Makefile:29: vbutil_key \
> > alphabetize this list
> >
> > http://codereview.chromium.org/3151005/diff/1/3
> > File utility/dev_sign_file.c (right):
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode50
> > utility/dev_sign_file.c:50: static int PrintHelp(char *progname) {
> > const char*?
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode89
> > utility/dev_sign_file.c:89: static int Sign(const char* filename, const
> > char* keyblock_file,
> > i think our style says all args on one line or one arg per line
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode91
> > utility/dev_sign_file.c:91: uint8_t *file_data;
> > put * w/ the type, not the variable
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode98
> > utility/dev_sign_file.c:98: FILE* f;
> > avoid 1-letter variables except for iterators (again, our style guide)
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode128
> > utility/dev_sign_file.c:128: preamble = CreateKernelPreamble(0UL, 0UL,
> > 0UL, 0UL,
> > will you always compile this on 32 or 64-bit machines? if it could be
> > either, 0UL could be 32 bits or 64 bits wide. You might consider
> > something like (uint64_t)0 instead if you need it to always be 64 bits
> > wide. I don't have the function header for CreateKernelPreamble, so this
> > may be a bad idea if it takes unsigned longs.
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode166
> > utility/dev_sign_file.c:166: uint8_t *file_data;
> > * by type
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode174
> > utility/dev_sign_file.c:174: uint64_t now = 0;
> > i'm not sure what 'now' means. usually it's a timestamp, but it doesn't
> > seem to be one here
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode218
> > utility/dev_sign_file.c:218: //HEY printf(" Signature:
> > %s\n", sign_key ? "valid" : "ignored");
> > delete this line?
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode234
> > utility/dev_sign_file.c:234: preamble, file_size, rsa)) {
> > seems like you can easily fit this on one line
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode252
> > utility/dev_sign_file.c:252: if (0 != VerifyData(file_data, file_size,
> > &preamble->body_signature,
> > all args on 1 line or 1 arg per line
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode259
> > utility/dev_sign_file.c:259: // HEY
> > delete?
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode271
> > utility/dev_sign_file.c:271: int i;
> > rename "option_index"?
> >
> > http://codereview.chromium.org/3151005/diff/1/3#newcode341
> > utility/dev_sign_file.c:341: return 1;
> > // unreached?
> >
> > http://codereview.chromium.org/3151005/show
>

[Bill Richardson, 2010-08-11 15:50:18]

Thanks for the feedback. I'll make these changes.

On Tue, Aug 10, 2010 at 8:40 PM, Andrew de los Reyes <adlr@chromium.org>wrote:

> On Tue, Aug 10, 2010 at 6:18 PM, Gaurav Shah <gauravsh@chromium.org>wrote:
>
>> Can you get C++ readability on C code?
>>
> Not sure, but it would be good to follow c++ readability where applicable,
> and when one writes a decent sized C++ CL, apply for C++ readability.
>
> -andrew
>
>> On Aug 10, 2010 9:09 PM, <adlr@chromium.org> wrote:
>> > LGTM in general. mostly nits w/ one potential 32/64 bit issue.
>> >
>> > it would be good to read our c++ style guide and apply for readability.
>> >
>> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
>> >
>> >
>> > http://codereview.chromium.org/3151005/diff/1/2
>> > File utility/Makefile (right):
>> >
>> > http://codereview.chromium.org/3151005/diff/1/2#newcode29
>> > utility/Makefile:29: vbutil_key \
>> > alphabetize this list
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3
>> > File utility/dev_sign_file.c (right):
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode50
>> > utility/dev_sign_file.c:50: static int PrintHelp(char *progname) {
>> > const char*?
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode89
>> > utility/dev_sign_file.c:89: static int Sign(const char* filename, const
>> > char* keyblock_file,
>> > i think our style says all args on one line or one arg per line
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode91
>> > utility/dev_sign_file.c:91: uint8_t *file_data;
>> > put * w/ the type, not the variable
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode98
>> > utility/dev_sign_file.c:98: FILE* f;
>> > avoid 1-letter variables except for iterators (again, our style guide)
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode128
>> > utility/dev_sign_file.c:128: preamble = CreateKernelPreamble(0UL, 0UL,
>> > 0UL, 0UL,
>> > will you always compile this on 32 or 64-bit machines? if it could be
>> > either, 0UL could be 32 bits or 64 bits wide. You might consider
>> > something like (uint64_t)0 instead if you need it to always be 64 bits
>> > wide. I don't have the function header for CreateKernelPreamble, so this
>> > may be a bad idea if it takes unsigned longs.
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode166
>> > utility/dev_sign_file.c:166: uint8_t *file_data;
>> > * by type
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode174
>> > utility/dev_sign_file.c:174: uint64_t now = 0;
>> > i'm not sure what 'now' means. usually it's a timestamp, but it doesn't
>> > seem to be one here
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode218
>> > utility/dev_sign_file.c:218: //HEY printf(" Signature:
>> > %s\n", sign_key ? "valid" : "ignored");
>> > delete this line?
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode234
>> > utility/dev_sign_file.c:234: preamble, file_size, rsa)) {
>> > seems like you can easily fit this on one line
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode252
>> > utility/dev_sign_file.c:252: if (0 != VerifyData(file_data, file_size,
>> > &preamble->body_signature,
>> > all args on 1 line or 1 arg per line
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode259
>> > utility/dev_sign_file.c:259: // HEY
>> > delete?
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode271
>> > utility/dev_sign_file.c:271: int i;
>> > rename "option_index"?
>> >
>> > http://codereview.chromium.org/3151005/diff/1/3#newcode341
>> > utility/dev_sign_file.c:341: return 1;
>> > // unreached?
>> >
>> > http://codereview.chromium.org/3151005/show
>>
>
>

[Bill Richardson, 2010-08-11 18:14:47]

This CL is closed, so I'll address these comments in a new one.

http://codereview.chromium.org/3151005/diff/1/3
File utility/dev_sign_file.c (right):

http://codereview.chromium.org/3151005/diff/1/3#newcode50
utility/dev_sign_file.c:50: static int PrintHelp(char *progname) {
done

http://codereview.chromium.org/3151005/diff/1/3#newcode89
utility/dev_sign_file.c:89: static int Sign(const char* filename, const char*
keyblock_file,
Actually, no. It should just look nice.

http://codereview.chromium.org/3151005/diff/1/3#newcode91
utility/dev_sign_file.c:91: uint8_t *file_data;
Done

http://codereview.chromium.org/3151005/diff/1/3#newcode128
utility/dev_sign_file.c:128: preamble = CreateKernelPreamble(0UL, 0UL, 0UL, 0UL,
Ah. My bad. Thanks.

http://codereview.chromium.org/3151005/diff/1/3#newcode166
utility/dev_sign_file.c:166: uint8_t *file_data;
Done.

http://codereview.chromium.org/3151005/diff/1/3#newcode174
utility/dev_sign_file.c:174: uint64_t now = 0;
Clarified.

http://codereview.chromium.org/3151005/diff/1/3#newcode218
utility/dev_sign_file.c:218: //HEY   printf("  Signature:           %s\n",
sign_key ? "valid" : "ignored");
Yes, thanks.

http://codereview.chromium.org/3151005/diff/1/3#newcode234
utility/dev_sign_file.c:234: preamble, file_size, rsa)) {
done

http://codereview.chromium.org/3151005/diff/1/3#newcode252
utility/dev_sign_file.c:252: if (0 != VerifyData(file_data, file_size,
&preamble->body_signature,
done.

http://codereview.chromium.org/3151005/diff/1/3#newcode259
utility/dev_sign_file.c:259: // HEY
Yes, thanks.

http://codereview.chromium.org/3151005/diff/1/3#newcode271
utility/dev_sign_file.c:271: int i;
Done.

http://codereview.chromium.org/3151005/diff/1/3#newcode341
utility/dev_sign_file.c:341: return 1;
Yes, but I don't like relying on the compiler to detect that I've handled all
the possible returns. I'll put a comment here.