diff --git a/src/plugins/dm.c b/src/plugins/dm.c index e4ec3b5a..51cc1e9c 100644 --- a/src/plugins/dm.c +++ b/src/plugins/dm.c @@ -178,19 +178,21 @@ gboolean bd_dm_remove (const gchar *map_name, GError **error) { gchar* bd_dm_name_from_node (const gchar *dm_node, GError **error) { gchar *ret = NULL; gboolean success = FALSE; + g_autofree gchar *sys_path = g_strdup_printf ("/sys/class/block/%s/dm/name", dm_node); - gchar *sys_path = g_strdup_printf ("/sys/class/block/%s/dm/name", dm_node); + if (!dm_node || strlen (dm_node) == 0) { + g_set_error (error, BD_DM_ERROR, BD_DM_ERROR_DEVICE_NOEXIST, + "No DM node specified"); + return NULL; + } if (access (sys_path, R_OK) != 0) { - g_free (sys_path); g_set_error (error, BD_DM_ERROR, BD_DM_ERROR_SYS, "Failed to access dm node's parameters under /sys"); return NULL; } success = g_file_get_contents (sys_path, &ret, NULL, error); - g_free (sys_path); - if (!success) { /* error is already populated */ g_free (ret); @@ -211,20 +213,21 @@ gchar* bd_dm_name_from_node (const gchar *dm_node, GError **error) { * Tech category: %BD_DM_TECH_MAP-%BD_DM_TECH_MODE_QUERY */ gchar* bd_dm_node_from_name (const gchar *map_name, GError **error) { - gchar *dev_path = NULL; - gchar *ret = NULL; - gchar *dev_mapper_path = g_strdup_printf ("/dev/mapper/%s", map_name); + g_autofree gchar *dev_path = NULL; + g_autofree gchar *dev_mapper_path = g_strdup_printf ("/dev/mapper/%s", map_name); + + if (!map_name || strlen (map_name) == 0) { + g_set_error (error, BD_DM_ERROR, BD_DM_ERROR_DEVICE_NOEXIST, + "No DM name specified"); + return NULL; + } dev_path = bd_utils_resolve_device (dev_mapper_path, error); - g_free (dev_mapper_path); if (!dev_path) /* error is already populated */ return NULL; - ret = g_path_get_basename (dev_path); - g_free (dev_path); - - return ret; + return g_path_get_basename (dev_path); } /** diff --git a/src/utils/dev_utils.c b/src/utils/dev_utils.c index 5468fa56..89a54b4c 100644 --- a/src/utils/dev_utils.c +++ b/src/utils/dev_utils.c @@ -19,6 +19,8 @@ #include #include +#include +#include #include "dev_utils.h" @@ -30,6 +32,24 @@ GQuark bd_utils_dev_utils_error_quark (void) return g_quark_from_static_string ("g-bd-utils-dev_utils-error-quark"); } +static gboolean _is_block_device (const gchar *path, GError **error) { + struct stat sb; + + if (stat (path, &sb) != 0) { + g_set_error (error, BD_UTILS_DEV_UTILS_ERROR, BD_UTILS_DEV_UTILS_ERROR_FAILED, + "Failed to stat '%s': %s", path, strerror_l (errno, _C_LOCALE)); + return FALSE; + } + + if (!S_ISBLK (sb.st_mode)) { + g_set_error (error, BD_UTILS_DEV_UTILS_ERROR, BD_UTILS_DEV_UTILS_ERROR_FAILED, + "'%s' is not a block device", path); + return FALSE; + } + + return TRUE; +} + /** * bd_utils_resolve_device: * @dev_spec: specification of the device (e.g. "/dev/sda", any symlink, or the name of a file @@ -40,12 +60,10 @@ GQuark bd_utils_dev_utils_error_quark (void) * for "/dev/md/my_raid") or %NULL in case of error */ gchar* bd_utils_resolve_device (const gchar *dev_spec, GError **error) { - gchar *path = NULL; - gchar *symlink = NULL; + g_autofree gchar *path = NULL; + g_autofree gchar *symlink = NULL; GError *l_error = NULL; - /* TODO: check that the resulting path is a block device? */ - if (!g_str_has_prefix (dev_spec, "/dev/")) path = g_strdup_printf ("/dev/%s", dev_spec); else @@ -56,11 +74,15 @@ gchar* bd_utils_resolve_device (const gchar *dev_spec, GError **error) { if (g_error_matches (l_error, G_FILE_ERROR, G_FILE_ERROR_INVAL)) { /* invalid argument -> not a symlink -> nothing to resolve */ g_clear_error (&l_error); - return path; + if (_is_block_device (path, error)) + return g_steal_pointer (&path); + else { + g_prefix_error (error, "Failed to resolve '%s': ", dev_spec); + return NULL; + } } else { /* some other error, just report it */ g_propagate_error (error, l_error); - g_free (path); return NULL; } } @@ -70,9 +92,13 @@ gchar* bd_utils_resolve_device (const gchar *dev_spec, GError **error) { path = g_strdup_printf ("/dev/%s", symlink + 3); else path = g_strdup_printf ("/dev/%s", symlink); - g_free (symlink); - return path; + if (_is_block_device (path, error)) + return g_steal_pointer (&path); + else { + g_prefix_error (error, "Failed to resolve '%s': ", dev_spec); + return NULL; + } } /** diff --git a/src/utils/dev_utils.h b/src/utils/dev_utils.h index daa66284..9786718f 100644 --- a/src/utils/dev_utils.h +++ b/src/utils/dev_utils.h @@ -25,6 +25,9 @@ GQuark bd_utils_dev_utils_error_quark (void); #define BD_UTILS_DEV_UTILS_ERROR bd_utils_dev_utils_error_quark () +/* "C" locale to get the locale-agnostic error messages */ +#define _C_LOCALE (locale_t) 0 + typedef enum { BD_UTILS_DEV_UTILS_ERROR_FAILED, } BDUtilsDevUtilsError; diff --git a/tests/dm_test.py b/tests/dm_test.py index 0ef3f7bf..00da4501 100644 --- a/tests/dm_test.py +++ b/tests/dm_test.py @@ -153,6 +153,12 @@ def test_name_node_bijection(self): self.assertEqual(BlockDev.dm_name_from_node(BlockDev.dm_node_from_name("testMap")), "testMap") + with self.assertRaisesRegex(GLib.GError, "No DM name specified"): + BlockDev.dm_node_from_name("") + + with self.assertRaisesRegex(GLib.GError, "No DM node specified"): + BlockDev.dm_name_from_node("") + class DMNoStorageTest(DevMapperTest): @tag_test(TestTags.NOSTORAGE) diff --git a/tests/utils_test.py b/tests/utils_test.py index 12f4abc5..b1756aee 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -406,57 +406,74 @@ def test_exec_large_input(self): self.assertTrue(status) -class UtilsDevUtilsTestCase(UtilsTestCase): - @tag_test(TestTags.NOSTORAGE, TestTags.CORE) +class UtilsStorageTestCase(UtilsTestCase): + def setUp(self): + self.addCleanup(self._clean_up) + self.dev_file = create_sparse_tempfile("utils_test", 1024**3) + try: + self.loop_dev = create_lio_device(self.dev_file) + except RuntimeError as e: + raise RuntimeError("Failed to setup loop device for testing: %s" % e) + + def _clean_up(self): + try: + delete_lio_device(self.loop_dev) + except RuntimeError: + # just move on, we can do no better here + pass + os.unlink(self.dev_file) + + def _setup_lvm(self): + ret, _out, _err = run_command ("pvcreate %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev) + self.assertEqual(ret, 0) + self.addCleanup(run_command, "pvremove %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev) + + ret, _out, _err = run_command ("vgcreate utilsTestVG %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev) + self.assertEqual(ret, 0) + self.addCleanup(run_command, "vgremove -y utilsTestVG --config \"devices {use_devicesfile = 0}\"") + + ret, _out, _err = run_command ("lvcreate -n utilsTestLV -L 12M utilsTestVG --config \"devices {use_devicesfile = 0}\"") + self.assertEqual(ret, 0) + self.addCleanup(run_command, "lvremove -y utilsTestVG/utilsTestLV --config \"devices {use_devicesfile = 0}\"") + + +class UtilsDevUtilsTestCase(UtilsStorageTestCase): + @tag_test(TestTags.CORE) def test_resolve_device(self): """Verify that resolving device spec works as expected""" with self.assertRaises(GLib.GError): BlockDev.utils_resolve_device("no_such_device") - dev = "/dev/libblockdev-test-dev" - with open(dev, "w"): - pass - self.addCleanup(os.unlink, dev) + self._setup_lvm() + dev_link = "/dev/mapper/utilsTestVG-utilsTestLV" + dev_link2 = "/dev/utilsTestVG/utilsTestLV" + dev_node = os.path.realpath(dev_link) # full path, no symlink, should just return the same - self.assertEqual(BlockDev.utils_resolve_device(dev), dev) + self.assertEqual(BlockDev.utils_resolve_device(dev_node), dev_node) # just the name of the device, should return the full path - self.assertEqual(BlockDev.utils_resolve_device(dev[5:]), dev) - - dev_dir = "/dev/libblockdev-test-dir" - os.mkdir(dev_dir) - self.addCleanup(os.rmdir, dev_dir) - - dev_link = dev_dir + "/test-dev-link" - os.symlink("../" + dev[5:], dev_link) - self.addCleanup(os.unlink, dev_link) + self.assertEqual(BlockDev.utils_resolve_device(dev_node[5:]), dev_node) # should resolve the symlink - self.assertEqual(BlockDev.utils_resolve_device(dev_link), dev) + self.assertEqual(BlockDev.utils_resolve_device(dev_link), dev_node) # should resolve the symlink even without the "/dev" prefix - self.assertEqual(BlockDev.utils_resolve_device(dev_link[5:]), dev) + self.assertEqual(BlockDev.utils_resolve_device(dev_link[5:]), dev_node) + # should resolve the other symlink too + self.assertEqual(BlockDev.utils_resolve_device(dev_link2), dev_node) -class UtilsDevUtilsSymlinksTestCase(UtilsTestCase): - def setUp(self): - self.addCleanup(self._clean_up) - self.dev_file = create_sparse_tempfile("lvm_test", 1024**3) - try: - self.loop_dev = create_lio_device(self.dev_file) - except RuntimeError as e: - raise RuntimeError("Failed to setup loop device for testing: %s" % e) + # should resolve the other symlink even without the "/dev" prefix + self.assertEqual(BlockDev.utils_resolve_device(dev_link2[5:]), dev_node) - def _clean_up(self): - try: - delete_lio_device(self.loop_dev) - except RuntimeError: - # just move on, we can do no better here - pass - os.unlink(self.dev_file) + # vg is not a block device, shouldn't be resolved + with self.assertRaisesRegex(GLib.GError, "not a block device"): + BlockDev.utils_resolve_device("/dev/utilsTestVG") + +class UtilsDevUtilsSymlinksTestCase(UtilsStorageTestCase): @tag_test(TestTags.CORE) def test_get_device_symlinks(self): """Verify that getting device symlinks works as expected""" @@ -472,17 +489,7 @@ def test_get_device_symlinks(self): self.assertGreaterEqual(len(symlinks), 2) # create an LV to get a device with more symlinks - ret, _out, _err = run_command ("pvcreate %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev) - self.assertEqual(ret, 0) - self.addCleanup(run_command, "pvremove %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev) - - ret, _out, _err = run_command ("vgcreate utilsTestVG %s --config \"devices {use_devicesfile = 0}\"" % self.loop_dev) - self.assertEqual(ret, 0) - self.addCleanup(run_command, "vgremove -y utilsTestVG --config \"devices {use_devicesfile = 0}\"") - - ret, _out, _err = run_command ("lvcreate -n utilsTestLV -L 12M utilsTestVG --config \"devices {use_devicesfile = 0}\"") - self.assertEqual(ret, 0) - self.addCleanup(run_command, "lvremove -y utilsTestVG/utilsTestLV --config \"devices {use_devicesfile = 0}\"") + self._setup_lvm() symlinks = BlockDev.utils_get_device_symlinks("utilsTestVG/utilsTestLV") # there should be at least 4 symlinks for an LV