[virt-tools-list] [PATCH] virtManager: Folder sharing implementation for SPICE session
Cole Robinson
crobinso at redhat.com
Tue Jan 14 20:28:11 UTC 2020
On 12/28/19 7:12 AM, Jitao Lu wrote:
> * This implements folder sharing for the built-in Spice client, tested
> working with Win10 guest.
> * The basic idea is taken from virt-viewer.
>
> Signed-off-by: Jitao Lu <dianlujitao at gmail.com>
> ---
> Feel free to improve the patch!
>
Nice work! This looks pretty good. Some comments
* It's unfortunate that there's no way for the spice client to
communicate that necessary webdav support is even available in the VM.
Seems like a support nightmare of questions like "why isn't folder share
working". Not much we can do about that though
* Calling this 'Share folder' is too generic IMO and ungooglable, also
conflicting with passing through filesystem devices as well. So IMO the
UI label should be 'SPICE WebDAV Folder' or something along those lines
* The files should be named spicewebdav.ui and spicewebdav.py to match,
along with the object naming. Please move the python file to
virtManager/details
There's a few style warnings from './setup.py pylint'
virtManager/foldershare.py:54: [W503] line break before binary operator
virtManager/foldershare.py:60: [W503] line break before binary operator
virtManager/details/viewers.py:801: [W503] line break before binary operator
virtManager/details/viewers.py:805: [W503] line break before binary operator
And some comments below
> ui/foldershare.ui | 129 +++++++++++++++++++++++++++++++++
> ui/vmwindow.ui | 9 +++
> virtManager/details/console.py | 8 ++
> virtManager/details/viewers.py | 63 ++++++++++++++++
> virtManager/foldershare.py | 90 +++++++++++++++++++++++
> virtManager/vmwindow.py | 8 ++
> 6 files changed, 307 insertions(+)
> create mode 100644 ui/foldershare.ui
> create mode 100644 virtManager/foldershare.py
>
> diff --git a/virtManager/details/console.py b/virtManager/details/console.py
> index 193e79eb..7c0b7495 100644
> --- a/virtManager/details/console.py
> +++ b/virtManager/details/console.py
> @@ -666,6 +666,9 @@ class vmmConsolePages(vmmGObjectUI):
> bool(is_viewer and self._viewer and
> self._viewer.console_has_usb_redirection() and
> self.vm.has_spicevmc_type_redirdev()))
> + self.widget("details-menu-folder-sharing").set_sensitive(
> + bool(is_viewer and self._viewer and
> + self._viewer.console_has_folder_sharing()))
>
> can_sendkey = (is_viewer and not paused)
> for c in self._keycombo_menu.get_children():
> @@ -1012,6 +1015,11 @@ class vmmConsolePages(vmmGObjectUI):
> self._viewer.console_has_usb_redirection())
> def details_viewer_get_usb_widget(self):
> return self._viewer.console_get_usb_widget()
> + def details_viewer_has_folder_sharing(self):
> + return bool(self._viewer and
> + self._viewer.console_has_folder_sharing())
> + def details_viewer_get_folder_share_dialog(self):
> + return self._viewer.console_get_folder_share_dialog()
> def details_viewer_get_pixbuf(self):
> return self._viewer.console_get_pixbuf()
>
> diff --git a/virtManager/details/viewers.py b/virtManager/details/viewers.py
> index 1614ba6d..188ecee1 100644
> --- a/virtManager/details/viewers.py
> +++ b/virtManager/details/viewers.py
> @@ -25,6 +25,7 @@ from virtinst import log
>
> from .sshtunnels import SSHTunnels
> from ..baseclass import vmmGObject
> +from ..foldershare import vmmFolderShare
>
>
> ##################################
> @@ -207,6 +208,11 @@ class Viewer(vmmGObject):
> def _has_agent(self):
> raise NotImplementedError()
>
> + def _has_folder_sharing(self):
> + raise NotImplementedError()
> + def _get_folder_share_dialog(self):
> + raise NotImplementedError()
> +
>
> ####################################
> # APIs accessed by vmmConsolePages #
> @@ -270,6 +276,11 @@ class Viewer(vmmGObject):
> def console_has_agent(self):
> return self._has_agent()
>
> + def console_has_folder_sharing(self):
> + return self._has_folder_sharing()
> + def console_get_folder_share_dialog(self):
> + return self._get_folder_share_dialog()
> +
> def console_remove_display_from_widget(self, widget):
> if self._display and self._display in widget.get_children():
> widget.remove(self._display)
> @@ -437,6 +448,11 @@ class VNCViewer(Viewer):
> def _has_agent(self):
> return False
>
> + def _has_folder_sharing(self):
> + return False
> + def _get_folder_share_dialog(self):
> + return None
> +
>
> #######################
> # Connection routines #
> @@ -489,6 +505,8 @@ class SpiceViewer(Viewer):
> self._main_channel_hids = []
> self._display_channel = None
> self._usbdev_manager = None
> + self._webdav_channel = None
> + self._folder_share_dialog = None
>
These need to be unset() in self.close too
>
> ###################
> @@ -628,6 +646,9 @@ class SpiceViewer(Viewer):
> SpiceClientGLib.RecordChannel] and
> not self._audio):
> self._audio = SpiceClientGLib.Audio.get(self._spice_session, None)
> + elif (type(channel) == SpiceClientGLib.WebdavChannel and
> + not self._webdav_channel):
> + self._webdav_channel = channel
> > def _agent_connected_cb(self, src, val):
> self.emit("agent-connected")
> @@ -760,3 +781,45 @@ class SpiceViewer(Viewer):
> if c.__class__ is SpiceClientGLib.UsbredirChannel:
> return True
> return False
> +
> + def _has_folder_sharing(self):
> + if not self._spice_session:
> + return False
> +
I think this self._spice_session check isn't necessary
> + return self._webdav_channel is not None
> +
> + def _get_folder_share_dialog(self):
> + if not self._spice_session:
> + return
> +
> + try:
> + if not self._folder_share_dialog:
> + self._folder_share_dialog = vmmFolderShare()
> + GObject.GObject.bind_property(
> + self._spice_session, "share-dir-ro", self._folder_share_dialog,
> + vmmFolderShare.SHARE_FOLDER_RO, GObject.BindingFlags.BIDIRECTIONAL
> + | GObject.BindingFlags.SYNC_CREATE)
> + GObject.GObject.bind_property(
> + self._spice_session, "shared-dir", self._folder_share_dialog,
> + vmmFolderShare.SHARED_FOLDER, GObject.BindingFlags.BIDIRECTIONAL
> + | GObject.BindingFlags.SYNC_CREATE)
> + self._folder_share_dialog.set_property(
> + vmmFolderShare.SHARE_FOLDER,
> + self._webdav_channel.get_property("port-opened"))
> + self._folder_share_dialog.connect("notify::" + vmmFolderShare.SHARE_FOLDER,
> + self._update_share_folder)
We don't really use GObject properties elsewhere in the code. I think
this would be more clear implemented using signals:
self._webdav_folder = vmmSpiceWebdav()
self._webdav_folder.connect("share-ro-changed",
self._on_webdav_folder_share_ro_changed)
etc.
> + except Exception as e:
> + log.error("Error launching 'Share folder' dialog: %s", e)
> +
> + return self._folder_share_dialog
> +
> + def _update_share_folder(self, dialog, ignore):
> + share = dialog.get_property(vmmFolderShare.SHARE_FOLDER)
> + if share:
> + log.debug("Enabling folder sharing, shared dir: %s read-only: %r",
> + self._spice_session.get_property("shared-dir"),
> + self._spice_session.get_property("share-dir-ro"))
> + self._webdav_channel.connect()
> + else:
> + log.debug("Disabling folder sharing")
> + self._webdav_channel.disconnect(SpiceClientGLib.ChannelEvent.NONE)
> diff --git a/virtManager/foldershare.py b/virtManager/foldershare.py
> new file mode 100644
> index 00000000..c92ccab4
> --- /dev/null
> +++ b/virtManager/foldershare.py
> @@ -0,0 +1,90 @@
> +# Copyright (C) 2019 Jitao Lu <dianlujitao at gmail.com>
> +#
> +# This work is licensed under the GNU GPLv2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +from gi.repository import GLib, GObject
> +
> +from .baseclass import vmmGObjectUI
> +
> +
> +class vmmFolderShare(vmmGObjectUI):
> + SHARE_FOLDER = "share-folder"
> + SHARE_FOLDER_RO = "share-folder-ro"
> + SHARED_FOLDER = "shared-folder"
> +
> + __gproperties__ = {
> + # 'name': (GObject.TYPE_*,
> + # nickname, long desc, (type related args), mode)
> + # Type related args can be min, max for int (etc.), or default value
> + # for strings and bool
> + SHARE_FOLDER:
> + (GObject.TYPE_BOOLEAN, "Share folder",
> + "Indicates whether to share folder", False, GObject.PARAM_READWRITE),
> + SHARE_FOLDER_RO: (GObject.TYPE_BOOLEAN, "Share folder read-only",
> + "Indicates whether to share folder in read-only",
> + False, GObject.PARAM_READWRITE),
> + SHARED_FOLDER:
> + (GObject.TYPE_STRING, "Shared folder", "Indicates the shared folder",
> + GLib.get_user_special_dir(GLib.USER_DIRECTORY_PUBLIC_SHARE),
> + GObject.PARAM_READWRITE),
> + }
> +
> + def __init__(self):
> + vmmGObjectUI.__init__(self, "foldershare.ui", "vmm-folder-share")
> + self._cleanup_on_app_close()
> + self.bind_escape_key_close()
> +
> + self.builder.connect_signals({
> + "on_vmm_folder_share_delete_event":
> + self.close,
> + "on_folder_share_close_clicked":
> + self.close,
> + })
> +
> + self.share_folder = False
> + self.share_folder_ro = False
> + self.shared_folder = GLib.get_user_special_dir(
> + GLib.USER_DIRECTORY_PUBLIC_SHARE)
> +
> + self.share_folder_cb = self.widget("share-folder-cb")
If these attributes aren't intended to be accessed by the owner class,
then please prefix the names with underscore so they are private
Thanks,
Cole
More information about the virt-tools-list
mailing list