I'll add the os.path.basename(); should we do that in both places?
Diff comments:
> diff --git a/cloudinit/sources/DataSourceOVF.py
> b/cloudinit/sources/DataSourceOVF.py
> index 24b45d5..4b02cbb 100644
> --- a/cloudinit/sources/DataSourceOVF.py
> +++ b/cloudinit/sources/DataSourceOVF.py
> @@ -406,29 +406,19 @@ def transport_iso9660(require_iso=True):
> else:
> mtype = None
>
> - devs = os.listdir("/dev/")
> - devs.sort()
> + # generate a list of devices with mtype filesystem, filter by regex
> + devs = [dev for dev in
> + util.find_devs_with("TYPE=%s" % mtype if mtype else None)
> + if cdmatch.match(dev[5:])]
No need to split it up, if you're ok, we can just put os.path.basename(dev)
inside the cdmatch.match()
> for dev in devs:
> - fullp = os.path.join("/dev/", dev)
> -
> - if (fullp in mounts or
> - not cdmatch.match(dev) or os.path.isdir(fullp)):
> - continue
> -
> try:
> - # See if we can read anything at all...??
> - util.peek_file(fullp, 512)
> - except IOError:
> - continue
> -
> - try:
> - (fname, contents) = util.mount_cb(fullp, get_ovf_env,
> mtype=mtype)
> + (fname, contents) = util.mount_cb(dev, get_ovf_env, mtype=mtype)
> except util.MountFailedError:
> - LOG.debug("%s not mountable as iso9660", fullp)
> + LOG.debug("%s not mountable as iso9660", dev)
> continue
>
> if contents is not False:
> - return (contents, fullp, fname)
> + return (contents, dev, fname)
>
> return (False, None, None)
>
> diff --git a/tests/unittests/test_datasource/test_ovf.py
> b/tests/unittests/test_datasource/test_ovf.py
> index 9dbf4dd..7c416a5 100644
> --- a/tests/unittests/test_datasource/test_ovf.py
> +++ b/tests/unittests/test_datasource/test_ovf.py
> @@ -70,4 +71,112 @@ class TestReadOvfEnv(test_helpers.TestCase):
> self.assertEqual({'password': "passw0rd"}, cfg)
> self.assertIsNone(ud)
>
> +
> +class TestTransportIso9660(test_helpers.CiTestCase):
> +
> + def setUp(self):
> + super(TestTransportIso9660, self).setUp()
> + self.add_patch('cloudinit.util.find_devs_with',
> + 'm_find_devs_with')
> + self.add_patch('cloudinit.util.mounts', 'm_mounts')
> + self.add_patch('cloudinit.util.mount_cb', 'm_mount_cb')
> + self.add_patch('cloudinit.sources.DataSourceOVF.get_ovf_env',
> + 'm_get_ovf_env')
> + self.m_get_ovf_env.return_value = ('myfile', 'mycontent')
> +
> + def test_find_already_mounted(self):
> + """Check we call get_ovf_env from on matching mounted devices"""
> + mounts = {
> + '/dev/sr9': {
> + 'fstype': 'iso9660',
> + 'mountpoint': 'wark/media/sr9',
> + 'opts': 'ro',
> + }
> + }
> + self.m_mounts.return_value = mounts
> +
> + (contents, fullp, fname) = dsovf.transport_iso9660()
> + self.assertEqual("mycontent", contents)
> + self.assertEqual("/dev/sr9", fullp)
> + self.assertEqual("myfile", fname)
> +
> + def test_find_already_mounted_skips_non_iso9660(self):
> + """Check we call get_ovf_env ignoring non iso9660"""
> + mounts = {
> + '/dev/xvdb': {
> + 'fstype': 'vfat',
> + 'mountpoint': 'wark/foobar',
> + 'opts': 'defaults,noatime',
> + },
> + '/dev/xvdc': {
> + 'fstype': 'iso9660',
> + 'mountpoint': 'wark/media/sr9',
> + 'opts': 'ro',
> + }
> + }
> + # ensure we check xvdb before xvdc
> + self.m_mounts.return_value = (
> + OrderedDict(sorted(mounts.items(), key=lambda t: t[0])))
The order only matters for the unittest; since I'm not mocking the regex match,
I don't have a way to check that we skip things that don't match; if I *order*
the results I think we can reasonably expect that we walked both entries in
the dictionary.
> +
> + (contents, fullp, fname) = dsovf.transport_iso9660()
> + self.assertEqual("mycontent", contents)
> + self.assertEqual("/dev/xvdc", fullp)
> + self.assertEqual("myfile", fname)
> +
> + def test_mount_cb_called_on_blkdevs_with_iso9660(self):
> + """Check we call mount_cb on blockdevs with iso9660 only"""
> + self.m_mounts.return_value = {}
> + self.m_find_devs_with.return_value = ['/dev/sr0']
> + self.m_mount_cb.return_value = ("myfile", "mycontent")
> +
> + (contents, fullp, fname) = dsovf.transport_iso9660()
> +
> + self.m_mount_cb.assert_called_with(
> + "/dev/sr0", dsovf.get_ovf_env, mtype="iso9660")
> + self.assertEqual("mycontent", contents)
> + self.assertEqual("/dev/sr0", fullp)
> + self.assertEqual("myfile", fname)
> +
> + def test_mount_cb_called_on_blkdevs_with_iso9660_check_regex(self):
> + """Check we call mount_cb on blockdevs with iso9660 and match
> regex"""
> + self.m_mounts.return_value = {}
> + self.m_find_devs_with.return_value = [
> + '/dev/abc', '/dev/my-cdrom', '/dev/sr0']
> + self.m_mount_cb.return_value = ("myfile", "mycontent")
> +
> + (contents, fullp, fname) = dsovf.transport_iso9660()
> +
> + self.m_mount_cb.assert_called_with(
> + "/dev/sr0", dsovf.get_ovf_env, mtype="iso9660")
> + self.assertEqual("mycontent", contents)
> + self.assertEqual("/dev/sr0", fullp)
> + self.assertEqual("myfile", fname)
> +
> + def test_mount_cb_not_called_no_matches(self):
> + """Check we don't call mount_cb if nothing matches"""
> + self.m_mounts.return_value = {}
> + self.m_find_devs_with.return_value = ['/dev/vg/myovf']
> +
> + (contents, fullp, fname) = dsovf.transport_iso9660()
> +
> + self.assertEqual(0, self.m_mount_cb.call_count)
> + self.assertEqual(False, contents)
> + self.assertIsNone(fullp)
> + self.assertIsNone(fname)
> +
> + def test_mount_cb_called_require_iso_false(self):
> + """Check we call mount_cb on blockdevs with require_iso=False"""
> + self.m_mounts.return_value = {}
> + self.m_find_devs_with.return_value = ['/dev/xvdz']
> + self.m_mount_cb.return_value = ("myfile", "mycontent")
> +
> + (contents, fullp, fname) = dsovf.transport_iso9660(require_iso=False)
> +
> + self.m_mount_cb.assert_called_with(
> + "/dev/xvdz", dsovf.get_ovf_env, mtype=None)
> + self.assertEqual("mycontent", contents)
> + self.assertEqual("/dev/xvdz", fullp)
> + self.assertEqual("myfile", fname)
> +
> +
> # vi: ts=4 expandtab
--
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/330995
Your team cloud-init commiters is requested to review the proposed merge of
~raharper/cloud-init:ds-ovf-use-util-find-devs-with into cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
