Review of attachment 327195 [details] [review]:

So, we have removable drives that can't be stopped, ejected, or have their media removed ? Isn't that going to be a problem for deciding what we can do with them ?

udisks seems to be detecting them appropriately, and tagging them, but the tag is not relayed to the upper layers in the stack. The existing heuristics, on the other hand, fail to detect some of them — e.g. if I have a USB device attached to a hub and I make the mistake of booting my laptop up with the hub plugged in.

(In reply to Matthias Clasen from comment #1)
> Review of attachment 327195 [details] [review] [review]:
> 
> So, we have removable drives that can't be stopped, ejected, or have their
> media removed ?

You can see such drive using GIO if you connect external usb disk and then restart the computer with the disk connected. The problem is that g_drive_can_stop != udisks_drive_get_can_power_off, because of the following code from udisks2 volume monitor:

    if (!drive->is_media_removable && !drive->coldplug)
      {
        if (udisks_drive_get_can_power_off (drive->udisks_drive))
          {
            drive->can_stop = TRUE;
          }
      }

I am not really sure if external drive may have udisks_drive_get_can_power_off() == false and can't be ejected, or don't have removable media...

So maybe it may be enough to just remove !drive->coldplug from the condition above, but I am not really sure it is good idea, because of the following comment:

     * Note that this heuristic has the nice
     * side-effect that USB-attached hard disks that are plugged in
     * when the computer starts up will not be powered off when the
     * user clicks the "eject" icon.

That's why I proposed g_drive_is_removable...

> Isn't that going to be a problem for deciding what we can do
> with them ?

We can still mount/unmount their volumes/mounts... though I didn't test the gtkplacessidebar if it handle such drives properly...

(In reply to Ondrej Holy from comment #3)
> ...
> So maybe it may be enough to just remove !drive->coldplug from the condition

I meant use udisks_drive_get_removable instead of !drive->coldplug, though it seems that udisks do it internally already...

(In reply to Ondrej Holy from comment #3)
> (In reply to Matthias Clasen from comment #1)
> > Review of attachment 327195 [details] [review] [review] [review]:
> > 
> > So, we have removable drives that can't be stopped, ejected, or have their
> > media removed ?
> 
> You can see such drive using GIO if you connect external usb disk and then
> restart the computer with the disk connected. The problem is that
> g_drive_can_stop != udisks_drive_get_can_power_off, because of the following
> code from udisks2 volume monitor:
> 
>     if (!drive->is_media_removable && !drive->coldplug)
>       {
>         if (udisks_drive_get_can_power_off (drive->udisks_drive))
>           {
>             drive->can_stop = TRUE;
>           }
>       }
> 
> I am not really sure if external drive may have
> udisks_drive_get_can_power_off() == false and can't be ejected, or don't
> have removable media...

Hmm I discussed it again with Peter Hatina from storaged. He thinks that there are no such drives, where:
CanPowerOff==MediaRemovable==Ejectable==false && Removable==true

> So maybe it may be enough to just remove !drive->coldplug from the condition

He thinks that Removable is reliable for external devices detection and also doesn't see any reasons, why we should block can_stop for coldplug devices if Removable is true...

So do you agree with the gvfs modification for can_stop instead of changes for glib/gtk?

(In reply to Ondrej Holy from comment #3)

> So maybe it may be enough to just remove !drive->coldplug from the condition
> above, but I am not really sure it is good idea, because of the following
> comment:
> 
>      * Note that this heuristic has the nice
>      * side-effect that USB-attached hard disks that are plugged in
>      * when the computer starts up will not be powered off when the
>      * user clicks the "eject" icon.
> 

I think the reboot-with-disk attached scenario shows that this heuristic is flawed. But now that you've explained the situation, using the simpler check in GTK+ is probably an ok solution, if you make sure that the rest of the code can handle such drives.

Created attachment 327569 [details] [review]
Improve external drives detection

I realized that same change is necessary also for gtkplacesview.c.

It works correctly for me. Sidebar is in sync with "Other Locations" view. It is possible to mount/unmount such volumes. External hard drive is shown even after restart...

Review of attachment 327569 [details] [review]:

ok

(In reply to Matthias Clasen from comment #8)
> Review of attachment 327569 [details] [review] [review]:
> 
> ok

Maybe I confused you by the patch description, where I wrote about recently added g_drive_is_removable, however this function is not there yet and Bug 765900 and Bug 765900 needs to be solved first, can you please take a look at them also?

Comment on attachment 327569 [details] [review]
Improve external drives detection

commit 55751fc

Ah, so this was the cause of the periodic smoketest failures in Continuous.

*** Bug 770923 has been marked as a duplicate of this bug. ***