Feature #15794

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

Added by intrigeri 2018-08-15 18:09:22 . Updated 2019-09-05 00:03:54 .

Status:
Resolved
Priority:
Low
Assignee:
intrigeri
Category:
Target version:
Tails_3.16
Start date:
2018-08-15
Due date:
% Done:

100%

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

Starter:
Affected tool:
Deliverable for:

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.

Files

error.png (28905 B) sajolida, 2019-07-20 09:22:52

Subtasks

Related issues

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

History

#1 Updated by intrigeri 2018-08-15 18:09:44

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 2018-08-15 18:13:00

  • Assignee set to segfault

I will look into it

#3 Updated by intrigeri 2018-10-31 15:57:24

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

#4 Updated by segfault 2019-06-17 13:26:35

  • Status changed from Confirmed to In Progress

Applied in changeset commit:tails|4d2fb0af50fd8452d5416e84ff5d07c509728f69.

#5 Updated by segfault 2019-06-17 13:28:43

  • 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 2019-06-19 10:36:04

I think logic of unmount should be different:

<code class="python">
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  

</code>

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 2019-06-19 10:36:40

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

#8 Updated by segfault 2019-06-21 08:44:27

  • 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 2019-06-24 09:09:54

  • 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 2019-06-24 09:18:39

> please merge.

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

#11 Updated by segfault 2019-06-25 20:08:36

  • 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 2019-07-10 10:32:38

  • Status changed from Fix committed to Resolved

#13 Updated by sajolida 2019-07-19 16:43:56

  • 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 2019-07-20 09:24:55

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 2019-07-20 17:17:24

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 Feature #14544?

#16 Updated by segfault 2019-07-20 17:18:06

  • Status changed from In Progress to Needs Validation

#17 Updated by sajolida 2019-07-22 11:21:03

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

#18 Updated by sajolida 2019-07-22 11:22:10

> Does this qualify for Feature #14544?

I’m not sure to understand the reason behind your question. Feature #15794 as a
whole could qualify for Feature #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 Feature #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! :)

#19 Updated by segfault 2019-07-22 20:44:27

sajolida wrote:
> > Does this qualify for Feature #14544?
>
> I’m not sure to understand the reason behind your question. Feature #15794 as a
> whole could qualify for Feature #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 Feature #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! :)

Maybe I misunderstood something. I can’t find the relationship status on Feature #14544, but somehow I thought that working on those tasks would qualify as FT work.

#20 Updated by sajolida 2019-07-24 16:40:48

That’s more a question to @intrigeri because it’s about how FT spends
its budget.

I use Feature #14544 as a parking lot for pretty much any ticket that could fit
in the scope. In January we did some prioritization work with intrigeri
to sort these by value/cost. See Feature #14544#note-75 for the top 7 tasks.

I don’t think that the tasks with the worse value/cost would qualify as
FT but you should check with @intrigeri. I would personally love it to
see FT workers be proactive on other tasks than the top 7 tasks :)

#21 Updated by intrigeri 2019-08-12 09:50:21

  • Assignee set to intrigeri

#22 Updated by intrigeri 2019-08-12 09:58:39

  • Status changed from Needs Validation to In Progress

Applied in changeset commit:tails|d7b53fc71cad01f58df994cfcf48435d164048f1.

#23 Updated by intrigeri 2019-08-12 09:58:40

  • Status changed from In Progress to Fix committed

Applied in changeset commit:tails|efc93192b744e31424421f0b996c3bc72e62fdce.

#24 Updated by CyrilBrulebois 2019-09-05 00:03:54

  • Status changed from Fix committed to Resolved