On Tue, Jul 08, 2008 at 10:44:42AM -0400, Cole Robinson wrote: > > + for dirpath, _, files in os.walk(path): > > You'll want to use a different variable other than '_' since this will > overwrite the gettext function in the local scope. Doesn't cause issues > in the current code, but could with future changes. Hmm, OK. _ is pretty standard for "don't care", what would you prefer? > > +def cleanup(msg, options, created_dir): > > + """ > > + After failure, clean up anything we created. Take a conservative > > + approach: only if we created the output directory do we delete > > + anything. > > + """ > > + logging.error(msg) > > + > > + if created_dir: > > + try: > > + rmrf(options.output_dir) > > + except OSError, e: > > + logging.error("Couldn't clean up output directory \"%s\": %s" % > > + (options.output_dir, e.strerror)) > > + > > + sys.exit(1) > > + > > Hmm, did you see the comment I made in my response to this original > patch? No, I don't think so. > I think a whitelist approach would be better: have convert_disks clean > up its own files, and keep all other file/directory creation in the > main() function (like it currently is) so it can handle the rest > of the cleanup on a per file basis. At the end, if the output_dir > is empty and we created it, we can delete it. Maybe it's overly > paranoid, but better to be safe than sorry." > > Any comment on this? Seems sensible, yes. regards, john _______________________________________________ et-mgmt-tools mailing list et-mgmt-tools@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/et-mgmt-tools