Hi Glenn,
Looks great overall, I have the following few comments.
DC_defs.py: I don't think these 3 defined values are used anywhere, since
the code to access the checksums are in a shell script. If they are not
being
used now, let's not define them until they need to be used.
generate_checksums:
- Most of the shell scripts I read have the functions defined for
the script right after any statically defined values. The compute_hash()
function is kinda in the middle of the file, can you move it?
- I think it is better to pass in $IMAGE and $HASH_TYPES as arguments
to the compute_hash() function. That way, the function we are absolutely
clear on exactly which value is being used, and it doesn't depend on things
getting set correctly before the function is called.
- Sometimes, you put {} around the variables, sometimes you don't, it is
best to always put them in.
Thanks,
--Karen
Glenn Lagasse wrote:
> Hi All,
>
> Could I get at least two reviewers for this change please? It's not
> going in to 2009.06 but I'd like to get some feedback from a few people.
>
> With this change, the distribution constructor can now generate
> cryptographic hashes for the images that it creates. This is done by
> specifying the hashes you want (must be supported by the digest(1)
> command which I validate using a Validator method) in the manifest used
> to create the images. The design is flexible in that you
> create/recreate hashes as a separate finalizer step (so if you change
> your mind and want different hash types you can get them without having
> to start the run over from scratch).
>
> I've tested this code using the following methods:
>
> 1) Sample manifest with proper values validates cleanly
> 2) Sample manifest with duplicate hash types fails validation
> 3) Sample manifest with checksums enabled but no hashes fails validation
> 4) Sample manifest with proper values creates hashes
> 5) Sample manifest with proper values creates hashes and can be resumed
> 6) Sample manifest without checksums validates cleanly
> 7) Sample manifest without checksums but specified finalizer script
> runs ok but displays warnings when the checksums finalizer script is
> run without having specified any checksums to generate
> 8) Sample manifest with checksum type not supported by digest command
> fails appropriately
> 9) Sample manifest, resumed after initial checksum generation, with
> different checksums works as expected (ie it generates the proper
> checksums as now specified in the manifest
>
> Webrev:
>
> http://cr.opensolaris.org/~glagasse/slim_5265
>
> Bug:
>
> http://defect.opensolaris.org/bz/show_bug.cgi?id=5265
>
> While there isn't any rush to review this since I've no place to
> integrate it yet I would appreciate comments no later than one week from
> tomorrow.
>
> Thanks!
>
>
