From 935e37747c134566d955e98db2c80035e76ae5fc Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Sat, 8 Jan 2022 22:18:24 +0100 Subject: [PATCH 1/7] udiskslinuxblockobject: Add udisks_linux_block_object_acquire_bsd_lock() Advisory locking of block devices undergoing major changes is getting increasingly useful and subject for extended use over the daemon. See this great writeup: https://systemd.io/BLOCK_DEVICE_LOCKING/ --- doc/udisks2-sections.txt.daemon.sections.in | 1 + src/udiskslinuxblockobject.c | 76 +++++++++++++++------ src/udiskslinuxblockobject.h | 4 ++ 3 files changed, 61 insertions(+), 20 deletions(-) diff --git a/doc/udisks2-sections.txt.daemon.sections.in b/doc/udisks2-sections.txt.daemon.sections.in index f35a104f2d..60dc488d3a 100644 --- a/doc/udisks2-sections.txt.daemon.sections.in +++ b/doc/udisks2-sections.txt.daemon.sections.in @@ -334,6 +334,7 @@ udisks_linux_block_object_contains_filesystem udisks_linux_block_object_lock_for_cleanup udisks_linux_block_object_try_lock_for_cleanup udisks_linux_block_object_release_cleanup_lock +udisks_linux_block_object_acquire_bsd_lock UDISKS_TYPE_LINUX_BLOCK_OBJECT UDISKS_LINUX_BLOCK_OBJECT diff --git a/src/udiskslinuxblockobject.c b/src/udiskslinuxblockobject.c index 18f9105c51..4ed5c64de8 100644 --- a/src/udiskslinuxblockobject.c +++ b/src/udiskslinuxblockobject.c @@ -1077,13 +1077,58 @@ udisks_linux_block_object_trigger_uevent_sync (UDisksLinuxBlockObject *object, gboolean udisks_linux_block_object_reread_partition_table (UDisksLinuxBlockObject *object, GError **error) +{ + int fd; + gboolean ret = TRUE; + + g_return_val_if_fail (UDISKS_IS_LINUX_BLOCK_OBJECT (object), FALSE); + g_warn_if_fail (!error || !*error); + + fd = udisks_linux_block_object_acquire_bsd_lock (object, TRUE, error); + if (fd < 0) + return FALSE; + + if (ioctl (fd, BLKRRPART) != 0) + { + gchar *device_file; + + device_file = udisks_linux_block_object_get_device_file (object); + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errno), + "Error re-reading partition table (BLKRRPART ioctl) on %s: %m", + device_file); + g_free (device_file); + ret = FALSE; + } + + close (fd); + + return ret; +} + +/* ---------------------------------------------------------------------------------------------------- */ + +/** + * udisks_linux_block_object_acquire_bsd_lock: + * @object: A #UDisksLinuxBlockObject. + * @exclusive: %TRUE for an exclusive lock, %FALSE for a shared one. + * @error: Return location for error. + * + * Attempts to acquire exclusive or shared BSD lock. Makes five attempts + * with 100ms delay when busy. See https://systemd.io/BLOCK_DEVICE_LOCKING/ + * for further details. + * + * Returns: File descriptor number of an open block device or %-1 on failure + * with @error set. Release the lock with close(). + */ +int +udisks_linux_block_object_acquire_bsd_lock (UDisksLinuxBlockObject *object, gboolean exclusive, GError **error) { UDisksLinuxDevice *device; const gchar *device_file; gint fd; - gboolean ret = TRUE; + gint num_tries = 0; - g_return_val_if_fail (UDISKS_IS_LINUX_BLOCK_OBJECT (object), FALSE); + g_return_val_if_fail (UDISKS_IS_LINUX_BLOCK_OBJECT (object), -1); g_warn_if_fail (!error || !*error); device = udisks_linux_block_object_get_device (object); @@ -1092,34 +1137,25 @@ udisks_linux_block_object_reread_partition_table (UDisksLinuxBlockObject *objec if (fd == -1) { g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errno), - "Error opening %s while re-reading partition table: %m", device_file); - ret = FALSE; + "Error opening %s while acquiring a BSD lock: %m", device_file); } else { - gint num_tries = 0; - - /* acquire an exclusive BSD lock to prevent udev probes. - * See also https://systemd.io/BLOCK_DEVICE_LOCKING - */ - while (flock (fd, LOCK_EX | LOCK_NB) != 0) + while (flock (fd, (exclusive ? LOCK_EX : LOCK_SH) | LOCK_NB) != 0) { g_usleep (100 * 1000); /* microseconds */ if (num_tries++ > 5) - break; - } - - if (ioctl (fd, BLKRRPART) != 0) - { - g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errno), - "Error re-reading partition table (BLKRRPART ioctl) on %s: %m", device_file); - ret = FALSE; + { + udisks_debug ("Failed to acquire %s BSD lock on %s: %m", + exclusive ? "exclusive" : "shared", + device_file); + break; + } } - close (fd); } g_object_unref (device); - return ret; + return fd; } /* ---------------------------------------------------------------------------------------------------- */ diff --git a/src/udiskslinuxblockobject.h b/src/udiskslinuxblockobject.h index 5c96df6b99..852c4b678a 100644 --- a/src/udiskslinuxblockobject.h +++ b/src/udiskslinuxblockobject.h @@ -52,6 +52,10 @@ void udisks_linux_block_object_lock_for_cleanup (UDisks gboolean udisks_linux_block_object_try_lock_for_cleanup (UDisksLinuxBlockObject *object); void udisks_linux_block_object_release_cleanup_lock (UDisksLinuxBlockObject *object); +int udisks_linux_block_object_acquire_bsd_lock (UDisksLinuxBlockObject *object, + gboolean exclusive, + GError **error); + G_END_DECLS #endif /* __UDISKS_LINUX_BLOCK_OBJECT_H__ */ From cb82ee8e78c7bb278aaf3760ff64f89700c4378b Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Sat, 8 Jan 2022 22:26:13 +0100 Subject: [PATCH 2/7] udiskslinuxpartitiontable: Rework block device locking Nowadays with synthetic uevent tagging in use partitioning can become more predictable. The right approach should be to acquire an advisory lock, make changes, unlock and trigger re-read. Changing the shared lock to exclusive seems to make a big difference and the underlying libblockdev-3 partitioning copes with it just fine. --- src/udiskslinuxpartitiontable.c | 142 ++++++++++++-------------------- 1 file changed, 51 insertions(+), 91 deletions(-) diff --git a/src/udiskslinuxpartitiontable.c b/src/udiskslinuxpartitiontable.c index 84ef197208..4889c4bdad 100644 --- a/src/udiskslinuxpartitiontable.c +++ b/src/udiskslinuxpartitiontable.c @@ -248,13 +248,13 @@ wait_for_partition (UDisksDaemon *daemon, #define MIB_SIZE (1048576L) static UDisksObject * -udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *table, - GDBusMethodInvocation *invocation, - guint64 offset, - guint64 size, - const gchar *type, - const gchar *name, - GVariant *options) +create_partition (UDisksPartitionTable *table, + GDBusMethodInvocation *invocation, + guint64 offset, + guint64 size, + const gchar *type, + const gchar *name, + GVariant *options) { const gchar *action_id = NULL; const gchar *message = NULL; @@ -273,6 +273,7 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta GError *error = NULL; UDisksBaseJob *job = NULL; const gchar *partition_type = NULL; + int fd = -1; object = udisks_daemon_util_dup_object (table, &error); if (object == NULL) @@ -331,10 +332,7 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta invocation)) goto out; - device_name = g_strdup (udisks_block_get_device (block)); - table_type = udisks_partition_table_dup_type_ (table); - wait_data = g_new0 (WaitForPartitionData, 1); if (g_strcmp0 (table_type, "dos") == 0) { char *endp; @@ -405,6 +403,16 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta goto out; } + /* Lock the device to prevent udev issuing a BLKRRPART ioctl + * call until we're done with changes. + */ + fd = udisks_linux_block_object_acquire_bsd_lock (UDISKS_LINUX_BLOCK_OBJECT (object), TRUE, &error); + if (fd < 0) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + /* Users might want to specify logical partitions start and size using size of * of the extended partition. If this happens we need to shift start (offset) * of the logical partition. @@ -414,6 +422,7 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta * use case. But we should definitely provide some functionality to get * right "numbers" and stop doing this. */ + device_name = g_strdup (udisks_block_get_device (block)); overlapping_part = bd_part_get_part_by_pos (device_name, offset, &error); if (overlapping_part != NULL && ! (overlapping_part->type & BD_PART_TYPE_FREESPACE)) { @@ -450,7 +459,7 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta UDISKS_ERROR, UDISKS_ERROR_FAILED, "Error creating partition on %s: %s", - udisks_block_get_device (block), + device_name, error->message); udisks_simple_job_complete (UDISKS_SIMPLE_JOB (job), FALSE, error->message); goto out; @@ -508,12 +517,27 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta } } - wait_data->ignore_container = (part_spec->type == BD_PART_TYPE_LOGICAL); - wait_data->pos_to_wait_for = part_spec->start + (part_spec->size / 2L); + /* hard work done, unlock and refresh */ + if (fd >= 0) + { + close (fd); + fd = -1; + } + + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (object), &error)) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object), + UDISKS_DEFAULT_WAIT_TIMEOUT); /* sit and wait for the partition to show up */ - g_warn_if_fail (wait_data->pos_to_wait_for > 0); + wait_data = g_new0 (WaitForPartitionData, 1); wait_data->partition_table_object = object; + wait_data->ignore_container = (part_spec->type == BD_PART_TYPE_LOGICAL); + wait_data->pos_to_wait_for = part_spec->start + (part_spec->size / 2L); + g_warn_if_fail (wait_data->pos_to_wait_for > 0); partition_object = udisks_daemon_wait_for_object_sync (daemon, wait_for_partition, wait_data, @@ -537,16 +561,11 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta goto out; } - /* Trigger uevent on the disk -- we sometimes get add-remove-add uevents for - the new partition without getting change event for the disks after the - last add event and this breaks the "Partitions" property on the - "PartitionTable" interface. */ - udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object), - UDISKS_DEFAULT_WAIT_TIMEOUT); - udisks_simple_job_complete (UDISKS_SIMPLE_JOB (job), TRUE, NULL); out: + if (fd >= 0) + close (fd); g_free (table_type); g_free (wait_data); g_clear_error (&error); @@ -561,27 +580,6 @@ udisks_linux_partition_table_handle_create_partition (UDisksPartitionTable *ta return partition_object; } -static int -flock_block_dev (UDisksPartitionTable *iface) -{ - UDisksObject *object = udisks_daemon_util_dup_object (iface, NULL); - UDisksBlock *block = object? udisks_object_peek_block (object) : NULL; - int fd = block? open (udisks_block_get_device (block), O_RDONLY) : -1; - - if (fd >= 0) - flock (fd, LOCK_SH | LOCK_NB); - - g_clear_object (&object); - return fd; -} - -static void -unflock_block_dev (int fd) -{ - if (fd >= 0) - close (fd); -} - /* runs in thread dedicated to handling @invocation */ static gboolean handle_create_partition (UDisksPartitionTable *table, @@ -592,42 +590,17 @@ handle_create_partition (UDisksPartitionTable *table, const gchar *name, GVariant *options) { - /* We (try to) take a shared lock on the partition table while - creating and formatting a new partition, here and also in - handle_create_partition_and_format. - - This lock prevents udevd from issuing a BLKRRPART ioctl call. - That ioctl is undesired because it might temporarily remove the - block device of the newly created block device. It does so only - temporarily, but it still happens that the block device is - missing exactly when wipefs or mkfs try to access it. - - Also, a pair of remove/add events will cause udisks to create a - new internal UDisksObject to represent the block device of the - partition. The code currently doesn't handle this and waits for - changes (such as an expected filesystem type or UUID) to a - obsolete internal object that will never see them. - */ - - int fd = flock_block_dev (table); - UDisksObject *partition_object = - udisks_linux_partition_table_handle_create_partition (table, - invocation, - offset, - size, - type, - name, - options); + UDisksObject *partition_object; + partition_object = create_partition (table, invocation, offset, size, type, name, options); if (partition_object) { - udisks_partition_table_complete_create_partition - (table, invocation, g_dbus_object_get_object_path (G_DBUS_OBJECT (partition_object))); + udisks_partition_table_complete_create_partition (table, + invocation, + g_dbus_object_get_object_path (G_DBUS_OBJECT (partition_object))); g_object_unref (partition_object); } - unflock_block_dev (fd); - return TRUE; /* returning TRUE means that we handled the method invocation */ } @@ -636,16 +609,16 @@ struct FormatCompleteData { UDisksPartitionTable *table; GDBusMethodInvocation *invocation; UDisksObject *partition_object; - int lock_fd; }; static void handle_format_complete (gpointer user_data) { struct FormatCompleteData *data = user_data; - udisks_partition_table_complete_create_partition - (data->table, data->invocation, g_dbus_object_get_object_path (G_DBUS_OBJECT (data->partition_object))); - unflock_block_dev (data->lock_fd); + + udisks_partition_table_complete_create_partition (data->table, + data->invocation, + g_dbus_object_get_object_path (G_DBUS_OBJECT (data->partition_object))); } static gboolean @@ -659,26 +632,15 @@ handle_create_partition_and_format (UDisksPartitionTable *table, const gchar *format_type, GVariant *format_options) { - /* See handle_create_partition for a motivation of taking the lock. - */ - - int fd = flock_block_dev (table); - UDisksObject *partition_object = - udisks_linux_partition_table_handle_create_partition (table, - invocation, - offset, - size, - type, - name, - options); + UDisksObject *partition_object; + partition_object = create_partition (table, invocation, offset, size, type, name, options); if (partition_object) { struct FormatCompleteData data; data.table = table; data.invocation = invocation; data.partition_object = partition_object; - data.lock_fd = fd; udisks_linux_block_handle_format (udisks_object_peek_block (partition_object), invocation, format_type, @@ -686,8 +648,6 @@ handle_create_partition_and_format (UDisksPartitionTable *table, handle_format_complete, &data); g_object_unref (partition_object); } - else - unflock_block_dev (fd); return TRUE; /* returning TRUE means that we handled the method invocation */ } From f73e42f08d28bead57f8cdac711fb822cc124558 Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Sat, 8 Jan 2022 22:35:13 +0100 Subject: [PATCH 3/7] udiskslinuxpartitiontable: Add state cleanup locking Rather advisory this alerts the state cleanup thread to avoid touching this device. --- src/udiskslinuxpartitiontable.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/udiskslinuxpartitiontable.c b/src/udiskslinuxpartitiontable.c index 4889c4bdad..57dccf7c81 100644 --- a/src/udiskslinuxpartitiontable.c +++ b/src/udiskslinuxpartitiontable.c @@ -43,6 +43,7 @@ #include "udiskslinuxblock.h" #include "udiskslinuxpartition.h" #include "udiskssimplejob.h" +#include "udisksstate.h" /** * SECTION:udiskslinuxpartitiontable @@ -261,6 +262,7 @@ create_partition (UDisksPartitionTable *table, UDisksBlock *block = NULL; UDisksObject *object = NULL; UDisksDaemon *daemon = NULL; + UDisksState *state = NULL; gchar *device_name = NULL; WaitForPartitionData *wait_data = NULL; UDisksObject *partition_object = NULL; @@ -283,6 +285,10 @@ create_partition (UDisksPartitionTable *table, } daemon = udisks_linux_block_object_get_daemon (UDISKS_LINUX_BLOCK_OBJECT (object)); + state = udisks_daemon_get_state (daemon); + + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); g_variant_lookup (options, "partition-type", "&s", &partition_type); @@ -566,6 +572,10 @@ create_partition (UDisksPartitionTable *table, out: if (fd >= 0) close (fd); + if (object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (object)); + if (state != NULL) + udisks_state_check (state); g_free (table_type); g_free (wait_data); g_clear_error (&error); From 6c4354ab488f97bddeb55fc5006035805796dfc7 Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Sun, 9 Jan 2022 13:47:31 +0100 Subject: [PATCH 4/7] udiskslinuxpartition: Rework device locking Advisory BSD lock is now held on a partition table block device and extra BLKRRPART ioctl is issued on it afterwards. --- src/udiskslinuxblock.c | 1 + src/udiskslinuxpartition.c | 193 +++++++++++++++++++++++++++++++------ src/udiskslinuxpartition.h | 1 + 3 files changed, 164 insertions(+), 31 deletions(-) diff --git a/src/udiskslinuxblock.c b/src/udiskslinuxblock.c index ce84d10b55..17d2fc5e84 100644 --- a/src/udiskslinuxblock.c +++ b/src/udiskslinuxblock.c @@ -3230,6 +3230,7 @@ format_set_partition_type (UDisksPartition *partition, { if (!udisks_linux_partition_set_type_sync (UDISKS_LINUX_PARTITION (partition), partition_type, + TRUE, /* lock_held */ caller_uid, NULL, /* cancellable */ error)) diff --git a/src/udiskslinuxpartition.c b/src/udiskslinuxpartition.c index 5fe803a6d2..9c7e038c0c 100644 --- a/src/udiskslinuxpartition.c +++ b/src/udiskslinuxpartition.c @@ -334,7 +334,7 @@ handle_set_flags (UDisksPartition *partition, UDisksObject *partition_table_object = NULL; UDisksPartitionTable *partition_table = NULL; UDisksBlock *partition_table_block = NULL; - gint fd = -1; + int fd = -1; uid_t caller_uid; gchar *partition_name = NULL; gboolean bootable = FALSE; @@ -363,10 +363,9 @@ handle_set_flags (UDisksPartition *partition, partition_name = udisks_block_dup_device (block); udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); - - /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ - fd = open (partition_name, O_RDONLY); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object))); job = udisks_daemon_launch_simple_job (daemon, UDISKS_OBJECT (object), @@ -381,6 +380,14 @@ handle_set_flags (UDisksPartition *partition, goto out; } + /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ + fd = udisks_linux_block_object_acquire_bsd_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), TRUE, &error); + if (fd < 0) + { + udisks_debug ("%s", error->message); + g_clear_error (&error); + } + if (g_strcmp0 (udisks_partition_table_get_type_ (partition_table), "gpt") == 0) { if (!bd_part_set_part_attributes (device_name, @@ -429,6 +436,19 @@ handle_set_flags (UDisksPartition *partition, goto out; } + /* unlock and re-read */ + if (fd >= 0) + { + close (fd); + fd = -1; + } + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), &error)) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), + UDISKS_DEFAULT_WAIT_TIMEOUT); udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object), UDISKS_DEFAULT_WAIT_TIMEOUT); udisks_partition_complete_set_flags (partition, invocation); @@ -437,6 +457,8 @@ handle_set_flags (UDisksPartition *partition, out: if (fd != -1) close (fd); + if (partition_table_object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); if (object != NULL) udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (object)); if (state != NULL) @@ -472,7 +494,7 @@ handle_set_name (UDisksPartition *partition, UDisksObject *partition_table_object = NULL; UDisksPartitionTable *partition_table = NULL; UDisksBlock *partition_table_block = NULL; - gint fd = -1; + int fd = -1; uid_t caller_uid; GError *error = NULL; UDisksBaseJob *job = NULL; @@ -492,10 +514,6 @@ handle_set_name (UDisksPartition *partition, daemon = udisks_linux_block_object_get_daemon (UDISKS_LINUX_BLOCK_OBJECT (object)); state = udisks_daemon_get_state (daemon); block = udisks_object_get_block (object); - - udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); - udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); - partition_table_object = udisks_daemon_find_object (daemon, udisks_partition_get_table (partition)); partition_table = udisks_object_get_partition_table (partition_table_object); partition_table_block = udisks_object_get_block (partition_table_object); @@ -503,8 +521,10 @@ handle_set_name (UDisksPartition *partition, device_name = udisks_block_dup_device (partition_table_block); partition_name = udisks_block_dup_device (block); - /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ - fd = open (partition_name, O_RDONLY); + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object))); job = udisks_daemon_launch_simple_job (daemon, UDISKS_OBJECT (object), @@ -519,6 +539,14 @@ handle_set_name (UDisksPartition *partition, goto out; } + /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ + fd = udisks_linux_block_object_acquire_bsd_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), TRUE, &error); + if (fd < 0) + { + udisks_debug ("%s", error->message); + g_clear_error (&error); + } + if (g_strcmp0 (udisks_partition_table_get_type_ (partition_table), "gpt") == 0) { if (strlen (name) > 36) @@ -556,6 +584,19 @@ handle_set_name (UDisksPartition *partition, goto out; } + /* unlock and re-read */ + if (fd >= 0) + { + close (fd); + fd = -1; + } + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), &error)) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), + UDISKS_DEFAULT_WAIT_TIMEOUT); udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object), UDISKS_DEFAULT_WAIT_TIMEOUT); udisks_partition_complete_set_name (partition, invocation); @@ -564,6 +605,8 @@ handle_set_name (UDisksPartition *partition, out: if (fd != -1) close (fd); + if (partition_table_object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); if (object != NULL) udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (object)); if (state != NULL) @@ -618,6 +661,7 @@ is_valid_uuid (const gchar *str) * udisks_linux_partition_set_type_sync: * @partition: A #UDisksLinuxPartition. * @type: The partition type to set. + * @lock_held: Whether to avoid acquiring an advisory BSD lock or not (e.g. chained call from Format()). * @caller_uid: The uid of the process requesting this change or 0. * @cancellable: A #GCancellable or %NULL. * @error: Return location for error or %NULL. @@ -630,6 +674,7 @@ is_valid_uuid (const gchar *str) gboolean udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, const gchar *type, + gboolean lock_held, uid_t caller_uid, GCancellable *cancellable, GError **error) @@ -638,11 +683,12 @@ udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, UDisksBlock *block = NULL; UDisksObject *object = NULL; UDisksDaemon *daemon = NULL; + UDisksState *state = NULL; gchar *device_name = NULL; UDisksObject *partition_table_object = NULL; UDisksPartitionTable *partition_table = NULL; UDisksBlock *partition_table_block = NULL; - gint fd = -1; + int fd = -1; gchar *partition_name = NULL; GError *loc_error = NULL; UDisksBaseJob *job = NULL; @@ -652,6 +698,7 @@ udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, goto out; daemon = udisks_linux_block_object_get_daemon (UDISKS_LINUX_BLOCK_OBJECT (object)); + state = udisks_daemon_get_state (daemon); block = udisks_object_get_block (object); partition_table_object = udisks_daemon_find_object (daemon, udisks_partition_get_table (UDISKS_PARTITION (partition))); @@ -661,8 +708,13 @@ udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, device_name = udisks_block_dup_device (partition_table_block); partition_name = udisks_block_dup_device (block); - /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ - fd = open (partition_name, O_RDONLY); + if (!lock_held) + { + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object))); + } job = udisks_daemon_launch_simple_job (daemon, UDISKS_OBJECT (object), @@ -677,6 +729,17 @@ udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, goto out; } + /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ + if (!lock_held) + { + fd = udisks_linux_block_object_acquire_bsd_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), TRUE, &loc_error); + if (fd < 0) + { + udisks_debug ("%s", loc_error->message); + g_clear_error (&loc_error); + } + } + if (g_strcmp0 (udisks_partition_table_get_type_ (partition_table), "gpt") == 0) { /* check that it's a valid GUID */ @@ -750,6 +813,19 @@ udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, goto out; } + /* unlock and re-read */ + if (fd >= 0) + { + close (fd); + fd = -1; + } + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), &loc_error)) + { + udisks_warning ("%s", loc_error->message); + g_clear_error (&loc_error); + } + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), + UDISKS_DEFAULT_WAIT_TIMEOUT); udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object), UDISKS_DEFAULT_WAIT_TIMEOUT); @@ -759,6 +835,15 @@ udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, out: if (fd != -1) close (fd); + if (!lock_held) + { + if (partition_table_object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); + if (object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (object)); + if (state != NULL) + udisks_state_check (state); + } g_free (partition_name); g_free (device_name); g_clear_object (&object); @@ -786,7 +871,7 @@ handle_set_type (UDisksPartition *partition, if (check_authorization (partition, invocation, options, &caller_uid)) { - if (!udisks_linux_partition_set_type_sync (UDISKS_LINUX_PARTITION (partition), type, caller_uid, NULL, &error)) + if (!udisks_linux_partition_set_type_sync (UDISKS_LINUX_PARTITION (partition), type, FALSE, caller_uid, NULL, &error)) { g_dbus_method_invocation_take_error (invocation, error); } @@ -848,7 +933,7 @@ handle_resize (UDisksPartition *partition, UDisksBaseJob *job = NULL; UDisksObject *partition_object = NULL; WaitForPartitionResizeData wait_data; - gint fd = 0; + int fd = -1; const gchar *part = NULL; if (!check_authorization (partition, invocation, options, &caller_uid)) @@ -863,8 +948,6 @@ handle_resize (UDisksPartition *partition, goto out; } - wait_data.object_path = g_dbus_object_get_object_path (G_DBUS_OBJECT (object)); - wait_data.new_size = 0; /* hit timeout in case of error */ daemon = udisks_linux_block_object_get_daemon (UDISKS_LINUX_BLOCK_OBJECT (object)); state = udisks_daemon_get_state (daemon); block = udisks_object_get_block (object); @@ -873,7 +956,9 @@ handle_resize (UDisksPartition *partition, partition_table_block = udisks_object_get_block (partition_table_object); udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object))); job = udisks_daemon_launch_simple_job (daemon, UDISKS_OBJECT (object), @@ -888,6 +973,14 @@ handle_resize (UDisksPartition *partition, goto out; } + /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ + fd = udisks_linux_block_object_acquire_bsd_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), TRUE, &error); + if (fd < 0) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + if (! bd_part_resize_part (udisks_block_get_device (partition_table_block), udisks_block_get_device (block), size, BD_PART_ALIGN_OPTIMAL, &error)) @@ -902,27 +995,34 @@ handle_resize (UDisksPartition *partition, goto out; } - /* Wait for partition property to be updated so that the partition interface - * will not disappear shortly after this method returns. - * Clients could either explicitly wait for an interface or try - * udisks_client_settle() to wait for interfaces to be present. - * If the partition size wasn't changed then there won't be any reappearing - * of the partition node or the interfaces. - */ + /* unlock and re-read */ + if (fd >= 0) + close (fd); + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), &error)) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), + UDISKS_DEFAULT_WAIT_TIMEOUT); + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object), + UDISKS_DEFAULT_WAIT_TIMEOUT); + /* Retrieve actual partition size from the kernel. This needs to be called + * once the fd is closed and BLKRRPART issued, otherwise it would return + * an old value. + */ + wait_data.object_path = g_dbus_object_get_object_path (G_DBUS_OBJECT (object)); + wait_data.new_size = 0; /* hit timeout in case of error */ fd = open (part, O_RDONLY); if (fd != -1) { - if (ioctl (fd, BLKGETSIZE64, &wait_data.new_size) == -1) { + if (ioctl (fd, BLKGETSIZE64, &wait_data.new_size) == -1) udisks_warning ("Could not query new partition size for %s", part); - } - close (fd); } else { udisks_warning ("Could not open %s to query new partition size", part); } - udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object ? partition_table_object : object), - UDISKS_DEFAULT_WAIT_TIMEOUT); partition_object = udisks_daemon_wait_for_object_sync (daemon, wait_for_partition_resize, &wait_data, @@ -934,6 +1034,10 @@ handle_resize (UDisksPartition *partition, udisks_simple_job_complete (UDISKS_SIMPLE_JOB (job), TRUE, NULL); out: + if (fd != -1) + close (fd); + if (partition_table_object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); if (object != NULL) udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (object)); if (state != NULL) @@ -969,6 +1073,7 @@ handle_delete (UDisksPartition *partition, gboolean teardown_flag = FALSE; GError *error = NULL; UDisksBaseJob *job = NULL; + int fd = -1; g_variant_lookup (options, "tear-down", "b", &teardown_flag); @@ -991,7 +1096,9 @@ handle_delete (UDisksPartition *partition, partition_table_block = udisks_object_get_block (partition_table_object); udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (object)); + udisks_linux_block_object_lock_for_cleanup (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (object))); + udisks_state_check_block (state, udisks_linux_block_object_get_device_number (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object))); if (teardown_flag) { @@ -1019,6 +1126,14 @@ handle_delete (UDisksPartition *partition, goto out; } + /* hold a file descriptor open to suppress BLKRRPART generated by the tools */ + fd = udisks_linux_block_object_acquire_bsd_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), TRUE, &error); + if (fd < 0) + { + udisks_debug ("%s", error->message); + g_clear_error (&error); + } + if (! bd_part_delete_part (device_name, partition_name, &error)) { g_dbus_method_invocation_return_error (invocation, @@ -1030,14 +1145,30 @@ handle_delete (UDisksPartition *partition, udisks_simple_job_complete (UDISKS_SIMPLE_JOB (job), FALSE, error->message); goto out; } - /* this is sometimes needed because parted(8) does not generate the uevent itself */ + + /* unlock and re-read */ + if (fd >= 0) + { + close (fd); + fd = -1; + } + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), &error)) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), UDISKS_DEFAULT_WAIT_TIMEOUT); + /* not trigerring uevent on @object, the block device should be gone by now */ udisks_partition_complete_delete (partition, invocation); udisks_simple_job_complete (UDISKS_SIMPLE_JOB (job), TRUE, NULL); out: + if (fd != -1) + close (fd); + if (partition_table_object != NULL) + udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); if (object != NULL) udisks_linux_block_object_release_cleanup_lock (UDISKS_LINUX_BLOCK_OBJECT (object)); if (state != NULL) diff --git a/src/udiskslinuxpartition.h b/src/udiskslinuxpartition.h index 696c91c103..3d369f3c36 100644 --- a/src/udiskslinuxpartition.h +++ b/src/udiskslinuxpartition.h @@ -36,6 +36,7 @@ void udisks_linux_partition_update (UDisksLinuxPartition *partit gboolean udisks_linux_partition_set_type_sync (UDisksLinuxPartition *partition, const gchar *type, + gboolean lock_held, uid_t caller_uid, GCancellable *cancellable, GError **error); From f802ca4c9b1f0a997e33640483262bcedb8fd5c6 Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Sun, 9 Jan 2022 14:35:28 +0100 Subject: [PATCH 5/7] udiskslinuxblock: Avoid re-reading partition table on a partition In case a protective partition table is created by a particular mkfs tool, avoid issuing BLKRRPART on a partition block device and find a parent device that is supposed to hold a partition table. --- src/udiskslinuxblock.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/src/udiskslinuxblock.c b/src/udiskslinuxblock.c index 17d2fc5e84..262b302beb 100644 --- a/src/udiskslinuxblock.c +++ b/src/udiskslinuxblock.c @@ -3576,22 +3576,33 @@ udisks_linux_block_handle_format (UDisksBlock *block, /* The mkfs program may not generate all the uevents we need - so explicitly * trigger an event here */ - if (fs_features && (fs_features->features & BD_FS_FEATURE_PARTITION_TABLE) == BD_FS_FEATURE_PARTITION_TABLE && - !udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (object), &error)) + if (fs_features && (fs_features->features & BD_FS_FEATURE_PARTITION_TABLE) == BD_FS_FEATURE_PARTITION_TABLE) { - udisks_warning ("%s", error->message); - g_clear_error (&error); + UDisksObject *partition_table_object; + + /* If formatting a partition, re-read partition table on a parent device */ + if (partition != NULL) + partition_table_object = udisks_daemon_find_object (daemon, udisks_partition_get_table (partition)); + else + partition_table_object = g_object_ref (object); + + if (partition_table_object != NULL) + { + if (!udisks_linux_block_object_reread_partition_table (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), &error)) + { + udisks_warning ("%s", error->message); + g_clear_error (&error); + } + if (partition_table_object != object) + udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (partition_table_object), + UDISKS_DEFAULT_WAIT_TIMEOUT); + trigger_uevent_on_nested_partitions (daemon, UDISKS_LINUX_BLOCK_OBJECT (partition_table_object)); + g_object_unref (partition_table_object); + } } udisks_linux_block_object_trigger_uevent_sync (UDISKS_LINUX_BLOCK_OBJECT (object_to_mkfs), UDISKS_DEFAULT_WAIT_TIMEOUT); - /* In case a protective partition table was potentially created, trigger uevents - * on all nested partitions. */ - if (fs_features && (fs_features->features & BD_FS_FEATURE_PARTITION_TABLE) == BD_FS_FEATURE_PARTITION_TABLE) - { - trigger_uevent_on_nested_partitions (daemon, UDISKS_LINUX_BLOCK_OBJECT (object)); - } - /* Wait for the desired filesystem interface */ wait_data.object = object_to_mkfs; wait_data.type = type; From 773e030d3be3898435cc33f933c771774e6b625c Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Sat, 8 Jan 2022 22:00:44 +0100 Subject: [PATCH 6/7] tests: Remove udevadm settle from test_60_partitioning Should not be necessary anymore now that we're more predictable. Added proper wipe on cleanup though. --- src/tests/dbus-tests/test_60_partitioning.py | 55 +++++++------------- 1 file changed, 19 insertions(+), 36 deletions(-) diff --git a/src/tests/dbus-tests/test_60_partitioning.py b/src/tests/dbus-tests/test_60_partitioning.py index 5820e25656..f3520aebac 100644 --- a/src/tests/dbus-tests/test_60_partitioning.py +++ b/src/tests/dbus-tests/test_60_partitioning.py @@ -17,9 +17,7 @@ def _remove_format(self, device): try: device.Format('empty', d, dbus_interface=self.iface_prefix + '.Block') except dbus.exceptions.DBusException: - self.udev_settle() - time.sleep(5) - device.Format('empty', d, dbus_interface=self.iface_prefix + '.Block') + pass def _create_format(self, device, ftype): device.Format(ftype, self.no_options, dbus_interface=self.iface_prefix + '.Block') @@ -28,9 +26,7 @@ def _remove_partition(self, part): try: part.Delete(self.no_options, dbus_interface=self.iface_prefix + '.Partition') except dbus.exceptions.DBusException: - self.udev_settle() - time.sleep(5) - part.Delete(self.no_options, dbus_interface=self.iface_prefix + '.Partition') + pass def test_create_mbr_partition(self): disk = self.get_object('/block_devices/' + os.path.basename(self.vdevs[0])) @@ -39,6 +35,7 @@ def test_create_mbr_partition(self): # create msdos partition table self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) pttype = self.get_property(disk, '.PartitionTable', 'Type') pttype.assertEqual('dos') @@ -55,7 +52,6 @@ def test_create_mbr_partition(self): # create partition path1 = disk.CreatePartition(dbus.UInt64(1024**2), dbus.UInt64(100 * 1024**2), part_type, '', self.no_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() part = self.bus.get_object(self.iface_prefix, path1) self.assertIsNotNone(part) @@ -103,7 +99,6 @@ def test_create_mbr_partition(self): # create another partition path2 = disk.CreatePartition(dbus.UInt64(1024**2 + (1024**2 + 100 * 1024**2)), dbus.UInt64(100 * 1024**2), part_type, '', self.no_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() part = self.bus.get_object(self.iface_prefix, path2) self.assertIsNotNone(part) @@ -113,7 +108,6 @@ def test_create_mbr_partition(self): # create yet another partition path3 = disk.CreatePartition(dbus.UInt64(1024**2 + 2 * (1024**2 + 100 * 1024**2)), dbus.UInt64(100 * 1024**2), part_type, '', self.no_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() part = self.bus.get_object(self.iface_prefix, path3) self.assertIsNotNone(part) @@ -136,11 +130,11 @@ def create_extended_partition(self, ext_options, log_options, part_type=''): # create msdos partition table self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) # create extended partition ext_path = disk.CreatePartition(dbus.UInt64(1024**2), dbus.UInt64(150 * 1024**2), part_type, '', ext_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() ext_part = self.bus.get_object(self.iface_prefix, ext_path) self.assertIsNotNone(ext_part) @@ -163,7 +157,6 @@ def create_extended_partition(self, ext_options, log_options, part_type=''): # create logical partition log_path = disk.CreatePartition(dbus.UInt64(1024**2), dbus.UInt64(50 * 1024**2), '', '', log_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() log_part = self.bus.get_object(self.iface_prefix, log_path) self.assertIsNotNone(log_part) @@ -180,7 +173,6 @@ def create_extended_partition(self, ext_options, log_options, part_type=''): # create one more logical partition log_path2 = disk.CreatePartition(dbus.UInt64(dbus_offset.value + dbus_size.value), dbus.UInt64(50 * 1024**2), '', '', log_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() log_part2 = self.bus.get_object(self.iface_prefix, log_path2) self.assertIsNotNone(log_part2) @@ -202,6 +194,7 @@ def test_fill_with_primary_partitions(self): # create msdos partition table self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) options = dbus.Dictionary({'partition-type': 'primary'}, signature='sv') offset = 1024**2 @@ -210,7 +203,6 @@ def test_fill_with_primary_partitions(self): # create primary partition path = disk.CreatePartition(dbus.UInt64(offset + i * (offset + size)), dbus.UInt64(size), '', '', options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() part = self.bus.get_object(self.iface_prefix, path) self.assertIsNotNone(part) @@ -232,6 +224,7 @@ def test_create_gpt_partition(self): pttype.assertEqual('gpt') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) gpt_name = 'home' gpt_type = '933ac7e1-2eb4-4f13-b844-0e14e2aef915' @@ -241,7 +234,6 @@ def test_create_gpt_partition(self): gpt_type, gpt_name, self.no_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() part = self.bus.get_object(self.iface_prefix, path) self.assertIsNotNone(part) @@ -290,6 +282,7 @@ def test_create_with_format(self): self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) # create partition with ext4 format path = disk.CreatePartitionAndFormat(dbus.UInt64(1024**2), dbus.UInt64(100 * 1024**2), '', '', @@ -345,6 +338,7 @@ def test_create_with_format_auto_type_mbr(self): self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) # create partition with udf format and automatically set partition type # it should be 0x07 @@ -376,10 +370,11 @@ def test_create_with_format_auto_type_gpt(self): disk = self.get_object('/block_devices/' + os.path.basename(self.vdevs[0])) self.assertIsNotNone(disk) - # create msdos partition table + # create gpt partition table self._create_format(disk, 'gpt') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) # create partition with udf format and automatically set partition type # it should be ebd0a0a2-b9e5-4433-87c0-68b6b72699c7 @@ -414,9 +409,7 @@ def _remove_format(self, device): try: device.Format('empty', d, dbus_interface=self.iface_prefix + '.Block') except dbus.exceptions.DBusException: - self.udev_settle() - time.sleep(5) - device.Format('empty', d, dbus_interface=self.iface_prefix + '.Block') + pass def _create_format(self, device, ftype): device.Format(ftype, self.no_options, dbus_interface=self.iface_prefix + '.Block') @@ -425,9 +418,7 @@ def _remove_partition(self, part): try: part.Delete(self.no_options, dbus_interface=self.iface_prefix + '.Partition') except dbus.exceptions.DBusException: - self.udev_settle() - time.sleep(5) - part.Delete(self.no_options, dbus_interface=self.iface_prefix + '.Partition') + pass def _create_partition(self, disk, start=1024**2, size=100 * 1024**2, fmt='ext4', type=''): if fmt: @@ -439,7 +430,6 @@ def _create_partition(self, disk, start=1024**2, size=100 * 1024**2, fmt='ext4', self.no_options, dbus_interface=self.iface_prefix + '.PartitionTable') - self.udev_settle() part = self.bus.get_object(self.iface_prefix, path) self.assertIsNotNone(part) @@ -451,14 +441,13 @@ def test_delete(self): self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) part = self._create_partition(disk) path = str(part.object_path) part.Delete(self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() - part_name = path.split('/')[-1] disk_name = os.path.basename(self.vdevs[0]) part_syspath = '/sys/block/%s/%s' % (disk_name, part_name) @@ -481,6 +470,7 @@ def test_dos_flags(self): self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) part = self._create_partition(disk) self.addCleanup(self._remove_partition, part) @@ -491,7 +481,6 @@ def test_dos_flags(self): # set boot flag (10000000(2), 128(10), 0x80(16)) part.SetFlags(dbus.UInt64(128), self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() # test flags value on types dbus_flags = self.get_property(part, '.Partition', 'Flags') @@ -510,6 +499,7 @@ def test_gpt_flags(self): self._create_format(disk, 'gpt') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) part = self._create_partition(disk, fmt=False, type=esp_guid) self.addCleanup(self._remove_partition, part) @@ -531,7 +521,6 @@ def test_gpt_flags(self): # set legacy BIOS bootable flag (100(2), 4(10), 0x4(16)) part.SetFlags(dbus.UInt64(4), self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() # test flags value on types dbus_flags = self.get_property(part, '.Partition', 'Flags') @@ -555,6 +544,7 @@ def test_gpt_type(self): self._create_format(disk, 'gpt') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) part = self._create_partition(disk) self.addCleanup(self._remove_partition, part) @@ -572,7 +562,6 @@ def test_gpt_type(self): home_guid = '933ac7e1-2eb4-4f13-b844-0e14e2aef915' part.SetType(home_guid, self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() # test flags value on types dbus_type = self.get_property(part, '.Partition', 'Type') @@ -589,6 +578,7 @@ def test_dos_type(self): self._create_format(disk, 'dos') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) part = self._create_partition(disk) self.addCleanup(self._remove_partition, part) @@ -606,7 +596,6 @@ def test_dos_type(self): part_type = '0x8e' # 'Linux LVM' type, see https://en.wikipedia.org/wiki/Partition_type#PID_8Eh part.SetType(part_type, self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() # test flags value on types dbus_type = self.get_property(part, '.Partition', 'Type') @@ -624,6 +613,7 @@ def test_resize(self): self._create_format(disk, 'gpt') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) disk_size = self.get_property(disk, '.Block', 'Size') @@ -649,8 +639,6 @@ def test_resize(self): part.Resize(0, self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() - part_offset.assertEqual(initial_offset) max_size = part_size.value part_size.assertGreater(initial_size) @@ -660,8 +648,6 @@ def test_resize(self): part.Resize(new_size, self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() - part_offset.assertEqual(initial_offset) # resize should guarantee at least the requested size part_size.assertGreater(new_size - 1) @@ -671,8 +657,6 @@ def test_resize(self): part.Resize(max_size, self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() - part_offset.assertEqual(initial_offset) part_size.assertGreater(max_size - 1) part_size.assertLess(disk_size.value) @@ -683,6 +667,7 @@ def test_name(self): self._create_format(disk, 'gpt') self.addCleanup(self._remove_format, disk) + self.addCleanup(self.wipe_fs, self.vdevs[0]) part = self._create_partition(disk) self.addCleanup(self._remove_partition, part) @@ -700,8 +685,6 @@ def test_name(self): part.SetName('test', self.no_options, dbus_interface=self.iface_prefix + '.Partition') - self.udev_settle() - # test flags value on types dbus_name = self.get_property(part, '.Partition', 'Name') dbus_name.assertEqual('test') From 0bfc558105dd907a20de5b6f6c989f0a0af98583 Mon Sep 17 00:00:00 2001 From: Tomas Bzatek Date: Mon, 7 Nov 2022 12:45:19 +0100 Subject: [PATCH 7/7] Revert "tests: Remove resolve_device tests involving partition creation" This reverts commit 3c413850be7776811b9bee75235feb2a22c24b88. --- src/tests/dbus-tests/test_10_basic.py | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/tests/dbus-tests/test_10_basic.py b/src/tests/dbus-tests/test_10_basic.py index 73ad08a42f..3d61d14082 100644 --- a/src/tests/dbus-tests/test_10_basic.py +++ b/src/tests/dbus-tests/test_10_basic.py @@ -318,6 +318,34 @@ def test_60_resolve_device(self): self.assertEqual(len(devices), 1) self.assertIn(object_path, devices) + # create a partition on another device + disk = self.get_object('/block_devices/' + os.path.basename(self.vdevs[2])) + self.assertIsNotNone(disk) + disk.Format('gpt', self.no_options, dbus_interface=self.iface_prefix + '.Block') + self.addCleanup(self._wipe, self.vdevs[2]) + part_label = 'PRTLBLX' + path = disk.CreatePartition(dbus.UInt64(1024**2), dbus.UInt64(100 * 1024**2), + '', part_label, self.no_options, + dbus_interface=self.iface_prefix + '.PartitionTable') + part = self.bus.get_object(self.iface_prefix, path) + self.assertIsNotNone(part) + part_uuid = self.get_property_raw(part, '.Partition', 'UUID') + self.assertIsNotNone(part_uuid) + part_name_val = self.get_property_raw(part, '.Partition', 'Name') + self.assertEquals(part_name_val, part_label) + + # check that partlabel and partuuid can be resolved + spec = dbus.Dictionary({'partlabel': part_label}, signature='sv') + devices = manager.ResolveDevice(spec, self.no_options) + object_path = '%s/block_devices/%s1' % (self.path_prefix, os.path.basename(self.vdevs[2])) + self.assertEqual(len(devices), 1) + self.assertIn(object_path, devices) + spec = dbus.Dictionary({'partuuid': part_uuid}, signature='sv') + devices = manager.ResolveDevice(spec, self.no_options) + object_path = '%s/block_devices/%s1' % (self.path_prefix, os.path.basename(self.vdevs[2])) + self.assertEqual(len(devices), 1) + self.assertIn(object_path, devices) + def test_80_device_presence(self): '''Test the debug devices are present on the bus''' for d in self.vdevs: