Project

General

Profile

Feature #15794

No feedback on failure to lock busy volume in "Unlock VeraCrypt Volumes"

Added by intrigeri 11 months ago. Updated 24 minutes ago.

Status:
Needs Validation
Priority:
Low
Assignee:
-
Category:
-
Target version:
Start date:
08/15/2018
Due date:
% Done:

100%

Feature Branch:
bugfix/15794-feedback-on-failure-to-lock
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

I've unlocked a VeraCrypt volume with this app, opened in in Files, opened a PDF found in that volume with Evince, and then felt adventurous and clicked the "x" button in "Unlock VeraCrypt Volumes" to lock it. Of course it does not work but the problem is: there's no feedback whatsoever. Note that if I click the "eject" icon in Files I get an explanation, as expected.

error.png View (28.2 KB) sajolida, 07/20/2019 09:22 AM


Related issues

Related to Tails - Feature #14544: Spend software developer time on smallish UX improvements In Progress 08/31/2018

Associated revisions

Revision 4d2fb0af (diff)
Added by segfault about 1 month ago

unlock-veracrypt-volumes: Show error message if locking fails (refs: #15794)

Revision 137a7aa4 (diff)
Added by segfault about 1 month ago

unlock-veracrypt-volumes: Make error message selectable (refs: #15794)

History

#1 Updated by intrigeri 11 months ago

I have no idea what the cost/benefit of fixing this bug is so I'm not setting any assignee nor "deliverable for".

#2 Updated by segfault 11 months ago

  • Assignee set to segfault

I will look into it

#3 Updated by intrigeri 9 months ago

  • Parent task deleted (#14480)

Three months later, no idea about the cost/benefit => unparenting.

#4 Updated by segfault about 1 month ago

  • Status changed from Confirmed to In Progress

#5 Updated by segfault about 1 month ago

  • Status changed from In Progress to Needs Validation
  • Assignee deleted (segfault)
  • Target version set to Tails_3.15
  • % Done changed from 0 to 20
  • Feature Branch set to bugfix/15794-feedback-on-failure-to-lock

I implemented showing an error message if the locking fails.

#6 Updated by hefee about 1 month ago

I think logic of unmount should be different:

def unmount(self):
        # [...]
        unmounted_at_least_once = False
        while self.udisks_object.get_filesystem().props.mount_points:       
            try:                                        
                # [...]                                                                                       
                unmounted_at_least_once = True                        
            except GLib.Error as e:
                # Ignore "not mounted" error if the volume was already unmounted
                if "org.freedesktop.UDisks2.Error.NotMounted" in e.message and unmounted_at_least_once:
                    return                                
                raise  

That means, if we detect an error, that one volume is already unmounted, we give up. Shouldn't the return replaced with a continue aka just ignore that mount points that are already unmounted but try to unmount everything and wait till self.udisks_object.get_filesystem().props.mount_points is empty. As umount may takes some time, we may want also add a small sleep ~0.5-1s, to give the devices time to shutdown, before we try again?

#7 Updated by hefee about 1 month ago

  • Status changed from Needs Validation to In Progress
  • Assignee set to segfault

#8 Updated by segfault about 1 month ago

  • Status changed from In Progress to Needs Validation
  • Assignee changed from segfault to hefee

hefee wrote:

I think logic of unmount should be different:

[...]

That means, if we detect an error, that one volume is already unmounted, we give up. Shouldn't the return replaced with a continue aka just ignore that mount points that are already unmounted but try to unmount everything and wait till self.udisks_object.get_filesystem().props.mount_points is empty.

I don't think that should be necessary. We don't unmount a specific mount point with the udisks unmount call. The assumption behind the "return in case of Error.NotMounted" is that this error is only returned if the filesystem is not mounted at all, in which case we don't want to continue retrying.

I checked whether there could be a race condition inside the unmount call which could throw a not-mounted error even though there are still mount points. That's not the case, this error is only returned if udisks_filesystem_get_mount_points doesn't return any mount points (see https://github.com/storaged-project/udisks/blob/56ff812a716d6bacda053705a5660cdb688df5c9/src/udiskslinuxfilesystem.c#L1827).

#9 Updated by hefee 28 days ago

  • Status changed from Needs Validation to In Progress
  • Assignee changed from hefee to segfault

Thanks for the explanation. So the review has passed, please merge.

#10 Updated by intrigeri 28 days ago

please merge.

… after 3.14.2 is out (otherwise it'll break stuff).

#11 Updated by segfault 27 days ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (segfault)
  • % Done changed from 20 to 100

Merged into stable -> devel -> feature/buster.

I forgot the Fix-committed bit in the merge commit message, sorry about that.

#12 Updated by CyrilBrulebois 12 days ago

  • Status changed from Fix committed to Resolved

#13 Updated by sajolida 3 days ago

  • Status changed from Resolved to In Progress
  • Priority changed from Normal to Low
  • Target version changed from Tails_3.15 to Tails_3.16

If I read 4d2fb0af50 correctly, the error message is now:

Couldn't lock volume {volume_name}:\n{error_message}

It explains what impact of the error is but it doesn't explain neither why the error happened nor how to solve it.

Could we reuse instead the message from GNOME when trying to eject an USB stick that is in use?

One or more applications are keeping the volume busy.

Which is quite more helpful.

Also, what's in "error_message"? If it's a technical log or error number of some short we could maybe do without it.

Still, it's better to have some error message than none, so I'm changing this to "Low" prio.

#14 Updated by sajolida 2 days ago

I probably wasn't explicit enough.

You'll find the current error message

I would prefer it to be:

**Locking the volume failed**

One or more applications are keeping the volume busy.

#15 Updated by segfault 2 days ago

sajolida wrote:

I probably wasn't explicit enough.

You'll find the current error message

I would prefer it to be:

**Locking the volume failed**

One or more applications are keeping the volume busy.

Done. Does this qualify for #14544?

#16 Updated by segfault 2 days ago

  • Status changed from In Progress to Needs Validation

#17 Updated by sajolida 25 minutes ago

  • Related to Feature #14544: Spend software developer time on smallish UX improvements added

#18 Updated by sajolida 24 minutes ago

Does this qualify for #14544?

I'm not sure to understand the reason behind your question. #15794 as a
whole could qualify for #14544 and writing a helpful error message is
part of this (and should have been on the first iteration in 3.15).

I'm using #14544 as a tool for planning and prioritizing fixes, so
adding such relationships after the problems are solved is not useful to
me personally. But it's possible so I did it! :)

Also available in: Atom PDF