Hi Jean, Here are my comments:
- Line 402-420. My understanding of this code segment is that you are getting a list of all the snap shots, and making sure that the order of the of checkpoints corresponds to the order of the snapshots list. Your list of checkpoints is built by reading the manifest, so, I understand that the checkpoints will be in the order that's specified. How do we ensure that the output of "zfs list...." will follow the same order? Right now, it just so happens that they follow the same order. Is there a rule or something that zfs datasets/snapshots will be listed in the order they are created? I don't see an option in "zfs list" to sort the datasets by the time they are created or something. That will certainly gurantee the output of "zfs list" will follow the order of things specified in the manifest. I am concern that if "zfs list" ever change the order of the output, then, our code won't work. - lines 346-348: I think it is inefficient to execute a system shell and do a grep on the temp file content since this loop is executed for each item on the step list. I think it is more efficient to read the content of the temp file into a python list and just search the internal list. - line 348: since you already found the step_num here, can we break out the loop? - Lines 333-337 and 389-395 are doing the same thing. I think it is better to have the common code in a function or something. Even better, you can make this function return a list of snapshot names. Also, I just saw something on the python docs page about how to do the shell pipeline in Python. Maybe you want to try something like this? http://docs.python.org/library/subprocess.html#subprocess-replacements Thanks, --Karen Jean McCormack wrote: > Please review the fix for > 5170 DC_determine_Resume_step should check snapshots existence not .step > files. > > Bug: > http://defect.opensolaris.org/bz/show_bug.cgi?id=5170 > > Webrev: > http://cr.opensolaris.org/~jeanm/slim_5170/ > > Testing included: > > 1) distro_const build -l <manifest> > > 2) distro_const build -r <legal value> <manifest> > > 3) distro_const build -r <illegal value> <manifest> > > 4) Modify manifest adding another finalizer script. > Repeat 1,2,3 > > 5) Modify manifest removing a finalizer script > Repeat 1,2,3 > > 6) Modify manifest shuffling the finalizer scripts > Repeat 1,2,3 > > Jean > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
