GNOME Bugzilla – Bug 692047
sharing: Don't show remote login on systems that don't support it
Last modified: 2013-01-31 14:12:47 UTC
It looks like the initial implementation of the remote login featured landed today but it's tied to systemd. 1. Either fallback support should be added so that enabling and disabling ssh works even without systemd, or... 2. The remote login option should be hidden when systemd isn't detected. This would be similar to what we do for the Media (or Bluetooth as in bug 691764) sharing features. #1 seems like it shouldn't be too hard. On Debian or Ubuntu, this could be done with update-rc.d enable/disable sshd and service sshd start/stop. Or alternatively, since sshd works automatically on Debian or Ubuntu simply installing sshd would turn it on and uninstalling would turn it off (sshd would probably still need to be stopped). I don't use Gentoo but I guess the syntax would be something like rc-update add/del sshd default and service sshd start/stop.
Created attachment 234110 [details] [review] sharing: hide the remote login button if the service is not available
Review of attachment 234110 [details] [review]: ::: panels/sharing/cc-remote-login.c @@ +80,1 @@ if (!path_variant) If it fails, you need to check whether it was cancelled before poking at the widgets itself, as we could be exiting the panel (especially as you're not taking references to the button or switch).
(In reply to comment #2) > Review of attachment 234110 [details] [review]: > > ::: panels/sharing/cc-remote-login.c > @@ +80,1 @@ > if (!path_variant) > > If it fails, you need to check whether it was cancelled before poking at the > widgets itself, as we could be exiting the panel (especially as you're not > taking references to the button or switch). Yes, I noticed this as well and it is an existing issue. I'll fix it in a separate patch and rebase the patch for this issue on top.
Created attachment 234196 [details] [review] sharing: hide the remote login button if the service is not available This also incorporates the widget referencing issue, as it was simpler to do in the same patch in the end.
Review of attachment 234196 [details] [review]: Use GCancellable while trying to get the proxy with g_dbus_proxy_new_for_bus () , and remove getting the bus. When the parent is destroyed, cancel the GCancellable and unref it. The proxy_finish() function would fail, and you should check that the error is not "G_IO_ERROR, G_IO_ERROR_CANCELLED" when in the error path, before hiding the button. ::: panels/sharing/cc-remote-login.c @@ +115,3 @@ + + /* hide the remote login button, since the service is not available */ + if (callback_data->button) You cannot access the button here, it might already be destroyed. @@ +193,3 @@ + if (button) + g_object_weak_ref (G_OBJECT (button), + (GWeakNotify) callback_data_weak_notify, callback_data); Looks to me like callback_data_weak_notify would be called twice, one for gtkswitch, once for button.
Created attachment 234902 [details] [review] sharing: hide the remote login button if the service is not available
(In reply to comment #5) > Review of attachment 234196 [details] [review]: > > Use GCancellable while trying to get the proxy with g_dbus_proxy_new_for_bus () > , and remove getting the bus. > When the parent is destroyed, cancel the GCancellable and unref it. > The proxy_finish() function would fail, and you should check that the error is > not "G_IO_ERROR, G_IO_ERROR_CANCELLED" when in the error path, before hiding > the button. I've added a GCancellable in the latest patch. I haven't used g_dbus_poxy_new_for_bus because in this case there are two calls to two different objects, so a single GDBusConnection is simpler than creating two GDBusProxy objects. > > ::: panels/sharing/cc-remote-login.c > @@ +115,3 @@ > + > + /* hide the remote login button, since the service is not available */ > + if (callback_data->button) > > You cannot access the button here, it might already be destroyed. This pointer is set to NULL when the button is destroyed, so this check prevents any operations on the destroyed object. > > @@ +193,3 @@ > + if (button) > + g_object_weak_ref (G_OBJECT (button), > + (GWeakNotify) callback_data_weak_notify, > callback_data); > > Looks to me like callback_data_weak_notify would be called twice, one for > gtkswitch, once for button. It is safe to call callback_data_weak_notify more than once and the latest patch improves handling of that case.
Made changes to use a cancellable right from the sharing panel instead, so that we can clear anything pending on exit. Attachment 234902 [details] pushed as 2bbac66 - sharing: hide the remote login button if the service is not available