Recommend rebooting after installing Samba
ClosedPublic

Authored by ngraham on May 28 2019, 5:06 PM.

Details

Summary

After Samba is installed, very frequently it will not work correctly until the machine is
rebooted. One potential reason is when the installed package has made group membership
changes, which only take effect after a reboot.

This patch implements a reboot recommendation along with a button to reboot the machine.
Both are shown immediately after Samba has been installed.

Because some expert users may understand the technical details of what's going on, the
reboot is only recommended, not required. If the window is closed and then re-opened,
the normal Samba sharing configuration UI is displayed instead of the reboot prompt.

Added a new function rather than using a Lambda because it may be useful for additional
purposes too (e.g. https://bugs.kde.org/show_bug.cgi?id=407846)

FEATURE: 407845
FIXED-IN: 20.04.0

Test Plan

http://s1.webmshare.com/Ry55q.webm (not uploaded to Phab due to file size limit)

  1. Remove Samba
  2. Go to share a folder
  3. Click "Install Samba"
  4. After the installation has completed, click the "Restart" button and see that the machine reboots
  1. Remove Samba again
  2. Go to share a folder
  3. Click "Install Samba"
  4. Close the window and re-open it instead of rebooting as recommended
  5. See that the Samba sharing config UI is all there

Diff Detail

Repository
R432 File Sharing (Samba) integration
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham requested review of this revision.May 28 2019, 5:06 PM
ngraham created this revision.
ngraham updated this revision to Diff 58801.May 28 2019, 5:08 PM

Revert unintentional whitespace change

bruns added a subscriber: bruns.May 28 2019, 6:03 PM

For group membership changes, only a relogin is required (at most).

The only relevant group membership here is the usershare group, which can be determined from the owner/group of the "usershare path = <dir>" directory (samba config). If the user can write to the directory already, everything is fine. If the user does not belong to a group which should be able to write to <dir>, neither a relogin nor a reboot will help.

For group membership changes, only a relogin is required (at most).

I know, but telling people to log out and log back in again involves more ways to mess up than just telling them to reboot. It's not really much slower or more invasive either.

The only relevant group membership here is the usershare group

On Arch the group name is actually sambashare. I don't believe there is a consistent standard here.

If the user does not belong to a group which should be able to write to <dir>, neither a relogin nor a reboot will help.

Right, this is tracked with https://bugs.kde.org/show_bug.cgi?id=407846 and I plan to tackle it next.

bruns added a comment.May 28 2019, 8:52 PM

For group membership changes, only a relogin is required (at most).

I know, but telling people to log out and log back in again involves more ways to mess up than just telling them to reboot. It's not really much slower or more invasive either.

I disagree. How could a relogin mess something up? Rebooting requires stopping everything outside the session. It may also require unlocking the harddisk, waiting for the computer to boot, repopulating caches ... Requesting to reboot may also give some bad press - "Rebooting, is this Windows??? ROFL!!!"

The only relevant group membership here is the usershare group

On Arch the group name is actually sambashare. I don't believe there is a consistent standard here.

The standard is to retrieve the group from the directory. KSambaShare already fetches the directory name from the samba config (to add a watch), exposing the owner group is trivial.

For group membership changes, only a relogin is required (at most).

I know, but telling people to log out and log back in again involves more ways to mess up than just telling them to reboot. It's not really much slower or more invasive either.

I disagree. How could a relogin mess something up? Rebooting requires stopping everything outside the session. It may also require unlocking the harddisk, waiting for the computer to boot, repopulating caches ... Requesting to reboot may also give some bad press - "Rebooting, is this Windows??? ROFL!!!"

I'm talking more about UX than technical concerns. Rebooting can be accomplished with a single button; logging out and back in requires a multi-step process that offers more ways for a user to mess it up.

The only relevant group membership here is the usershare group

On Arch the group name is actually sambashare. I don't believe there is a consistent standard here.

The standard is to retrieve the group from the directory. KSambaShare already fetches the directory name from the samba config (to add a watch), exposing the owner group is trivial.

How can I do that?

sitter added a subscriber: sitter.Jan 28 2020, 1:27 PM

random comment: since we install through polkit the distro should deal with this. polkit can report whether a reboot is necessary, and discover has support for that, so in theory if a distro/polkit backend reports it as necessary (because they added a new group), it should report that to polkit so discover can notify the user. so, maaaaaaaybe this shouldn't actually be decided by us at all. now whether or not we can trust distros to actually take care of this properly is another matter entirely

as for the patch: KSambaShare (part of KIO) ought to get a new function bool userCanChange() or some such which simply runs return QFileInfo(userSharePath).isWritable(). i.e. to determine whether or not a reboot is (like) required you simply need to check whether the user can currently write to the directory where the user shares are stored.
I don't think that is really blocking the diff though. it's a refinement to be sure, but given the reboot notification can be entirely ignored it's hardly a critical blocker for this diff.

LGTM, but I'd prefer if @apol weighs in.

samba/filepropertiesplugin/sambausershareplugin.h
63

shouldn't this rather be a KMessageWidget? As I recall we usually use KMWs for this type of notification.

ngraham added inline comments.Jan 28 2020, 4:39 PM
samba/filepropertiesplugin/sambausershareplugin.h
63

Yes, but I'd prefer to do that in another patch and port all the inline messages at once.

sitter added inline comments.Jan 28 2020, 4:42 PM
samba/filepropertiesplugin/sambausershareplugin.h
63

Ah! There's more. Fine with me then.

apol added a comment.Mar 11 2020, 2:48 PM

So yes, Discover will notify about updates. That doesn't mean it should be shown here too.

If anything, we should be pushing for such updates to happen at startup/shutdown because it doesn't seem like the user benefits from it being swapped at runtime, but that's a different question.

HTH

In D21466#625834, @apol wrote:

So yes, Discover will notify about updates. That doesn't mean it should be shown here too.

To clarify: are you -1 this diff?

If anything, we should be pushing for such updates to happen at startup/shutdown because it doesn't seem like the user benefits from it being swapped at runtime, but that's a different question.

"Such updates" being group changes?

apol added a comment.Mar 12 2020, 12:26 PM
In D21466#625834, @apol wrote:

So yes, Discover will notify about updates. That doesn't mean it should be shown here too.

To clarify: are you -1 this diff?

no, sorry, I meant to +1. ETOOMANYNEGATIVES.

If anything, we should be pushing for such updates to happen at startup/shutdown because it doesn't seem like the user benefits from it being swapped at runtime, but that's a different question.

"Such updates" being group changes?

I'd like to know which updates will need a reboot before updating and just move them to update offline.

Just start/restart services, reboot will not do anything else.

sitter accepted this revision.Mar 12 2020, 12:36 PM
This revision is now accepted and ready to land.Mar 12 2020, 12:36 PM
sitter edited the summary of this revision. (Show Details)Mar 13 2020, 1:00 PM
This revision was automatically updated to reflect the committed changes.