Peter Tribble wrote: > On Thu, May 22, 2008 at 6:38 PM, Jean McCormack <Jean.McCormack at sun.com> > wrote: > >> I'm putting the main application for the new DC out for code review. >> It consists of the checkpointing code and command line parsing. >> >> webrev: >> http://cr.opensolaris.org/~jeanm/DC_python/ >> > > I'm not too familiar with python, but a handful of things struck > me as I read through the code: > > 1. It runs shell commands to do a lot of the work. I always worry > when I see this, as it's much safer (and quicker) to use the language > itself. For instance, you could use the shutil module to replace the call > to cp, unlink() to replace the call to rm, filecmp to replace the call to > diff, > listdir with fnmatch to replace the call to ls. > Yes. This was my first Python program and I missed those. I'll make modifications. > 2. Clearly there isn't yet a zfs module. But where running a shell command > is necessary, should the PATH be explicitly set, or absolute paths to the > commands used? > I think absolute paths would be best. I'll make those modifications. Thanks for catching that. > 3. In DC_determine_resume_step, you concatenate > '/tmp/resumelist_' and str(os.getpid()) multiple times. > Would it be better to construct the file once? Or even use > the tempfile module to generate the filename safely for you. > I agree. I'll make the changes to create the filename once and then use that.
thanks for your feedback, Jean > Thanks, > >
