Re: [PATCH 02 of 11] Cleanup on failure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Fedora Users]     [Fedora Legacy List]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux