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

On Tue, Jan 31, 2017 at 3:19 PM, Scott Moser <email address hidden> wrote:

> close.
>
>
> Diff comments:
>
> >
> > === modified file 'curtin/block/clear_holders.py'
> > --- curtin/block/clear_holders.py 2016-08-30 21:56:13 +0000
> > +++ curtin/block/clear_holders.py 2017-01-31 20:57:11 +0000
> > @@ -62,6 +63,25 @@
> > return os.path.realpath(os.path.join(sysfs_path, 'bcache',
> 'cache'))
> >
> >
> > +def umount_device(device):
> > + """
> > + Attempt to unmount a device and any other devices which may be
> > + mounted on top of the devices mountpoint
> > + """
> > + proc_mounts = block.get_proc_mounts()
> > + mount_entries = [entry for entry in proc_mounts if device ==
> entry[0]]
> > + if len(mount_entries) > 0:
> > + for me in mount_entries:
> > + LOG.debug('Device is mounted: %s', me)
> > + mount_point = me[1]
> > + submounts = [entry for entry in proc_mounts
> > + if mount_point in entry[1]]
>
> need to do if mount_point + "/" in entry[1]
> if you had:
> /opt
> /opt2
> that would go awry, causing you to attempt to unmount /opt2. and probably
> should do startswith also.
>

I'll add unittests to get it right.

>
> submounts = [entry for entry in proc_mounts if entry[1].startswith(mount_point
> + "/")]
>
> > + umount_list = sorted(submounts, key=lambda x: x[1],
> reverse=True)
>
> i'm pretty sure this is still wrong.
> you dont want to sort submounts on the second field in /proc/mounts. that
> could go awry. You want to walk /proc/mounts in reverse. Not the somewhat
> arbitrary order that C locale sorting by the second field will give you.
>

I'll add unittests, what I really need to sort by the number of submounts,
walk to the leaf and umount on the way back.
You mentioned we have this in mount-image-callback; I'll see about bringing
that logic over to python and have a unittest.

I may drop it from this MR as this isn't needed but it's useful for
restarting control-c'd curtin installs which leave devices mounted.

I also need to add a unittest for that exclusive_open to ensure we take the
right paths, and additional unittest to test that
we invoke clear_holders in the case that we find new holders after
partitioning.

>
> > + for mp in [m[1] for m in umount_list]:
> > + LOG.debug('Umounting submounts at: %s', mp)
> > + util.do_umount(mp)
> > +
> > +
> > def shutdown_bcache(device):
> > """
> > Shut down bcache for specified bcache device
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.debug-
> mdadm/+merge/315805
> You are the owner of lp:~raharper/curtin/trunk.debug-mdadm.
>