Hi Jean, Thank you for making the changes. I still have a few comments:
- line 34: why combine the 3 separate lines into this one line? I don't think we need everything from dc_utils.py. - line 409: I think we don't need to mention the empty snapshot here - line 686: why is the "-r" removed from here as well as the finalizer_rollback.py? Thanks, --Karen Jean McCormack wrote: > Karen, > > I believe I've addressed all of your concerns. The webrev is at : > http://cr.opensolaris.org/~jeanm/slim_5170/ > > Jean > > Karen Tung wrote: >> 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 >>> >> >
