Code review comment for lp:~raharper/curtin/trunk.debug-mdadm

On Mon, Jan 30, 2017 at 12:32 PM, Scott Moser <email address hidden> wrote:

> I think this looks good.
> there are some small requests for enhancement inline.
>
> Diff comments:
>
> > + holders = get_holders(path)
> > + LOG.error('Device holders with exclusive access: %s', holders)
> > + mount_points = util.device_is_mounted(path)
> > + LOG.error('Device mounts: %s', mount_points)
> > + raise
> > +
> > + with os.fdopen(fd, "rb+") as fp:
> > while True:
> > pbuf = readfunc(buflen)
> > pos = fp.tell()
>
> i think this is sane, and gets 'fd' above closed except in the case where
> this open fails.
> i think if os.fdopen(fd) fails, then fd will not ever get a close() on it.
> Not that it should fail, but
> " If fdopen() raises an exception, it leaves fd untouched (unclosed)"
>

In the case the os.fdopen fails; I don't think fd is still valid for
closing. Even if it were, we cannot tell
as fd is *just* an int, which may or maynot point to a real file-descriptor
anymore.
If we blindly call os.close(fd), we may encounter an OSError ("Bad file
descriptor") if the fd is already
closed.

Lastly, as we exit wipe_file, the fd will leave scope and Python will reap
the fd.

If you can find a counter-example where we are leaking an fd, please let me
know.

Ryan