From 228374fe50d99ff826e5629af3160c59742b7a3c Mon Sep 17 00:00:00 2001 From: Mike Fleetwood Date: Thu, 11 Jul 2019 19:16:09 +0100 Subject: [PATCH] Switch to POSIX open() in detect_filesystem_internal() (!46)(#16) For every partitioned device that GParted scans it triggers a set of udev change events from it's internal file system signature detection function. This is the sequence of events: gparted |set_devices_thread(pdevices) pdevices->["/dev/sdb"] gparted | ped_device_get("/dev/sdb") libparted | open("/dev/sdb", O_RDONLY) = 8 libparted | close(8) gparted | useable_device(lp_device) lp_device->path="/dev/sdb" gparted | open("/dev/sdb", O_RDONLY|O_NONBLOCK) = 8 gparted | read(8, ...) gparted | close(8) gparted | set_device_from_disk(device, device_path="/dev/sdb") gparted | get_device(device_path="/dev/sdb", ..., flush=true) gparted | ped_device_get() gparted | flush_device(lp_device) lp_device->path="/dev/sdb" gparted | ped_device_open() libparted | open("/dev/sdb", O_RDWR) = 8 gparted | ped_device_sync() gparted | ped_device_close() libparted | close(8) udev(async)| KERNEL remove /devices/.../sdb/sdb1 (block) udev(async)| KERNEL remove /devices/.../sdb/sdb2 (block) udev(async)| KERNEL change /devices/.../sdb (block) udev(async)| KERNEL add /devices/.../sdb/sdb1 (block) udev(async)| KERNEL add /devices/.../sdb/sdb2 (block) udev(async)| UDEV remove /devices/.../sdb/sdb1 (block) udev(async)| UDEV remove /devices/.../sdb/sdb2 (block) udev(async)| UDEV change /devices/.../sdb (block) udev(async)| UDEV add /devices/.../sdb/sdb1 (block) udev(async)| UDEV add /devices/.../sdb/sdb2 (block) gparted | settle_device() gparted | Utils::execute_command("udevadm settle --timeout=1") gparted | detect_filesystem(lp_device, NULL, ...) lp_device->path="/dev/sdb" gparted | detect_filesystem_internal(lp_device, NULL) lp_device->path="/dev/sdb" gparted | ped_device_open() libparted | open("/dev/sdb", O_RDWR) = 8 gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_read() gparted | ped_device_close() libparted | close(8) udev(async)| KERNEL remove /devices/.../sdb/sdb1 (block) udev(async)| KERNEL remove /devices/.../sdb/sdb2 (block) udev(async)| KERNEL change /devices/.../sdb (block) udev(async)| KERNEL add /devices/.../sdb/sdb1 (block) udev(async)| KERNEL add /devices/.../sdb/sdb2 (block) udev(async)| UDEV remove /devices/.../sdb/sdb1 (block) udev(async)| UDEV remove /devices/.../sdb/sdb2 (block) udev(async)| UDEV change /devices/.../sdb (block) udev(async)| UDEV add /devices/.../sdb/sdb1 (block) udev(async)| UDEV add /devices/.../sdb/sdb2 (block) gparted | get_disk(lp_device, lp_disk, strict=false) gparted | ped_disk_new(lp_device) lp_device->path="/dev/sdb" libparted | open("/dev/sdb", O_RDWR) = 8 libparted | close(8) udev(async)| KERNEL remove /devices/.../sdb/sdb1 (block) udev(async)| KERNEL remove /devices/.../sdb/sdb2 (block) udev(async)| KERNEL change /devices/.../sdb (block) udev(async)| KERNEL add /devices/.../sdb/sdb1 (block) udev(async)| KERNEL add /devices/.../sdb/sdb2 (block) udev(async)| UDEV remove /devices/.../sdb/sdb1 (block) udev(async)| UDEV remove /devices/.../sdb/sdb2 (block) udev(async)| UDEV change /devices/.../sdb (block) udev(async)| UDEV add /devices/.../sdb/sdb1 (block) udev(async)| UDEV add /devices/.../sdb/sdb2 (block) gparted | settle_device() gparted | Utils::execute_command("udevadm settle --timeout=1") gparted | set_device_partitions() gparted | detect_filesystem() gparted | set_partition_label_and_uuid(partition) partition.get_path()="/dev/sdb1" gparted | read_label() gparted | ext2::read_label() gparted | Utils::execute_command("e2label /dev/sdb1") Note that the udev block device change events triggered in detect_filesystem_internal() occur before the waited for ones in the second commit "Wait for udev to recreate /dev/PTN entries when querying partition FSs (!46)" in get_disk() so these were already waited for. The call chain is: set_devices_thread() set_device_from_disk() detect_filesystem() detect_filesystem_internal() ped_device_open() ped_device_read() ped_device_close() This occurs because file system detection is performed on whole disk devices, but the device contains a partition table, so blkid won't report any content GParted accepts as a file system, so it tries it's own internal detection. Fix this by changing detect_filesystem_internal() to use POSIX open(), read() and close() so the file can be opened read-only and udev change events aren't triggered. Note that when using detect_filesystem_internal() on a partition this also changes the device it reads from. Before it used libparted which always reads from the whole disk device, now it reads from the partition device. This doesn't matter because GParted ensures the data is consistent between them [2] and blkid reads from each partition device which GParted already uses. As the new code avoids using the libparted API, and just skips to the next signature on lseek() and read() errors, it therefore won't generate libparted exceptions such as this when scanning very small drives: Invalid argument during seek for read on /dev/PTN [1] f8faee637787329c07771e495c9b26abc9ac1603 Avoid whole disk FAT being detected as MSDOS partition table (#743181) [2] 3bea067596e3e2d6513cda2a66df1b3e4fa432fb Flush devices when scanning to prevent reading stale signatures (#723842) Closes !46 - Whole device FAT32 file system reports device busy warning from mlabel Closes #16 - "invalid argument for seek()" error on very small (<=40KiB) drives --- include/GParted_Core.h | 2 +- src/GParted_Core.cc | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/include/GParted_Core.h b/include/GParted_Core.h index 736e315a..b9b12ad8 100644 --- a/include/GParted_Core.h +++ b/include/GParted_Core.h @@ -89,7 +89,7 @@ private: std::vector & messages ); void set_luks_partition( PartitionLUKS & partition ); void set_partition_label_and_uuid( Partition & partition ); - static FSType detect_filesystem_internal( PedDevice * lp_device, PedPartition * lp_partition ); + static FSType detect_filesystem_internal(const Glib::ustring& path, const PedDevice* lp_device); static FSType detect_filesystem( PedDevice * lp_device, PedPartition * lp_partition, std::vector & messages ); void read_label( Partition & partition ) ; diff --git a/src/GParted_Core.cc b/src/GParted_Core.cc index 5c0d223a..d5ada63f 100644 --- a/src/GParted_Core.cc +++ b/src/GParted_Core.cc @@ -1116,9 +1116,10 @@ void GParted_Core::set_partition_label_and_uuid( Partition & partition ) } } + // GParted simple internal file system signature detection. Use sparingly. Only when // (old versions of) blkid and libparted don't recognise a signature. -FSType GParted_Core::detect_filesystem_internal( PedDevice * lp_device, PedPartition * lp_partition ) +FSType GParted_Core::detect_filesystem_internal(const Glib::ustring& path, const PedDevice* lp_device) { char magic1[16]; // Big enough for largest signatures[].sig1 or sig2 char magic2[16]; @@ -1128,7 +1129,8 @@ FSType GParted_Core::detect_filesystem_internal( PedDevice * lp_device, PedParti if ( ! buf ) return FS_UNKNOWN; - if ( ! ped_device_open( lp_device ) ) + int fd = open(path.c_str(), O_RDONLY|O_NONBLOCK); + if (fd == -1) { free( buf ); return FS_UNKNOWN; @@ -1189,13 +1191,11 @@ FSType GParted_Core::detect_filesystem_internal( PedDevice * lp_device, PedParti if ( len1 == 0UL || ( signatures[i].sig2 != NULL && len2 == 0UL ) ) continue; // Don't allow 0 length signatures to match - Sector start = 0LL; - if ( lp_partition ) - start = lp_partition->geom.start; - start += signatures[i].offset1 / lp_device->sector_size; + Byte_Value read_offset = signatures[i].offset1 / lp_device->sector_size * lp_device->sector_size; memset( buf, 0, lp_device->sector_size ); - if ( ped_device_read( lp_device, buf, start, 1 ) != 0 ) + if (lseek(fd, read_offset, SEEK_SET) == read_offset && + read(fd, buf, lp_device->sector_size) == lp_device->sector_size ) { memcpy( magic1, buf + signatures[i].offset1 % lp_device->sector_size, len1 ); @@ -1215,7 +1215,7 @@ FSType GParted_Core::detect_filesystem_internal( PedDevice * lp_device, PedParti } } - ped_device_close( lp_device ); + close(fd); free( buf ); return fstype; @@ -1321,7 +1321,7 @@ FSType GParted_Core::detect_filesystem( PedDevice * lp_device, PedPartition * lp } // (Q4) Fallback to GParted simple internal file system detection - FSType fstype = detect_filesystem_internal( lp_device, lp_partition ); + FSType fstype = detect_filesystem_internal(path, lp_device); if ( fstype != FS_UNKNOWN ) return fstype;