Created attachment 1623 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives

It looks good!

Just one comment: Instead of hard-coded /sbin:/bin use to add "sbin" paths into environment path method similar to that is in the patch bb36045b?

Yes, this would probably be better.
In fact, I've just copy/paste part of "tdelibs/tdeioslave/file/file.cc" for this code, which also have hardcoded pmount :-(

Created attachment 1624 [details]
tdelibs : add support for udisks1 and udisks2 (in file.cc)

I tested the patch and I came across one little problem. In pmount is utf8 option used only for vfat. In proposed patch is used regardless of the file system => most file systems fail.

Are you still using pmount after applying the patch ? If so, the pmount parameters should not have changed, the pmount related code was not modified !

I confirm the problem using udisks2 and an EXT2 filesystem, it fails saying "iocharset=" option is forbidden.

If you look at my code, you can see that the option is given to the command line ONLY if the "utf8" or "locale" option was given by the calling application. By default, no "iocharset" option is used.

By trying 'pmount' manually from the command line, it looks like it ignores the "-c utf8" option when I mount the EXT2 filesystem. So it works, but it should not.

I think we should fix 2 things:

1) ensure that the calling application (tdebase/tdeioslave/media/mediamanager/tdehardwarebackend.cpp) does NOT asks UTF8 for filesystem that don't support it.

2) imitate pmount behaviour in tdelibs: strips the invalid/unsupported options for filesystems that don't support it.

(Odpověď na komentář #7)
> I confirm the problem using udisks2 and an EXT2 filesystem, it fails saying
> "iocharset=" option is forbidden.
> 
> If you look at my code, you can see that the option is given to the command
> line ONLY if the "utf8" or "locale" option was given by the calling
> application. By default, no "iocharset" option is used.
> 
> By trying 'pmount' manually from the command line, it looks like it ignores the
> "-c utf8" option when I mount the EXT2 filesystem. So it works, but it should
> not.
> 
> I think we should fix 2 things:
> 
> 1) ensure that the calling application
> (tdebase/tdeioslave/media/mediamanager/tdehardwarebackend.cpp) does NOT asks
> UTF8 for filesystem that don't support it.
> 
> 2) imitate pmount behaviour in tdelibs: strips the invalid/unsupported options
> for filesystems that don't support it.

I would chose this point of view: Option "utf8" or "locale" is not equivalent to option "iocharset". Therefore, should be "translated" to options depending on the file system type => a similar way as it does pmount.

By adjusting options specifically by file system could be also used other options that seem to be ignored for now - journaling (ext3, 4), shortname (vfat).

It appears that the preparation mount options could be common for udisk and udisks2, to avoid duplicating code in at least one file.

Created attachment 1625 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives (2)

Updated patch: now it filters out the "iocharset=" option for filesystems that do not support it.

I have an USB key containing 2 partitions (FAT32 + EXT2) and now I can mount and unmount both of them, using udisks2.
I confirm that the FAT32 has "utf8" mount option, whereas EXT2 has not.

(En réponse au commentaire 8)
> (Odpověď na komentář #7)
> I would chose this point of view: Option "utf8" or "locale" is not equivalent
> to option "iocharset". Therefore, should be "translated" to options depending
> on the file system type => a similar way as it does pmount.
> 
> By adjusting options specifically by file system could be also used other
> options that seem to be ignored for now - journaling (ext3, 4), shortname
> (vfat).
> 
> It appears that the preparation mount options could be common for udisk and
> udisks2, to avoid duplicating code in at least one file.

From what I see, among the 3 mount programs we have, only pmount does not support all options.
In fact, in udisks and udisks2, all options are passed directly to the "mount" command. So it should be easy to implement the current disabled options, like shortname ...

Created attachment 1627 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives (3)

I tried revise your patch, as I mentioned in a comment. Please your opinion. And if you agree with the adjustment, please test it.

Not tested your patch yet, but looks good.
But for mount, the code looks for: udisks2/udisks/pmount, whereas for unmount we look for pumount/udisks2/udisks .
I think the utilities order should be consistent.

E.g. with your patch, if pmount is installed on my computer, TDE will use "udisks2" for mounting and "pumount" for unmouting !

Created attachment 1629 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives (4)

Patch updated:
1) reoder programs in umount
2) add journaling option for ext3, ext4
3) I am concerned the use command.ascii(), because the command may contain device name or mount point => data with utf8. That's why I changed it to command.local8Bit().

Please, test it :)

Created attachment 1630 [details]
tdelibs : fix tdehw backend media name with UTF8 characters

I tested the patch for mount and I have another patch to fix media name with utf8 characters

I've tested both patches and it looks like everything is fine. Well done for the UTF8 media names :-)

On my test machine with Ubuntu 13.10 (Saucy) I could try all mount options - pmount, udisks and udisk2. Pmount and udisks work fine. Udisks2 on vfat partition not work:

GDBus.Error:org.freedesktop.UDisks2.Error.OptionNotPermitted: Mount option `utf8' is not allowed

I also noticed that the utf8 option is also supported filesystems ntfs and iso9660. We should add this option for these file systems?

Created attachment 1631 [details]
tdelibs : fix handling paths with UTF8 characters in tdehw storage backend

I have one more patch for tde storage backend, but do not know where it is possible to test behavior before / after the patch.

Ah sorry, on my current PC (not the same as yesterday), I had "utf8 charset" checkbox disabled in TDE mounting preferences for vfat, so it worked but I had no utf8 mount option.
When mounting using "udisksctl" from the command line, I see the same error message as you.
In my previous patch, I did not use the "utf8" option, only the "iocharset=utf8" option. Are you sure they are really different ?

And yes, I think we should add "utf8" option where possible, so OK to add it for iso9660 and ntfs...

Are these patches ready to test? Slackware does not come with pmount as a stock package but does support udisks(2). I would like to test.

Will these patches be merged into one patch?

Created attachment 1632 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives (5)

After further testing, looks like "udisksctl" always add "utf8" option to the VFAT filesystem mounting. This is probably why it does not want "utf8" again as an option.

Updated patch so that "utf8" flag is set for UDISKS1 but not UDISKS2.

Created attachment 1633 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives (6)

Adds 'utf8' option for iso9660 and ntfs, UDISKS1 only.

Created attachment 1637 [details]
tdelibs : add support for udisks1 and udisks2 to mount/unmount removable drives (7)

I have a few comments:

1) Place, to which was moved handling utf8 option has two problems:

+ is after "optionString.remove(0, 1);" which remove the leading comma - if utf8 was the first / only one settings leading comma will be undesirable.

+ is after "mountOptions.contains("filesystem")" - would cause confusion in the options

2) For ntfs udisk and udisks2 assume ntfs-3g => does not allow options utf8 and iocharset.

3) I checked the source code udisks and udisk2 and both internally use utf8. It seems that for fat, ntfs and iso9660 we do not need to handle utf8 whatsoever. A preparation of options can be shared again.

Updated patch attached.

I tested pmount, udisks and udisks2 along with fat, ext3 and ntfs.
All combination has worked fine.
Please test it also. :)

Note: I have observed that the eject on usb disk reports problems for udisks2 and also udisks function. However, I'll leave that for a separate bug message.

Your latest patch works for me (ext2, FAT32).
I have no problem with neither umount nor eject function, using udisks1 and udisks2.

(Odpověď na komentář #23)
> Your latest patch works for me (ext2, FAT32).
> I have no problem with neither umount nor eject function, using udisks1 and
> udisks2.

May be related to file system name (and mount point) contain utf8 characters.
May be related to test drive had two partitions.

In any case, I received such a message:

[tdehwlib] Failed to eject drive '/dev/sdb2' via udisks2, falling back to alternate mechanism
[ERROR] org.freedesktop.DBus.Error.ServiceUnknown
[tdehwlib] Failed to eject drive '/dev/sdb1' via udisks, falling back to alternate mechanism
[tdehwlib] Failed to eject drive '/dev/sdb1' via 'eject' command

Created attachment 1641 [details]
tdelibs : add support for udisk1 and udisk2 in tdeioslave/file

I modified the patch for tdeioslave/file to use also order
udisks2 / udisks / pmount.

Since the function is otherwise unchanged, I assume that all the patches can be pushed to Git. Since the original work for udisks patches belongs to you, François, I bring you as an author.

Looks good => I think you can push all patches to GIT.

Patches pushed to GIT:

+ attachment 1630 [details] in hash 337146de
+ attachment 1631 [details] in hash 24b097d1
+ attachment 1637 [details] and attachment 1641 [details] in hash 990c3797
  (with small adjustments)

Note: In commit 7a8b0da2 (tdebase) were added the conditions that allow to pass mount options to tdehw storage backend.

Francois, Slavek,

Today I built and installed a fresh package set that includes the patches from this bug report. Removable devices are working here without pmount. Thank you and job well done!

New side-effect when using UDISKS2 to mount EXT4 partition:
udisks2 complains that I cannot pass option "data=ordered"...

E.g, in command line:
$ udisksctl mount -b /dev/sdb1 -o data=ordered
Error mounting /dev/sdb1: GDBus.Error:org.freedesktop.UDisks2.Error.OptionNotPermitted: Mount option `data=ordered' is not allowed

Created attachment 1657 [details]
tdelibs : Disable unallowed options for udisks/udisks2 in tdehw storage backend

Oops. As I see none of mount backends (pmount/udisks/udisks2) have support for "data=" for ext3/ext4.

I'm afraid that due to the fact that for JFS has no extra options, it would be a problem also with iocharset=utf8 for JFS.

Created attachment 1658 [details]
tdebase : Fix allowing utf8 mount options in tdehw storage backend

I adjusted allowing utf8 option depending on the file system.
It would be incorrect display option for all unknown file systems.

Patches pushed to GIT:
+ attachment 1657 [details] in hash 58852c67
+ attachment 1658 [details] in hash eb091e7d

I assume that the problem should be solved again.

I just had a user report that TDE (or at least the media:/ tdeioslave) doesn't seem to properly read the path udisks mounted the flash drive on; this leads to strange file not found errors when deleting files from a flash drive, as the tdeioslave attempts to remove the file from an incorrect path (e.g. /media/Flash) instead of where the flash drive is actually mounted (e.g. /media/user/Flash).

I confirm some strange behaviour here.
Running Mageia 3, USB drives mounted through udisks2 appear under /run/media/<username>/<medianame>

I can click the device icon to mount it and open in Dolphin.
The access path looks correct here.
But then, in Dolphin, when I double-click in a subfolder inside the drive, the URL changes to something like "//sdb1" and Dolphin displays a message such as "the specified folder does not exist".

When browsing folders through the "root filesystem" menu (red folder icon), it works well. When browsing through the "removable media", it fails.

Created attachment 1705 [details]
tdelibs : add '/run' folder to the slow freq polling directories list

Currently, I have both dolphin and konqueror installed.

I have no problem when using Konqueror as file manager.
When I open the "Devices" view, then open my USB drive, the Konqueror URL is "system:/media/sdb1", and the actual mount point is "/run/media/francois/sdb1".
I can browse folders/copy/move/delete item. The URL bar always show the "system:/media" URL, e.g. "system:/media/sdb1/subfolder1/subfolder2" ...

When using Dolphin: I open manually dolphin, by default, I'm in home directory. On the left "bookmarks" pane, I click "Storage Devices" (translation may be inaccurate), Now the URL bar shows "Storage Devices" but I guess I'm in the "system:/media" URL. I double-click my USB drive, then I'm redirected to the entire URL "/run/media/francois/<medianame>", and the bookmark pane switches to 
"root filsystem". I can browse, copy, delete, no problem.
=> Not the same behaviour as Konqueror, but still works.

Now, when using the TDE tray icon for removable media, I click on my USB drive icon, then choose the first item "open in new window".
Case 1: the USB drive was already mounted => Dolphin opens in "/run/media/francois/<medianame>" folder, no problem.
Case 2: the USB was NOT mounted. It is mounted, then Dolphin opens with "media:/sdb1" URL, NOT "system:/media/sdb1" ! I can see my drive content. But when double-clicking on a subfolder, I'm redirected to "/sdb1/<subfolder>" URL, which is obviously invalid.

Another scenario: this time I unplug/plug again my USB key. I have a "TDE Daemon" popup which asks me what action I want. I choose "open in new window".
This time it opens Dolphin with "system:/media/sdb1", and I can see my drive content. But when opening a subfolder, I'm redirected to "/media/sdb1/<subfolder>", which does not exist.

Now I've uninstalled Dolphin entirely.

Using Tray icon "open in new window":
1) If USB key was already mounted, Konqueror opens "/run/media/francois/<medianame>", and it works normally.
2) if USB key was NOT mounted, Konqueror opens "media:/sdb1", and it works normally.

Using the "TDE Daemon" popup when plugging USB key:
"Open in new window" opens "system:/media/sdb1" and it works normally.


So, it looks like only Dolphin file manager is having problems, so maybe we should investigate it.
Still, depending on the circumstances, there are different URL for using the same media, which may cause confusion, even though they should all work.

I think that Dolphin in fact does not like the "system:/" and "media:/" URL at all, or is cheating somewhere to make it work when media are mounted under "/media".

Open dolphin, manually type URL "system:/media/sdb1" . It opens correctly.
Double click a subfolder. It should open "system:/media/sdb1/<subfolder>" but simply opens "/sdb1/<subfolder>". Where has the "system:/media" part of the URL gone ?

Manually type URL "media:/sdb1". Double-click a subfolder => fail to open "/sdb1/<subfolder>".

Manually type URL "media:/sdb1/<subfolder>". Double click a subfolder inside the subfolder => fail to open

(In reply to comment #34)
> I just had a user report that TDE (or at least the media:/ tdeioslave) doesn't
> seem to properly read the path udisks mounted the flash drive on; this leads to
> strange file not found errors when deleting files from a flash drive, as the
> tdeioslave attempts to remove the file from an incorrect path (e.g.
> /media/Flash) instead of where the flash drive is actually mounted (e.g.
> /media/user/Flash).

This should be fixed in GIT hash 23006ee.  I don't know if this fixes Dolphin or not, but it might.

Comment on attachment 1705 [details]
tdelibs : add '/run' folder to the slow freq polling directories list

Pushed to GIT in hash 47bbb9e4.

Do all of the changes made here mean Trinity no longer can be built to use pmount? Some folks might prefer pmount over udisks(2)?

(In reply to comment #42)
> Do all of the changes made here mean Trinity no longer can be built to use
> pmount? Some folks might prefer pmount over udisks(2)?

TDE will simply prefer udisks(2) if it is available.  It will still fall back to using pmount if needed.

Tim

With today's git, the problem with Dolphin I mentionned above is gone.
I guess you fixed the issue in tdelibs.

Sounds like this is resolved then.  If not please reopen this report.

Thanks for reporting, and for all the debugging!

>TDE will simply prefer udisks(2) if it is available.  It will still fall back
>to using pmount if needed.
How do we force compiling with and using pmount rather than udisks?