Re: [Spice-devel] [spice-gtk 06/13] win-usb-dev: remove unused procedure
On Sun, Mar 10, 2019 at 04:46:05PM +0200, Yuri Benditovich wrote:
> Remove unused g_udev_device_get_sysfs_attr.
>
Indeed, last user was apparently removed in 45cfbe8f86c in July 2013.
Acked-by: Christophe Fergeau
> Signed-off-by: Yuri Benditovich
> ---
> src/win-usb-dev.c | 18 --
> src/win-usb-dev.h | 1 -
> 2 files changed, 19 deletions(-)
>
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index d96e52a..85ffd26 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -521,24 +521,6 @@ const gchar *g_udev_device_get_property(GUdevDevice
> *udev, const gchar *property
> return NULL;
> }
>
> -const gchar *g_udev_device_get_sysfs_attr(GUdevDevice *udev, const gchar
> *attr)
> -{
> -GUdevDeviceInfo* udevinfo;
> -
> -g_return_val_if_fail(G_UDEV_DEVICE(udev), NULL);
> -g_return_val_if_fail(attr != NULL, NULL);
> -
> -udevinfo = udev->priv->udevinfo;
> -g_return_val_if_fail(udevinfo != NULL, NULL);
> -
> -
> -if (g_strcmp0(attr, "bDeviceClass") == 0) {
> -return udevinfo->sclass;
> -}
> -g_warn_if_reached();
> -return NULL;
> -}
> -
> #ifdef DEBUG_GUDEV_DEVICE_LISTS
> static void g_udev_device_print_list(GList *l, const gchar *msg)
> {
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index f3c7466..b7b7eda 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -85,7 +85,6 @@ GList *g_udev_client_query_by_subsystem(GUdevClient
> *client, const gchar *subsys
>
> GType g_udev_device_get_type(void) G_GNUC_CONST;
> const gchar *g_udev_device_get_property(GUdevDevice *udev, const gchar
> *property);
> -const gchar *g_udev_device_get_sysfs_attr(GUdevDevice *udev, const gchar
> *attr);
>
> GQuark g_udev_client_error_quark(void);
> #define G_UDEV_CLIENT_ERROR g_udev_client_error_quark()
> --
> 2.17.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 04/13] usb-redir: do not add device if one with the same bus:addr exists
On Sun, Mar 10, 2019 at 04:46:03PM +0200, Yuri Benditovich wrote:
> In initial device enumeration hotplug notification can be
> called twice with the same libusb device. For details, see
> http://libusb.sourceforge.net/api-1.0/group__libusb__hotplug.html#ga00e0c69ddf1fb1b6774dc918192e8dc7
> Filter out devices that already present in the list.
> Remove indentical call in spice_usb_device_manager_add_udev,
> which add devices under Windows.
>
> Signed-off-by: Yuri Benditovich
> ---
> src/usb-device-manager.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index debba4d..5cf7ebb 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -962,6 +962,17 @@ static void
> spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> return;
>
> +if (spice_usb_device_manager_find_device(self,
> +libusb_get_bus_number(libdev),
> +libusb_get_device_address(libdev))) {
Forgot to mention that indentation is slightly off.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 04/13] usb-redir: do not add device if one with the same bus:addr exists
On Sun, Mar 10, 2019 at 04:46:03PM +0200, Yuri Benditovich wrote:
> In initial device enumeration hotplug notification can be
"In initial device enumeration, hotplug notification..."
> called twice with the same libusb device. For details, see
> http://libusb.sourceforge.net/api-1.0/group__libusb__hotplug.html#ga00e0c69ddf1fb1b6774dc918192e8dc7
> Filter out devices that already present in the list.
"This commit filters out the devices that are already known to
SpiceUsbDeviceManager."
> Remove indentical call in spice_usb_device_manager_add_udev,
> which add devices under Windows.
"It also removes the identical call ..., which adds devices..."
Acked-by: Christophe Fergeau
>
> Signed-off-by: Yuri Benditovich
> ---
> src/usb-device-manager.c | 22 +++---
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index debba4d..5cf7ebb 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -962,6 +962,17 @@ static void
> spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> return;
>
> +if (spice_usb_device_manager_find_device(self,
> +libusb_get_bus_number(libdev),
> +libusb_get_device_address(libdev))) {
> +SPICE_DEBUG("device not added %d:%d %04x:%04x",
> +libusb_get_bus_number(libdev),
> +libusb_get_device_address(libdev),
> +desc.idVendor,
> +desc.idProduct);
> +return;
> +}
> +
> device = (SpiceUsbDevice*)spice_usb_device_new(libdev);
> if (!device)
> return;
> @@ -1025,7 +1036,6 @@ static void
> spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> {
> SpiceUsbDeviceManagerPrivate *priv = self->priv;
> libusb_device *libdev = NULL, **dev_list = NULL;
> -SpiceUsbDevice *device;
> const gchar *devtype;
> int i, bus, address;
>
> @@ -1039,16 +1049,6 @@ static void
> spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> return;
> }
>
> -device = spice_usb_device_manager_find_device(self, bus, address);
> -if (device) {
> -SPICE_DEBUG("USB device 0x%04x:0x%04x at %d.%d already exists,
> ignored",
> -spice_usb_device_get_vid(device),
> -spice_usb_device_get_pid(device),
> -spice_usb_device_get_busnum(device),
> -spice_usb_device_get_devaddr(device));
> -return;
> -}
> -
> if (priv->coldplug_list)
> dev_list = priv->coldplug_list;
> else
> --
> 2.17.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 03/13] usb-redir: reuse libusb context under Windows
On Sun, Mar 10, 2019 at 04:46:02PM +0200, Yuri Benditovich wrote:
> Do not create own libusb context in usb-device-manager.
> Reuse existing context created by win-usb-dev instead.
I'd rephrase this slightly
« On Windows, do not create a new libusb context in
usb-device-manager.c, but reuse the existing one created by
win-usb-dev.c » + an explanation why we want to do that.
Iirc, we need a shared libusb_context if we want to share libusb_device
between the 2 files, so I'd add that to the commit log.
Apart from this,
Acked-by: Christophe Fergeau
>
> Signed-off-by: Yuri Benditovich
> ---
> src/usb-device-manager.c | 11 +--
> src/win-usb-dev.c| 4
> src/win-usb-dev.h| 1 +
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 6435be8..debba4d 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -282,8 +282,9 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> SpiceUsbDeviceManagerPrivate *priv = self->priv;
> GList *list;
> GList *it;
> -int rc;
>
> +#ifndef G_OS_WIN32
> +int rc;
> /* Initialize libusb */
> rc = libusb_init(&priv->context);
> if (rc < 0) {
> @@ -293,11 +294,6 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> "Error initializing USB support: %s [%i]", desc, rc);
> return FALSE;
> }
> -
> -#ifdef G_OS_WIN32
> -#if LIBUSB_API_VERSION >= 0x01000106
> -libusb_set_option(priv->context, LIBUSB_OPTION_USE_USBDK);
> -#endif
> #endif
>
> /* Start listening for usb devices plug / unplug */
> @@ -307,6 +303,7 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> g_warning("Error initializing GUdevClient");
> return FALSE;
> }
> +priv->context = g_udev_client_get_context(priv->udev);
> g_signal_connect(G_OBJECT(priv->udev), "uevent",
> G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
> /* Do coldplug (detection of already connected devices) */
> @@ -402,8 +399,10 @@ static void spice_usb_device_manager_finalize(GObject
> *gobject)
> g_clear_object(&priv->udev);
> #endif
> g_return_if_fail(priv->event_thread == NULL);
> +#ifndef G_OS_WIN32
> if (priv->context)
> libusb_exit(priv->context);
> +#endif
> free(priv->auto_conn_filter_rules);
> free(priv->redirect_on_connect_rules);
> #ifdef G_OS_WIN32
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index d0eae06..a8d922f 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -113,6 +113,10 @@ GUdevClient *g_udev_client_new(void)
> return singleton;
> }
>
> +libusb_context *g_udev_client_get_context(GUdevClient *client)
> +{
> +return client->priv->ctx;
> +}
>
> /*
> * devs [in,out] an empty devs list in, full devs list out
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index 0f34a01..f3c7466 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -80,6 +80,7 @@ struct _GUdevClientClass
>
> GType g_udev_client_get_type(void) G_GNUC_CONST;
> GUdevClient *g_udev_client_new(void);
> +libusb_context *g_udev_client_get_context(GUdevClient *client);
> GList *g_udev_client_query_by_subsystem(GUdevClient *client, const gchar
> *subsystem);
>
> GType g_udev_device_get_type(void) G_GNUC_CONST;
> --
> 2.17.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 02/13] usb-redir: remove unused 'subsystem' parameter
On Sun, Mar 10, 2019 at 04:46:01PM +0200, Yuri Benditovich wrote:
> Removing unused parameter for GUdevClient constructor.
I'd explicitly mention that it's possible because we no longer use the
external libgudev, and at this point g_udev_client_new() is just
internal API used by the windows code.
Apart from this minor comment,
Acked-by: Christophe Fergeau
Christophe
>
> Signed-off-by: Yuri Benditovich
> ---
> src/usb-device-manager.c | 5 +
> src/win-usb-dev.c| 2 +-
> src/win-usb-dev.h| 2 +-
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 6a36cfa..6435be8 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -283,9 +283,6 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> GList *list;
> GList *it;
> int rc;
> -#ifdef G_OS_WIN32
> -const gchar *const subsystems[] = {"usb", NULL};
> -#endif
>
> /* Initialize libusb */
> rc = libusb_init(&priv->context);
> @@ -305,7 +302,7 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
>
> /* Start listening for usb devices plug / unplug */
> #ifdef G_OS_WIN32
> -priv->udev = g_udev_client_new(subsystems);
> +priv->udev = g_udev_client_new();
> if (priv->udev == NULL) {
> g_warning("Error initializing GUdevClient");
> return FALSE;
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 327976d..d0eae06 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -104,7 +104,7 @@ GQuark g_udev_client_error_quark(void)
> return g_quark_from_static_string("win-gudev-client-error-quark");
> }
>
> -GUdevClient *g_udev_client_new(const gchar* const *subsystems)
> +GUdevClient *g_udev_client_new(void)
> {
> if (singleton != NULL)
> return g_object_ref(singleton);
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index 7f40197..0f34a01 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -79,7 +79,7 @@ struct _GUdevClientClass
> };
>
> GType g_udev_client_get_type(void) G_GNUC_CONST;
> -GUdevClient *g_udev_client_new(const gchar* const *subsystems);
> +GUdevClient *g_udev_client_new(void);
> GList *g_udev_client_query_by_subsystem(GUdevClient *client, const gchar
> *subsystem);
>
> GType g_udev_device_get_type(void) G_GNUC_CONST;
> --
> 2.17.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk 01/13] usb-redir: replace USE_GUDEV with G_OS_WIN32
Acked-by: Christophe Fergeau
On Sun, Mar 10, 2019 at 04:46:00PM +0200, Yuri Benditovich wrote:
> Replacing USE_GUDEV with G_OS_WIN32 anywhere. GUDEV simulation
> is used only in Windows build.
>
> Signed-off-by: Yuri Benditovich
> ---
> src/usb-device-manager.c | 26 +++---
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 2578350..6a36cfa 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -29,11 +29,7 @@
>
> #ifdef G_OS_WIN32
> #include "usbdk_api.h"
> -#endif
> -
> -#if defined(G_OS_WIN32)
> #include "win-usb-dev.h"
> -#define USE_GUDEV /* win-usb-dev.h provides a fake gudev interface */
> #endif
>
> #include "channel-usbredir-priv.h"
> @@ -106,7 +102,7 @@ struct _SpiceUsbDeviceManagerPrivate {
> struct usbredirfilter_rule *redirect_on_connect_rules;
> int auto_conn_filter_rules_count;
> int redirect_on_connect_rules_count;
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> GUdevClient *udev;
> libusb_device **coldplug_list; /* Avoid needless reprobing during init */
> #else
> @@ -156,7 +152,7 @@ static void channel_destroy(SpiceSession *session,
> SpiceChannel *channel,
> gpointer user_data);
> static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
>gpointer user_data);
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> const gchar *action,
> GUdevDevice *udevice,
> @@ -210,7 +206,7 @@ G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
> static void
> _set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
> {
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
> #else
> self->priv->redirecting = is_redirecting;
> @@ -235,7 +231,7 @@ gboolean
> spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager *self)
> {
> #ifdef USE_USBREDIR
>
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> gboolean redirecting;
> g_object_get(self->priv->udev, "redirecting", &redirecting, NULL);
> return redirecting;
> @@ -287,7 +283,7 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> GList *list;
> GList *it;
> int rc;
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> const gchar *const subsystems[] = {"usb", NULL};
> #endif
>
> @@ -308,7 +304,7 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> #endif
>
> /* Start listening for usb devices plug / unplug */
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> priv->udev = g_udev_client_new(subsystems);
> if (priv->udev == NULL) {
> g_warning("Error initializing GUdevClient");
> @@ -366,7 +362,7 @@ static void spice_usb_device_manager_dispose(GObject
> *gobject)
> SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(gobject);
> SpiceUsbDeviceManagerPrivate *priv = self->priv;
>
> -#ifndef USE_GUDEV
> +#ifndef G_OS_WIN32
> if (priv->hp_handle) {
> spice_usb_device_manager_stop_event_listening(self);
> if (g_atomic_int_get(&priv->event_thread_run)) {
> @@ -405,7 +401,7 @@ static void spice_usb_device_manager_finalize(GObject
> *gobject)
> g_ptr_array_unref(priv->devices);
>
> #ifdef USE_USBREDIR
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> g_clear_object(&priv->udev);
> #endif
> g_return_if_fail(priv->event_thread == NULL);
> @@ -737,7 +733,7 @@ static void
> spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
> /* -- */
> /* gudev / libusb Helper functions*/
>
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> static gboolean spice_usb_device_manager_get_udev_bus_n_address(
> SpiceUsbDeviceManager *manager, GUdevDevice *udev,
> int *bus, int *address)
> @@ -927,7 +923,7 @@
> spice_usb_device_manager_device_match(SpiceUsbDeviceManager *self,
> SpiceUsbDevic
> spice_usb_device_get_devaddr(device) == address);
> }
>
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> static gboolean
> spice_usb_device_manager_libdev_match(SpiceUsbDeviceManager *self,
> libusb_device *libdev,
>const int bus, const int address)
> @@ -1027,7 +1023,7 @@ static void
> spice_usb_device_manager_remove_dev(SpiceUsbDeviceManager *self,
> spice_usb_device_unref(device);
> }
>
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
>GUdevDevice*udev)
> {
> --
> 2.17.1
>
> ___
> Spice-devel
Re: [Spice-devel] [PATCH spice-common v3 0/5] Generate C declarations automatically
> > Series looks good to me, > > Reviewed-by: Christophe Fergeau > Why not ack? Not good enough? Not tested? Missing something? > On Mon, Mar 11, 2019 at 01:59:09PM +, Frediano Ziglio wrote: > > Allows to declare C declarations (that were in common/messages.h) > > automatically. > > I added an attribute to: > > - be compatible; > > - allows to declare structure in different way than automatic > > ones; > > - allows to use different headers (as draw.h). > > > > Changes since v2: > > - use a --generate-header option instead of --generated-header not passing > > file name but infere from output file name; > > - remove some TODOs not much related; > > - commit messages improvements. > > > > Frediano Ziglio (5): > > codegen: Factor out a function to write output file > > codegen: Generate headers while generating code > > codegen: Allows to generate C declarations automatically > > Allow to generate C declarations for spice.proto > > Generate automatically most C message declarations > > > > common/Makefile.am | 22 +- > > common/client_marshallers.h | 2 +- > > common/meson.build | 27 +- > > common/messages.h | 493 +--- > > python_modules/ptypes.py| 64 + > > spice.proto | 192 +++--- > > spice_codegen.py| 108 ++-- > > 7 files changed, 284 insertions(+), 624 deletions(-) > > Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-common v3 0/5] Generate C declarations automatically
Series looks good to me, Reviewed-by: Christophe Fergeau On Mon, Mar 11, 2019 at 01:59:09PM +, Frediano Ziglio wrote: > Allows to declare C declarations (that were in common/messages.h) > automatically. > I added an attribute to: > - be compatible; > - allows to declare structure in different way than automatic > ones; > - allows to use different headers (as draw.h). > > Changes since v2: > - use a --generate-header option instead of --generated-header not passing > file name but infere from output file name; > - remove some TODOs not much related; > - commit messages improvements. > > Frediano Ziglio (5): > codegen: Factor out a function to write output file > codegen: Generate headers while generating code > codegen: Allows to generate C declarations automatically > Allow to generate C declarations for spice.proto > Generate automatically most C message declarations > > common/Makefile.am | 22 +- > common/client_marshallers.h | 2 +- > common/meson.build | 27 +- > common/messages.h | 493 +--- > python_modules/ptypes.py| 64 + > spice.proto | 192 +++--- > spice_codegen.py| 108 ++-- > 7 files changed, 284 insertions(+), 624 deletions(-) > > -- > 2.20.1 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 1/2] docs: Fix typo
Signed-off-by: Frediano Ziglio --- docs/spice_threading_model.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/spice_threading_model.txt b/docs/spice_threading_model.txt index df4922168..9351141c8 100644 --- a/docs/spice_threading_model.txt +++ b/docs/spice_threading_model.txt @@ -1,7 +1,7 @@ How does SPICE handle threading --- -Lots of code run in a single thread. +Lots of code runs in a single thread. Qemu usually calls SPICE from its main thread except on callbacks to code already running in different threads. -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading
Signed-off-by: Frediano Ziglio --- docs/spice_threading_model.txt | 8 1 file changed, 8 insertions(+) diff --git a/docs/spice_threading_model.txt b/docs/spice_threading_model.txt index 9351141c8..25a3a030c 100644 --- a/docs/spice_threading_model.txt +++ b/docs/spice_threading_model.txt @@ -39,6 +39,14 @@ connect, disconnect and migrate. Connect and migrate are asynchronous (the job is done while the current thread is doing something else) while disconnect is synchronous (the main thread will wait for termination). +One aspect to take into consideration is the event scheduling. SPICE uses some +`SpiceCoreInterface` to handle events. As the events will be handled from a +thread based on the core interface you have to use the correct core. Each +channel has an associated core interface which can be retrieved using +`red_channel_get_core_interface`. There's also a main core interface you can get +using `reds_get_core_interface`. `reds_core_timer_*` and `reds_core_watch_*` +functions use the main core interface. + Reference counting and ownership -> pointer -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-common v3 5/5] Generate automatically most C message declarations
Signed-off-by: Frediano Ziglio
---
common/messages.h | 495 +-
spice.proto | 192 +-
2 files changed, 102 insertions(+), 585 deletions(-)
diff --git a/common/messages.h b/common/messages.h
index 36ee59d..5cda1d1 100644
--- a/common/messages.h
+++ b/common/messages.h
@@ -42,10 +42,6 @@
SPICE_BEGIN_DECLS
-typedef struct SpiceMsgData {
-uint8_t data[0];
-} SpiceMsgData;
-
typedef struct SpiceMsgCompressedData {
uint8_t type;
uint32_t uncompressed_size;
@@ -53,438 +49,8 @@ typedef struct SpiceMsgCompressedData {
uint8_t *compressed_data;
} SpiceMsgCompressedData;
-typedef struct SpiceMsgEmpty {
-uint8_t padding;
-} SpiceMsgEmpty;
-
-typedef struct SpiceMsgInputsInit {
-uint32_t keyboard_modifiers;
-} SpiceMsgInputsInit;
-
-typedef struct SpiceMsgInputsKeyModifiers {
-uint32_t modifiers;
-} SpiceMsgInputsKeyModifiers;
-
-typedef struct SpiceMsgMainMultiMediaTime {
-uint32_t time;
-} SpiceMsgMainMultiMediaTime;
-
-typedef struct SpiceMigrationDstInfo {
-uint16_t port;
-uint16_t sport;
-uint32_t host_size;
-uint8_t *host_data;
-uint32_t cert_subject_size;
-uint8_t *cert_subject_data;
-} SpiceMigrationDstInfo;
-
-typedef struct SpiceMsgMainMigrationBegin {
-SpiceMigrationDstInfo dst_info;
-} SpiceMsgMainMigrationBegin;
-
-typedef struct SpiceMsgMainMigrateBeginSeamless {
-SpiceMigrationDstInfo dst_info;
-uint32_t src_mig_version;
-} SpiceMsgMainMigrateBeginSeamless;
-
-typedef struct SpiceMsgcMainMigrateDstDoSeamless {
-uint32_t src_version;
-} SpiceMsgcMainMigrateDstDoSeamless;
-
-typedef struct SpiceMsgMainMigrationSwitchHost {
-uint16_t port;
-uint16_t sport;
-uint32_t host_size;
-uint8_t *host_data;
-uint32_t cert_subject_size;
-uint8_t *cert_subject_data;
-} SpiceMsgMainMigrationSwitchHost;
-
-
-typedef struct SpiceMsgMigrate {
-uint32_t flags;
-} SpiceMsgMigrate;
-
-typedef struct SpiceResourceID {
-uint8_t type;
-uint64_t id;
-} SpiceResourceID;
-
-typedef struct SpiceResourceList {
-uint16_t count;
-SpiceResourceID resources[0];
-} SpiceResourceList;
-
-typedef struct SpiceMsgSetAck {
-uint32_t generation;
-uint32_t window;
-} SpiceMsgSetAck;
-
-typedef struct SpiceMsgcAckSync {
- uint32_t generation;
-} SpiceMsgcAckSync;
-
-typedef struct SpiceWaitForChannel {
-uint8_t channel_type;
-uint8_t channel_id;
-uint64_t message_serial;
-} SpiceWaitForChannel;
-
-typedef struct SpiceMsgWaitForChannels {
-uint8_t wait_count;
-SpiceWaitForChannel wait_list[0];
-} SpiceMsgWaitForChannels;
-
-typedef struct SpiceChannelId {
-uint8_t type;
-uint8_t id;
-} SpiceChannelId;
-
-typedef struct SpiceMsgMainInit {
-uint32_t session_id;
-uint32_t display_channels_hint;
-uint32_t supported_mouse_modes;
-uint32_t current_mouse_mode;
-uint32_t agent_connected;
-uint32_t agent_tokens;
-uint32_t multi_media_time;
-uint32_t ram_hint;
-} SpiceMsgMainInit;
-
-typedef struct SpiceMsgDisconnect {
-uint64_t time_stamp;
-uint32_t reason; // SPICE_ERR_?
-} SpiceMsgDisconnect;
-
-typedef struct SpiceMsgNotify {
-uint64_t time_stamp;
-uint32_t severity;
-uint32_t visibilty;
-uint32_t what;
-uint32_t message_len;
-uint8_t message[0];
-} SpiceMsgNotify;
-
-typedef struct SpiceMsgChannels {
-uint32_t num_of_channels;
-SpiceChannelId channels[0];
-} SpiceMsgChannels;
-
-typedef struct SpiceMsgMainName {
-uint32_t name_len;
-uint8_t name[0];
-} SpiceMsgMainName;
-
-typedef struct SpiceMsgMainUuid {
-uint8_t uuid[16];
-} SpiceMsgMainUuid;
-
-typedef struct SpiceMsgMainMouseMode {
-uint32_t supported_modes;
-uint32_t current_mode;
-} SpiceMsgMainMouseMode;
-
-typedef struct SpiceMsgPing {
-uint32_t id;
-uint64_t timestamp;
-void *data;
-uint32_t data_len;
-} SpiceMsgPing;
-
-typedef struct SpiceMsgMainAgentDisconnect {
-uint32_t error_code; // SPICE_ERR_?
-} SpiceMsgMainAgentDisconnect;
-
#define SPICE_AGENT_MAX_DATA_SIZE 2048
-typedef struct SpiceMsgMainAgentTokens {
-uint32_t num_tokens;
-} SpiceMsgMainAgentTokens, SpiceMsgcMainAgentTokens, SpiceMsgcMainAgentStart;
-
-typedef struct SpiceMsgMainAgentTokens SpiceMsgMainAgentConnectedTokens;
-
-typedef struct SpiceMsgcClientInfo {
-uint64_t cache_size;
-} SpiceMsgcClientInfo;
-
-typedef struct SpiceMsgcMainMouseModeRequest {
-uint32_t mode;
-} SpiceMsgcMainMouseModeRequest;
-
-typedef struct SpiceCursor {
-uint32_t flags;
-SpiceCursorHeader header;
-uint32_t data_size;
-uint8_t *data;
-} SpiceCursor;
-
-typedef struct SpiceMsgDisplayMode {
-uint32_t x_res;
-uint32_t y_res;
-uint32_t bits;
-} SpiceMsgDisplayMode;
-
-typedef struct SpiceMsgSurfaceCreate {
-uint32_t surface_id;
-uint32_t width;
-uint32_t height;
-uint32_t format;
-uint32_t flags;
-} SpiceMsgSurfaceCreate;
-
-typedef struct SpiceMsgS
[Spice-devel] [PATCH spice-common v3 2/5] codegen: Generate headers while generating code
Python script generates code and header together however allowed to save only one of them. Allows to save both of them together to reduce number of time we call Python script. Signed-off-by: Frediano Ziglio --- common/Makefile.am | 16 ++-- common/client_marshallers.h | 2 +- common/meson.build | 17 + spice_codegen.py| 15 +-- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/common/Makefile.am b/common/Makefile.am index 3318009..0ad448b 100644 --- a/common/Makefile.am +++ b/common/Makefile.am @@ -112,21 +112,17 @@ MARSHALLERS_DEPS = \ generated_client_demarshallers.c: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-demarshallers --client --include common/messages.h $< $@ >/dev/null -generated_client_marshallers.h: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) - $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers -P --client --include common/messages.h -H $< $@ >/dev/null - -generated_client_marshallers.c: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) - $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers -P --include client_marshallers.h --client $< $@ >/dev/null +generated_client_marshallers.c generated_client_marshallers.h: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) + $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers -P --include client_marshallers.h --client \ + --generate-header $< $@ >/dev/null generated_server_demarshallers.c: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-demarshallers --server --include common/messages.h $< $@ >/dev/null STRUCTS = -M String -M Rect -M Point -M DisplayBase -M Fill -M Opaque -M Copy -M Blend -M Blackness -M Whiteness -M Invers -M Rop3 -M Stroke -M Text -M Transparent -M AlphaBlend -M Composite -generated_server_marshallers.c: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) - $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers $(STRUCTS) --server --include common/messages.h $< $@ >/dev/null - -generated_server_marshallers.h: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) - $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers $(STRUCTS) --server --include common/messages.h -H $< $@ >/dev/null +generated_server_marshallers.c generated_server_marshallers.h: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS) + $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py --generate-marshallers $(STRUCTS) --server --include common/messages.h \ +--generate-header $< $@ >/dev/null EXTRA_DIST = \ $(CLIENT_MARSHALLERS) \ diff --git a/common/client_marshallers.h b/common/client_marshallers.h index f082934..b67b98e 100644 --- a/common/client_marshallers.h +++ b/common/client_marshallers.h @@ -21,9 +21,9 @@ #include +#include "messages.h" #include "common/generated_client_marshallers.h" #include "marshaller.h" -#include "messages.h" SPICE_BEGIN_DECLS diff --git a/common/meson.build b/common/meson.build index 156297b..b1e58c5 100644 --- a/common/meson.build +++ b/common/meson.build @@ -65,8 +65,13 @@ spice_common_dep = declare_dependency(link_with : spice_common_lib, if spice_common_generate_client_code targets = [ ['client_demarshallers', spice_proto, 'generated_client_demarshallers.c', ['--generate-demarshallers', '--client', '--include', 'common/messages.h', '@INPUT@', '@OUTPUT@']], -['client_marshalers', spice_proto, 'generated_client_marshallers.c', ['--generate-marshallers', '-P', '--client', '--include', 'client_marshallers.h', '@INPUT@', '@OUTPUT@']], -['client_marshallers_h', spice_proto, 'generated_client_marshallers.h', ['--generate-marshallers', '-P', '--client', '--include', 'common/messages.h', '-H', '@INPUT@', '@OUTPUT@']], +['client_marshallers', spice_proto, + ['generated_client_marshallers.c', 'generated_client_marshallers.h'], + ['--generate-marshallers', '--generate-header', +'-P', '--client', '--include', 'client_marshallers.h', +'@INPUT@', '@OUTPUT0@' + ] +] ] spice_common_client_sources = [ @@ -116,8 +121,12 @@ if spice_common_generate_server_code targets = [ ['server_demarshallers', spice_proto, 'generated_server_demarshallers.c', ['--generate-demarshallers', '--server', '--include', 'common/messages.h', '@INPUT@', '@OUTPUT@']], -['server_marshallers', spice_proto, 'generated_server_marshallers.c', ['--generate-marshallers', '--server'] + structs_args + ['--include', 'common/messages.h', '@INPUT@', '@OUTPUT@']], -['server_marshallers_h', spice_proto, 'generated_server_marshallers.h', ['--generate-marshallers', '--server'] + structs_args + ['--include', 'common/messages.h',
[Spice-devel] [PATCH spice-common v3 0/5] Generate C declarations automatically
Allows to declare C declarations (that were in common/messages.h) automatically. I added an attribute to: - be compatible; - allows to declare structure in different way than automatic ones; - allows to use different headers (as draw.h). Changes since v2: - use a --generate-header option instead of --generated-header not passing file name but infere from output file name; - remove some TODOs not much related; - commit messages improvements. Frediano Ziglio (5): codegen: Factor out a function to write output file codegen: Generate headers while generating code codegen: Allows to generate C declarations automatically Allow to generate C declarations for spice.proto Generate automatically most C message declarations common/Makefile.am | 22 +- common/client_marshallers.h | 2 +- common/meson.build | 27 +- common/messages.h | 493 +--- python_modules/ptypes.py| 64 + spice.proto | 192 +++--- spice_codegen.py| 108 ++-- 7 files changed, 284 insertions(+), 624 deletions(-) -- 2.20.1 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-common v3 1/5] codegen: Factor out a function to write output file
This will be reused to generate C declaration automatically.
Signed-off-by: Frediano Ziglio
Acked-by: Christophe Fergeau
---
spice_codegen.py | 46 +-
1 file changed, 25 insertions(+), 21 deletions(-)
diff --git a/spice_codegen.py b/spice_codegen.py
index f3190da..8049820 100755
--- a/spice_codegen.py
+++ b/spice_codegen.py
@@ -105,6 +105,30 @@ def write_enums(writer, describe=False):
writer.writeln("#endif /* _H_SPICE_ENUMS */")
+def write_content(dest_file, content, keep_identical_file):
+if keep_identical_file:
+try:
+f = open(dest_file, 'rb')
+old_content = f.read()
+f.close()
+
+if content == old_content:
+six.print_("No changes to %s" % dest_file)
+return
+
+except IOError:
+pass
+
+f = open(dest_file, 'wb')
+if six.PY3:
+f.write(bytes(content, 'UTF-8'))
+else:
+f.write(content)
+f.close()
+
+six.print_("Wrote %s" % dest_file)
+
+
parser = OptionParser(usage="usage: %prog [options]
")
parser.add_option("-e", "--generate-enums",
action="store_true", dest="generate_enums", default=False,
@@ -286,25 +310,5 @@ if options.header:
content = writer.header.getvalue()
else:
content = writer.getvalue()
-if options.keep_identical_file:
-try:
-f = open(dest_file, 'rb')
-old_content = f.read()
-f.close()
-
-if content == old_content:
-six.print_("No changes to %s" % dest_file)
-sys.exit(0)
-
-except IOError:
-pass
-
-f = open(dest_file, 'wb')
-if six.PY3:
-f.write(bytes(content, 'UTF-8'))
-else:
-f.write(content)
-f.close()
-
-six.print_("Wrote %s" % dest_file)
+write_content(dest_file, content, options.keep_identical_file)
sys.exit(0)
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-common v3 4/5] Allow to generate C declarations for spice.proto
Generate and include C declarations.
Next patch will use this facility.
Since none of the spice.proto types are decorated with @declare,
adding the #include in messages.h won't have any bad consequences.
Signed-off-by: Frediano Ziglio
---
common/Makefile.am | 6 --
common/meson.build | 10 +-
common/messages.h | 2 ++
3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/common/Makefile.am b/common/Makefile.am
index 0ad448b..44b2616 100644
--- a/common/Makefile.am
+++ b/common/Makefile.am
@@ -109,8 +109,9 @@ MARSHALLERS_DEPS = \
# Note despite being autogenerated these are not part of CLEANFILES, they are
# actually a part of EXTRA_DIST, to avoid the need for pyparser by end users
-generated_client_demarshallers.c: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
- $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py
--generate-demarshallers --client --include common/messages.h $< $@ >/dev/null
+generated_client_demarshallers.c generated_messages.h:
$(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
+ $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py
--generate-demarshallers --client --include common/messages.h \
+ --generated-declaration-file generated_messages.h $< $@ >/dev/null
generated_client_marshallers.c generated_client_marshallers.h:
$(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
$(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py
--generate-marshallers -P --include client_marshallers.h --client \
@@ -127,6 +128,7 @@ generated_server_marshallers.c
generated_server_marshallers.h: $(top_srcdir)/spi
EXTRA_DIST = \
$(CLIENT_MARSHALLERS) \
$(SERVER_MARSHALLERS) \
+ generated_messages.h\
meson.build \
canvas_base.c \
canvas_base.h \
diff --git a/common/meson.build b/common/meson.build
index b1e58c5..2d76d2b 100644
--- a/common/meson.build
+++ b/common/meson.build
@@ -64,7 +64,15 @@ spice_common_dep = declare_dependency(link_with :
spice_common_lib,
#
if spice_common_generate_client_code
targets = [
-['client_demarshallers', spice_proto, 'generated_client_demarshallers.c',
['--generate-demarshallers', '--client', '--include', 'common/messages.h',
'@INPUT@', '@OUTPUT@']],
+['client_demarshallers', spice_proto,
+ ['generated_client_demarshallers.c', 'generated_messages.h'],
+ ['--generate-demarshallers',
+'--client',
+'--include', 'common/messages.h',
+'--generated-declaration-file', '@OUTPUT1@',
+'@INPUT@', '@OUTPUT0@'
+ ]
+],
['client_marshallers', spice_proto,
['generated_client_marshallers.c', 'generated_client_marshallers.h'],
['--generate-marshallers', '--generate-header',
diff --git a/common/messages.h b/common/messages.h
index f740a8c..36ee59d 100644
--- a/common/messages.h
+++ b/common/messages.h
@@ -558,6 +558,8 @@ typedef struct SpiceMsgDisplayGlDraw {
uint32_t h;
} SpiceMsgDisplayGlDraw;
+#include
+
SPICE_END_DECLS
#endif // H_SPICE_COMMON_MESSAGES
--
2.20.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-common v3 3/5] codegen: Allows to generate C declarations automatically
Allows to specify a @declare attribute for messages and structure
that can generate the needed C structures.
Signed-off-by: Frediano Ziglio
---
python_modules/ptypes.py | 64
spice_codegen.py | 47 +
2 files changed, 111 insertions(+)
diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
index 05c594e..b08170e 100644
--- a/python_modules/ptypes.py
+++ b/python_modules/ptypes.py
@@ -70,6 +70,8 @@ valid_attributes=set([
'zero',
# this attribute does not exist on the network, fill just structure with
the value
'virtual',
+# generate C structure declarations from protocol definition
+'declare',
])
attributes_with_arguments=set([
@@ -483,6 +485,26 @@ class ArrayType(Type):
def c_type(self):
return self.element_type.c_type()
+def generate_c_declaration(self, writer, member):
+name = member.name
+if member.has_attr("chunk"):
+return writer.writeln('SpiceChunks *%s;' % name)
+if member.has_attr("as_ptr"):
+len_var = member.attributes["as_ptr"][0]
+writer.writeln('uint32_t %s;' % len_var)
+return writer.writeln('%s *%s;' % (self.c_type(), name))
+if member.has_attr("to_ptr"):
+return writer.writeln('%s *%s;' % (self.c_type(), name))
+if member.has_attr("ptr_array"):
+return writer.writeln('%s *%s[0];' % (self.c_type(), name))
+if member.has_end_attr() or self.is_remaining_length():
+return writer.writeln('%s %s[0];' % (self.c_type(), name))
+if self.is_constant_length():
+return writer.writeln('%s %s[%s];' % (self.c_type(), name,
self.size))
+if self.is_identifier_length():
+return writer.writeln('%s *%s;' % (self.c_type(), name))
+raise NotImplementedError('unknown array %s' % str(self))
+
class PointerType(Type):
def __init__(self, target_type):
Type.__init__(self)
@@ -517,6 +539,15 @@ class PointerType(Type):
def get_num_pointers(self):
return 1
+def generate_c_declaration(self, writer, member):
+target_type = self.target_type
+is_array = target_type.is_array()
+if not is_array or target_type.is_identifier_length():
+writer.writeln("%s *%s;" % (target_type.c_type(), member.name))
+return
+raise NotImplementedError('Some pointers to array declarations are not
implemented %s' %
+member)
+
class Containee:
def __init__(self):
self.attributes = {}
@@ -612,6 +643,14 @@ class Member(Containee):
names = [prefix + "_" + name for name in names]
return names
+def generate_c_declaration(self, writer):
+if self.has_attr("zero"):
+return
+if self.is_pointer() or self.is_array():
+self.member_type.generate_c_declaration(writer, self)
+return
+return writer.writeln("%s %s;" % (self.member_type.c_type(),
self.name))
+
class SwitchCase:
def __init__(self, values, member):
self.values = values
@@ -735,6 +774,17 @@ class Switch(Containee):
names = names + c.get_pointer_names(marshalled)
return names
+def generate_c_declaration(self, writer):
+if self.has_attr("anon") and len(self.cases) == 1:
+self.cases[0].member.generate_c_declaration(writer)
+return
+writer.writeln('union {')
+writer.indent()
+for m in self.cases:
+m.member.generate_c_declaration(writer)
+writer.unindent()
+writer.writeln('} %s;' % self.name)
+
class ContainerType(Type):
def is_fixed_sizeof(self):
for m in self.members:
@@ -845,6 +895,20 @@ class ContainerType(Type):
return member
+def generate_c_declaration(self, writer):
+if not self.has_attr('declare'):
+return
+name = self.c_type()
+writer.writeln('typedef struct %s {' % name)
+writer.indent()
+for m in self.members:
+m.generate_c_declaration(writer)
+if len(self.members) == 0:
+# make sure generated structure are not empty
+writer.writeln("char dummy[0];")
+writer.unindent()
+writer.writeln('} %s;' % name).newline()
+
class StructType(ContainerType):
def __init__(self, name, members, attribute_list):
Type.__init__(self)
diff --git a/spice_codegen.py b/spice_codegen.py
index 54d655d..5846337 100755
--- a/spice_codegen.py
+++ b/spice_codegen.py
@@ -176,6 +176,8 @@ parser.add_option("--license", dest="license",
parser.add_option("--generate-header",
action="store_true", dest="generate_header", default=False,
help="Generate also the header")
+parser.add_option("--generated-declaration-file",
dest="generated_declaration_file", metavar="FILE",
+ help="Name of the file
Re: [Spice-devel] [PATCH spice-common 4/9] codegen: Allows to generate C declarations automatically
On Fri, Mar 08, 2019 at 04:47:35PM -0500, Frediano Ziglio wrote:
> >
> > On Sun, Mar 03, 2019 at 07:10:25PM +, Frediano Ziglio wrote:
> > > Allows to specify a @declare attribute for messages and structure
> > > that can generate the needed C structures.
> > >
> > > Signed-off-by: Frediano Ziglio
> > > ---
> > > python_modules/ptypes.py | 64
> > > spice_codegen.py | 47 +
> > > 2 files changed, 111 insertions(+)
> > >
> > > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > > index 7dca78d..64c198e 100644
> > > --- a/python_modules/ptypes.py
> > > +++ b/python_modules/ptypes.py
> > > @@ -72,6 +72,8 @@ valid_attributes=set([
> > > 'zero',
> > > # this attribute does not exist on the network, fill just structure
> > > with the value
> > > 'virtual',
> > > +# generate C structure declarations from protocol definition
> > > +'declare',
> > > ])
> > >
> > > attributes_with_arguments=set([
> > > @@ -485,6 +487,26 @@ class ArrayType(Type):
> > > def c_type(self):
> > > return self.element_type.c_type()
> > >
> > > +def generate_c_declaration(self, writer, member):
> > > +name = member.name
> > > +if member.has_attr("chunk"):
> > > +return writer.writeln('SpiceChunks *%s;' % name)
> > > +if member.has_attr("as_ptr"):
> > > +len_var = member.attributes["as_ptr"][0]
> > > +writer.writeln('uint32_t %s;' % len_var)
> > > +return writer.writeln('%s *%s;' % (self.c_type(), name))
> > > +if member.has_attr("to_ptr"): # TODO len
> > > +return writer.writeln('%s *%s;' % (self.c_type(), name))
> > > +if member.has_attr("ptr_array"): # TODO where the length is
> > > stored? overflow?
> > > +return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> > > +if member.has_end_attr() or self.is_remaining_length(): # TODO
> > > len
> > > +return writer.writeln('%s %s[0];' % (self.c_type(), name))
> >
> > These TODO are worrying, but only the has_end_attr() one seems to be run
> > at the moment, the others seem to be dead code?
> >
>
> Indeed. And also is more a "for security we should avoid that in the
> protocol" so it's not much related to code generation.
> The idea is that "if you have an array as output you should also have
> the length in the output otherwise you are probably generating an
> overflow".
>
> The check should be done parsing the protocol file, not generating
> a specific code from it (surely demarshaller code is affected).
>
> Maybe the best is to move these TODOs in a separate patch.
> And possibly I'll keep it somewhere when I'll have the time to
> generate the proper checks.
Hmm, ok, might indeed be better to move them to a different commit.
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-common 3/9] codegen: Generate headers while generating code
On Fri, Mar 08, 2019 at 04:42:24PM -0500, Frediano Ziglio wrote: > > > > > > Bit unsure about the option name. --header-name maybe? Or just infer it > > from the .c file name? > > > > It's similar to --generate-enums and similars. Yes, on the other end this > option is an additional generation and has a parameter so it not a big > match indeed. The infer of the header is a good suggestion. > Maybe a --generate-also-header ? I don't like the -also- ;) I don't think it's worth debating the name for ages, if --generate-header is good for you, let's go for it for consistency with --generate-enus & friends (which I did not notice before). Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-common 5/9] Allow to generate C declarations for spice.proto
On Fri, Mar 08, 2019 at 04:51:20PM -0500, Frediano Ziglio wrote: > > > > On Sun, Mar 03, 2019 at 07:10:26PM +, Frediano Ziglio wrote: > > > Generate and include C declarations. > > > Next patch will use this facility. > > > > Since none of the spice.proto types are decorated with @declare, adding > > the #include in messages.h won't have any bad consequences > > > > Are you suggesting to add a comment or to remove the last hunk? I would add this in the commit log. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay
On Mon, Mar 11, 2019 at 12:12:45PM +, Victor Toso wrote: > Hi, > > On Mon, Mar 11, 2019 at 12:33:20PM +0100, Christophe Fergeau wrote: > > Hey, > > > > One comment, even if the commit log mentions a bug #, it does not seem > > to be describing what the bug is/how it's fixed (maybe this can be > > inferred from the traces, but I did not read them closely as the log > > does not hint that a bug can be seen by looking at them). > > Oh, well. There is several ways we can write a commit log, like > there is several ways on how we tell a story. Most of the times, > I try to explain what the change is about. I do refer to BZ that > they fix because it is helpful for maintainers and backporting > patches but I don't see why I need to duplicate BZ comments in > the commit log too. > > This patch avoids sending multiple monitors_config message to the > client during short period of time and I tried to explain that. > I'm more than happy to understand how I can improve the commit > log but I did not understand how from your comment. If this commit is fixing a specific bug, then the commit log should have some description of it/what the fix is. With git, it's entirely possible that one will be looking at that commit while offline/with bad internet connection. The bug report may have moved to a different system with a different bug # in the mean time, the bug report may be long and confusing, ... So having a summary in the commit log fixing the bug is imo helpful. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] qxl-wddm-dod: prevent failure of power on transition
- Original Message -
> From: "Yuri Benditovich"
> To: spice-devel@lists.freedesktop.org
> Cc: y...@daynix.com
> Sent: Thursday, February 28, 2019 9:42:17 AM
> Subject: [Spice-devel] [PATCH] qxl-wddm-dod: prevent failure of power on
> transition
>
> On return from S3 the driver may receive failure when
> calls DxgkCbAcquirePostDisplayOwnership. In general the
> driver is not expected to use this method when returns
> from S3 but it is not simple to distinguish between S3
> and S4 power transition (possible, due to hybrid sleep).
> Current commit avoids returning error on power transition
> and obtains best known default video mode info instead.
>
> Signed-off-by: Yuri Benditovich
Acked-by: Marek Kedzierski
> ---
> qxldod/QxlDod.cpp | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 525cdc3..95c0e97 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -5211,9 +5211,10 @@ NTSTATUS
> HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
>
> if (!NT_SUCCESS(Status))
> {
> -DbgPrint(TRACE_LEVEL_ERROR, ("QxlDod::AcquireDisplayInfo failed with
> status 0x%X Width = %d\n",
> +DbgPrint(TRACE_LEVEL_WARNING, ("QxlDod::AcquireDisplayInfo failed
> with status 0x%X Width = %d\n",
> Status, DispInfo.Width));
> -return STATUS_UNSUCCESSFUL;
> +DispInfo.Width = 0;
> +Status = STATUS_SUCCESS;
> }
>
> if (DispInfo.Width == 0)
> --
> 2.16.1.windows.4
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay
Hi,
On Mon, Mar 11, 2019 at 07:34:19AM -0400, Frediano Ziglio wrote:
> > Changes v1->v2
> > * Using reds_core_timer_* api, which calls the timeout function from the
> > right context (Frediano);
>
> Sorry, these functions suffers the same problem, you have to
> use the core interface of the DisplayChannel.
>
> OT: I was thinking to add some extra checks on these thread
> conditions.
I checked but not fully :(
timer_start() calls:
g_source_attach(timer->source, timer->context);
...and I thought timer->context was the right context here but
one can see that in timer_add()
timer->context = iface->main_context;
>
> > * Remove the SpiceTimer on finalize (Frediano)
> >
> > server/display-channel-private.h | 2 ++
> > server/display-channel.c | 27 +--
> > 2 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/server/display-channel-private.h
> > b/server/display-channel-private.h
> > index 58179531..848abe81 100644
> > --- a/server/display-channel-private.h
> > +++ b/server/display-channel-private.h
> > @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
> >
> > int gl_draw_async_count;
> >
> > +SpiceTimer *monitors_config_update_timer;
> > +
> > /* TODO: some day unify this, make it more runtime.. */
> > stat_info_t add_stat;
> > stat_info_t exclude_stat;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 9bb7fa44..a2f95956 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
> > display_channel_destroy_surfaces(self);
> > image_cache_reset(&self->priv->image_cache);
> >
> > +if (self->priv->monitors_config_update_timer) {
> > +RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> > +reds_core_timer_remove(reds,
> > self->priv->monitors_config_update_timer);
> > +self->priv->monitors_config_update_timer = NULL;
> > +}
> > +
> > if (spice_extra_checks) {
> > unsigned int count;
> > _Drawable *drawable;
> > @@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
> > NULL);
> > if (display) {
> > display_channel_set_stream_video(display, stream_video);
> > +display->priv->monitors_config_update_timer =
> > reds_core_timer_add(reds,
> > +(SpiceTimerFunc) display_channel_push_monitors_config,
> > display);
>
> Why not putting this in _init or constructed ?
You tell me, I'm not sure where it fits better in the server
arch. I thought that at the time we create the display is good
enough. Let me know if I should move it.
>
> > }
> > return display;
> > }
> > @@ -2435,6 +2443,11 @@ void
> > display_channel_push_monitors_config(DisplayChannel *display)
> > {
> > DisplayChannelClient *dcc;
> >
> > +if (display->priv->monitors_config_update_timer) {
> > +RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
> > +reds_core_timer_cancel(reds,
> > display->priv->monitors_config_update_timer);
> > +}
> > +
> > FOREACH_DCC(display, dcc) {
> > dcc_push_monitors_config(dcc);
> > }
> > @@ -2444,14 +2457,24 @@ void
> > display_channel_update_monitors_config(DisplayChannel *display,
> > QXLMonitorsConfig *config,
> > uint16_t count, uint16_t
> > max_allowed)
> > {
> > +RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
> >
> > -if (display->priv->monitors_config)
> > +if (display->priv->monitors_config) {
> > monitors_config_unref(display->priv->monitors_config);
> > +}
> >
> > display->priv->monitors_config =
> > monitors_config_new(config->heads, count, max_allowed);
> >
> > -display_channel_push_monitors_config(display);
> > +if (!display->priv->monitors_config_update_timer) {
> > +spice_warning("No timer for monitor_config updates");
>
> Why a warning?
> I think the code you wrote should initilise the timer once
> and should never be NULL.
It should never be NULL indeed.
> > +display_channel_push_monitors_config(display);
> > +return;
> > +}
> > +
> > +/* Cancel previous timer, if it was set */
> > +reds_core_timer_cancel(reds,
> > display->priv->monitors_config_update_timer);
> > +reds_core_timer_start(reds,
> > display->priv->monitors_config_update_timer,
> > 200);
> > }
> >
> > void display_channel_set_monitors_config_to_primary(DisplayChannel
> > *display)
>
> I had a second though on this patch. Let's assume I have only a monitor and
> this sequence:
> - monitor_config
> - update
> Your code will change potentially to:
> - update
> - monitor_config
> Now, the client will receive an update before the monitor
> configuration, potentially wi
Re: [Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay
Hi, On Mon, Mar 11, 2019 at 12:33:20PM +0100, Christophe Fergeau wrote: > Hey, > > One comment, even if the commit log mentions a bug #, it does not seem > to be describing what the bug is/how it's fixed (maybe this can be > inferred from the traces, but I did not read them closely as the log > does not hint that a bug can be seen by looking at them). Oh, well. There is several ways we can write a commit log, like there is several ways on how we tell a story. Most of the times, I try to explain what the change is about. I do refer to BZ that they fix because it is helpful for maintainers and backporting patches but I don't see why I need to duplicate BZ comments in the commit log too. This patch avoids sending multiple monitors_config message to the client during short period of time and I tried to explain that. I'm more than happy to understand how I can improve the commit log but I did not understand how from your comment. Victor > Christophe > > On Mon, Mar 11, 2019 at 10:02:27AM +, Victor Toso wrote: > > From: Victor Toso > > > > The device driver might take a few interactions to reconfigure all > > displays but in each change it can send it down to spice-server. The > > client is not interested in temporary states, only the final monitor > > config can be helpful. > > > > This patch adds a 200ms delay to wait for more changes from guest > > device. Tested only with QXL in Fedora 29. > > > > Simple example below, with 4 displays enabled but removing the display 1 > > (starting from 0), spice server receives: > > > > | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing > > monitors config > > | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors > > config count:3 max:4 > > | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997 > > | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 > > 1024x740 > > | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0 > > 1024x740 > > > > Sends to the client #1 > > > > | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing > > monitors config > > | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors > > config count:3 max:4 > > | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997 > > | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0 > > | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0 > > 1024x740 > > > > Sends to the client #2 > > > > | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace > > | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing > > monitors config > > | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors > > config count:1 max:4 > > | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997 > > | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing > > monitors config > > | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors > > config count:3 max:4 > > | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997 > > | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0 > > | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 > > 1024x740 > > > > Sends to the client #3 > > > > | 16:50:14.973: > > display-channel.c:2462:display_channel_update_monitors_config: Will update > > monitors in 200ms ~~ > > | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing > > monitors config > > | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors > > config count:4 max:4 > > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997 > > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0 > > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 > > 1024x740 > > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0 > > 1024x740 > > > > Sends to the client #4 and final > > > > With this patch, only the last one is sent. > > Resolves: rhbz#1673072 > > Signed-off-by: Victor Toso > > --- > > > > Changes v1->v2 > > * Using reds_core_timer_* api, which calls the timeout function from the > > right context (Frediano); > > * Remove the SpiceTimer on finalize (Frediano) > > > > server/display-channel-private.h | 2 ++ > > server/display-channel.c | 27 +-- > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/server/display-channel-private.h > > b/server/display-channel-private.h > > index 58179531..848abe81 100644 > > --- a/server/display-channel-private.h > > +++ b/server/display-channel-private.h > > @@ -118,6 +118,8 @@ struct DisplayChannelPrivate > > > > int gl_draw_async_count; > > > > +SpiceTimer *monitors_config_update_timer; > > + > > /* TODO: some day unify this, make it more runtime.. */ > > stat_info_t add_stat; > > stat_inf
Re: [Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay
>
> From: Victor Toso
>
> The device driver might take a few interactions to reconfigure all
> displays but in each change it can send it down to spice-server. The
> client is not interested in temporary states, only the final monitor
> config can be helpful.
>
> This patch adds a 200ms delay to wait for more changes from guest
> device. Tested only with QXL in Fedora 29.
>
> Simple example below, with 4 displays enabled but removing the display 1
> (starting from 0), spice server receives:
>
> | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors config
> | count:3 max:4
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0
> | 1024x740
>
> Sends to the client #1
>
> | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors config
> | count:3 max:4
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0
> | 1024x740
>
> Sends to the client #2
>
> | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
> | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors config
> | count:1 max:4
> | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
> | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors config
> | count:3 max:4
> | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 1024x740
>
> Sends to the client #3
>
> | 16:50:14.973:
> | display-channel.c:2462:display_channel_update_monitors_config: Will
> | update monitors in 200ms ~~
> | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing
> | monitors config
> | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors config
> | count:4 max:4
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0
> | 1024x740
>
> Sends to the client #4 and final
>
> With this patch, only the last one is sent.
> Resolves: rhbz#1673072
> Signed-off-by: Victor Toso
> ---
>
> Changes v1->v2
> * Using reds_core_timer_* api, which calls the timeout function from the
> right context (Frediano);
Sorry, these functions suffers the same problem, you have to use the core
interface of the DisplayChannel.
OT: I was thinking to add some extra checks on these thread conditions.
> * Remove the SpiceTimer on finalize (Frediano)
>
> server/display-channel-private.h | 2 ++
> server/display-channel.c | 27 +--
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/server/display-channel-private.h
> b/server/display-channel-private.h
> index 58179531..848abe81 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
>
> int gl_draw_async_count;
>
> +SpiceTimer *monitors_config_update_timer;
> +
> /* TODO: some day unify this, make it more runtime.. */
> stat_info_t add_stat;
> stat_info_t exclude_stat;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 9bb7fa44..a2f95956 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
> display_channel_destroy_surfaces(self);
> image_cache_reset(&self->priv->image_cache);
>
> +if (self->priv->monitors_config_update_timer) {
> +RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> +reds_core_timer_remove(reds,
> self->priv->monitors_config_update_timer);
> +self->priv->monitors_config_update_timer = NULL;
> +}
> +
> if (spice_extra_checks) {
> unsigned int count;
> _Drawable *drawable;
> @@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
> NULL);
> if (display) {
> display_channel_set_stream_video(display, stream_video);
> +display->priv->monitors_config_update_timer =
> reds_core_timer_add(reds,
Re: [Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay
Hey,
One comment, even if the commit log mentions a bug #, it does not seem
to be describing what the bug is/how it's fixed (maybe this can be
inferred from the traces, but I did not read them closely as the log
does not hint that a bug can be seen by looking at them).
Christophe
On Mon, Mar 11, 2019 at 10:02:27AM +, Victor Toso wrote:
> From: Victor Toso
>
> The device driver might take a few interactions to reconfigure all
> displays but in each change it can send it down to spice-server. The
> client is not interested in temporary states, only the final monitor
> config can be helpful.
>
> This patch adds a 200ms delay to wait for more changes from guest
> device. Tested only with QXL in Fedora 29.
>
> Simple example below, with 4 displays enabled but removing the display 1
> (starting from 0), spice server receives:
>
> | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing
> monitors config
> | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors config
> count:3 max:4
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
>
> Sends to the client #1
>
> | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing
> monitors config
> | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors config
> count:3 max:4
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
>
> Sends to the client #2
>
> | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
> | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing
> monitors config
> | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors config
> count:1 max:4
> | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
> | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing
> monitors config
> | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors config
> count:3 max:4
> | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 1024x740
>
> Sends to the client #3
>
> | 16:50:14.973:
> display-channel.c:2462:display_channel_update_monitors_config: Will update
> monitors in 200ms ~~
> | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing
> monitors config
> | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors config
> count:4 max:4
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
>
> Sends to the client #4 and final
>
> With this patch, only the last one is sent.
> Resolves: rhbz#1673072
> Signed-off-by: Victor Toso
> ---
>
> Changes v1->v2
> * Using reds_core_timer_* api, which calls the timeout function from the
> right context (Frediano);
> * Remove the SpiceTimer on finalize (Frediano)
>
> server/display-channel-private.h | 2 ++
> server/display-channel.c | 27 +--
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/server/display-channel-private.h
> b/server/display-channel-private.h
> index 58179531..848abe81 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
>
> int gl_draw_async_count;
>
> +SpiceTimer *monitors_config_update_timer;
> +
> /* TODO: some day unify this, make it more runtime.. */
> stat_info_t add_stat;
> stat_info_t exclude_stat;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 9bb7fa44..a2f95956 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
> display_channel_destroy_surfaces(self);
> image_cache_reset(&self->priv->image_cache);
>
> +if (self->priv->monitors_config_update_timer) {
> +RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> +reds_core_timer_remove(reds,
> self->priv->monitors_config_update_timer);
> +self->priv->monitors_config_update_timer = NULL;
> +}
> +
> if (spice_extra_checks) {
> unsigned int count;
> _Drawable *drawable;
> @@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
> NULL);
> if (display) {
> display_channel_se
[Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay
From: Victor Toso
The device driver might take a few interactions to reconfigure all
displays but in each change it can send it down to spice-server. The
client is not interested in temporary states, only the final monitor
config can be helpful.
This patch adds a 200ms delay to wait for more changes from guest
device. Tested only with QXL in Fedora 29.
Simple example below, with 4 displays enabled but removing the display 1
(starting from 0), spice server receives:
| 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing monitors
config
| 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors config
count:3 max:4
| 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
| 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 1024x740
| 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
Sends to the client #1
| 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing monitors
config
| 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors config
count:3 max:4
| 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
| 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
| 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
Sends to the client #2
| 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
| 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing monitors
config
| 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors config
count:1 max:4
| 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
| 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing monitors
config
| 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors config
count:3 max:4
| 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
| 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
| 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 1024x740
Sends to the client #3
| 16:50:14.973: display-channel.c:2462:display_channel_update_monitors_config:
Will update monitors in 200ms ~~
| 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing monitors
config
| 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors config
count:4 max:4
| 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
| 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
| 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 1024x740
| 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
Sends to the client #4 and final
With this patch, only the last one is sent.
Resolves: rhbz#1673072
Signed-off-by: Victor Toso
---
Changes v1->v2
* Using reds_core_timer_* api, which calls the timeout function from the
right context (Frediano);
* Remove the SpiceTimer on finalize (Frediano)
server/display-channel-private.h | 2 ++
server/display-channel.c | 27 +--
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/server/display-channel-private.h b/server/display-channel-private.h
index 58179531..848abe81 100644
--- a/server/display-channel-private.h
+++ b/server/display-channel-private.h
@@ -118,6 +118,8 @@ struct DisplayChannelPrivate
int gl_draw_async_count;
+SpiceTimer *monitors_config_update_timer;
+
/* TODO: some day unify this, make it more runtime.. */
stat_info_t add_stat;
stat_info_t exclude_stat;
diff --git a/server/display-channel.c b/server/display-channel.c
index 9bb7fa44..a2f95956 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
display_channel_destroy_surfaces(self);
image_cache_reset(&self->priv->image_cache);
+if (self->priv->monitors_config_update_timer) {
+RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
+reds_core_timer_remove(reds, self->priv->monitors_config_update_timer);
+self->priv->monitors_config_update_timer = NULL;
+}
+
if (spice_extra_checks) {
unsigned int count;
_Drawable *drawable;
@@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
NULL);
if (display) {
display_channel_set_stream_video(display, stream_video);
+display->priv->monitors_config_update_timer = reds_core_timer_add(reds,
+(SpiceTimerFunc) display_channel_push_monitors_config,
display);
}
return display;
}
@@ -2435,6 +2443,11 @@ void display_channel_push_monitors_config(DisplayChannel
*display)
{
DisplayChannelClient *dcc;
+if (display->priv->monitors_config_update_timer) {
+RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
+reds_core_timer_cancel(reds,
display->priv->monitors_config_update_timer)
Re: [Spice-devel] [spice v1 1/2] display-channel: monitors_config: add 200ms delay
On Fri, Mar 08, 2019 at 04:32:04PM -0500, Frediano Ziglio wrote:
hi,
> >
> > From: Victor Toso
> >
> > The device driver might take a few interactions to reconfigure all
> > displays but in each change it can send it down to spice-server. The
> > client is not interested in temporary states, only the final monitor
> > config can be helpful.
> >
> > This patch adds a 200ms delay to wait for more changes from guest
> > device. Tested only with QXL in Fedora 29.
> >
>
> Which kind of guest? Windows or Linux?
Fedora guest
> > Simple example below, with 4 displays enabled but removing the display 1
> > (starting from 0), spice server receives:
> >
> > | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing
> > | monitors config
> > | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors
> > config
> > | count:3 max:4
> > | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
> > | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0
> > 1024x740
> > | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0
> > | 1024x740
> >
> > Sends to the client #1
> >
> > | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing
> > | monitors config
> > | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors
> > config
> > | count:3 max:4
> > | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
> > | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
> > | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0
> > | 1024x740
> >
> > Sends to the client #2
> >
> > | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
> > | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing
> > | monitors config
> > | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors
> > config
> > | count:1 max:4
> > | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
> > | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing
> > | monitors config
> > | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors
> > config
> > | count:3 max:4
> > | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
> > | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
> > | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0
> > 1024x740
> >
> > Sends to the client #3
> >
> > | 16:50:14.973:
> > | display-channel.c:2462:display_channel_update_monitors_config: Will
> > | update monitors in 200ms ~~
> > | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing
> > | monitors config
> > | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors
> > config
> > | count:4 max:4
> > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
> > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
> > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0
> > 1024x740
> > | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0
> > | 1024x740
> >
> > Sends to the client #4 and final
> >
> > With this patch, only the last one is sent.
> > Resolves: rhbz#1673072
> > Signed-off-by: Victor Toso
> > ---
> > server/display-channel-private.h | 2 ++
> > server/display-channel.c | 16 +---
> > server/display-channel.h | 2 +-
> > 3 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/server/display-channel-private.h
> > b/server/display-channel-private.h
> > index 58179531..f1a4314c 100644
> > --- a/server/display-channel-private.h
> > +++ b/server/display-channel-private.h
> > @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
> >
> > int gl_draw_async_count;
> >
> > +guint monitors_config_update_delay_id;
> > +
> > /* TODO: some day unify this, make it more runtime.. */
> > stat_info_t add_stat;
> > stat_info_t exclude_stat;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index e68ed10f..7e2584ec 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2430,13 +2430,17 @@ gboolean
> > display_channel_validate_surface(DisplayChannel *display, uint32_t surf
> > return TRUE;
> > }
> >
> > -void display_channel_push_monitors_config(DisplayChannel *display)
> > +gboolean display_channel_push_monitors_config(gpointer userdata)
> > {
> > DisplayChannelClient *dcc;
> > +DisplayChannel *display = userdata;
> > +
> > +display->priv->monitors_config_update_delay_id = 0;
> >
> > FOREACH_DCC(display, dcc) {
> > dcc_push_monitors_config(dcc);
> > }
> > +return G_SOURCE_REMOVE;
> > }
> >
> > void display_channel_update_monitors_config(DisplayChannel *display,
> > @@ -2444,13 +2448,19 @@ void
> > display_channel_update_monitors_config(DisplayChannel *display,
> >
