GNOME Bugzilla – Bug 732565
Attempting to format device during creating of encrypted partition fails
Last modified: 2017-08-21 19:09:30 UTC
* Create a new encrypted partition * This takes a little while, so become bored and decide you want to format the entire device instead Expected behavior: unusable controls like Format are insensitive while a long-running operation is in progress Actual behavior: I can click Format and get an unfriendly error message: Error wiping device: Command-line `wipefs -a "/dev/sdb1"' exited with non-zero exit status 1: wipefs: error: /dev/sdb1: probing initialization failed: Device or resource busy (udisks-error-quark, 0)
Created attachment 355784 [details] [review] Disable action buttons during jobs At least from the GNOME Disks side this prevents these error messages or even destructive actions.
Maybe it's possible to cover even more corner cases, like e.g. looking at jobs of other volumes - I better try to get this done first.
Created attachment 355795 [details] [review] Disable action buttons during jobs
Review of attachment 355795 [details] [review]: Looks good apart from some cosmetic issues :-) ::: src/disks/gduapplication.c @@ +576,3 @@ + + jobs = udisks_client_get_jobs_for_object (client, object); + jobs = g_list_concat (jobs, gdu_application_get_local_jobs_for_object (application, object)); Wouldn't be better to add the object in objects_to_check? objects_to_check = g_list_prepend (objects_to_check, g_object_ref (object)); So you can simplify the code then... @@ +590,3 @@ + UDisksEncrypted *encrypted_for_object; + + block_for_object = udisks_object_peek_block (object_iter); I would move this inside "if (encrypted_for_object != NULL)" statement... @@ +609,3 @@ + if (encrypted_for_object != NULL) + { + UDisksBlock *cleartext; \n ::: src/disks/gduwindow.c @@ +1879,3 @@ } + if (!window->has_any_volume_job) Wouldn't be better to do it as the following? sensitive = !window->has_any_volume_job; gtk_widget_set_sensitive (window->devtab_drive_generic_button, sensitive); ... or set sensitivity for parent widget instead? @@ +1888,3 @@ + else + { + gtk_widget_set_sensitive (window->devtab_grid_toolbar, FALSE); This is probably redundant, because it is rewritten later... @@ +1893,3 @@ + gtk_widget_set_sensitive (window->devtab_drive_power_off_button, FALSE); + gtk_widget_set_sensitive (window->devtab_drive_loop_detach_button, FALSE); + } nitpick: I would add \n, but it is up to you... @@ +1970,3 @@ GList *jobs) { + window->has_drive_job = jobs != NULL; window->has_drive_job = (jobs != NULL); @@ +1978,3 @@ GList *jobs) { + window->has_volume_job = window->has_volume_job || jobs != NULL; window->has_volume_job = (window->has_volume_job || jobs != NULL); @@ +2967,3 @@ if (object != NULL) { + window->has_volume_job = gdu_application_has_running_job (window->application, object); Not sure about, but isn't this redundant? Isn't enough to check this in update_volume_jobs? ::: src/libgdu/gduutils.h @@ +73,3 @@ gchar *gdu_utils_get_pretty_uri (GFile *file); +GList *get_all_contained_objects (UDisksClient *client, GList *gdu_utils_get_all_contained_objects (UDisksClient *client,
Created attachment 355940 [details] [review] Disable action buttons during jobs Good, I changed the place where the variables are assigned and hope it helps for readability even if the overall architecture is not so flexible.
Review of attachment 355940 [details] [review]: ::: src/disks/gduwindow.c @@ +1887,3 @@ + gtk_widget_set_sensitive (window->devtab_drive_loop_detach_button, drive_sensitivity); + + selected_volume_sensitivity = !window->has_volume_job && !window->has_drive_job; selected_volume_sensitivity = (!window->has_volume_job && !window->has_drive_job); @@ +1963,2 @@ { + window->has_volume_job = FALSE; /* comes before update_volume_jobs */ Wouldn't be finally better to move all these logic in update_jobs (so you can remove window->has_*_job at all)? Is necessary to have origin_object as a param? Can't be simply window->current_object used instead?
Created attachment 356231 [details] [review] Disable action buttons during jobs Thanks, good hint! If I would remove the variables the whole retrieval of drive jobs would have to be done again with code copy for the volume update case…
Review of attachment 356231 [details] [review]: Looks good, just it pretty flickers during e.g. resizing... can't we add e.g. some dummy-job for the operations like resizing? or some short timeout before setting sensitivity to true?
Review of attachment 356231 [details] [review]: Btw this still doesn't prevent e.g. creating of a new partition when resizing...
Created attachment 357571 [details] [review] Disable action buttons during jobs (no change, just added the URL in the commit message)
Created attachment 357572 [details] [review] Delay sensitivity during job updates The whole state of the job updates is rather complicated with all these chained calls. This is more like a hack, but other solutions are out of sight with the current architecture (also a change might not fullfill all promises).
Review of attachment 357572 [details] [review]: Thanks! I do not see any flickering with this patch, great! But the sensitivity should be changed immediately if somebody changes the drive for example. ::: src/disks/gduwindow.c @@ +1957,3 @@ + g_source_remove (window->delay_job_update_id); + + window->delay_job_update_id = g_timeout_add (300, delayed_job_update, window); Please add #define with value on the top file...
Created attachment 357674 [details] [review] Delay sensitivity during job updates Yes, that wasn't working at all! Now it should be ok.
Review of attachment 357674 [details] [review]: Looks ok for drives, but the same problem for volumes... :-( It would be nice to get those patches in this release as well. It would be a nice addition not only for resize feature...! ::: src/disks/gduwindow.c @@ +1925,3 @@ + window->is_delayed_job_update = TRUE; + update_all (window); + window->is_delayed_job_update = FALSE; nitpick: The flag is changed only for the function calls, so would be better to use function argument instead. (If you want to avoid changing update_all on all places, you can rename update_all to e.g. update_all_internal and create macro/function with name update_all which just calls update_all_internal with correct param).
Created attachment 357742 [details] [review] Delay sensitivity during job updates Sorry for the bad patch, this effect slipped through while testing… Yes, global state is not nice in general, so even if it was more a method of holding the patch small and not a real global state I've just made it a proper function argument :)
Review of attachment 357742 [details] [review]: Looks ok and seems working correctly. Just the "changed" on GduVolumeGrid is emitted also on "changed" from UDisksClient (not only when a user changes the selection), so not sure it can't be a problem in some cases...
(Shouldn't be a problem because it could only cause a flicker during the transtion from one to the next job but that behavior was not observed.)