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 >> >
