From cc4687a2aa38c9c5f541473ec7ce89721fe42285 Mon Sep 17 00:00:00 2001 From: Mike Fleetwood Date: Sat, 13 May 2023 13:43:50 +0100 Subject: [PATCH] Also check links to block devices when skipping BlockSpecial unit tests (!113) Fragment of a failed CI test job from a GiLab job runner which didn't allow creation of block special devices looked like: $ tests/makedev.sh mknod -m 0660 /dev/sda b 8 0 mknod: '/dev/sda': Operation not permitted chown: cannot access '/dev/sda': No such file or directory mknod -m 0660 /dev/sda1 b 8 1 mknod: '/dev/sda1': Operation not permitted chown: cannot access '/dev/sda1': No such file or directory mkdir: created directory '/dev/disk' mkdir: created directory '/dev/disk/by-id/' '/dev/disk/by-id/gparted-sda' -> '/dev/sda' test/makedev.sh attempted to create two block devices it wanted for testing, but that failed with "Operation not permitted". It then created dangling symbolic link /dev/disk/by-id/gparted-sda -> /dev/sda; gparted-sda pointed to a name which didn't exist. Despite the previous commit testing and skipping every test where the block device doesn't exist this unit test still failed: [ RUN ] BlockSpecialTest.NamedBlockSpecialObjectBySymlinkMatches test_BlockSpecial.cc:186: Failure Failed follow_link_name(): Failed to resolve symbolic link '/dev/disk/by-id/gparted-sda' test_BlockSpecial.cc:271: Skip test. Block device '' does not exist [ FAILED ] BlockSpecialTest.NamedBlockSpecialObjectBySymlinkMatches (0 ms) The unit test called get_link_name() which read the directory /dev/disk/by-id and found symbolic link gparted-sda. It then called follow_link_name() passing /dev/disk/by-id/gparted-sda which used realpath(3) to get the canonicalised absolute pathname, which includes following links. But as gparted-sda pointed to a non-existent file it failed and reported message "Failed to resolve symbolic link ...". Then after that the unit test skips the non-existent block device, but the test has already failed at that point. Fix the unit test by also checking the symbolic link points to an existing block device before calling follow_link_name() on it. This works because SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST() uses stat(3), which follows symbolic links, in it's verification. Also put SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST() immediately after each initialisation of a block device name for some sort of consistency with it's need in this fixed NamedBlockSpecialObjectBySymlinkMatches unit test. Closed !113 - Fix occasional GitLab CI test jobs failures on BlockSpecial unit tests --- tests/test_BlockSpecial.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_BlockSpecial.cc b/tests/test_BlockSpecial.cc index d849415a..99680d46 100644 --- a/tests/test_BlockSpecial.cc +++ b/tests/test_BlockSpecial.cc @@ -251,8 +251,8 @@ TEST( BlockSpecialTest, NamedBlockSpecialObjectBlockDeviceDuplicate ) TEST( BlockSpecialTest, TwoNamedBlockSpecialObjectBlockDevices ) { std::string bname1 = get_block_name( 0 ); - std::string bname2 = get_block_name( 1 ); SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST(bname1); + std::string bname2 = get_block_name( 1 ); SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST(bname2); // Test that two different named block devices produce different @@ -267,6 +267,7 @@ TEST( BlockSpecialTest, TwoNamedBlockSpecialObjectBlockDevices ) TEST( BlockSpecialTest, NamedBlockSpecialObjectBySymlinkMatches ) { std::string lname = get_link_name(); + SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST(lname); std::string bname = follow_link_name( lname ); SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST(bname); @@ -366,8 +367,8 @@ TEST( BlockSpecialTest, OperatorEqualsPlainFileAndBlockDevice ) TEST( BlockSpecialTest, OperatorEqualsTwoDifferentBlockDevices ) { std::string bname1 = get_block_name( 0 ); - std::string bname2 = get_block_name( 1 ); SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST(bname1); + std::string bname2 = get_block_name( 1 ); SKIP_IF_BLOCK_DEVICE_DOESNT_EXIST(bname2); // Test inequality of two different named block device BlockSpecial objects.