Project

General

Profile

Feature #15794

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

Added by intrigeri over 1 year ago. Updated 2 months ago.

Status:
Resolved
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 5 months ago

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

Revision 137a7aa4 (diff)
Added by segfault 5 months ago

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

Revision d7b53fc7 (diff)
Added by intrigeri 3 months ago

Update POT and PO files (refs: #15794).

… so we can test build reproducibility of the topic branch.

Revision efc93192
Added by intrigeri 3 months ago

Merge branch 'bugfix/15794-feedback-on-failure-to-lock' into stable (Fix-committed: #15794)

History

#1 Updated by intrigeri over 1 year 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 over 1 year ago

  • Assignee set to segfault

I will look into it

#3 Updated by intrigeri about 1 year ago

  • Parent task deleted (#14480)

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

#4 Updated by segfault 5 months ago

  • Status changed from Confirmed to In Progress

#5 Updated by segfault 5 months 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 5 months 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 5 months ago

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

#8 Updated by segfault 5 months 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 5 months 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 5 months ago

please merge.

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

#11 Updated by segfault 5 months 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 4 months ago

  • Status changed from Fix committed to Resolved

#13 Updated by sajolida 4 months 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 4 months 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 4 months 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 4 months ago

  • Status changed from In Progress to Needs Validation

#17 Updated by sajolida 4 months ago

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

#18 Updated by sajolida 4 months 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! :)

#19 Updated by segfault 4 months ago

sajolida wrote:

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! :)

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

#20 Updated by sajolida 4 months ago

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

I use #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 #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 3 months ago

  • Assignee set to intrigeri

#22 Updated by intrigeri 3 months ago

  • Status changed from Needs Validation to In Progress

#23 Updated by intrigeri 3 months ago

  • Status changed from In Progress to Fix committed

#24 Updated by CyrilBrulebois 2 months ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF