Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add udisks2 support #13897

Merged
merged 1 commit into from May 23, 2018

Conversation

Projects
None yet
7 participants
@edu-tsen
Copy link
Contributor

commented May 15, 2018

Description

Adds UDisks2Provider for handling mounts/umounts of removable devices.

See also Ticket #17560

Motivation and Context

Re-enable mounting and unmounting of removable media for non-root users.

Needed setup:

  • Adjust Polkit rules allowing action for org.freedesktop.udisks2.*
  • Add <handlemounting>true</handlemounting> to advancedsettings.xml if automount is wanted

PS: Old udisks support and automount feature seems to be undocumented in wiki.

How Has This Been Tested?

Tested on Ubuntu 18.04

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
class CUDisks2Drive
{
public:
std::string m_object;

This comment has been minimized.

Copy link
@fritsch

fritsch May 16, 2018

Member

I'd made them private and rather the other Udiskclasses that directly use it friend classes to not provide direct access to everyone.

This comment has been minimized.

Copy link
@edu-tsen

edu-tsen May 16, 2018

Author Contributor

@fritsch I found it more explicit to turn helper classes into nested classes. This also allows uncluttering the class names and to turn the non-member helper funcion into a static member.

Please check 252724d

@fritsch fritsch requested review from pkerling and wsnipex May 16, 2018

@fritsch

This comment has been minimized.

Copy link
Member

commented May 16, 2018

@wsnipex time to drop udisks1?

@fritsch

This comment has been minimized.

Copy link
Member

commented May 16, 2018

@edu-tsen and before I forget it: Thank you very much!

@wsnipex
Copy link
Member

left a comment

didn't have time to test it yet, but it looks good.
thanks for your contribution

}
else
{
CLog::Log(LOGDEBUG, "UDisks2: Refuse to mount %s", toString());

This comment has been minimized.

Copy link
@wsnipex

wsnipex May 16, 2018

Member

could you log a reason/dbus error here?

This comment has been minimized.

Copy link
@edu-tsen

edu-tsen May 16, 2018

Author Contributor

@wsnipex I tried to make the log messages more explicit. DBus errors will already be logged by CDBusMessage - the check for DBUS_MESSAGE_TYPE_ERROR is probably redundant?

Please check 14d5d8e

@mkreisl

This comment has been minimized.

Copy link

commented May 16, 2018

@wsnipex time to drop udisks1?

It is long overdue, udisk1 has been dropped in Debian Stretch already, for example

@wsnipex

This comment has been minimized.

Copy link
Member

commented May 16, 2018

yes, udisks1 can go, but it doesn't have to be part of this PR.

@wsnipex

This comment has been minimized.

Copy link
Member

commented May 17, 2018

looks good to me

@fritsch

This comment has been minimized.

Copy link
Member

commented May 19, 2018

Pleash squash all commits to one "Implement Udisks2" support.

@pkerling I would shove it in after that - as it adds new functionality without touching existing code I'd say we can fix the minors after that, seems you are busy at the moment for review.

@Rohitrayi

This comment has been minimized.

Copy link

commented May 19, 2018

@edu-tsen edu-tsen force-pushed the edu-tsen:udisks2 branch from 14d5d8e to 350a183 May 22, 2018

@edu-tsen

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

Squashed and force-pushed

@wsnipex wsnipex merged commit 325597a into xbmc:master May 23, 2018

1 check passed

default You're awesome. Have a cookie
Details
@wsnipex

This comment has been minimized.

Copy link
Member

commented May 23, 2018

thank you

@pkerling

This comment has been minimized.

Copy link
Member

commented May 26, 2018

Seems I'm late to the party :-) I'd actually started doing this myself a year ago and mostly forgot about it cough

I'd have added some stylistic minors but overall it looks OK, it's merged now anyway and if it works - perfect. I'll certainly give it a try some time next week since there wasn't much reaction here concerning practical tests.

Thanks for the work!

@mkreisl

This comment has been minimized.

Copy link

commented May 26, 2018

there wasn't much reaction here concerning practical tests.

Because it works 😄 (XBian, Debian Stretch, umounting mounted volumes)

@pkerling

This comment has been minimized.

Copy link
Member

commented May 26, 2018

Because it works

Great, but I can't really know that without anyone saying anything ^^

@pkerling

This comment has been minimized.

Copy link
Member

commented May 27, 2018

Works fine for me too 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.