Re: [Spice-devel] [PATCH spice-common] meson: fix building for big-endian host

[Victor Toso]

Hi,

On Thu, Jan 17, 2019 at 03:04:32AM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> autofoo build-sys defines WORDS_BIGENDIAN, and spice-common code uses it.
> 
> Later, I think it would make sense to switch to G_BIG_ENDIAN instead.

Indeed,

> Fixes:
> https://gitlab.freedesktop.org/spice/spice-common/issues/2
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  meson.build | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 049409b..8579680 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -33,6 +33,9 @@ spice_common_config_data = configuration_data()
>  if get_option('extra-checks')
>spice_common_config_data.set('ENABLE_EXTRA_CHECKS', '1')
>  endif
> +if host_machine.endian() == 'big'
> +  spice_common_config_data.set('WORDS_BIGENDIAN', '1')
> +endif

Shouldn't it be target_machine.endian() ?

>  spice_common_generate_code = get_option('generate-code')
>  spice_common_generate_client_code = spice_common_generate_code == 'all' or 
> spice_common_generate_code == 'client'
> -- 
> 2.20.1.98.gecbdaf0899
> 
> ___
> 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] [PATCH spice-gtk v2] spicy-screenshot: use G_BIG_ENDIAN

[Victor Toso]

Hi,

On Thu, Jan 17, 2019 at 03:08:43AM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> WORDS_BIGENDIAN is defined by autofoo macro.
> meson doesn't define it. Let's use the GLib defines instead.

We will probably need to fix spice-common too.

> Signed-off-by: Marc-André Lureau 

Acked-by: Victor Toso 

> ---
>  tools/spicy-screenshot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/spicy-screenshot.c b/tools/spicy-screenshot.c
> index 97bd4da..7511bdf 100644
> --- a/tools/spicy-screenshot.c
> +++ b/tools/spicy-screenshot.c
> @@ -65,7 +65,7 @@ static int write_ppm_32(void)
>  n = d_width * d_height;
>  p = d_data;
>  while (n > 0) {
> -#ifdef WORDS_BIGENDIAN
> +#if G_BYTE_ORDER == G_BIG_ENDIAN
>  fputc(p[1], fp);
>  fputc(p[2], fp);
>  fputc(p[3], fp);
> -- 
> 2.20.1.98.gecbdaf0899
> 
> ___
> 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] issues linking with openssl 1.1.0

[Marc-André Lureau]

Hey Iordan

On Thu, Jan 17, 2019 at 3:45 AM i iordanov  wrote:
>
> Hello,
>
> Happy new year!
>
> I tried to build spice-gtk 0.34 and 0.35 with OpenSSL 1.1.0, and I got some 
> linking errors:
>
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
>  spice_channel_class_intern_init: error: undefined reference to 
> 'SSL_library_init'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
>  spice_channel_class_intern_init: error: undefined reference to 
> 'SSL_load_error_strings'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
>  spice_channel_coroutine: error: undefined reference to 'SSLv23_method'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
>  spice_channel_coroutine: error: undefined reference to 'sk_value'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
>  spice_channel_coroutine: error: undefined reference to 'sk_num'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
>  spice_channel_coroutine: error: undefined reference to 'sk_pop_free'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(ssl_verify.o):ssl_verify.c:function
>  openssl_verify: error: undefined reference to 'sk_num'
> jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(ssl_verify.o):ssl_verify.c:function
>  openssl_verify: error: undefined reference to 'sk_value'
> clang++: error: linker command failed with exit code 1 (use -v to see 
> invocation)
> make: *** [obj/local/x86/libspice.so] Error 1
>
> I had to revert to 1.0.2, but wanted to ask - is there a plan to support 
> 1.1.0/1.1.1?
>

Happy new year! :)

It compiles fine against openssl-libs-1.1.1-3.fc29.x86_64

I checked 'SSL_library_init', it is is not exported by the library,
but the header redefines it:

openssl/ssl.h
1098:#  define OpenSSL_add_ssl_algorithms()   SSL_library_init()
1099:#  define SSLeay_add_ssl_algorithms()SSL_library_init()
1938:#  define SSL_library_init() OPENSSL_init_ssl(0, NULL)

Is this under your own build environment or it can be reproduced on a
known distro?

thanks



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] issues linking with openssl 1.1.0

[i iordanov]

Hello,

Happy new year!

I tried to build spice-gtk 0.34 and 0.35 with OpenSSL 1.1.0, and I got some
linking errors:

jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
spice_channel_class_intern_init: error: undefined reference to
'SSL_library_init'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
spice_channel_class_intern_init: error: undefined reference to
'SSL_load_error_strings'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
spice_channel_coroutine: error: undefined reference to 'SSLv23_method'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
spice_channel_coroutine: error: undefined reference to 'sk_value'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
spice_channel_coroutine: error: undefined reference to 'sk_num'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(spice-channel.o):spice-channel.c:function
spice_channel_coroutine: error: undefined reference to 'sk_pop_free'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(ssl_verify.o):ssl_verify.c:function
openssl_verify: error: undefined reference to 'sk_num'
jni/src/../libs/deps/x86/root/lib/libspice-client-glib-2.0.a(ssl_verify.o):ssl_verify.c:function
openssl_verify: error: undefined reference to 'sk_value'
clang++: error: linker command failed with exit code 1 (use -v to see
invocation)
make: *** [obj/local/x86/libspice.so] Error 1

I had to revert to 1.0.2, but wanted to ask - is there a plan to support
1.1.0/1.1.1?

Thanks!
iordan

-- 
The conscious mind has only one thread of execution.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-common] quic: fix sign-compare warning

[marcandre . lureau]

From: Marc-André Lureau 

../subprojects/spice-common/common/quic.c: In function 'fill_model_structures':
../subprojects/spice-common/common/quic.c:695:55: error: comparison of integer 
expressions of different signedness: 'int' and 'unsigned int' 
[-Werror=sign-compare]
 spice_assert(free_counter - family_stat->counters == nbuckets * ncounters);
   ^~

Signed-off-by: Marc-André Lureau 
---
 common/quic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/quic.c b/common/quic.c
index c28974e..1760274 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -692,7 +692,7 @@ static void fill_model_structures(SPICE_GNUC_UNUSED Encoder 
*encoder, FamilyStat
 bnumber++;
 } while (bend < levels - 1);
 
-spice_assert(free_counter - family_stat->counters == nbuckets * ncounters);
+spice_assert(free_counter - family_stat->counters == (ptrdiff_t)(nbuckets 
* ncounters));
 }
 
 static void find_model_params(Encoder *encoder,
-- 
2.20.1.98.gecbdaf0899

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk v2] spicy-screenshot: use G_BIG_ENDIAN

[marcandre . lureau]

From: Marc-André Lureau 

WORDS_BIGENDIAN is defined by autofoo macro.
meson doesn't define it. Let's use the GLib defines instead.

Signed-off-by: Marc-André Lureau 
---
 tools/spicy-screenshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/spicy-screenshot.c b/tools/spicy-screenshot.c
index 97bd4da..7511bdf 100644
--- a/tools/spicy-screenshot.c
+++ b/tools/spicy-screenshot.c
@@ -65,7 +65,7 @@ static int write_ppm_32(void)
 n = d_width * d_height;
 p = d_data;
 while (n > 0) {
-#ifdef WORDS_BIGENDIAN
+#if G_BYTE_ORDER == G_BIG_ENDIAN
 fputc(p[1], fp);
 fputc(p[2], fp);
 fputc(p[3], fp);
-- 
2.20.1.98.gecbdaf0899

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] spicy-screenshot: use G_BIG_ENDIAN

[Marc-André Lureau]

On Thu, Jan 17, 2019 at 3:04 AM  wrote:
>
> From: Marc-André Lureau 
>
> WORDS_BIGENDIAN is defined by autofoo macro.
> meson doesn't define it. Let's use the GLib define instead.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  tools/spicy-screenshot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/spicy-screenshot.c b/tools/spicy-screenshot.c
> index 97bd4da..26f5947 100644
> --- a/tools/spicy-screenshot.c
> +++ b/tools/spicy-screenshot.c
> @@ -65,7 +65,7 @@ static int write_ppm_32(void)
>  n = d_width * d_height;
>  p = d_data;
>  while (n > 0) {
> -#ifdef WORDS_BIGENDIAN
> +#ifdef G_BIG_ENDIAN

Bad patch, sending a new one

>  fputc(p[1], fp);
>  fputc(p[2], fp);
>  fputc(p[3], fp);
> --
> 2.20.1.98.gecbdaf0899
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-common] meson: fix building for big-endian host

[marcandre . lureau]

From: Marc-André Lureau 

autofoo build-sys defines WORDS_BIGENDIAN, and spice-common code uses it.

Later, I think it would make sense to switch to G_BIG_ENDIAN instead.

Fixes:
https://gitlab.freedesktop.org/spice/spice-common/issues/2

Signed-off-by: Marc-André Lureau 
---
 meson.build | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/meson.build b/meson.build
index 049409b..8579680 100644
--- a/meson.build
+++ b/meson.build
@@ -33,6 +33,9 @@ spice_common_config_data = configuration_data()
 if get_option('extra-checks')
   spice_common_config_data.set('ENABLE_EXTRA_CHECKS', '1')
 endif
+if host_machine.endian() == 'big'
+  spice_common_config_data.set('WORDS_BIGENDIAN', '1')
+endif
 
 spice_common_generate_code = get_option('generate-code')
 spice_common_generate_client_code = spice_common_generate_code == 'all' or 
spice_common_generate_code == 'client'
-- 
2.20.1.98.gecbdaf0899

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk] spicy-screenshot: use G_BIG_ENDIAN

[marcandre . lureau]

From: Marc-André Lureau 

WORDS_BIGENDIAN is defined by autofoo macro.
meson doesn't define it. Let's use the GLib define instead.

Signed-off-by: Marc-André Lureau 
---
 tools/spicy-screenshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/spicy-screenshot.c b/tools/spicy-screenshot.c
index 97bd4da..26f5947 100644
--- a/tools/spicy-screenshot.c
+++ b/tools/spicy-screenshot.c
@@ -65,7 +65,7 @@ static int write_ppm_32(void)
 n = d_width * d_height;
 p = d_data;
 while (n > 0) {
-#ifdef WORDS_BIGENDIAN
+#ifdef G_BIG_ENDIAN
 fputc(p[1], fp);
 fputc(p[2], fp);
 fputc(p[3], fp);
-- 
2.20.1.98.gecbdaf0899

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2] gtk-session: clipboard request: notify agent on failure

[james harvey]

On Wed, Jan 16, 2019 at 6:08 AM Marc-André Lureau
 wrote:
> On Wed, Jan 16, 2019 at 2:47 PM james harvey  wrote:
> > Am I understanding correctly that the Jan 10 patches which completely
> > fix spice for me and others experiencing clipboard problems aren't
> > being added in any form, and that instead the clipboard is being left
> > broken, just less broken so it doesn't freeze the guest?  I don't know
> > exactly when it broke, but spice didn't used to act this way.
>
> That's important information, because spice-gtk clipboard code didn't
> change that much afaik.

I can definitely understand when it broke would be helpful.
Unfortunately, there is a couple of years gap in my spice usage, so I
don't think it's something I can pin down.  I don't know if it would
be possible to run years old spice, or if dependencies from or to
spice would get tripped up and spin the project into basically
bisecting an entire system as a whole, based on packages available on
given dates.  Even if it was, it's of course possible it might be
another package's change that made this stop working.  While that
would be possible to do given Arch's historical repos, it would be a
massive undertaking.  With the amount of other project bisecting I've
done lately, it's not something I'd be able to do anytime soon.

> > That's going to leave me and others perpetually patching spice
> > releases, and someday when they don't apply cleanly, being at a loss
> > and being forced to look at other protocols.
> >
> > Apologies if I'm misunderstanding.
>
> You misunderstand, there is a lot of activity on this problem atm and
> it is currently scheduled for 0.36. However, since it is not clear
> whether this is a regression and the fixes are *not* trivial, it
> should be fine to schedule for 0.37. Either early release after that,
> and fixes can be backported too most probably.

Glad to hear!
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Sam Ravnborg]

Hi Daniel.

> v5: Actually try to sort them, and while at it, sort all the ones I
> touch.

Applied this variant on top of drm-misc and did a build test.
Looked good for ia64, x86 and alpha.

Took a closer look at the changes to atmel_hlcd - and they looked OK.

But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and
drm_kms_helper_poll_fini().
But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe
have a driver here where we have plugged the drm_poll infrastructure,
but it is not in use.

>  include/drm/drm_crtc_helper.h | 16 ---

The list of include files in this file could be dropped and replaced by:
struct drm_connector;
struct drm_device;
struct drm_display_mode;
struct drm_encoder;
struct drm_framebuffer;
struct drm_mode_set;
struct drm_modeset_acquire_ctx;

I tried to do so on top of your patch.
But there were too many build errros and I somehow lost the motivation.


>  include/drm/drm_probe_helper.h| 27 +++
This on the other hand is fine - as expected as this is a new file.

But the above is just some random comments so:

Acked-by: Sam Ravnborg 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer

[Christophe Fergeau]

On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau 
> wrote:
> > In _SpiceUsbDeviceManagerPrivate, you replaced
> > #ifndef G_OS_WIN32
> > libusb_device *libdev;
> > #endif
> >
> > with
> >
> > #ifndef G_OS_WIN32
> > SpiceUsbBackendDevice *bdev;
> > #endif
> >
> > The #ifdef is there because of this comment in
> > spice_usb_device_manager_device_to_bdev:
> >
> >   /*
> >* On win32 we need to do this the hard and slow way, by asking libusb to
> >* re-enumerate all devices and then finding a matching device.
> >* We cannot cache the libusb_device like we do under Linux since the
> >* driver swap we do under windows invalidates the cached libdev.
> >*/
> >
> > After your patch, spice_usb_device_manager_device_to_bdev is no longer
> > at the right level of indirection imo, it looks up the 'bdev' when
> > needed on Windows in usb-device-manager.c, but my understanding of that
> > comment is that any libusb call within SpiceUsbBackendDevice should not
> > use a cached libusb_device?
> >
> >
> >
> As fas as I understand, there is no change in the level of indirection.
> Previously was:
> On Linux: usb-device-manager receives libusb device on hotplug indication
> and uses it until the device disappears.
> On Windows: each time the usb-device-manager needs the libusb device it
> takes fresh list of libusb devices and finds one according to known device
> properties.
> 
> Now:
> On Linux: usb-device-manager receives backend device wrapping libusb device
> on hotplug indication and uses it until the device disappears
> On Windows: each time the usb-device-manager needs the backend device it
> takes fresh list of new backend devices and finds one according to known
> device properties.
> So, the backend device is always fresh one and it always have fresh libusb
> device.
> 
> This can be simplified with removal of all these lookups in the
> usb-device-manager (and I already illustrated how) but after this commit is
> done.
> If from your point of view this is the requirement to existing commit,
> please let me know (then we will need to reevaluate our plans and our
> ability to deliver the patches in reasonable time).

I'll quote again the comment:

   /*
* On win32 we need to do this the hard and slow way, by asking libusb to
* re-enumerate all devices and then finding a matching device.
* We cannot cache the libusb_device like we do under Linux since the
* driver swap we do under windows invalidates the cached libdev.
*/

It says we cannot cache a libusb_device because it can change behind our
back, so we need to reenumerate all devices and lookup the right
libusb_device before every call to libusb.
Before your changes, we did not hold any long-lasting references to a
libusb_device on Windows, so we were guaranteed to do that lookup before
every call. After your changes, SpiceUsbBackendDevice is holding a
long-lasting reference to a libusb_device, but does not care about the
win32 issue. Only usb-device-manager is making sure the libusb_device
it's going to use is valid. This does not make sense to me to leak this
implementation detail to usb-device-manager. SpiceUsbBackend should hide
it, otherwise its API is going to be error-prone.

When you say this can be simplified by removing this lookup, are you
confirming that this comment I quoted above is obsolete?

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Virgl remote support

[Francois Gouget]

Hi,


I'm picking up the Virgl remote support patchset with the goal of 
adapting it for use with Xspice.

If I'm not mistaken the latest version of this patchset is in the 
virgl-spice branches of the following repositories:

https://cgit.freedesktop.org/~fziglio/spice-protocol/log/?h=virgl-spice
https://cgit.freedesktop.org/~fziglio/spice-common/log/?h=virgl-spice

https://cgit.freedesktop.org/~fziglio/spice-server/log/?h=virgl-spice

  This one also has a virgl_sort branch which is much more recent and 
  has me quite curious. But it's not clear to me if it is really 
  related. There is also a plain virgl branch but that one seems even 
  older.

https://cgit.freedesktop.org/~fziglio/qemu/log/?h=virgl-spice

  This one would not be needed for Xspice but I may use it for 
  reference.


And there has been no Virgl work on the Xspice side of things, right?

-- 
Francois Gouget 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/3] drm/qxl: add mode/framebuffer check functions

[Daniel Vetter]

On Wed, Jan 16, 2019 at 12:28:00PM +0100, Gerd Hoffmann wrote:
> > > +static int qxl_check_mode(struct qxl_device *qdev,
> > > +   unsigned int width,
> > > +   unsigned int height)
> > > +{
> > > + if (width * height * 4 > qdev->vram_size)
> > 
> > Is someone checking for integer overflows already?
> 
> Need to have a look.  This is just ...

The addfb ioctl checks for integer overflows for you.
-Daniel

> 
> > > - if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
> > > - DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
> > > - return -EINVAL;
> > > - }
> 
> ... that check moved into the new function.
> 
> cheers,
>   Gerd
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-streaming-agent 6/8 v3] Interface + implementation of getting device display info

[Lukáš Hrázký]

Adds an interface method to the FrameCapture class to get the device
display info (device address and device display id) for each display of
the graphics device that is captured.

Also adds functions to the API implementing this functionality for X11
in variants with and without DRM (the non-DRM version is rather limited
and may not work for more complex setups) as well as some helper
functions to make it easier for plugins to implement this and avoid code
duplication.

Implements the new interface method for the two built-in plugins
(mjpeg-fallback and gst-plugin).

Signed-off-by: Lukáš Hrázký 
---
 configure.ac  |   2 +
 include/spice-streaming-agent/Makefile.am |   2 +
 .../spice-streaming-agent/display-info.hpp|  52 +++
 .../spice-streaming-agent/frame-capture.hpp   |  13 +
 .../x11-display-info.hpp  |  57 
 src/Makefile.am   |   7 +
 src/display-info.cpp  | 100 ++
 src/gst-plugin.cpp|  14 +
 src/mjpeg-fallback.cpp|  17 +-
 src/spice-streaming-agent.cpp |  13 +
 src/unittests/Makefile.am |   6 +
 src/utils.cpp |  46 +++
 src/utils.hpp |   4 +
 src/x11-display-info.cpp  | 302 ++
 14 files changed, 633 insertions(+), 2 deletions(-)
 create mode 100644 include/spice-streaming-agent/display-info.hpp
 create mode 100644 include/spice-streaming-agent/x11-display-info.hpp
 create mode 100644 src/display-info.cpp
 create mode 100644 src/utils.cpp
 create mode 100644 src/x11-display-info.cpp

diff --git a/configure.ac b/configure.ac
index e66e6d8..fd18efe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -34,8 +34,10 @@ SPICE_PROTOCOL_MIN_VER=0.12.14
 PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 
$SPICE_PROTOCOL_MIN_VER])
 AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 
+PKG_CHECK_MODULES(DRM, libdrm)
 PKG_CHECK_MODULES(X11, x11)
 PKG_CHECK_MODULES(XFIXES, xfixes)
+PKG_CHECK_MODULES(XRANDR, xrandr)
 
 PKG_CHECK_MODULES(JPEG, libjpeg, , [
 AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
diff --git a/include/spice-streaming-agent/Makefile.am 
b/include/spice-streaming-agent/Makefile.am
index bcd679b..96c1a57 100644
--- a/include/spice-streaming-agent/Makefile.am
+++ b/include/spice-streaming-agent/Makefile.am
@@ -1,8 +1,10 @@
 NULL =
 public_includedir = $(includedir)/spice-streaming-agent
 public_include_HEADERS = \
+   display-info.hpp \
error.hpp \
frame-capture.hpp \
plugin.hpp \
+   x11-display-info.hpp \
$(NULL)
 
diff --git a/include/spice-streaming-agent/display-info.hpp 
b/include/spice-streaming-agent/display-info.hpp
new file mode 100644
index 000..f16212b
--- /dev/null
+++ b/include/spice-streaming-agent/display-info.hpp
@@ -0,0 +1,52 @@
+/* \copyright
+ * Copyright 2018 Red Hat Inc. All rights reserved.
+ */
+
+#ifndef SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
+#define SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
+
+#include 
+#include 
+
+
+namespace spice __attribute__ ((visibility ("default"))) {
+namespace streaming_agent {
+
+/**
+ * Lists graphics cards listed in the DRM sybsystem in /sys/class/drm.
+ * Throws an instance of Error in case of an I/O error.
+ *
+ * @return a vector of paths of all graphics cards present in /sys/class/drm
+ */
+std::vector list_cards();
+
+/**
+ * Reads a single number in hex format from a file.
+ * Throws an instance of Error in case of an I/O or parsing error.
+ *
+ * @param path the path to the file
+ * @return the number parsed from the file
+ */
+uint32_t read_hex_number_from_file(const std::string );
+
+/**
+ * Resolves any symlinks and then extracts the PCI path from the canonical path
+ * to a card. Returns the path in the following format:
+ * "pci//./.../."
+ *
+ *  is the PCI domain, followed by . of any PCI bridges
+ * in the chain leading to the device. The last . is the
+ * graphics device. All of , ,  are hexadecimal numbers
+ * with the following number of digits:
+ *   : 4
+ *   : 2
+ *   : 1
+ *
+ * @param device_path the path to the card
+ * @return the device address
+ */
+std::string get_device_address(const std::string _path);
+
+}} // namespace spice::streaming_agent
+
+#endif // SPICE_STREAMING_AGENT_DISPLAY_INFO_HPP
diff --git a/include/spice-streaming-agent/frame-capture.hpp 
b/include/spice-streaming-agent/frame-capture.hpp
index 51a2987..c244fb9 100644
--- a/include/spice-streaming-agent/frame-capture.hpp
+++ b/include/spice-streaming-agent/frame-capture.hpp
@@ -6,7 +6,11 @@
  */
 #ifndef SPICE_STREAMING_AGENT_FRAME_CAPTURE_HPP
 #define SPICE_STREAMING_AGENT_FRAME_CAPTURE_HPP
+
+#include 
 #include 
+#include 
+#include 
 
 #include 
 
@@ -29,6 +33,13 @@ struct FrameInfo
 bool stream_start;
 };
 
+struct DeviceDisplayInfo
+{
+uint32_t stream_id;
+std::string device_address;
+uint32_t 

[Spice-devel] [PATCH spice 4/8 v3] Receive the GraphicsDeviceInfo message from the streaming agent

[Lukáš Hrázký]

Receives the GraphicsDeviceInfo message from the streaming agent and
stores the data in a list on the streaming device.

Signed-off-by: Lukáš Hrázký 
---
 server/display-limits.h|  3 ++
 server/red-qxl.c   |  2 +-
 server/red-stream-device.c | 78 +-
 server/red-stream-device.h |  8 
 4 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/server/display-limits.h b/server/display-limits.h
index e875149b..d79d3211 100644
--- a/server/display-limits.h
+++ b/server/display-limits.h
@@ -25,4 +25,7 @@
 /** Maximum number of streams created by spice-server */
 #define NUM_STREAMS 50
 
+/** Maximum length of the device address string */
+#define MAX_DEVICE_ADDRESS_LEN 256
+
 #endif /* DISPLAY_LIMITS_H_ */
diff --git a/server/red-qxl.c b/server/red-qxl.c
index a56d9a52..943ccb08 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -37,11 +37,11 @@
 #include "dispatcher.h"
 #include "red-parse-qxl.h"
 #include "red-channel-client.h"
+#include "display-limits.h"
 
 #include "red-qxl.h"
 
 
-#define MAX_DEVICE_ADDRESS_LEN 256
 #define MAX_MONITORS_COUNT 16
 
 struct QXLState {
diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index 215ddbe7..6c90d77d 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -37,6 +37,7 @@ struct StreamDevice {
 StreamMsgCapabilities capabilities;
 StreamMsgCursorSet cursor_set;
 StreamMsgCursorMove cursor_move;
+StreamMsgDeviceDisplayInfo device_display_info;
 uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
 } *msg;
 uint32_t msg_pos;
@@ -49,6 +50,7 @@ struct StreamDevice {
 CursorChannel *cursor_channel;
 SpiceTimer *close_timer;
 uint32_t frame_mmtime;
+StreamDeviceDisplayInfo device_display_info;
 };
 
 struct StreamDeviceClass {
@@ -64,8 +66,8 @@ static void char_device_set_state(RedCharDevice *char_dev, 
int state);
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 SPICE_GNUC_WARN_UNUSED_RESULT;
 
-static StreamMsgHandler handle_msg_format, handle_msg_data, 
handle_msg_cursor_set,
-handle_msg_cursor_move, handle_msg_capabilities;
+static StreamMsgHandler handle_msg_format, handle_msg_device_display_info, 
handle_msg_data,
+handle_msg_cursor_set, handle_msg_cursor_move, handle_msg_capabilities;
 
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
@@ -146,6 +148,13 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 handled = handle_msg_format(dev, sin);
 }
 break;
+case STREAM_TYPE_DEVICE_DISPLAY_INFO:
+if (dev->hdr.size > sizeof(StreamMsgDeviceDisplayInfo) + 
MAX_DEVICE_ADDRESS_LEN) {
+handled = handle_msg_invalid(dev, sin, "StreamMsgDeviceDisplayInfo 
too large");
+} else {
+handled = handle_msg_device_display_info(dev, sin);
+}
+break;
 case STREAM_TYPE_DATA:
 if (dev->hdr.size > 32*1024*1024) {
 handled = handle_msg_invalid(dev, sin, "STREAM_DATA too large");
@@ -271,6 +280,71 @@ handle_msg_format(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 return true;
 }
 
+static bool
+handle_msg_device_display_info(StreamDevice *dev, SpiceCharDeviceInstance *sin)
+{
+SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
+
+if (spice_extra_checks) {
+spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
+spice_assert(dev->hdr.type == STREAM_TYPE_DEVICE_DISPLAY_INFO);
+}
+
+if (dev->msg_len < dev->hdr.size) {
+dev->msg = g_realloc(dev->msg, dev->hdr.size);
+dev->msg_len = dev->hdr.size;
+}
+
+/* read from device */
+ssize_t n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size - 
dev->msg_pos);
+if (n <= 0) {
+return dev->msg_pos == dev->hdr.size;
+}
+
+dev->msg_pos += n;
+if (dev->msg_pos != dev->hdr.size) { /* some bytes are still missing */
+return false;
+}
+
+StreamMsgDeviceDisplayInfo *display_info_msg = 
>msg->device_display_info;
+
+size_t device_address_len = 
GUINT32_FROM_LE(display_info_msg->device_address_len);
+if (device_address_len > MAX_DEVICE_ADDRESS_LEN) {
+g_warning("Received a device address longer than %u (%zu), "
+  "will be truncated!", MAX_DEVICE_ADDRESS_LEN, 
device_address_len);
+device_address_len = sizeof(dev->device_display_info.device_address);
+}
+
+if (device_address_len == 0) {
+g_warning("Zero length device_address in  DeviceDisplayInfo message, 
ignoring.");
+return true;
+}
+
+if (display_info_msg->device_address + device_address_len > (uint8_t*) 
dev->msg + dev->hdr.size) {
+g_warning("Malformed DeviceDisplayInfo message, device_address length 
(%zu) "
+  "goes beyond 

[Spice-devel] [PATCH spice 5/8 v3] Send the graphics device info from streaming agent to the vd_agent

[Lukáš Hrázký]

Adds the graphics device info from the streaming device(s) to the
VDAgentGraphicsDeviceInfo message sent to the vd_agent.

Signed-off-by: Lukáš Hrázký 
---
 server/red-stream-device.c | 18 +++
 server/red-stream-device.h |  7 +
 server/reds.c  | 63 --
 3 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index 6c90d77d..e7ed9f76 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -342,6 +342,8 @@ handle_msg_device_display_info(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 dev->device_display_info.device_address,
 dev->device_display_info.device_display_id);
 
+
reds_send_device_display_info(red_char_device_get_server(RED_CHAR_DEVICE(dev)));
+
 return true;
 }
 
@@ -799,6 +801,22 @@ stream_device_init(StreamDevice *dev)
 dev->msg_len = sizeof(*dev->msg);
 }
 
+StreamDeviceDisplayInfo *stream_device_get_device_display_info(StreamDevice 
*dev)
+{
+return >device_display_info;
+}
+
+int32_t stream_device_get_stream_channel_id(StreamDevice *dev)
+{
+if (dev->stream_channel == NULL) {
+return -1;
+}
+
+int32_t channel_id;
+g_object_get(dev->stream_channel, "id", _id, NULL);
+return channel_id;
+}
+
 static StreamDevice *
 stream_device_new(SpiceCharDeviceInstance *sin, RedsState *reds)
 {
diff --git a/server/red-stream-device.h b/server/red-stream-device.h
index 996be016..d7ab5e41 100644
--- a/server/red-stream-device.h
+++ b/server/red-stream-device.h
@@ -56,6 +56,13 @@ StreamDevice *stream_device_connect(RedsState *reds, 
SpiceCharDeviceInstance *si
  */
 void stream_device_create_channel(StreamDevice *dev);
 
+StreamDeviceDisplayInfo *stream_device_get_device_display_info(StreamDevice 
*dev);
+
+/**
+ * Returns -1 if the StreamDevice doesn't have a channel yet.
+ */
+int32_t stream_device_get_stream_channel_id(StreamDevice *dev);
+
 G_END_DECLS
 
 #endif /* STREAM_DEVICE_H */
diff --git a/server/reds.c b/server/reds.c
index 88a82419..22e81d73 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -902,14 +902,33 @@ void reds_send_device_display_info(RedsState *reds)
 }
 g_debug("Sending device display info to the agent:");
 
-size_t message_size = sizeof(VDAgentGraphicsDeviceInfo);
 QXLInstance *qxl;
+RedCharDevice *dev;
+
+size_t message_size = sizeof(VDAgentGraphicsDeviceInfo);
+
+// size for the qxl device info
 FOREACH_QXL_INSTANCE(reds, qxl) {
 message_size +=
 (sizeof(VDAgentDeviceDisplayInfo) + 
strlen(red_qxl_get_device_address(qxl)) + 1) *
 red_qxl_get_monitors_count(qxl);
 }
 
+// size for the stream device info
+GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
+if (IS_STREAM_DEVICE(dev)) {
+size_t device_address_len =
+
strlen(stream_device_get_device_display_info(STREAM_DEVICE(dev))->device_address);
+
+if (device_address_len == 0) {
+// the device info wasn't set (yet), don't send it
+continue;
+}
+
+message_size += (sizeof(VDAgentDeviceDisplayInfo) + 
device_address_len + 1);
+}
+}
+
 RedCharDeviceWriteBuffer *char_dev_buf = 
vdagent_new_write_buffer(reds->agent_dev,
  VD_AGENT_GRAPHICS_DEVICE_INFO,
  message_size,
@@ -925,6 +944,8 @@ void reds_send_device_display_info(RedsState *reds)
 graphics_device_info->count = 0;
 
 VDAgentDeviceDisplayInfo *device_display_info = 
graphics_device_info->display_info;
+
+// add the qxl devices to the message
 FOREACH_QXL_INSTANCE(reds, qxl) {
 for (size_t i = 0; i < red_qxl_get_monitors_count(qxl); ++i) {
 device_display_info->channel_id = qxl->id;
@@ -936,7 +957,45 @@ void reds_send_device_display_info(RedsState *reds)
 device_display_info->device_address_len =
 strlen((char*) device_display_info->device_address) + 1;
 
-g_debug("   channel_id: %u monitor_id: %u, device_address: %s, 
device_display_id: %u",
+g_debug("   (qxl)channel_id: %u monitor_id: %u, 
device_address: %s, device_display_id: %u",
+device_display_info->channel_id,
+device_display_info->monitor_id,
+device_display_info->device_address,
+device_display_info->device_display_id);
+
+device_display_info = (VDAgentDeviceDisplayInfo*) ((char*) 
device_display_info +
+sizeof(VDAgentDeviceDisplayInfo) + 
device_display_info->device_address_len);
+
+graphics_device_info->count++;
+}
+}
+
+// add the stream devices to the message
+GLIST_FOREACH(reds->char_devices, RedCharDevice, dev) {
+if (IS_STREAM_DEVICE(dev)) {
+StreamDevice *stream_dev = 

[Spice-devel] [PATCH spice-protocol 2/8 v3] Add the StreamMsgGraphicsDeviceInfo message

[Lukáš Hrázký]

The message contains information about the graphics device and monitor
belonging to a particular video stream (which maps to a channel) from
the streaming agent.

Signed-off-by: Lukáš Hrázký 
Acked-by: Frediano Ziglio 
---
 spice/stream-device.h | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/spice/stream-device.h b/spice/stream-device.h
index 6add42b..c70690a 100644
--- a/spice/stream-device.h
+++ b/spice/stream-device.h
@@ -90,6 +90,8 @@ typedef enum StreamMsgType {
 STREAM_TYPE_CURSOR_SET,
 /* guest cursor position */
 STREAM_TYPE_CURSOR_MOVE,
+/* the graphics device display information message (device address and 
display id) */
+STREAM_TYPE_DEVICE_DISPLAY_INFO,
 } StreamMsgType;
 
 typedef enum StreamCapabilities {
@@ -140,6 +142,35 @@ typedef struct StreamMsgData {
 uint8_t data[0];
 } StreamMsgData;
 
+/* This message contains information about the graphics device and monitor
+ * belonging to a particular video stream (which maps to a channel) from
+ * the streaming agent.
+ *
+ * The device_address is the hardware address of the device (e.g. PCI),
+ * device_display_id is the id of the monitor on the device.
+ *
+ * The supported device address format is:
+ * "pci//./.../."
+ *
+ * The "pci" identifies the rest of the string as a PCI address. It is the only
+ * supported address at the moment, other identifiers can be introduced later.
+ *  is the PCI domain, followed by . of any PCI bridges
+ * in the chain leading to the device. The last . is the
+ * graphics device. All of , ,  are hexadecimal numbers
+ * with the following number of digits:
+ *   : 4
+ *   : 2
+ *   : 1
+ *
+ * Sent from the streaming agent to the server.
+ */
+typedef struct StreamMsgDeviceDisplayInfo {
+uint32_t stream_id;
+uint32_t device_display_id;
+uint32_t device_address_len;
+uint8_t device_address[0];  // a zero-terminated string
+} StreamMsgDeviceDisplayInfo;
+
 /* Tell to stop current stream and possibly start a new one.
  * This message is sent by the host to the guest.
  * Allows to communicate the codecs supported by the clients.
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH vd_agent 8/8 v3] Receive the graphics_device_info message

[Lukáš Hrázký]

The graphics_device_info message contains the device display ID
information (device address and device display ID). Stores the data in a
hash table in vdagent.

Signed-off-by: Lukáš Hrázký 
---
 src/vdagent/vdagent.c|  3 ++
 src/vdagent/x11-priv.h   |  1 +
 src/vdagent/x11-randr.c  | 65 
 src/vdagent/x11.c| 13 
 src/vdagent/x11.h|  1 +
 src/vdagentd-proto-strings.h |  1 +
 src/vdagentd-proto.h |  1 +
 src/vdagentd/vdagentd.c  | 16 +
 8 files changed, 101 insertions(+)

diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
index 9642a30..aa52ee9 100644
--- a/src/vdagent/vdagent.c
+++ b/src/vdagent/vdagent.c
@@ -240,6 +240,9 @@ static void daemon_read_complete(struct udscs_connection 
**connp,
   ((VDAgentFileXferDataMessage 
*)data)->id);
 }
 break;
+case VDAGENTD_GRAPHICS_DEVICE_INFO:
+vdagent_x11_handle_graphics_device_info(agent->x11, data, 
header->size);
+break;
 case VDAGENTD_CLIENT_DISCONNECTED:
 vdagent_clipboards_release_all(agent->clipboards);
 if (vdagent_finalize_file_xfer(agent)) {
diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
index b31b0a5..0e954cf 100644
--- a/src/vdagent/x11-priv.h
+++ b/src/vdagent/x11-priv.h
@@ -139,6 +139,7 @@ struct vdagent_x11 {
 int xrandr_minor;
 int has_xinerama;
 int dont_send_guest_xorg_res;
+GHashTable *graphics_display_infos;
 };
 
 extern int (*vdagent_x11_prev_error_handler)(Display *, XErrorEvent *);
diff --git a/src/vdagent/x11-randr.c b/src/vdagent/x11-randr.c
index 192b888..405fca9 100644
--- a/src/vdagent/x11-randr.c
+++ b/src/vdagent/x11-randr.c
@@ -727,6 +727,71 @@ static void dump_monitors_config(struct vdagent_x11 *x11,
 }
 }
 
+typedef struct GraphicsDisplayInfo {
+char device_address[256];
+uint32_t device_display_id;
+} GraphicsDisplayInfo;
+
+// handle the device info message from the server. This will allow us to
+// maintain a mapping from spice display id to xrandr output
+void vdagent_x11_handle_graphics_device_info(struct vdagent_x11 *x11, uint8_t 
*data, size_t size)
+{
+VDAgentGraphicsDeviceInfo *graphics_device_info = 
(VDAgentGraphicsDeviceInfo *)data;
+VDAgentDeviceDisplayInfo *device_display_info = 
graphics_device_info->display_info;
+
+void *buffer_end = data + size;
+
+syslog(LOG_INFO, "Received Graphics Device Info:");
+
+for (size_t i = 0; i < graphics_device_info->count; ++i) {
+if ((void*) device_display_info > buffer_end ||
+(void*) (_display_info->device_address +
+device_display_info->device_address_len) > buffer_end) {
+syslog(LOG_ERR, "Malformed graphics_display_info message, "
+   "extends beyond the end of the buffer");
+break;
+}
+
+GraphicsDisplayInfo *value = g_malloc(sizeof(GraphicsDisplayInfo));
+value->device_address[0] = '\0';
+
+size_t device_address_len = device_display_info->device_address_len;
+if (device_address_len > sizeof(value->device_address)) {
+syslog(LOG_ERR, "Received a device address longer than %lu, "
+   "will be truncated!", device_address_len);
+device_address_len = sizeof(value->device_address);
+}
+
+strncpy(value->device_address,
+(char*) device_display_info->device_address,
+device_address_len);
+
+if (device_address_len > 0) {
+value->device_address[device_address_len - 1] = '\0';  // make 
sure the string is terminated
+} else {
+syslog(LOG_WARNING, "Zero length device_address received for 
channel_id: %u, monitor_id: %u",
+   device_display_info->channel_id, 
device_display_info->monitor_id);
+}
+
+value->device_display_id = device_display_info->device_display_id;
+
+syslog(LOG_INFO, "   channel_id: %u monitor_id: %u device_address: %s, 
"
+   "device_display_id: %u",
+   device_display_info->channel_id,
+   device_display_info->monitor_id,
+   value->device_address,
+   value->device_display_id);
+
+g_hash_table_insert(x11->graphics_display_infos,
+GUINT_TO_POINTER(device_display_info->channel_id + 
device_display_info->monitor_id),
+value);
+
+device_display_info = (VDAgentDeviceDisplayInfo*) ((char*) 
device_display_info +
+sizeof(VDAgentDeviceDisplayInfo) + 
device_display_info->device_address_len);
+}
+}
+
+
 /*
  * Set monitor configuration according to client request.
  *
diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
index 53d3c48..2473383 100644
--- a/src/vdagent/x11.c
+++ b/src/vdagent/x11.c
@@ -196,6 +196,12 @@ static gchar *vdagent_x11_get_wm_name(struct vdagent_x11 
*x11)
 #endif
 }
 
+static void 

[Spice-devel] [PATCH spice-streaming-agent 7/8 v3] Send the GraphicsDeviceInfo to the server

[Lukáš Hrázký]

Adds serialization of the GraphicsDeviceInfo message and sends it to the
server when it starts to stream.

Signed-off-by: Lukáš Hrázký 
---
 src/spice-streaming-agent.cpp | 54 +--
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 3024d98..891e4cb 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -93,6 +94,33 @@ public:
 }
 };
 
+class DeviceDisplayInfoMessage : public 
OutboundMessage
+{
+public:
+DeviceDisplayInfoMessage(const DeviceDisplayInfo ) : 
OutboundMessage(info) {}
+
+static size_t size(const DeviceDisplayInfo )
+{
+return sizeof(PayloadType) + std::min(info.device_address.length(), 
255lu) + 1;
+}
+
+void write_message_body(StreamPort _port, const DeviceDisplayInfo 
)
+{
+std::string device_address = info.device_address;
+if (device_address.length() > 255) {
+syslog(LOG_WARNING,
+   "device address of stream id %u is longer than 255 bytes, 
trimming.", info.stream_id);
+device_address = device_address.substr(0, 255);
+}
+StreamMsgDeviceDisplayInfo strm_msg_info{};
+strm_msg_info.stream_id = info.stream_id;
+strm_msg_info.device_display_id = info.device_display_id;
+strm_msg_info.device_address_len = device_address.length() + 1;
+stream_port.write(_msg_info, sizeof(strm_msg_info));
+stream_port.write(device_address.c_str(), device_address.length() + 1);
+}
+};
+
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static std::set client_codecs;
@@ -217,17 +245,25 @@ do_capture(StreamPort _port, FrameLog _log)
 throw std::runtime_error("cannot find a suitable capture system");
 }
 
+std::vector display_info;
 try {
-std::vector display_info = 
capture->get_device_display_info();
-syslog(LOG_DEBUG, "Got device info of %lu devices from the 
plugin", display_info.size());
-for (const auto  : display_info) {
-syslog(LOG_DEBUG, "   id %u: device address %s, device display 
id: %u",
-   info.id,
-   info.device_address.c_str(),
-   info.device_display_id);
-}
+display_info = capture->get_device_display_info();
 } catch (const Error ) {
-syslog(LOG_ERR, "Error while getting device info: %s", e.what());
+syslog(LOG_ERR, "Error while getting device display info: %s", 
e.what());
+}
+
+syslog(LOG_DEBUG, "Got device info of %zu devices from the plugin", 
display_info.size());
+for (const auto  : display_info) {
+syslog(LOG_DEBUG, "   stream id %u: device address: %s, device 
display id: %u",
+   info.stream_id,
+   info.device_address.c_str(),
+   info.device_display_id);
+}
+
+if (display_info.size() > 0) {
+stream_port.send(display_info[0]);
+} else {
+syslog(LOG_ERR, "Empty device display info from the plugin");
 }
 
 while (!quit_requested && streaming_requested) {
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-protocol 1/8 v3] Add the VDAgentGraphicsDeviceInfo message

[Lukáš Hrázký]

The message serves for passing the device address and device display ID
information for all display channels from SPICE server to the vd_agent.

Signed-off-by: Lukáš Hrázký 
Acked-by: Frediano Ziglio 
---
 spice/vd_agent.h | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/spice/vd_agent.h b/spice/vd_agent.h
index dda7044..42ec77a 100644
--- a/spice/vd_agent.h
+++ b/spice/vd_agent.h
@@ -91,6 +91,7 @@ enum {
 VD_AGENT_CLIENT_DISCONNECTED,
 VD_AGENT_MAX_CLIPBOARD,
 VD_AGENT_AUDIO_VOLUME_SYNC,
+VD_AGENT_GRAPHICS_DEVICE_INFO,
 VD_AGENT_END_MESSAGE,
 };
 
@@ -248,6 +249,27 @@ typedef struct SPICE_ATTR_PACKED VDAgentAudioVolumeSync {
 uint16_t volume[0];
 } VDAgentAudioVolumeSync;
 
+typedef struct SPICE_ATTR_PACKED VDAgentDeviceDisplayInfo {
+uint32_t channel_id;
+uint32_t monitor_id;
+uint32_t device_display_id;
+uint32_t device_address_len;
+uint8_t device_address[0];  // a zero-terminated string
+} VDAgentDeviceDisplayInfo;
+
+
+/* This message contains the mapping of (channel_id, monitor_id) pair to a
+ * "physical" (virtualized) device and its monitor identified by device_address
+ * and device_display_id.
+ *
+ * It's used on the vd_agent to identify the guest monitors for the
+ * mouse_position and monitors_config messages.
+ */
+typedef struct SPICE_ATTR_PACKED VDAgentGraphicsDeviceInfo {
+uint32_t count;
+VDAgentDeviceDisplayInfo display_info[0];
+} VDAgentGraphicsDeviceInfo;
+
 enum {
 VD_AGENT_CAP_MOUSE_STATE = 0,
 VD_AGENT_CAP_MONITORS_CONFIG,
@@ -264,6 +286,7 @@ enum {
 VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
 VD_AGENT_CAP_FILE_XFER_DISABLED,
 VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
+VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
 VD_AGENT_END_CAP,
 };
 
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v2] NEWS: prepare v0.36 release

[Frediano Ziglio]

>
> Hi,
> 
> On 1/16/19 12:35 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   NEWS | 25 +
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/NEWS b/NEWS
> > index 3f54f77..1693621 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,3 +1,28 @@
> > +v0.36
> > +=
> > +
> > +- Add meson build: autotools will be removed in a future release
> > +- Deprecate PulseAudio backend: it will be removed in a future
> > +  release, please use GStreamer instead and report issues
> > +- Add H265 stream support (requires GStreamer support)
> > +- Add SpiceQmpPort helper to interact with QEMU monitor over a Spice port
> > +- Display a message if EGL support is required (with dmabuf local
> > rendering)
> > +- Many GstVideoOverlay improvements
> > +- Smooth-scrolling improvements
> > +- Fix accumulating decoded frames with GStreamer
> > +- Fix reconnection handling
> > +- Fix small memory leaks due to bad refcount of images in the cache
> > +- Fix build for newer LibreSSL 2.7
> > +- Fix a crash on Windows with libusb 1.0.22 when UsbDk is not installed
> > +- Require Gtk >= 3.22
> > +- Require GStreamer >= 1.0
> > +- Require spice-protocol >= 1.12.15
> > +- Require usbredir >= 0.5
> > +- Require libusb >= 1.0.16
> > +- Require libcacard >= 2.5.1
> > +- Require lz4 >= 1.7.3
> > +- Require json-glib >= 1.0
> 
> 
> Maybe worth mentioning also spice-common was updated and that gsteramer
> is a requirement now
>

Personally I think spice-common is just an internal requirement no
not much sense to specify it.

For GStreamer maybe changing

  - Require GStreamer >= 1.0

to

  - Require GStreamer >= 1.0, now not optional

(but looks weird)
 
> I partly missed the related IRC chat yesterday so if no additional
> patches/features asked for v0.36 it's fine with me.
> 
> 
> Snir.
> 

For me the patch is fine,
Acked-by: Frediano Ziglio 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk] meson: improve gtk-doc build

[marcandre . lureau]

From: Marc-André Lureau 

- Fix the following warnings:
./spice-gtk-sections.txt:467: warning: No declaration found for 
SPICE_GTK_CHECK_VERSION.
./spice-gtk-sections.txt:468: warning: No declaration found for 
SPICE_GTK_MAJOR_VERSION.
./spice-gtk-sections.txt:469: warning: No declaration found for 
SPICE_GTK_MICRO_VERSION.
./spice-gtk-sections.txt:470: warning: No declaration found for 
SPICE_GTK_MINOR_VERSION.

- fixxref for glib and gtk (thus requires gtk+ to build doc)

- And other minor simplifications.

After autotools is removed, we should try to use --rebuild-types. For
now I prefer not to touch it :)

Signed-off-by: Marc-André Lureau 
---
 doc/reference/meson.build | 31 +--
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/doc/reference/meson.build b/doc/reference/meson.build
index a121e66..72fcabe 100644
--- a/doc/reference/meson.build
+++ b/doc/reference/meson.build
@@ -38,14 +38,33 @@ ignore_headers = [
 
 spice_gtk_doc_dep = declare_dependency(link_with : [spice_client_gtk_lib, 
spice_client_glib_lib])
 
-gnome.gtkdoc('spice-gtk',
- content_files : ['spice-gtk-overrides.txt', 
'spice-gtk-overrides.txt'],
+glib_prefix = dependency('glib-2.0').get_pkgconfig_variable('prefix')
+glib_docpath = join_paths(glib_prefix, 'share', 'gtk-doc', 'html')
+gtk_prefix = dependency('gtk+-3.0').get_pkgconfig_variable('prefix')
+gtk_docpath = join_paths(gtk_prefix, 'share', 'gtk-doc', 'html')
+docpath = join_paths(spice_gtk_datadir, 'gtk-doc', 'html')
+
+gnome.gtkdoc(meson.project_name(),
  dependencies : spice_gtk_doc_dep,
- main_xml : 'spice-gtk-docs.xml',
- gobject_typesfile : files('spice-gtk.types'),
+ main_xml : meson.project_name() + '-docs.xml',
+ gobject_typesfile : meson.project_name() + '.types',
  ignore_headers : ignore_headers,
  include_directories: spice_gtk_include,
  c_args : '-DSPICE_COMPILATION',
  install : true,
- scan_args : ['--deprecated-guards="SPICE_DISABLE_DEPRECATED"', 
'--ignore-decorators="G_GNUC_INTERNAL"'],
- src_dir : join_paths(meson.source_root(), 'src'))
+ scan_args : [
+   '--deprecated-guards="SPICE_DISABLE_DEPRECATED"',
+   '--ignore-decorators="G_GNUC_INTERNAL"'
+ ],
+ src_dir : [
+   join_paths(meson.source_root(), 'src'),
+   join_paths(meson.build_root(), 'src'),
+ ],
+ fixxref_args: [
+   '--html-dir=@0@'.format(docpath),
+   '--extra-dir=@0@'.format(join_paths(glib_docpath, 'glib')),
+   '--extra-dir=@0@'.format(join_paths(glib_docpath, 'gobject')),
+   '--extra-dir=@0@'.format(join_paths(glib_docpath, 'gio')),
+   '--extra-dir=@0@'.format(join_paths(gtk_docpath, 'gtk3')),
+ ],
+)
-- 
2.20.1.98.gecbdaf0899

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v2] NEWS: prepare v0.36 release

[Snir Sheriber]

Hi,

On 1/16/19 12:35 PM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
  NEWS | 25 +
  1 file changed, 25 insertions(+)

diff --git a/NEWS b/NEWS
index 3f54f77..1693621 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,28 @@
+v0.36
+=
+
+- Add meson build: autotools will be removed in a future release
+- Deprecate PulseAudio backend: it will be removed in a future
+  release, please use GStreamer instead and report issues
+- Add H265 stream support (requires GStreamer support)
+- Add SpiceQmpPort helper to interact with QEMU monitor over a Spice port
+- Display a message if EGL support is required (with dmabuf local rendering)
+- Many GstVideoOverlay improvements
+- Smooth-scrolling improvements
+- Fix accumulating decoded frames with GStreamer
+- Fix reconnection handling
+- Fix small memory leaks due to bad refcount of images in the cache
+- Fix build for newer LibreSSL 2.7
+- Fix a crash on Windows with libusb 1.0.22 when UsbDk is not installed
+- Require Gtk >= 3.22
+- Require GStreamer >= 1.0
+- Require spice-protocol >= 1.12.15
+- Require usbredir >= 0.5
+- Require libusb >= 1.0.16
+- Require libcacard >= 2.5.1
+- Require lz4 >= 1.7.3
+- Require json-glib >= 1.0



Maybe worth mentioning also spice-common was updated and that gsteramer 
is a requirement now


I partly missed the related IRC chat yesterday so if no additional 
patches/features asked for v0.36 it's fine with me.



Snir.



+
  v0.35
  =
  

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice 4/8 v2] Receive the GraphicsDeviceInfo message from the streaming agent

[Lukáš Hrázký]

On Tue, 2019-01-15 at 08:31 -0500, Frediano Ziglio wrote:
> > 
> > Receives the GraphicsDeviceInfo message from the streaming agent and
> > stores the data in a list on the streaming device.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  server/display-limits.h|  3 ++
> >  server/red-qxl.c   |  2 +-
> >  server/red-stream-device.c | 67 --
> >  server/red-stream-device.h |  8 +
> >  server/reds.c  |  1 +
> >  5 files changed, 78 insertions(+), 3 deletions(-)
> > 
> > diff --git a/server/display-limits.h b/server/display-limits.h
> > index e875149b..d79d3211 100644
> > --- a/server/display-limits.h
> > +++ b/server/display-limits.h
> > @@ -25,4 +25,7 @@
> >  /** Maximum number of streams created by spice-server */
> >  #define NUM_STREAMS 50
> >  
> > +/** Maximum length of the device address string */
> > +#define MAX_DEVICE_ADDRESS_LEN 256
> > +
> >  #endif /* DISPLAY_LIMITS_H_ */
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index a56d9a52..943ccb08 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -37,11 +37,11 @@
> >  #include "dispatcher.h"
> >  #include "red-parse-qxl.h"
> >  #include "red-channel-client.h"
> > +#include "display-limits.h"
> >  
> >  #include "red-qxl.h"
> >  
> >  
> > -#define MAX_DEVICE_ADDRESS_LEN 256
> >  #define MAX_MONITORS_COUNT 16
> >  
> >  struct QXLState {
> > diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> > index 215ddbe7..20bf115d 100644
> > --- a/server/red-stream-device.c
> > +++ b/server/red-stream-device.c
> > @@ -37,6 +37,7 @@ struct StreamDevice {
> >  StreamMsgCapabilities capabilities;
> >  StreamMsgCursorSet cursor_set;
> >  StreamMsgCursorMove cursor_move;
> > +StreamMsgDeviceDisplayInfo device_display_info;
> >  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> >  } *msg;
> >  uint32_t msg_pos;
> > @@ -49,6 +50,7 @@ struct StreamDevice {
> >  CursorChannel *cursor_channel;
> >  SpiceTimer *close_timer;
> >  uint32_t frame_mmtime;
> > +StreamDeviceDisplayInfo device_display_info;
> >  };
> >  
> >  struct StreamDeviceClass {
> > @@ -64,8 +66,8 @@ static void char_device_set_state(RedCharDevice *char_dev,
> > int state);
> >  typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> >  *sin)
> >  SPICE_GNUC_WARN_UNUSED_RESULT;
> >  
> > -static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_cursor_set,
> > -handle_msg_cursor_move, handle_msg_capabilities;
> > +static StreamMsgHandler handle_msg_format, handle_msg_device_display_info,
> > handle_msg_data,
> > +handle_msg_cursor_set, handle_msg_cursor_move, handle_msg_capabilities;
> >  
> >  static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
> >  *sin,
> > const char *error_msg)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -146,6 +148,13 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  handled = handle_msg_format(dev, sin);
> >  }
> >  break;
> > +case STREAM_TYPE_DEVICE_DISPLAY_INFO:
> > +if (dev->hdr.size > sizeof(StreamMsgDeviceDisplayInfo) +
> > MAX_DEVICE_ADDRESS_LEN) {
> > +handled = handle_msg_invalid(dev, sin,
> > "StreamMsgDeviceDisplayInfo too large");
> > +} else {
> > +handled = handle_msg_device_display_info(dev, sin);
> > +}
> > +break;
> >  case STREAM_TYPE_DATA:
> >  if (dev->hdr.size > 32*1024*1024) {
> >  handled = handle_msg_invalid(dev, sin, "STREAM_DATA too 
> > large");
> > @@ -271,6 +280,60 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  return true;
> >  }
> >  
> > +static bool
> > +handle_msg_device_display_info(StreamDevice *dev, SpiceCharDeviceInstance
> > *sin)
> > +{
> > +SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> > +
> > +if (spice_extra_checks) {
> > +spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> > +spice_assert(dev->hdr.type == STREAM_TYPE_DEVICE_DISPLAY_INFO);
> > +}
> > +
> > +if (dev->msg_len < dev->hdr.size) {
> > +dev->msg = g_realloc(dev->msg, dev->hdr.size);
> > +dev->msg_len = dev->hdr.size;
> > +}
> > +
> > +/* read from device */
> > +ssize_t n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size 
> > -
> > dev->msg_pos);
> > +if (n <= 0) {
> > +return dev->msg_pos == dev->hdr.size;
> > +}
> > +
> > +dev->msg_pos += n;
> > +if (dev->msg_pos != dev->hdr.size) { /* some bytes are still missing */
> > +return false;
> > +}
> > +
> > +StreamMsgDeviceDisplayInfo *display_info_msg =
> > >msg->device_display_info;
> > +
> > +size_t device_address_len =
> > GUINT32_FROM_LE(display_info_msg->device_address_len);
> > +if 

Re: [Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Frediano Ziglio]

> 
> Hi
> 
> On Wed, Jan 16, 2019 at 3:38 PM Frediano Ziglio  wrote:
> >
> > >
> > > Hi
> > >
> > > On Wed, Jan 16, 2019 at 3:14 PM Frediano Ziglio 
> > > wrote:
> > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > >
> > > > > >
> > > > > > On Fri, Jan 11, 2019 at 3:03 PM 
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Marc-André Lureau 
> > > > > > >
> > > > > > > There is a racy bug in pulsesrc that we can't easily workaround:
> > > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> > > > > > >
> > > > > > > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> > > > > > >
> > > > > > > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > > > > > > actual source or mimicking GstAutoDetect is unnecessarily
> > > > > > > complicated.
> > > > > > >
> > > > > > > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it
> > > > > > > won't
> > > > > > > be
> > > > > > > picked.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau 
> > > > > >
> > > > > > I think we agreed that this is the way to go.
> > > > > >
> > > > > > ack for v0.36, or should we continue the discussion for v0.37?
> > > > >
> > > > > ack for v0.36
> > > > >
> > > > > >
> > > > > > >  src/spice-gstaudio.c | 28 
> > > > > > >  1 file changed, 28 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > > > > index d0cfbc6..49e9ae6 100644
> > > > > > > --- a/src/spice-gstaudio.c
> > > > > > > +++ b/src/spice-gstaudio.c
> > > > > > > @@ -527,7 +527,35 @@ SpiceGstaudio
> > > > > > > *spice_gstaudio_new(SpiceSession
> > > > > > > *session, GMainContext *context,
> > > > > > >const char *name)
> > > > > > >  {
> > > > > > >  GError *err = NULL;
> > > > > > > +
> > > > > > >  if (gst_init_check(NULL, NULL, )) {
> > > > > > > +GstPluginFeature *pulsesrc;
> > > > > > > +
> > > > > > > +pulsesrc =
> > > > > > > gst_registry_lookup_feature(gst_registry_get(),
> > > > > > > "pulsesrc");
> > > > > > > +if (pulsesrc) {
> > > > > > > +unsigned major, minor, micro;
> > > > > > > +GstPlugin *plugin =
> > > > > > > gst_plugin_feature_get_plugin(pulsesrc);
> > > > > > > +
> > > > > > > +if (sscanf(gst_plugin_get_version(plugin),
> > > > > > > "%u.%u.%u",
> > > > > > > +   , , ) != 3) {
> >
> > I think this test will fail with versions like 1.15 too.
> > Maybe a
> >
> > unsigned major, minor = 0, micro = 0;
> >
> > if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u", , ,
> > ) < 2) {
> > 
> 
> GStreamer versions are always at least 3 digits afaik. Other versions,
> we can just ignore.
> 

Not much documentation at
https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstPlugin.html#GST-PLUGIN-DEFINE:CAPS

The current code is not actually ignoring, it would return NULL so disabling
GstAudio.

> >
> > OT: is it worth this code on Windows ?
> 
> PA used to build on windows. I don't know today.
> 
> >
> > > > > > > +g_warn_if_reached();
> > > > > > > +gst_object_unref(plugin);
> > > > > > > +gst_object_unref(pulsesrc);
> > > > > > > +return NULL;
> > > > > > > +}
> > > > > > > +
> > > > > > > +if (major < 1 ||
> > > > > > > +(major == 1 && minor < 14) ||
> > > > > > > +(major == 1 && minor == 14 && micro < 5)) {
> > > > > > > +g_warning("Bad pulsesrc version %s, lowering its
> > > > > > > rank",
> > > > > > > +  gst_plugin_get_version(plugin));
> > > > > > > +gst_plugin_feature_set_rank(pulsesrc,
> > > > > > > GST_RANK_NONE);
> > > > > > > +}
> > > > > > > +
> > > > > > > +gst_object_unref(plugin);
> > > > > > > +gst_object_unref(pulsesrc);
> > > > > > > +}
> > > > > > > +
> > > > > > >  return g_object_new(SPICE_TYPE_GSTAUDIO,
> > > > > > >  "session", session,
> > > > > > >  "main-context", context,
> > > >
> > > > Looks like the version merged in master is quite different from this
> > > > version,
> > > > in particular:
> > > >
> > > > if (maj < 1 || min < 15) {
> > > > g_warning("Bad pulsesrc version, lowering its rank");
> > > > gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > > > }
> > > >
> > > > will lower the rank even if the version is 2.0 which probably is higher
> > > > than 1.15.
> > > >
> > > > By the way, it looks like you pushed the wrong patch, like this old one
> > > > https://lists.freedesktop.org/archives/spice-devel/2019-January/047138.html
> > > > which was not acked.
> > >
> > > 

Re: [Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 3:38 PM Frediano Ziglio  wrote:
>
> >
> > Hi
> >
> > On Wed, Jan 16, 2019 at 3:14 PM Frediano Ziglio  wrote:
> > >
> > > >
> > > > Hi,
> > > >
> > > > On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > >
> > > > > On Fri, Jan 11, 2019 at 3:03 PM  wrote:
> > > > > >
> > > > > > From: Marc-André Lureau 
> > > > > >
> > > > > > There is a racy bug in pulsesrc that we can't easily workaround:
> > > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> > > > > >
> > > > > > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> > > > > >
> > > > > > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > > > > > actual source or mimicking GstAutoDetect is unnecessarily
> > > > > > complicated.
> > > > > >
> > > > > > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it won't
> > > > > > be
> > > > > > picked.
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau 
> > > > >
> > > > > I think we agreed that this is the way to go.
> > > > >
> > > > > ack for v0.36, or should we continue the discussion for v0.37?
> > > >
> > > > ack for v0.36
> > > >
> > > > >
> > > > > >  src/spice-gstaudio.c | 28 
> > > > > >  1 file changed, 28 insertions(+)
> > > > > >
> > > > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > > > index d0cfbc6..49e9ae6 100644
> > > > > > --- a/src/spice-gstaudio.c
> > > > > > +++ b/src/spice-gstaudio.c
> > > > > > @@ -527,7 +527,35 @@ SpiceGstaudio *spice_gstaudio_new(SpiceSession
> > > > > > *session, GMainContext *context,
> > > > > >const char *name)
> > > > > >  {
> > > > > >  GError *err = NULL;
> > > > > > +
> > > > > >  if (gst_init_check(NULL, NULL, )) {
> > > > > > +GstPluginFeature *pulsesrc;
> > > > > > +
> > > > > > +pulsesrc = gst_registry_lookup_feature(gst_registry_get(),
> > > > > > "pulsesrc");
> > > > > > +if (pulsesrc) {
> > > > > > +unsigned major, minor, micro;
> > > > > > +GstPlugin *plugin =
> > > > > > gst_plugin_feature_get_plugin(pulsesrc);
> > > > > > +
> > > > > > +if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> > > > > > +   , , ) != 3) {
>
> I think this test will fail with versions like 1.15 too.
> Maybe a
>
> unsigned major, minor = 0, micro = 0;
>
> if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u", , , 
> ) < 2) {
> 

GStreamer versions are always at least 3 digits afaik. Other versions,
we can just ignore.

>
> OT: is it worth this code on Windows ?

PA used to build on windows. I don't know today.

>
> > > > > > +g_warn_if_reached();
> > > > > > +gst_object_unref(plugin);
> > > > > > +gst_object_unref(pulsesrc);
> > > > > > +return NULL;
> > > > > > +}
> > > > > > +
> > > > > > +if (major < 1 ||
> > > > > > +(major == 1 && minor < 14) ||
> > > > > > +(major == 1 && minor == 14 && micro < 5)) {
> > > > > > +g_warning("Bad pulsesrc version %s, lowering its
> > > > > > rank",
> > > > > > +  gst_plugin_get_version(plugin));
> > > > > > +gst_plugin_feature_set_rank(pulsesrc,
> > > > > > GST_RANK_NONE);
> > > > > > +}
> > > > > > +
> > > > > > +gst_object_unref(plugin);
> > > > > > +gst_object_unref(pulsesrc);
> > > > > > +}
> > > > > > +
> > > > > >  return g_object_new(SPICE_TYPE_GSTAUDIO,
> > > > > >  "session", session,
> > > > > >  "main-context", context,
> > >
> > > Looks like the version merged in master is quite different from this
> > > version,
> > > in particular:
> > >
> > > if (maj < 1 || min < 15) {
> > > g_warning("Bad pulsesrc version, lowering its rank");
> > > gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > > }
> > >
> > > will lower the rank even if the version is 2.0 which probably is higher
> > > than 1.15.
> > >
> > > By the way, it looks like you pushed the wrong patch, like this old one
> > > https://lists.freedesktop.org/archives/spice-devel/2019-January/047138.html
> > > which was not acked.
> >
> > yes, wrong version, my bad!
> >
> > Thanks for noticing!!
> >
>
> Maybe more clean to revert the pushed one and merge an updated one?
>
> Frediano



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH vd_agent_linux] test-file-xfers: Make test succeed if run as root

[Frediano Ziglio]

If test is run with root (for instance on Gitlab CI) creating
a file on a directory with no permission will succeed and
test will fail.
Instead create a link trying to create a file in /proc/1
directory (which fails also using root account).

Signed-off-by: Frediano Ziglio 
---
 tests/test-file-xfers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

CI results at https://gitlab.freedesktop.org/fziglio/vd_agent/pipelines/15140

diff --git a/tests/test-file-xfers.c b/tests/test-file-xfers.c
index 41d333e..9995336 100644
--- a/tests/test-file-xfers.c
+++ b/tests/test-file-xfers.c
@@ -65,8 +65,8 @@ int main(int argc, char *argv[])
 test_file("subdir/test.txt", "./test-dir/subdir/test.txt");
 
 // create a file in a directory with no permissions
-assert(system("chmod 555 test-dir/subdir") == 0);
-test_file("subdir/test2.txt", NULL);
+assert(system("ln -s /proc/1 test-dir/baddir") == 0);
+test_file("baddir/test2.txt", NULL);
 
 // try to create a file with a path where there's a file (should fail)
 test_file("test.txt/out", NULL);
@@ -77,7 +77,7 @@ int main(int argc, char *argv[])
 // create a file with same name above, should not strip the filename
 test_file("sub.dir/test", "./test-dir/sub.dir/test (1)");
 
-assert(system("chmod 755 test-dir/subdir && rm -rf test-dir") == 0);
+assert(system("rm -rf test-dir") == 0);
 
 return 0;
 }
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Frediano Ziglio]

> 
> Hi
> 
> On Wed, Jan 16, 2019 at 3:14 PM Frediano Ziglio  wrote:
> >
> > >
> > > Hi,
> > >
> > > On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > >
> > > >
> > > > On Fri, Jan 11, 2019 at 3:03 PM  wrote:
> > > > >
> > > > > From: Marc-André Lureau 
> > > > >
> > > > > There is a racy bug in pulsesrc that we can't easily workaround:
> > > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> > > > >
> > > > > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> > > > >
> > > > > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > > > > actual source or mimicking GstAutoDetect is unnecessarily
> > > > > complicated.
> > > > >
> > > > > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it won't
> > > > > be
> > > > > picked.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau 
> > > >
> > > > I think we agreed that this is the way to go.
> > > >
> > > > ack for v0.36, or should we continue the discussion for v0.37?
> > >
> > > ack for v0.36
> > >
> > > >
> > > > >  src/spice-gstaudio.c | 28 
> > > > >  1 file changed, 28 insertions(+)
> > > > >
> > > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > > index d0cfbc6..49e9ae6 100644
> > > > > --- a/src/spice-gstaudio.c
> > > > > +++ b/src/spice-gstaudio.c
> > > > > @@ -527,7 +527,35 @@ SpiceGstaudio *spice_gstaudio_new(SpiceSession
> > > > > *session, GMainContext *context,
> > > > >const char *name)
> > > > >  {
> > > > >  GError *err = NULL;
> > > > > +
> > > > >  if (gst_init_check(NULL, NULL, )) {
> > > > > +GstPluginFeature *pulsesrc;
> > > > > +
> > > > > +pulsesrc = gst_registry_lookup_feature(gst_registry_get(),
> > > > > "pulsesrc");
> > > > > +if (pulsesrc) {
> > > > > +unsigned major, minor, micro;
> > > > > +GstPlugin *plugin =
> > > > > gst_plugin_feature_get_plugin(pulsesrc);
> > > > > +
> > > > > +if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> > > > > +   , , ) != 3) {

I think this test will fail with versions like 1.15 too.
Maybe a

unsigned major, minor = 0, micro = 0;

if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u", , , 
) < 2) {


OT: is it worth this code on Windows ?

> > > > > +g_warn_if_reached();
> > > > > +gst_object_unref(plugin);
> > > > > +gst_object_unref(pulsesrc);
> > > > > +return NULL;
> > > > > +}
> > > > > +
> > > > > +if (major < 1 ||
> > > > > +(major == 1 && minor < 14) ||
> > > > > +(major == 1 && minor == 14 && micro < 5)) {
> > > > > +g_warning("Bad pulsesrc version %s, lowering its
> > > > > rank",
> > > > > +  gst_plugin_get_version(plugin));
> > > > > +gst_plugin_feature_set_rank(pulsesrc,
> > > > > GST_RANK_NONE);
> > > > > +}
> > > > > +
> > > > > +gst_object_unref(plugin);
> > > > > +gst_object_unref(pulsesrc);
> > > > > +}
> > > > > +
> > > > >  return g_object_new(SPICE_TYPE_GSTAUDIO,
> > > > >  "session", session,
> > > > >  "main-context", context,
> >
> > Looks like the version merged in master is quite different from this
> > version,
> > in particular:
> >
> > if (maj < 1 || min < 15) {
> > g_warning("Bad pulsesrc version, lowering its rank");
> > gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > }
> >
> > will lower the rank even if the version is 2.0 which probably is higher
> > than 1.15.
> >
> > By the way, it looks like you pushed the wrong patch, like this old one
> > https://lists.freedesktop.org/archives/spice-devel/2019-January/047138.html
> > which was not acked.
> 
> yes, wrong version, my bad!
> 
> Thanks for noticing!!
> 

Maybe more clean to revert the pushed one and merge an updated one?

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 3:14 PM Frediano Ziglio  wrote:
>
> >
> > Hi,
> >
> > On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > >
> > > On Fri, Jan 11, 2019 at 3:03 PM  wrote:
> > > >
> > > > From: Marc-André Lureau 
> > > >
> > > > There is a racy bug in pulsesrc that we can't easily workaround:
> > > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> > > >
> > > > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> > > >
> > > > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > > > actual source or mimicking GstAutoDetect is unnecessarily complicated.
> > > >
> > > > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it won't be
> > > > picked.
> > > >
> > > > Signed-off-by: Marc-André Lureau 
> > >
> > > I think we agreed that this is the way to go.
> > >
> > > ack for v0.36, or should we continue the discussion for v0.37?
> >
> > ack for v0.36
> >
> > >
> > > >  src/spice-gstaudio.c | 28 
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > > index d0cfbc6..49e9ae6 100644
> > > > --- a/src/spice-gstaudio.c
> > > > +++ b/src/spice-gstaudio.c
> > > > @@ -527,7 +527,35 @@ SpiceGstaudio *spice_gstaudio_new(SpiceSession
> > > > *session, GMainContext *context,
> > > >const char *name)
> > > >  {
> > > >  GError *err = NULL;
> > > > +
> > > >  if (gst_init_check(NULL, NULL, )) {
> > > > +GstPluginFeature *pulsesrc;
> > > > +
> > > > +pulsesrc = gst_registry_lookup_feature(gst_registry_get(),
> > > > "pulsesrc");
> > > > +if (pulsesrc) {
> > > > +unsigned major, minor, micro;
> > > > +GstPlugin *plugin = 
> > > > gst_plugin_feature_get_plugin(pulsesrc);
> > > > +
> > > > +if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> > > > +   , , ) != 3) {
> > > > +g_warn_if_reached();
> > > > +gst_object_unref(plugin);
> > > > +gst_object_unref(pulsesrc);
> > > > +return NULL;
> > > > +}
> > > > +
> > > > +if (major < 1 ||
> > > > +(major == 1 && minor < 14) ||
> > > > +(major == 1 && minor == 14 && micro < 5)) {
> > > > +g_warning("Bad pulsesrc version %s, lowering its rank",
> > > > +  gst_plugin_get_version(plugin));
> > > > +gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > > > +}
> > > > +
> > > > +gst_object_unref(plugin);
> > > > +gst_object_unref(pulsesrc);
> > > > +}
> > > > +
> > > >  return g_object_new(SPICE_TYPE_GSTAUDIO,
> > > >  "session", session,
> > > >  "main-context", context,
>
> Looks like the version merged in master is quite different from this version,
> in particular:
>
> if (maj < 1 || min < 15) {
> g_warning("Bad pulsesrc version, lowering its rank");
> gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> }
>
> will lower the rank even if the version is 2.0 which probably is higher
> than 1.15.
>
> By the way, it looks like you pushed the wrong patch, like this old one
> https://lists.freedesktop.org/archives/spice-devel/2019-January/047138.html
> which was not acked.

yes, wrong version, my bad!

Thanks for noticing!!


-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/3] drm/qxl: add mode/framebuffer check functions

[Gerd Hoffmann]

> > +static int qxl_check_mode(struct qxl_device *qdev,
> > + unsigned int width,
> > + unsigned int height)
> > +{
> > +   if (width * height * 4 > qdev->vram_size)
> 
> Is someone checking for integer overflows already?

Need to have a look.  This is just ...

> > -   if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
> > -   DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
> > -   return -EINVAL;
> > -   }

... that check moved into the new function.

cheers,
  Gerd

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > 
> > On Fri, Jan 11, 2019 at 3:03 PM  wrote:
> > >
> > > From: Marc-André Lureau 
> > >
> > > There is a racy bug in pulsesrc that we can't easily workaround:
> > > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> > >
> > > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> > >
> > > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > > actual source or mimicking GstAutoDetect is unnecessarily complicated.
> > >
> > > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it won't be
> > > picked.
> > >
> > > Signed-off-by: Marc-André Lureau 
> > 
> > I think we agreed that this is the way to go.
> > 
> > ack for v0.36, or should we continue the discussion for v0.37?
> 
> ack for v0.36
> 
> > 
> > >  src/spice-gstaudio.c | 28 
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > > index d0cfbc6..49e9ae6 100644
> > > --- a/src/spice-gstaudio.c
> > > +++ b/src/spice-gstaudio.c
> > > @@ -527,7 +527,35 @@ SpiceGstaudio *spice_gstaudio_new(SpiceSession
> > > *session, GMainContext *context,
> > >const char *name)
> > >  {
> > >  GError *err = NULL;
> > > +
> > >  if (gst_init_check(NULL, NULL, )) {
> > > +GstPluginFeature *pulsesrc;
> > > +
> > > +pulsesrc = gst_registry_lookup_feature(gst_registry_get(),
> > > "pulsesrc");
> > > +if (pulsesrc) {
> > > +unsigned major, minor, micro;
> > > +GstPlugin *plugin = gst_plugin_feature_get_plugin(pulsesrc);
> > > +
> > > +if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> > > +   , , ) != 3) {
> > > +g_warn_if_reached();
> > > +gst_object_unref(plugin);
> > > +gst_object_unref(pulsesrc);
> > > +return NULL;
> > > +}
> > > +
> > > +if (major < 1 ||
> > > +(major == 1 && minor < 14) ||
> > > +(major == 1 && minor == 14 && micro < 5)) {
> > > +g_warning("Bad pulsesrc version %s, lowering its rank",
> > > +  gst_plugin_get_version(plugin));
> > > +gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > > +}
> > > +
> > > +gst_object_unref(plugin);
> > > +gst_object_unref(pulsesrc);
> > > +}
> > > +
> > >  return g_object_new(SPICE_TYPE_GSTAUDIO,
> > >  "session", session,
> > >  "main-context", context,

Looks like the version merged in master is quite different from this version,
in particular:

if (maj < 1 || min < 15) {
g_warning("Bad pulsesrc version, lowering its rank");
gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
}

will lower the rank even if the version is 2.0 which probably is higher
than 1.15.

By the way, it looks like you pushed the wrong patch, like this old one
https://lists.freedesktop.org/archives/spice-devel/2019-January/047138.html
which was not acked.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2] gtk-session: clipboard request: notify agent on failure

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 2:47 PM james harvey  wrote:
>
> On Wed, Jan 16, 2019 at 4:10 AM Victor Toso  wrote:
> >
> > From: Victor Toso 
> >
> > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > data till some timeout happens in the system, blocking copy-paste
> > feature and possibly freezing applications.
> >
> > A way to reproduce is copy-paste big clipboard data between two spice
> > clients.
> >
> > Also add some documentation to the checks, in order to quickly
> > understand what they are about.
> >
> > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > v1 -> v2:
> > - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> > - Removed the reply on read only. (Marc-André)
> >
> > Signed-off-by: Victor Toso 
>
> Am I understanding correctly that the Jan 10 patches which completely
> fix spice for me and others experiencing clipboard problems aren't
> being added in any form, and that instead the clipboard is being left
> broken, just less broken so it doesn't freeze the guest?  I don't know
> exactly when it broke, but spice didn't used to act this way.

That's important information, because spice-gtk clipboard code didn't
change that much afaik.

>
> This patch (v2) alone, on top of current git (ecf9358) at least fixes
> the freezing.  But, if that's all that's added, it's not something I
> can stand using.  It doesn't help the clipboard problems.  It makes me
> want to break my keyboard.  Paste randomly failing this often is
> extremely frustrating and distracting.
>
> Log of it breaking after a single paste, so the second one fails is
> here: https://termbin.com/5d69
>
> The last spice-git I ran was f18589b with the Jan 10 patches:
> * [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event
> * [spice-gtk v3 2/3] gtk-session: clipboard: x11: owner-change improvement
> * [spice-gtk v3 3/3] gtk-session: clipboard: x11: do not request data
> while on focus
> * [Spice-devel] [spice-gtk] gtk-session: prefer early check to agent
> connectivity
>
> That was rock solid.  I have no clipboard problems.

Cool! now we need to have clear understanding for the changes to
accept them upstream, that's what we are working on.

>
> That's going to leave me and others perpetually patching spice
> releases, and someday when they don't apply cleanly, being at a loss
> and being forced to look at other protocols.
>
> Apologies if I'm misunderstanding.

You misunderstand, there is a lot of activity on this problem atm and
it is currently scheduled for 0.36. However, since it is not clear
whether this is a regression and the fixes are *not* trivial, it
should be fine to schedule for 0.37. Either early release after that,
and fixes can be backported too most probably.



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 02:33:49PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jan 16, 2019 at 1:18 PM Victor Toso  wrote:
> >
> > Hi,
> >
> > On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > > > Not true. Possible client-side bug, reproducible on X11, as I
> > > > discussed before. As mentioned in the comment, we are sending
> > > > wrong selection-grab to the guest.
> > >
> > > Sorry, I don't follow "we are sending wrong selection-grab" ?
> >
> > I think I tried too hard already to make you understand that on
> > X11, that's happening while user is copying in the guest VM.
> 
> Sorry, I don't know what you are talking about. When there is a
> complicated question/reply, you should update the comments or
> commit message, so we don't have to dig in old conversations.

Old... Yesterday and the day before and last week...

https://lists.freedesktop.org/archives/spice-devel/2019-January/047192.html

Commit log:

A way to reproduce is copy-paste big clipboard data between
two spice clients.

It is missing 'on x11'.

> > > > > Not sure sending a reply is going to help much in that case...
> > > >
> > > > It helps. Instead of blocking the application, it fails to paste
> > > > the clipboard and all is good.
> > >
> > > But if the agent logic is already wrong, sending a reply isn't going
> > > to help, it could do anything..
> >
> > It might have been a valid request at the time it was sent, if
> > you accept that we sent the selection-grab.
> 
> "we" == the client?
>
> 
> In this case, clipboard_by_guest[] should be false.

I give up Feel free to ignore.

> 
> >
> > > > > > + * sent a bad selection-grab to the agent or the agent is 
> > > > > > buggy. */
> > > > > > +if (s->clipboard_by_guest[selection]) {
> > > > > > +SPICE_DEBUG("clipboard: agent request: grab on hold by 
> > > > > > agent, possible race");
> > > > > > +goto notify_agent;
> > > > > > +}
> > > > > > +
> > > > > > +/* The selection-request by agent should happen only if the 
> > > > > > clipboard data is set
> > > > > > + * by client */
> > > > > > +if (!s->clip_grabbed[selection]) {
> > > > > > +SPICE_DEBUG("clipboard: agent request: data set by agent, 
> > > > > > possible race");
> > > > > > +goto notify_agent;
> > > > > > +}
> > > > >
> > > > > This could be adding more race conditions. clip_grabbed is set
> > > > > asynchronously after owner changed, and indicate if the grab
> > > > > message was sent to the agent,
> > > >
> > > > We send a selection-release on owner-change. If we receive a
> > > > selection-request before agent receives the selection-release,
> > > > this is expected but we should notify the guest, afaics.
> > > >
> > > > > as you correctly say. It doesn't mean you can't request client
> > > > > clipboard content.
> > > >
> > > > > I understand the racy case, but the condition seems wrong, it
> > > > > should attempt to request current client clipboard content, and
> > > > > fail/succeed after.
> > > >
> > > > No. It should request the content from the grab that was sent. If
> > >
> > > There is no such thing as clipboard ID/nth in any clipboard protocol I
> > > know of. User simply request current clipboard content (not the
> > > nth), whether it changed since last grab is not important: it
> > > will retrieve something of the requested type or nothing.
> >
> > Not about the id, but selection metadata itself or the @type
> > parameter in clipboard_request()
> 
> If the clipboard content (of the requested type) can't be retrieve, it
> will fail. It shouldn't be handled here.
> 
> >
> > > > clipboard changed, previous grab gets invalidated and a
> > > > selection-release is sent and another selection-grab is sent with
> > > > the metadata of the new grab.
> > > >
> > > > Proving recent data from old grab seems wrong.
> > >
> > > It is not.
> > >
> > > > >
> > > > > > +/* Ready only, still should reply to agent to avoid it waiting 
> > > > > > for data */
> > > > >
> > > > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > > > that the client is read-only though. So this may be acceptable for
> > > > > now, but we should have a TODO/FIXME..
> > > >
> > > > Well, it should not happen anyway as on read-only we should not
> > > > send selection-grab. I'll fix and resend.
> > >
> > > Right
> > >
> > > >
> > > > Thanks for the quick review.
> > > >
> > > > > > +if (read_only(self)) {
> > > > > > +g_warning("clipboard: agent request: read only, deny 
> > > > > > request");
> > > > > > +goto notify_agent;
> > > > > > +}
> > > > > >
> > > > > >  if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > > > >  gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > > > @@ -1039,6 +1057,10 @@ static gboolean 
> > > > > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > > > >  }
> > > > > >
> > > > > >  return TRUE;
> > > > > > +
> > 

Re: [Spice-devel] [spice-gtk v2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 05:46:58AM -0500, james harvey wrote:
> On Wed, Jan 16, 2019 at 4:10 AM Victor Toso  wrote:
> >
> > From: Victor Toso 
> >
> > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > data till some timeout happens in the system, blocking copy-paste
> > feature and possibly freezing applications.
> >
> > A way to reproduce is copy-paste big clipboard data between two spice
> > clients.
> >
> > Also add some documentation to the checks, in order to quickly
> > understand what they are about.
> >
> > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > v1 -> v2:
> > - Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
> > - Removed the reply on read only. (Marc-André)
> >
> > Signed-off-by: Victor Toso 
> 
> Am I understanding correctly that the Jan 10 patches which completely
> fix spice for me and others experiencing clipboard problems aren't
> being added in any form, and that instead the clipboard is being left
> broken, just less broken so it doesn't freeze the guest?  I don't know
> exactly when it broke, but spice didn't used to act this way.

No, it'll probably be kept broken broken, not just broken, it
seems.

> This patch (v2) alone, on top of current git (ecf9358) at least fixes
> the freezing.  But, if that's all that's added, it's not something I
> can stand using.  It doesn't help the clipboard problems.  It makes me
> want to break my keyboard.  Paste randomly failing this often is
> extremely frustrating and distracting.
> 
> Log of it breaking after a single paste, so the second one fails is
> here: https://termbin.com/5d69
> 
> The last spice-git I ran was f18589b with the Jan 10 patches:
> * [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event
> * [spice-gtk v3 2/3] gtk-session: clipboard: x11: owner-change improvement
> * [spice-gtk v3 3/3] gtk-session: clipboard: x11: do not request data
> while on focus
> * [Spice-devel] [spice-gtk] gtk-session: prefer early check to agent
> connectivity
> 
> That was rock solid.  I have no clipboard problems.
> 
> That's going to leave me and others perpetually patching spice
> releases, and someday when they don't apply cleanly, being at a
> loss and being forced to look at other protocols.
> 
> Apologies if I'm misunderstanding.


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 v1 2/2] gtk-session: clipboard request: notify agent on failure

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 1:18 PM Victor Toso  wrote:
>
> Hi,
>
> On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > > Not true. Possible client-side bug, reproducible on X11, as I
> > > discussed before. As mentioned in the comment, we are sending
> > > wrong selection-grab to the guest.
> >
> > Sorry, I don't follow "we are sending wrong selection-grab" ?
>
> I think I tried too hard already to make you understand that on
> X11, that's happening while user is copying in the guest VM.

Sorry, I don't know what you are talking about. When there is a
complicated question/reply, you should update the comments or commit
message, so we don't have to dig in old conversations.

>
> > > > Not sure sending a reply is going to help much in that case...
> > >
> > > It helps. Instead of blocking the application, it fails to paste
> > > the clipboard and all is good.
> >
> > But if the agent logic is already wrong, sending a reply isn't going
> > to help, it could do anything..
>
> It might have been a valid request at the time it was sent, if
> you accept that we sent the selection-grab.

"we" == the client?

In this case, clipboard_by_guest[] should be false.

>
> > > > > + * sent a bad selection-grab to the agent or the agent is buggy. 
> > > > > */
> > > > > +if (s->clipboard_by_guest[selection]) {
> > > > > +SPICE_DEBUG("clipboard: agent request: grab on hold by 
> > > > > agent, possible race");
> > > > > +goto notify_agent;
> > > > > +}
> > > > > +
> > > > > +/* The selection-request by agent should happen only if the 
> > > > > clipboard data is set
> > > > > + * by client */
> > > > > +if (!s->clip_grabbed[selection]) {
> > > > > +SPICE_DEBUG("clipboard: agent request: data set by agent, 
> > > > > possible race");
> > > > > +goto notify_agent;
> > > > > +}
> > > >
> > > > This could be adding more race conditions. clip_grabbed is set
> > > > asynchronously after owner changed, and indicate if the grab
> > > > message was sent to the agent,
> > >
> > > We send a selection-release on owner-change. If we receive a
> > > selection-request before agent receives the selection-release,
> > > this is expected but we should notify the guest, afaics.
> > >
> > > > as you correctly say. It doesn't mean you can't request client
> > > > clipboard content.
> > >
> > > > I understand the racy case, but the condition seems wrong, it
> > > > should attempt to request current client clipboard content, and
> > > > fail/succeed after.
> > >
> > > No. It should request the content from the grab that was sent. If
> >
> > There is no such thing as clipboard ID/nth in any clipboard protocol I
> > know of. User simply request current clipboard content (not the
> > nth), whether it changed since last grab is not important: it
> > will retrieve something of the requested type or nothing.
>
> Not about the id, but selection metadata itself or the @type
> parameter in clipboard_request()

If the clipboard content (of the requested type) can't be retrieve, it
will fail. It shouldn't be handled here.

>
> > > clipboard changed, previous grab gets invalidated and a
> > > selection-release is sent and another selection-grab is sent with
> > > the metadata of the new grab.
> > >
> > > Proving recent data from old grab seems wrong.
> >
> > It is not.
> >
> > > >
> > > > > +/* Ready only, still should reply to agent to avoid it waiting 
> > > > > for data */
> > > >
> > > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > > that the client is read-only though. So this may be acceptable for
> > > > now, but we should have a TODO/FIXME..
> > >
> > > Well, it should not happen anyway as on read-only we should not
> > > send selection-grab. I'll fix and resend.
> >
> > Right
> >
> > >
> > > Thanks for the quick review.
> > >
> > > > > +if (read_only(self)) {
> > > > > +g_warning("clipboard: agent request: read only, deny 
> > > > > request");
> > > > > +goto notify_agent;
> > > > > +}
> > > > >
> > > > >  if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > > >  gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > > @@ -1039,6 +1057,10 @@ static gboolean 
> > > > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > > >  }
> > > > >
> > > > >  return TRUE;
> > > > > +
> > > > > +notify_agent:
> > > > > +spice_main_channel_clipboard_selection_notify(s->main, 
> > > > > selection, type, NULL, 0);
> > > > > +return FALSE;
> > > > >  }
> > > > >
> > > > >  static void clipboard_release(SpiceMainChannel *main, guint 
> > > > > selection,
> > > > > --
> > > > > 2.20.1
> > > > >
> > > > > ___
> > > > > Spice-devel mailing list
> > > > > Spice-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Marc-André Lureau
> >
> >
> >
> > --
> > 

Re: [Spice-devel] [spice-gtk [rfc] 0/2] Clipboard managers and Spice

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 10:03 AM Victor Toso  wrote:
>
> Hi,
>
> On Wed, Jan 16, 2019 at 12:08:31AM +0400, Marc-André Lureau wrote:
> > On Tue, Jan 15, 2019 at 8:11 PM Victor Toso  wrote:
> > >
> > > From: Victor Toso 
> > >
> > > Hi,
> > >
> > > Several iteractions trying to avoid some bug in X11 but in the end I
> > > found the iteraction with Clibpoard managers (or any other application
> > > that request/set clipboard data) a bit more urgent.
> > >
> > > Simple try here, to not allow another application to request clipboard
> > > data from guest while the user is theoretically interacting with that
> > > guest machine as spice client holds the keyboard-grab.
> > >
> > > As pointed out by elmarco [0], that might be something desireable. So,
> > > I'm introducing the possibility to enable it but have it disabled by
> > > default.
> >
> > Iho, this kind of desktop policy doesn't belong in spice-gtk.
>
> Spice protocol implements this grab/request/release and who
> send/receive those messages should be wary of them.

It's a proxy, it shouldn't have to enforce desktop policies

>
> > If you don't trust the desktop, how can you trust the client
> > itself?
>
> What? How many process do you have running in you machine? Do you
> know every little thing about them? Do you expect all users to
> trust every piece of software running on all target desktops?
>
> For all I know, browsers might run malicious javascript code that
> interact with clipboard; X11 is bad by design in ta regard; In
> GNOME3, GPaste running with x11 backend was able to get clipboard
> data
>
> > Isn't it already the clipboard behaviour on Wayland?
>
> Why you bring 'wayland' on a generic proposal while you nack x11
> proposal by bringing 'not being generic'?

To point out that this kind of decision must be enforced at desktop
level, not at application level.
I can imagine there are various tricks to "break" what you try to enforce here.

>
> > If really more secure, shouldn't it be enforced at a
> > lower-level, at gtk level?
>
> Gtk handles application policies. Spice, in some regards, should
> extend that, if the rationale here is good enough for that.

May be, we need to think carefully about it.

>
> > In any case, I don't think this needs to delay v0.36, since
> > it's not a regression. Hopefully, you agree and we can solve
> > this for the next release.
>
> You are eager to do a release :)

yes, we should do more regular releases.

> I should probably have put the 'since 0.37' for the proposal but
> this is jut a RFC, I wish we could discuss why this is important
> instead of merging it quickly for a release or not.
>
> (so, yes, go ahead with the release and thanks for pushing for
> that!)

thanks, then I will revisit your proposal.

> > > Tested on X11 and Wayland clients.
> > >
> > > There are more than on away to achieve this idea so feedback is welcome.
> > >
> > > I expect that the spice client would implement some sort to commandline
> > > option like --allow-clipobard-managers to enable/disable the
> > > SpiceGtkSession property on the fly. For now, there is an option in
> > > spicy testing tool.
> > >
> > > James, would be great if you could verify if this series keep your
> > > environment bug free.
> > >
> > > Cheers,
> > >
> > > Victor Toso (2):
> > >   gtk-session: introduce clipboard-managers property
> >
> >
> > >   gtk-session: add request targets delayed
> > >
> > >  src/spice-gtk-session.c | 128 +---
> > >  tools/spicy.c   |   6 ++
> > >  2 files changed, 125 insertions(+), 9 deletions(-)
> > >
> > > --
> > > 2.20.1
> > >
> >
> >
> > --
> > Marc-André Lureau
>
> Victor



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Too strict permission in agent (was: Factor out function to create file)

[Frediano Ziglio]

> 
> Make easier to test file creation
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/vdagent/file-xfers.c | 88 
>  src/vdagent/file-xfers.h |  1 +
>  2 files changed, 53 insertions(+), 36 deletions(-)
> 
> diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
> index 77029e7..941524a 100644
> --- a/src/vdagent/file-xfers.c
> +++ b/src/vdagent/file-xfers.c
> @@ -178,13 +178,58 @@ static uint64_t get_free_space_available(const char
> *path)
>  return stat.f_bsize * stat.f_bavail;
>  }
>  
> +int
> +vdagent_file_xfers_create_file(const char *save_dir, char **file_name_p)
> +{
> +char *file_path = NULL;
> +char *dir = NULL;
> +char *path = NULL;
> +int file_fd = -1;
> +int i;
> +struct stat st;
> +
> +file_path = g_build_filename(save_dir, *file_name_p, NULL);
> +dir = g_path_get_dirname(file_path);
> +if (g_mkdir_with_parents(dir, S_IRWXU) == -1) {

Not a regression of this patch, but, isn't these permissions too strict?
All new subdirectories are created with 700 permissions.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH vd_agent_linux v2 5/7] Add a test to test file creation

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 09:49:22AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Victor Toso 
> ---
> Changes since v1:
> - move file to a new tests/ directory;
> - add missing .gitignore changes;
> - improve test to catch errors in open() other than EEXISTS.
> ---
>  .gitignore  | 17 +
>  Makefile.am | 22 
>  tests/test-file-xfers.c | 77 +
>  3 files changed, 107 insertions(+), 9 deletions(-)
>  create mode 100644 tests/test-file-xfers.c
> 
> diff --git a/.gitignore b/.gitignore
> index ae47a90..76d4081 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,19 +1,16 @@
>  *~
> +*.o
> +.deps
> +.dirstamp
>  data/spice-vdagent*.1
>  src/config.h
>  src/config.h.in
>  src/spice-vdagent
>  src/spice-vdagentd
>  src/stamp-h1
> -src/*.o
> -src/.deps
> -src/.dirstamp
> -src/vdagent/*.o
> -src/vdagent/.deps
> -src/vdagent/.dirstamp
> -src/vdagentd/*.o
> -src/vdagentd/.deps
> -src/vdagentd/.dirstamp
> +tests/test-*.log
> +tests/test-*.trs
> +tests/test-file-xfers
>  config.log
>  config.status
>  aclocal.m4
> @@ -25,3 +22,5 @@ install-sh
>  Makefile.in
>  Makefile
>  missing
> +test-driver
> +test-suite.log
> diff --git a/Makefile.am b/Makefile.am
> index 3e405bc..97b8bf0 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -3,6 +3,8 @@ NULL =
>  
>  bin_PROGRAMS = src/spice-vdagent
>  sbin_PROGRAMS = src/spice-vdagentd
> +check_PROGRAMS = tests/test-file-xfers
> +TESTS = $(check_PROGRAMS)
>  
>  common_sources = \
>   src/udscs.c \
> @@ -44,6 +46,26 @@ src_spice_vdagent_SOURCES =\
>   src/vdagent/vdagent.c   \
>   $(NULL)
>  
> +tests_test_file_xfers_CFLAGS =   \
> + $(SPICE_CFLAGS) \
> + $(GLIB2_CFLAGS) \
> + -I$(srcdir)/src \
> + -I$(srcdir)/src/vdagent \
> + -DUDSCS_NO_SERVER   \
> + $(NULL)
> +
> +tests_test_file_xfers_LDADD =\
> + $(SPICE_LIBS)   \
> + $(GLIB2_LIBS)   \
> + $(NULL)
> +
> +tests_test_file_xfers_SOURCES =  \
> + $(common_sources)   \
> + src/vdagent/file-xfers.c\
> + src/vdagent/file-xfers.h\
> + tests/test-file-xfers.c \
> + $(NULL)
> +
>  src_spice_vdagentd_CFLAGS =  \
>   $(DBUS_CFLAGS)  \
>   $(LIBSYSTEMD_DAEMON_CFLAGS) \
> diff --git a/tests/test-file-xfers.c b/tests/test-file-xfers.c
> new file mode 100644
> index 000..e40a89b
> --- /dev/null
> +++ b/tests/test-file-xfers.c
> @@ -0,0 +1,77 @@
> +/*  test-file-xfers.c  - test file transfer
> +
> +Copyright 2019 Red Hat, Inc.
> +
> +This program is free software: you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, either version 3 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program.  If not, see .
> +*/
> +#include 
> +
> +#undef NDEBUG
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "file-xfers.h"
> +
> +static void test_file(const char *file_name, const char *out)
> +{
> +char *fn = g_strdup(file_name);
> +int fd = vdagent_file_xfers_create_file("./test-dir", );
> +if (out) {
> +g_assert_cmpint(fd, !=, -1);
> +g_assert_cmpstr(fn, ==, out);
> +close(fd);
> +g_assert_cmpint(access(out, W_OK), ==, 0);
> +} else {
> +g_assert_cmpint(fd, ==, -1);
> +}
> +g_free(fn);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +assert(system("rm -rf test-dir && mkdir test-dir") == 0);
> +
> +// create a file
> +test_file("test.txt", "./test-dir/test.txt");
> +
> +// create a file with an existing name
> +for (int i = 1; i < 64; ++i) {
> +char out_name[64];
> +sprintf(out_name, "./test-dir/test (%d).txt", i);
> +test_file("test.txt", out_name);
> +}
> +
> +// check too much files with the same name
> +test_file("test.txt", NULL);
> +
> +// create a file in a subdirectory not existing
> +test_file("subdir/test.txt", "./test-dir/subdir/test.txt");
> +
> +// create a file in a directory with no permissions
> +assert(system("chmod 

[Spice-devel] [PATCH vd_agent_linux v2 5/7] Add a test to test file creation

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
Changes since v1:
- move file to a new tests/ directory;
- add missing .gitignore changes;
- improve test to catch errors in open() other than EEXISTS.
---
 .gitignore  | 17 +
 Makefile.am | 22 
 tests/test-file-xfers.c | 77 +
 3 files changed, 107 insertions(+), 9 deletions(-)
 create mode 100644 tests/test-file-xfers.c

diff --git a/.gitignore b/.gitignore
index ae47a90..76d4081 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,19 +1,16 @@
 *~
+*.o
+.deps
+.dirstamp
 data/spice-vdagent*.1
 src/config.h
 src/config.h.in
 src/spice-vdagent
 src/spice-vdagentd
 src/stamp-h1
-src/*.o
-src/.deps
-src/.dirstamp
-src/vdagent/*.o
-src/vdagent/.deps
-src/vdagent/.dirstamp
-src/vdagentd/*.o
-src/vdagentd/.deps
-src/vdagentd/.dirstamp
+tests/test-*.log
+tests/test-*.trs
+tests/test-file-xfers
 config.log
 config.status
 aclocal.m4
@@ -25,3 +22,5 @@ install-sh
 Makefile.in
 Makefile
 missing
+test-driver
+test-suite.log
diff --git a/Makefile.am b/Makefile.am
index 3e405bc..97b8bf0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3,6 +3,8 @@ NULL =
 
 bin_PROGRAMS = src/spice-vdagent
 sbin_PROGRAMS = src/spice-vdagentd
+check_PROGRAMS = tests/test-file-xfers
+TESTS = $(check_PROGRAMS)
 
 common_sources =   \
src/udscs.c \
@@ -44,6 +46,26 @@ src_spice_vdagent_SOURCES =  \
src/vdagent/vdagent.c   \
$(NULL)
 
+tests_test_file_xfers_CFLAGS = \
+   $(SPICE_CFLAGS) \
+   $(GLIB2_CFLAGS) \
+   -I$(srcdir)/src \
+   -I$(srcdir)/src/vdagent \
+   -DUDSCS_NO_SERVER   \
+   $(NULL)
+
+tests_test_file_xfers_LDADD =  \
+   $(SPICE_LIBS)   \
+   $(GLIB2_LIBS)   \
+   $(NULL)
+
+tests_test_file_xfers_SOURCES =\
+   $(common_sources)   \
+   src/vdagent/file-xfers.c\
+   src/vdagent/file-xfers.h\
+   tests/test-file-xfers.c \
+   $(NULL)
+
 src_spice_vdagentd_CFLAGS =\
$(DBUS_CFLAGS)  \
$(LIBSYSTEMD_DAEMON_CFLAGS) \
diff --git a/tests/test-file-xfers.c b/tests/test-file-xfers.c
new file mode 100644
index 000..e40a89b
--- /dev/null
+++ b/tests/test-file-xfers.c
@@ -0,0 +1,77 @@
+/*  test-file-xfers.c  - test file transfer
+
+Copyright 2019 Red Hat, Inc.
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see .
+*/
+#include 
+
+#undef NDEBUG
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "file-xfers.h"
+
+static void test_file(const char *file_name, const char *out)
+{
+char *fn = g_strdup(file_name);
+int fd = vdagent_file_xfers_create_file("./test-dir", );
+if (out) {
+g_assert_cmpint(fd, !=, -1);
+g_assert_cmpstr(fn, ==, out);
+close(fd);
+g_assert_cmpint(access(out, W_OK), ==, 0);
+} else {
+g_assert_cmpint(fd, ==, -1);
+}
+g_free(fn);
+}
+
+int main(int argc, char *argv[])
+{
+assert(system("rm -rf test-dir && mkdir test-dir") == 0);
+
+// create a file
+test_file("test.txt", "./test-dir/test.txt");
+
+// create a file with an existing name
+for (int i = 1; i < 64; ++i) {
+char out_name[64];
+sprintf(out_name, "./test-dir/test (%d).txt", i);
+test_file("test.txt", out_name);
+}
+
+// check too much files with the same name
+test_file("test.txt", NULL);
+
+// create a file in a subdirectory not existing
+test_file("subdir/test.txt", "./test-dir/subdir/test.txt");
+
+// create a file in a directory with no permissions
+assert(system("chmod 555 test-dir/subdir") == 0);
+test_file("subdir/test2.txt", NULL);
+
+// try to create a file with a path where there's a file (should fail)
+test_file("test.txt/out", NULL);
+
+assert(system("chmod 755 test-dir/subdir && rm -rf test-dir") == 0);
+
+return 0;
+}
-- 
2.20.1

___
Spice-devel mailing list

Re: [Spice-devel] [PATCH 1/3] drm/qxl: add mode/framebuffer check functions

[Frediano Ziglio]

> 
> Add a helper functions to check video modes.  Also add a helper to check
> framebuffer buffer objects, using the former for consistency.  That way
> we should not fail in qxl_primary_atomic_check() because video modes
> which are too big will not be added to the mode list in the first place.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 37 +++--
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 1f8fddcc34..07a37d52c4 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -191,6 +191,21 @@ void qxl_display_read_client_monitors_config(struct
> qxl_device *qdev)
>   }
>  }
>  
> +static int qxl_check_mode(struct qxl_device *qdev,
> +   unsigned int width,
> +   unsigned int height)
> +{
> + if (width * height * 4 > qdev->vram_size)

Is someone checking for integer overflows already?

> + return -ENOMEM;
> + return 0;
> +}
> +
> +static int qxl_check_framebuffer(struct qxl_device *qdev,
> +  struct qxl_bo *bo)
> +{
> + return qxl_check_mode(qdev, bo->surf.width, bo->surf.height);
> +}
> +
>  static int qxl_add_monitors_config_modes(struct drm_connector *connector,
>   unsigned *pwidth,
>   unsigned *pheight)
> @@ -466,12 +481,7 @@ static int qxl_primary_atomic_check(struct drm_plane
> *plane,
>  
>   bo = gem_to_qxl_bo(state->fb->obj[0]);
>  
> - if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
> - DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
> - return -EINVAL;
> - }
> -
> - return 0;
> + return qxl_check_framebuffer(qdev, bo);
>  }
>  
>  static int qxl_primary_apply_cursor(struct drm_plane *plane)
> @@ -941,20 +951,11 @@ static enum drm_mode_status qxl_conn_mode_valid(struct
> drm_connector *connector,
>  {
>   struct drm_device *ddev = connector->dev;
>   struct qxl_device *qdev = ddev->dev_private;
> - int i;
>  
> - /* TODO: is this called for user defined modes? (xrandr --add-mode)
> -  * TODO: check that the mode fits in the framebuffer */
> + if (qxl_check_mode(qdev, mode->hdisplay, mode->vdisplay) != 0)
> + return MODE_BAD;
>  
> - if (qdev->monitors_config_width == mode->hdisplay &&
> - qdev->monitors_config_height == mode->vdisplay)
> - return MODE_OK;
> -
> - for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
> - if (common_modes[i].w == mode->hdisplay && common_modes[i].h ==
> mode->vdisplay)
> - return MODE_OK;
> - }
> - return MODE_BAD;
> + return MODE_OK;
>  }
>  
>  static struct drm_encoder *qxl_best_encoder(struct drm_connector *connector)

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH vd_agent_linux 1/7] file-xfers: Initialise correctly AgentFileXferTask::file_fd field

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 04:15:50AM -0500, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > For some reason I miss in my inbox the 6/7 patch. Looking at,
> > seems that mailman got it.
> > 
> > 
> > https://lists.freedesktop.org/archives/spice-devel/2019-January/047268.html
> > 
> > My only suggestion is to move the test to root directory instead
> > of srcdir. I'd prefer also to use the glib test framework [0] but
> 
> Do you mean src/, a new tests/ directory (not under src/) or just
> with configure.ac and such?

Yes, tests/ side-by-side with src/
Not sure what you mean by `with configure.ac`

> > tbh, from basically zero tests to 1, we can improve later if
> > needed :)
> > 
> > [0] https://developer.gnome.org/glib/stable/glib-Testing.html
> > 
> 
> Is using some, but currently they are not separate tests, each
> part (or most of them) rely on the previous executed so
> skipping some would make other fails.

No worries,

> 
> > Besides the location for the test, for the series
> > Acked-by: Victor Toso 
> > 
> > On Tue, Jan 15, 2019 at 06:49:56PM +, Frediano Ziglio wrote:
> > > Correct invalid value for a file descriptor is -1, not 0.
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  src/vdagent/file-xfers.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
> > > index b5aedd0..78d5db3 100644
> > > --- a/src/vdagent/file-xfers.c
> > > +++ b/src/vdagent/file-xfers.c
> > > @@ -133,6 +133,7 @@ static AgentFileXferTask *vdagent_parse_start_msg(
> > >  goto error;
> > >  }
> > >  task = g_new0(AgentFileXferTask, 1);
> > > +task->file_fd = -1;
> > >  task->id = msg->id;
> > >  task->file_name = g_key_file_get_string(
> > >  keyfile, "vdagent-file-xfer", "name", );
> 
> Frediano


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 v1 2/2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > Not true. Possible client-side bug, reproducible on X11, as I
> > discussed before. As mentioned in the comment, we are sending
> > wrong selection-grab to the guest.
> 
> Sorry, I don't follow "we are sending wrong selection-grab" ?

I think I tried too hard already to make you understand that on
X11, that's happening while user is copying in the guest VM.

> > > Not sure sending a reply is going to help much in that case...
> >
> > It helps. Instead of blocking the application, it fails to paste
> > the clipboard and all is good.
> 
> But if the agent logic is already wrong, sending a reply isn't going
> to help, it could do anything..

It might have been a valid request at the time it was sent, if
you accept that we sent the selection-grab.

> > > > + * sent a bad selection-grab to the agent or the agent is buggy. */
> > > > +if (s->clipboard_by_guest[selection]) {
> > > > +SPICE_DEBUG("clipboard: agent request: grab on hold by agent, 
> > > > possible race");
> > > > +goto notify_agent;
> > > > +}
> > > > +
> > > > +/* The selection-request by agent should happen only if the 
> > > > clipboard data is set
> > > > + * by client */
> > > > +if (!s->clip_grabbed[selection]) {
> > > > +SPICE_DEBUG("clipboard: agent request: data set by agent, 
> > > > possible race");
> > > > +goto notify_agent;
> > > > +}
> > >
> > > This could be adding more race conditions. clip_grabbed is set
> > > asynchronously after owner changed, and indicate if the grab
> > > message was sent to the agent,
> >
> > We send a selection-release on owner-change. If we receive a
> > selection-request before agent receives the selection-release,
> > this is expected but we should notify the guest, afaics.
> >
> > > as you correctly say. It doesn't mean you can't request client
> > > clipboard content.
> >
> > > I understand the racy case, but the condition seems wrong, it
> > > should attempt to request current client clipboard content, and
> > > fail/succeed after.
> >
> > No. It should request the content from the grab that was sent. If
> 
> There is no such thing as clipboard ID/nth in any clipboard protocol I
> know of. User simply request current clipboard content (not the
> nth), whether it changed since last grab is not important: it
> will retrieve something of the requested type or nothing.

Not about the id, but selection metadata itself or the @type
parameter in clipboard_request()

> > clipboard changed, previous grab gets invalidated and a
> > selection-release is sent and another selection-grab is sent with
> > the metadata of the new grab.
> >
> > Proving recent data from old grab seems wrong.
> 
> It is not.
> 
> > >
> > > > +/* Ready only, still should reply to agent to avoid it waiting for 
> > > > data */
> > >
> > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > that the client is read-only though. So this may be acceptable for
> > > now, but we should have a TODO/FIXME..
> >
> > Well, it should not happen anyway as on read-only we should not
> > send selection-grab. I'll fix and resend.
> 
> Right
> 
> >
> > Thanks for the quick review.
> >
> > > > +if (read_only(self)) {
> > > > +g_warning("clipboard: agent request: read only, deny request");
> > > > +goto notify_agent;
> > > > +}
> > > >
> > > >  if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > >  gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > @@ -1039,6 +1057,10 @@ static gboolean 
> > > > clipboard_request(SpiceMainChannel *main, guint selection,
> > > >  }
> > > >
> > > >  return TRUE;
> > > > +
> > > > +notify_agent:
> > > > +spice_main_channel_clipboard_selection_notify(s->main, selection, 
> > > > type, NULL, 0);
> > > > +return FALSE;
> > > >  }
> > > >
> > > >  static void clipboard_release(SpiceMainChannel *main, guint selection,
> > > > --
> > > > 2.20.1
> > > >
> > > > ___
> > > > Spice-devel mailing list
> > > > Spice-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau


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-gtk v3] gst: check pulsesrc version >= 1.14.5

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 12:43:06PM +0400, Marc-André Lureau wrote:
> Hi
> 
> 
> On Fri, Jan 11, 2019 at 3:03 PM  wrote:
> >
> > From: Marc-André Lureau 
> >
> > There is a racy bug in pulsesrc that we can't easily workaround:
> > https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
> >
> > It is fixed with 1.15, and will be backported to upcoming 1.14.5.
> >
> > PulseAudio may not be picked by autoaudiosrc, but looking up the
> > actual source or mimicking GstAutoDetect is unnecessarily complicated.
> >
> > When pulsesrc < 1.14.5 is detected, let's drop its rank, so it won't be 
> > picked.
> >
> > Signed-off-by: Marc-André Lureau 
> 
> I think we agreed that this is the way to go.
> 
> ack for v0.36, or should we continue the discussion for v0.37?

ack for v0.36

> 
> >  src/spice-gstaudio.c | 28 
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> > index d0cfbc6..49e9ae6 100644
> > --- a/src/spice-gstaudio.c
> > +++ b/src/spice-gstaudio.c
> > @@ -527,7 +527,35 @@ SpiceGstaudio *spice_gstaudio_new(SpiceSession 
> > *session, GMainContext *context,
> >const char *name)
> >  {
> >  GError *err = NULL;
> > +
> >  if (gst_init_check(NULL, NULL, )) {
> > +GstPluginFeature *pulsesrc;
> > +
> > +pulsesrc = gst_registry_lookup_feature(gst_registry_get(), 
> > "pulsesrc");
> > +if (pulsesrc) {
> > +unsigned major, minor, micro;
> > +GstPlugin *plugin = gst_plugin_feature_get_plugin(pulsesrc);
> > +
> > +if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> > +   , , ) != 3) {
> > +g_warn_if_reached();
> > +gst_object_unref(plugin);
> > +gst_object_unref(pulsesrc);
> > +return NULL;
> > +}
> > +
> > +if (major < 1 ||
> > +(major == 1 && minor < 14) ||
> > +(major == 1 && minor == 14 && micro < 5)) {
> > +g_warning("Bad pulsesrc version %s, lowering its rank",
> > +  gst_plugin_get_version(plugin));
> > +gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> > +}
> > +
> > +gst_object_unref(plugin);
> > +gst_object_unref(pulsesrc);
> > +}
> > +
> >  return g_object_new(SPICE_TYPE_GSTAUDIO,
> >  "session", session,
> >  "main-context", context,
> > --
> > 2.20.1.98.gecbdaf0899
> >
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> 
> -- 
> Marc-André Lureau
> ___
> 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] [spice-gtk v2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

From: Victor Toso 

Similar to 172c521271a3d - if we don't, the agent might be waiting for
data till some timeout happens in the system, blocking copy-paste
feature and possibly freezing applications.

A way to reproduce is copy-paste big clipboard data between two spice
clients.

Also add some documentation to the checks, in order to quickly
understand what they are about.

Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876

v1 -> v2:
- Fixed comment on s->clipboard_by_guest[selection] check (Marc-André)
- Removed the reply on read only. (Marc-André)

Signed-off-by: Victor Toso 
---
 src/spice-gtk-session.c | 29 +
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1a19bca..82a5b35 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -1015,12 +1015,29 @@ static gboolean clipboard_request(SpiceMainChannel 
*main, guint selection,
 int m;
 
 cb = get_clipboard_from_selection(s, selection);
-g_return_val_if_fail(cb != NULL, FALSE);
-g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
-g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
+if (cb == NULL) {
+goto notify_agent;
+}
+
+/* As we are holding clipboard data sent by selection-grab from guest, the
+ * selection-request of clipboard data from guest must fail. We either sent
+ * a bad selection-grab to the guest or the agent is buggy. */
+if (s->clipboard_by_guest[selection]) {
+SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible 
race");
+goto notify_agent;
+}
 
-if (read_only(self))
+/* The selection-request by agent should happen only if the clipboard data 
is set
+ * by client */
+if (!s->clip_grabbed[selection]) {
+SPICE_DEBUG("clipboard: agent request: data set by agent, possible 
race");
+goto notify_agent;
+}
+
+/* On read only, should not reply to the agent. */
+if (read_only(self)) {
 return FALSE;
+}
 
 if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
 gtk_clipboard_request_text(cb, clipboard_received_text_cb,
@@ -1039,6 +1056,10 @@ static gboolean clipboard_request(SpiceMainChannel 
*main, guint selection,
 }
 
 return TRUE;
+
+notify_agent:
+spice_main_channel_clipboard_selection_notify(s->main, selection, type, 
NULL, 0);
+return FALSE;
 }
 
 static void clipboard_release(SpiceMainChannel *main, guint selection,
-- 
2.20.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 12:56 PM Victor Toso  wrote:
>
> Hi,
>
> On Wed, Jan 16, 2019 at 12:40:36PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Jan 16, 2019 at 11:44 AM Victor Toso  wrote:
> > >
> > > From: Victor Toso 
> > >
> > > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > > data till some timeout happens in the system, blocking copy-paste
> > > feature and possibly freezing applications.
> > >
> > > A way to reproduce is copy-paste big clipboard data between two spice
> > > clients.
> > >
> > > Also add some documentation to the checks, in order to quickly
> > > understand what they are about.
> > >
> > > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > >
> > > Signed-off-by: Victor Toso 
> > > ---
> > >  src/spice-gtk-session.c | 32 +++-
> > >  1 file changed, 27 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > > index 1a19bca..aa812d1 100644
> > > --- a/src/spice-gtk-session.c
> > > +++ b/src/spice-gtk-session.c
> > > @@ -1015,12 +1015,30 @@ static gboolean 
> > > clipboard_request(SpiceMainChannel *main, guint selection,
> > >  int m;
> > >
> > >  cb = get_clipboard_from_selection(s, selection);
> > > -g_return_val_if_fail(cb != NULL, FALSE);
> > > -g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, 
> > > FALSE);
> > > -g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> > > +if (cb == NULL) {
> > > +goto notify_agent;
> > > +}
> > >
> > > -if (read_only(self))
> > > -return FALSE;
> > > +/* As we are holding clipboard data sent by selection-grab from 
> > > agent, the
> > > + * selection-request of clipboard data from client OS must fail. We 
> > > either
> >
> > from client OS? Here it's a signal handler for guest selection-request.
>
> Yes, comment is wrong, should be:
>
> /* As we are holding clipboard data sent by selection-grab from guest, the
>  * selection-request of clipboard data from guest must fail. We either
>  * sent a bad selection-grab to the guest or the agent is buggy. */
>
> > This would clearly be a guest-side bug if we reach that
> > condition (events are processed in order, and
> > clipboard_by_guest is set synchronously).
>
> Not true. Possible client-side bug, reproducible on X11, as I
> discussed before. As mentioned in the comment, we are sending
> wrong selection-grab to the guest.

Sorry, I don't follow "we are sending wrong selection-grab" ?

>
> > Not sure sending a reply is going to help much in that case...
>
> It helps. Instead of blocking the application, it fails to paste
> the clipboard and all is good.

But if the agent logic is already wrong, sending a reply isn't going
to help, it could do anything..

>
> > > + * sent a bad selection-grab to the agent or the agent is buggy. */
> > > +if (s->clipboard_by_guest[selection]) {
> > > +SPICE_DEBUG("clipboard: agent request: grab on hold by agent, 
> > > possible race");
> > > +goto notify_agent;
> > > +}
> > > +
> > > +/* The selection-request by agent should happen only if the 
> > > clipboard data is set
> > > + * by client */
> > > +if (!s->clip_grabbed[selection]) {
> > > +SPICE_DEBUG("clipboard: agent request: data set by agent, 
> > > possible race");
> > > +goto notify_agent;
> > > +}
> >
> > This could be adding more race conditions. clip_grabbed is set
> > asynchronously after owner changed, and indicate if the grab
> > message was sent to the agent,
>
> We send a selection-release on owner-change. If we receive a
> selection-request before agent receives the selection-release,
> this is expected but we should notify the guest, afaics.
>
> > as you correctly say. It doesn't mean you can't request client
> > clipboard content.
>
> > I understand the racy case, but the condition seems wrong, it
> > should attempt to request current client clipboard content, and
> > fail/succeed after.
>
> No. It should request the content from the grab that was sent. If

There is no such thing as clipboard ID/nth in any clipboard protocol I
know of. User simply request current clipboard content (not the nth),
whether it changed since last grab is not important: it will retrieve
something of the requested type or nothing.

> clipboard changed, previous grab gets invalidated and a
> selection-release is sent and another selection-grab is sent with
> the metadata of the new grab.
>
> Proving recent data from old grab seems wrong.

It is not.

> >
> > > +/* Ready only, still should reply to agent to avoid it waiting for 
> > > data */
> >
> > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > that the client is read-only though. So this may be acceptable for
> > 

Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 12:40:36PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jan 16, 2019 at 11:44 AM Victor Toso  wrote:
> >
> > From: Victor Toso 
> >
> > Similar to 172c521271a3d - if we don't, the agent might be waiting for
> > data till some timeout happens in the system, blocking copy-paste
> > feature and possibly freezing applications.
> >
> > A way to reproduce is copy-paste big clipboard data between two spice
> > clients.
> >
> > Also add some documentation to the checks, in order to quickly
> > understand what they are about.
> >
> > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> >
> > Signed-off-by: Victor Toso 
> > ---
> >  src/spice-gtk-session.c | 32 +++-
> >  1 file changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 1a19bca..aa812d1 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -1015,12 +1015,30 @@ static gboolean clipboard_request(SpiceMainChannel 
> > *main, guint selection,
> >  int m;
> >
> >  cb = get_clipboard_from_selection(s, selection);
> > -g_return_val_if_fail(cb != NULL, FALSE);
> > -g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
> > -g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> > +if (cb == NULL) {
> > +goto notify_agent;
> > +}
> >
> > -if (read_only(self))
> > -return FALSE;
> > +/* As we are holding clipboard data sent by selection-grab from agent, 
> > the
> > + * selection-request of clipboard data from client OS must fail. We 
> > either
> 
> from client OS? Here it's a signal handler for guest selection-request.

Yes, comment is wrong, should be:

/* As we are holding clipboard data sent by selection-grab from guest, the
 * selection-request of clipboard data from guest must fail. We either
 * sent a bad selection-grab to the guest or the agent is buggy. */

> This would clearly be a guest-side bug if we reach that
> condition (events are processed in order, and
> clipboard_by_guest is set synchronously).

Not true. Possible client-side bug, reproducible on X11, as I
discussed before. As mentioned in the comment, we are sending
wrong selection-grab to the guest.

> Not sure sending a reply is going to help much in that case...

It helps. Instead of blocking the application, it fails to paste
the clipboard and all is good.

> > + * sent a bad selection-grab to the agent or the agent is buggy. */
> > +if (s->clipboard_by_guest[selection]) {
> > +SPICE_DEBUG("clipboard: agent request: grab on hold by agent, 
> > possible race");
> > +goto notify_agent;
> > +}
> > +
> > +/* The selection-request by agent should happen only if the clipboard 
> > data is set
> > + * by client */
> > +if (!s->clip_grabbed[selection]) {
> > +SPICE_DEBUG("clipboard: agent request: data set by agent, possible 
> > race");
> > +goto notify_agent;
> > +}
> 
> This could be adding more race conditions. clip_grabbed is set
> asynchronously after owner changed, and indicate if the grab
> message was sent to the agent,

We send a selection-release on owner-change. If we receive a
selection-request before agent receives the selection-release,
this is expected but we should notify the guest, afaics.

> as you correctly say. It doesn't mean you can't request client
> clipboard content.

> I understand the racy case, but the condition seems wrong, it
> should attempt to request current client clipboard content, and
> fail/succeed after.

No. It should request the content from the grab that was sent. If
clipboard changed, previous grab gets invalidated and a
selection-release is sent and another selection-grab is sent with
the metadata of the new grab.

Proving recent data from old grab seems wrong.

> 
> > +/* Ready only, still should reply to agent to avoid it waiting for 
> > data */
> 
> No, read-only shouldn't reply. We are lacking a way to tell the guest
> that the client is read-only though. So this may be acceptable for
> now, but we should have a TODO/FIXME..

Well, it should not happen anyway as on read-only we should not
send selection-grab. I'll fix and resend.

Thanks for the quick review.

> > +if (read_only(self)) {
> > +g_warning("clipboard: agent request: read only, deny request");
> > +goto notify_agent;
> > +}
> >
> >  if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> >  gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > @@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel 
> > *main, guint selection,
> >  }
> >
> >  return TRUE;
> > +
> > +notify_agent:
> > +spice_main_channel_clipboard_selection_notify(s->main, selection, 
> > type, 

Re: [Spice-devel] [PATCH spice-gtk v3] gst: check pulsesrc version >= 1.14.5

[Marc-André Lureau]

Hi


On Fri, Jan 11, 2019 at 3:03 PM  wrote:
>
> From: Marc-André Lureau 
>
> There is a racy bug in pulsesrc that we can't easily workaround:
> https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/merge_requests/69
>
> It is fixed with 1.15, and will be backported to upcoming 1.14.5.
>
> PulseAudio may not be picked by autoaudiosrc, but looking up the
> actual source or mimicking GstAutoDetect is unnecessarily complicated.
>
> When pulsesrc < 1.14.5 is detected, let's drop its rank, so it won't be 
> picked.
>
> Signed-off-by: Marc-André Lureau 

I think we agreed that this is the way to go.

ack for v0.36, or should we continue the discussion for v0.37?

>  src/spice-gstaudio.c | 28 
>  1 file changed, 28 insertions(+)
>
> diff --git a/src/spice-gstaudio.c b/src/spice-gstaudio.c
> index d0cfbc6..49e9ae6 100644
> --- a/src/spice-gstaudio.c
> +++ b/src/spice-gstaudio.c
> @@ -527,7 +527,35 @@ SpiceGstaudio *spice_gstaudio_new(SpiceSession *session, 
> GMainContext *context,
>const char *name)
>  {
>  GError *err = NULL;
> +
>  if (gst_init_check(NULL, NULL, )) {
> +GstPluginFeature *pulsesrc;
> +
> +pulsesrc = gst_registry_lookup_feature(gst_registry_get(), 
> "pulsesrc");
> +if (pulsesrc) {
> +unsigned major, minor, micro;
> +GstPlugin *plugin = gst_plugin_feature_get_plugin(pulsesrc);
> +
> +if (sscanf(gst_plugin_get_version(plugin), "%u.%u.%u",
> +   , , ) != 3) {
> +g_warn_if_reached();
> +gst_object_unref(plugin);
> +gst_object_unref(pulsesrc);
> +return NULL;
> +}
> +
> +if (major < 1 ||
> +(major == 1 && minor < 14) ||
> +(major == 1 && minor == 14 && micro < 5)) {
> +g_warning("Bad pulsesrc version %s, lowering its rank",
> +  gst_plugin_get_version(plugin));
> +gst_plugin_feature_set_rank(pulsesrc, GST_RANK_NONE);
> +}
> +
> +gst_object_unref(plugin);
> +gst_object_unref(pulsesrc);
> +}
> +
>  return g_object_new(SPICE_TYPE_GSTAUDIO,
>  "session", session,
>  "main-context", context,
> --
> 2.20.1.98.gecbdaf0899
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Oleksandr Andrushchenko]

On 1/15/19 2:26 PM, Neil Armstrong wrote:

On 15/01/2019 11:41, Daniel Vetter wrote:

Having the probe helper stuff (which pretty much everyone needs) in
the drm_crtc_helper.h file (which atomic drivers should never need) is
confusing. Split them out.

To make sure I actually achieved the goal here I went through all
drivers. And indeed, all atomic drivers are now free of
drm_crtc_helper.h includes.

v2: Make it compile. There was so much compile fail on arm drivers
that I figured I'll better not include any of the acks on v1.

v3: Massive rebase because i915 has lost a lot of drmP.h includes, but
not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h
there was still one, which this patch largely removes. Which means
rolling out lots more includes all over.

This will also conflict with ongoing drmP.h cleanup by others I
expect.

v3: Rebase on top of atomic bochs.

Cc: Sam Ravnborg 
Cc: Jani Nikula 
Cc: Laurent Pinchart 
Acked-by: Rodrigo Vivi  (v2)
Acked-by: Benjamin Gaignard  (v2)
Signed-off-by: Daniel Vetter 
Cc: linux-arm-ker...@lists.infradead.org
Cc: virtualizat...@lists.linux-foundation.org
Cc: etna...@lists.freedesktop.org
Cc: linux-samsung-...@vger.kernel.org
Cc: intel-...@lists.freedesktop.org
Cc: linux-media...@lists.infradead.org
Cc: linux-amlo...@lists.infradead.org
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Cc: spice-devel@lists.freedesktop.org
Cc: amd-...@lists.freedesktop.org
Cc: linux-renesas-...@vger.kernel.org
Cc: linux-rockc...@lists.infradead.org
Cc: linux-st...@st-md-mailman.stormreply.com
Cc: linux-te...@vger.kernel.org
Cc: xen-de...@lists.xen.org
---
Merging this is going to be a bit a mess due to all the ongoing drmP.h
cleanups. I think the following should work:
- Apply Sam's prep patches for removing drmP.h from
   drm_modeset_helper.h
- Get the i915 drmP.h cleanup backmerged into drm-misc-next
- Apply this patch.
- Apply Sam's patch to remove drmP.h from drm_modeset_helper.h
- All through drm-misc-next, which has some potential for trivial
   conflicts around #includes with other drivers unfortunately.

I hope there's no other driver who'll blow up accidentally because
someone else is doing a drmP.h cleanup. Laurent maybe?

Jani, ack on this?
-Daniel
---
  .../gpu/drm/amd/amdgpu/amdgpu_connectors.c|  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c|  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h  |  1 +
  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
  .../amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c  |  2 +-
  .../display/amdgpu_dm/amdgpu_dm_services.c|  2 +-
  drivers/gpu/drm/arc/arcpgu_crtc.c |  2 +-
  drivers/gpu/drm/arc/arcpgu_drv.c  |  2 +-
  drivers/gpu/drm/arc/arcpgu_sim.c  |  2 +-
  drivers/gpu/drm/arm/hdlcd_crtc.c  |  2 +-
  drivers/gpu/drm/arm/hdlcd_drv.c   |  2 +-
  drivers/gpu/drm/arm/malidp_crtc.c |  2 +-
  drivers/gpu/drm/arm/malidp_drv.c  |  2 +-
  drivers/gpu/drm/arm/malidp_mw.c   |  2 +-
  drivers/gpu/drm/armada/armada_510.c   |  2 +-
  drivers/gpu/drm/armada/armada_crtc.c  |  2 +-
  drivers/gpu/drm/armada/armada_crtc.h  |  2 +
  drivers/gpu/drm/armada/armada_drv.c   |  2 +-
  drivers/gpu/drm/armada/armada_fb.c|  2 +-
  drivers/gpu/drm/ast/ast_drv.c |  1 +
  drivers/gpu/drm/ast/ast_mode.c|  1 +
  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c|  2 +-
  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h  |  2 +-
  drivers/gpu/drm/bochs/bochs_drv.c |  1 +
  drivers/gpu/drm/bochs/bochs_kms.c |  1 +
  drivers/gpu/drm/bridge/adv7511/adv7511.h  |  5 +-
  drivers/gpu/drm/bridge/analogix-anx78xx.c |  2 +-
  .../drm/bridge/analogix/analogix_dp_core.c|  2 +-
  drivers/gpu/drm/bridge/cdns-dsi.c |  2 +-
  drivers/gpu/drm/bridge/dumb-vga-dac.c |  2 +-
  .../bridge/megachips-stdp-ge-b850v3-fw.c  |  2 +-
  drivers/gpu/drm/bridge/nxp-ptn3460.c  |  2 +-
  drivers/gpu/drm/bridge/panel.c|  2 +-
  drivers/gpu/drm/bridge/parade-ps8622.c|  2 +-
  drivers/gpu/drm/bridge/sii902x.c  |  2 +-
  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  2 +-
  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c |  2 +-
  drivers/gpu/drm/bridge/tc358764.c |  2 +-
  drivers/gpu/drm/bridge/tc358767.c |  2 +-
  drivers/gpu/drm/bridge/ti-sn65dsi86.c |  2 +-
  drivers/gpu/drm/bridge/ti-tfp410.c|  2 +-
  drivers/gpu/drm/cirrus/cirrus_drv.c   |  1 +
  drivers/gpu/drm/cirrus/cirrus_mode.c  |  1 +
  drivers/gpu/drm/drm_atomic_helper.c   |  1 -
  drivers/gpu/drm/drm_dp_mst_topology.c |  2 +-
  drivers/gpu/drm/drm_modeset_helper.c  |  2 +-
  drivers/gpu/drm/drm_probe_helper.c|  2 +-
  

[Spice-devel] [PATCH 1/1] Update all paths /var/run -> /run

[Christian Hesse]

Commit 098268a33c7c8008ccec9050aea8f0763f1c06d5 (systemd: Update path
in unit file) was a really incomplete change. Let's change every
occurrence of /var/run to /run.

Signed-off-by: Christian Hesse 
---
 README  | 2 +-
 data/spice-vdagentd | 4 ++--
 data/spice-vdagentd.socket  | 2 +-
 data/tmpfiles.d/spice-vdagentd.conf | 2 +-
 src/vdagentd-proto.h| 2 +-
 src/vdagentd/vdagentd.c | 2 +-
 src/vdagentd/xorg-conf.c| 4 ++--
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/README b/README
index 2eefbc4..cb2e5b9 100644
--- a/README
+++ b/README
@@ -31,7 +31,7 @@ Features:
   Then connect to the vm with the multiple monitor client which you want to
   use it with using: "spicec --full-screen=auto-config" (or the user portal
   equivalent). At this point the agent will write out a:
-  /var/run/spice-vdagentd/xorg.conf.spice file. With all the necessary magic
+  /run/spice-vdagentd/xorg.conf.spice file. With all the necessary magic
   to get Xinerama working. Move this file to /etc/X11/xorg.conf, then kill
   Xorg so that it will get restarted and you should be good to go.
 * Limited support for setups with multiple Screens (multiple qxl devices each
diff --git a/data/spice-vdagentd b/data/spice-vdagentd
index 306de6b..3070a04 100755
--- a/data/spice-vdagentd
+++ b/data/spice-vdagentd
@@ -27,7 +27,7 @@
 exec="/usr/sbin/spice-vdagentd"
 prog="spice-vdagentd"
 port="/dev/virtio-ports/com.redhat.spice.0"
-pid="/var/run/spice-vdagentd/spice-vdagentd.pid"
+pid="/run/spice-vdagentd/spice-vdagentd.pid"
 
 [ -e /etc/sysconfig/$prog ] && . /etc/sysconfig/$prog
 
@@ -38,7 +38,7 @@ start() {
 [ -c $port ] || exit 0
 modprobe uinput > /dev/null 2>&1
 # In case the previous running vdagentd crashed
-rm -f /var/run/spice-vdagentd/spice-vdagent-sock
+rm -f /run/spice-vdagentd/spice-vdagent-sock
 echo -n $"Starting $prog: "
 daemon --pidfile $pid $exec $SPICE_VDAGENTD_EXTRA_ARGS
 retval=$?
diff --git a/data/spice-vdagentd.socket b/data/spice-vdagentd.socket
index adace91..9123bd2 100644
--- a/data/spice-vdagentd.socket
+++ b/data/spice-vdagentd.socket
@@ -2,4 +2,4 @@
 Description=Activation socket for spice guest agent daemon
 
 [Socket]
-ListenStream=/var/run/spice-vdagentd/spice-vdagent-sock
+ListenStream=/run/spice-vdagentd/spice-vdagent-sock
diff --git a/data/tmpfiles.d/spice-vdagentd.conf 
b/data/tmpfiles.d/spice-vdagentd.conf
index 1a79963..417b84b 100644
--- a/data/tmpfiles.d/spice-vdagentd.conf
+++ b/data/tmpfiles.d/spice-vdagentd.conf
@@ -1,2 +1,2 @@
 # spice-vdagentd needs this and does not create it itself
-d /var/run/spice-vdagentd 0755 root root -
+d /run/spice-vdagentd 0755 root root -
diff --git a/src/vdagentd-proto.h b/src/vdagentd-proto.h
index f72a890..197f325 100644
--- a/src/vdagentd-proto.h
+++ b/src/vdagentd-proto.h
@@ -23,7 +23,7 @@
 #ifndef __VDAGENTD_PROTO_H
 #define __VDAGENTD_PROTO_H
 
-#define VDAGENTD_SOCKET "/var/run/spice-vdagentd/spice-vdagent-sock"
+#define VDAGENTD_SOCKET "/run/spice-vdagentd/spice-vdagent-sock"
 
 #define DEFAULT_VIRTIO_PORT_PATH "/dev/virtio-ports/com.redhat.spice.0"
 
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index f5ba3c2..d0f771a 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -56,7 +56,7 @@ struct agent_data {
 };
 
 /* variables */
-static const char *pidfilename = "/var/run/spice-vdagentd/spice-vdagentd.pid";
+static const char *pidfilename = "/run/spice-vdagentd/spice-vdagentd.pid";
 
 static gchar *portdev = NULL;
 static gchar *vdagentd_socket = NULL;
diff --git a/src/vdagentd/xorg-conf.c b/src/vdagentd/xorg-conf.c
index 97aaacb..463f891 100644
--- a/src/vdagentd/xorg-conf.c
+++ b/src/vdagentd/xorg-conf.c
@@ -54,8 +54,8 @@ void vdagentd_write_xorg_conf(VDAgentMonitorsConfig 
*monitor_conf)
 .subvendor_id = PCI_MATCH_ANY,
 .subdevice_id = PCI_MATCH_ANY,
 };
-const char *xorg_conf = "/var/run/spice-vdagentd/xorg.conf.spice";
-const char *xorg_conf_old = "/var/run/spice-vdagentd/xorg.conf.spice.old";
+const char *xorg_conf = "/run/spice-vdagentd/xorg.conf.spice";
+const char *xorg_conf_old = "/run/spice-vdagentd/xorg.conf.spice.old";
 
 r = rename(xorg_conf, xorg_conf_old);
 if (r && errno != ENOENT) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Tuesday, 15 January 2019 12:41:37 EET Daniel Vetter wrote:
> Having the probe helper stuff (which pretty much everyone needs) in
> the drm_crtc_helper.h file (which atomic drivers should never need) is
> confusing. Split them out.
> 
> To make sure I actually achieved the goal here I went through all
> drivers. And indeed, all atomic drivers are now free of
> drm_crtc_helper.h includes.
> 
> v2: Make it compile. There was so much compile fail on arm drivers
> that I figured I'll better not include any of the acks on v1.
> 
> v3: Massive rebase because i915 has lost a lot of drmP.h includes, but
> not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h
> there was still one, which this patch largely removes. Which means
> rolling out lots more includes all over.
> 
> This will also conflict with ongoing drmP.h cleanup by others I
> expect.
> 
> v3: Rebase on top of atomic bochs.
> 
> Cc: Sam Ravnborg 
> Cc: Jani Nikula 
> Cc: Laurent Pinchart 
> Acked-by: Rodrigo Vivi  (v2)
> Acked-by: Benjamin Gaignard  (v2)
> Signed-off-by: Daniel Vetter 
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: etna...@lists.freedesktop.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: spice-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-te...@vger.kernel.org
> Cc: xen-de...@lists.xen.org
> ---
> Merging this is going to be a bit a mess due to all the ongoing drmP.h
> cleanups. I think the following should work:
> - Apply Sam's prep patches for removing drmP.h from
>   drm_modeset_helper.h
> - Get the i915 drmP.h cleanup backmerged into drm-misc-next
> - Apply this patch.
> - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h
> - All through drm-misc-next, which has some potential for trivial
>   conflicts around #includes with other drivers unfortunately.
> 
> I hope there's no other driver who'll blow up accidentally because
> someone else is doing a drmP.h cleanup. Laurent maybe?

There's a drmP.h cleanup in the R-Car DU driver, but it doesn't conflict with 
this patch, the combination of both compiles fine.

> Jani, ack on this?
> -Daniel
> ---

[snip]

>  drivers/gpu/drm/bridge/adv7511/adv7511.h  |  5 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  2 +-
>  drivers/gpu/drm/bridge/dumb-vga-dac.c |  2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  2 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c|  2 +-
>  drivers/gpu/drm/drm_atomic_helper.c   |  1 -
>  drivers/gpu/drm/drm_dp_mst_topology.c |  2 +-
>  drivers/gpu/drm/drm_modeset_helper.c  |  2 +-
>  drivers/gpu/drm/drm_probe_helper.c|  2 +-
>  drivers/gpu/drm/drm_simple_kms_helper.c   |  2 +-
>  drivers/gpu/drm/omapdrm/omap_connector.c  |  2 +-
>  drivers/gpu/drm/omapdrm/omap_crtc.c   |  2 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c|  2 +-
>  drivers/gpu/drm/omapdrm/omap_drv.h|  2 +-
>  drivers/gpu/drm/omapdrm/omap_encoder.c|  2 +-
>  drivers/gpu/drm/omapdrm/omap_fb.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c|  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c   |  2 +-
>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c |  1 +
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c  |  1 +
>  drivers/gpu/drm/shmobile/shmob_drm_kms.c  |  1 +
>  include/drm/drm_crtc_helper.h | 16 --
>  include/drm/drm_probe_helper.h| 50 +++

For the above files, with the comments below addressed,

Reviewed-by: Laurent Pinchart 

>  227 files changed, 289 insertions(+), 200 deletions(-)
>  create mode 100644 include/drm/drm_probe_helper.h

[snip]

> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 73d8ccb97742..d52ffab41eb4
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -14,8 +14,11 @@
>  #include 
>  #include 
> 
> -#include 
> +#include 

The probe helpers are needed in adv7511_drv.c only, I would move the include 
there.

>  #include 
> +#include 
> +#include 
> +#include 

Please keep the headers alphabetically sorted, here and in all other drivers.

>  #define ADV7511_REG_CHIP_REVISION0x00
>  #define 

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Alex Deucher]

On Tue, Jan 15, 2019 at 5:41 AM Daniel Vetter  wrote:
>
> Having the probe helper stuff (which pretty much everyone needs) in
> the drm_crtc_helper.h file (which atomic drivers should never need) is
> confusing. Split them out.
>
> To make sure I actually achieved the goal here I went through all
> drivers. And indeed, all atomic drivers are now free of
> drm_crtc_helper.h includes.
>
> v2: Make it compile. There was so much compile fail on arm drivers
> that I figured I'll better not include any of the acks on v1.
>
> v3: Massive rebase because i915 has lost a lot of drmP.h includes, but
> not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h
> there was still one, which this patch largely removes. Which means
> rolling out lots more includes all over.
>
> This will also conflict with ongoing drmP.h cleanup by others I
> expect.
>
> v3: Rebase on top of atomic bochs.
>
> Cc: Sam Ravnborg 
> Cc: Jani Nikula 
> Cc: Laurent Pinchart 
> Acked-by: Rodrigo Vivi  (v2)
> Acked-by: Benjamin Gaignard  (v2)
> Signed-off-by: Daniel Vetter 
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: etna...@lists.freedesktop.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: spice-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-te...@vger.kernel.org
> Cc: xen-de...@lists.xen.org
> ---
> Merging this is going to be a bit a mess due to all the ongoing drmP.h
> cleanups. I think the following should work:
> - Apply Sam's prep patches for removing drmP.h from
>   drm_modeset_helper.h
> - Get the i915 drmP.h cleanup backmerged into drm-misc-next
> - Apply this patch.
> - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h
> - All through drm-misc-next, which has some potential for trivial
>   conflicts around #includes with other drivers unfortunately.
>
> I hope there's no other driver who'll blow up accidentally because
> someone else is doing a drmP.h cleanup. Laurent maybe?
>
> Jani, ack on this?
> -Daniel

amdgpu and radeon:
Acked-by: Alex Deucher 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Marc-André Lureau]

Hi

On Wed, Jan 16, 2019 at 11:44 AM Victor Toso  wrote:
>
> From: Victor Toso 
>
> Similar to 172c521271a3d - if we don't, the agent might be waiting for
> data till some timeout happens in the system, blocking copy-paste
> feature and possibly freezing applications.
>
> A way to reproduce is copy-paste big clipboard data between two spice
> clients.
>
> Also add some documentation to the checks, in order to quickly
> understand what they are about.
>
> Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Victor Toso 
> ---
>  src/spice-gtk-session.c | 32 +++-
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 1a19bca..aa812d1 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -1015,12 +1015,30 @@ static gboolean clipboard_request(SpiceMainChannel 
> *main, guint selection,
>  int m;
>
>  cb = get_clipboard_from_selection(s, selection);
> -g_return_val_if_fail(cb != NULL, FALSE);
> -g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
> -g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
> +if (cb == NULL) {
> +goto notify_agent;
> +}
>
> -if (read_only(self))
> -return FALSE;
> +/* As we are holding clipboard data sent by selection-grab from agent, 
> the
> + * selection-request of clipboard data from client OS must fail. We 
> either

from client OS? Here it's a signal handler for guest selection-request.

This would clearly be a guest-side bug if we reach that condition
(events are processed in order, and clipboard_by_guest is set
synchronously). Not sure sending a reply is going to help much in that
case...

> + * sent a bad selection-grab to the agent or the agent is buggy. */
> +if (s->clipboard_by_guest[selection]) {
> +SPICE_DEBUG("clipboard: agent request: grab on hold by agent, 
> possible race");
> +goto notify_agent;
> +}
> +
> +/* The selection-request by agent should happen only if the clipboard 
> data is set
> + * by client */
> +if (!s->clip_grabbed[selection]) {
> +SPICE_DEBUG("clipboard: agent request: data set by agent, possible 
> race");
> +goto notify_agent;
> +}

This could be adding more race conditions. clip_grabbed is set
asynchronously after owner changed, and indicate if the grab message
was sent to the agent, as you correctly say. It doesn't mean you can't
request client clipboard content.

I understand the racy case, but the condition seems wrong, it should
attempt to request current client clipboard content, and fail/succeed
after.

> +/* Ready only, still should reply to agent to avoid it waiting for data 
> */

No, read-only shouldn't reply. We are lacking a way to tell the guest
that the client is read-only though. So this may be acceptable for
now, but we should have a TODO/FIXME..

> +if (read_only(self)) {
> +g_warning("clipboard: agent request: read only, deny request");
> +goto notify_agent;
> +}
>
>  if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
>  gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> @@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel 
> *main, guint selection,
>  }
>
>  return TRUE;
> +
> +notify_agent:
> +spice_main_channel_clipboard_selection_notify(s->main, selection, type, 
> NULL, 0);
> +return FALSE;
>  }
>
>  static void clipboard_release(SpiceMainChannel *main, guint selection,
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 2/3] drm/qxl: add qxl_add_mode helper function

[Gerd Hoffmann]

Add a helper function to add custom video modes to a connector.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 84 +++
 1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 07a37d52c4..b87f72f59e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -206,15 +206,36 @@ static int qxl_check_framebuffer(struct qxl_device *qdev,
return qxl_check_mode(qdev, bo->surf.width, bo->surf.height);
 }
 
-static int qxl_add_monitors_config_modes(struct drm_connector *connector,
- unsigned *pwidth,
- unsigned *pheight)
+static int qxl_add_mode(struct drm_connector *connector,
+   unsigned int width,
+   unsigned int height,
+   bool preferred)
+{
+   struct drm_device *dev = connector->dev;
+   struct qxl_device *qdev = dev->dev_private;
+   struct drm_display_mode *mode = NULL;
+   int rc;
+
+   rc = qxl_check_mode(qdev, width, height);
+   if (rc != 0)
+   return 0;
+
+   mode = drm_cvt_mode(dev, width, height, 60, false, false, false);
+   if (preferred)
+   mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = width;
+   mode->vdisplay = height;
+   drm_mode_set_name(mode);
+   drm_mode_probed_add(connector, mode);
+   return 1;
+}
+
+static int qxl_add_monitors_config_modes(struct drm_connector *connector)
 {
struct drm_device *dev = connector->dev;
struct qxl_device *qdev = dev->dev_private;
struct qxl_output *output = drm_connector_to_qxl_output(connector);
int h = output->index;
-   struct drm_display_mode *mode = NULL;
struct qxl_head *head;
 
if (!qdev->monitors_config)
@@ -229,19 +250,7 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
head = >client_monitors_config->heads[h];
DRM_DEBUG_KMS("head %d is %dx%d\n", h, head->width, head->height);
 
-   mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
-   false);
-   mode->type |= DRM_MODE_TYPE_PREFERRED;
-   mode->hdisplay = head->width;
-   mode->vdisplay = head->height;
-   drm_mode_set_name(mode);
-   *pwidth = head->width;
-   *pheight = head->height;
-   drm_mode_probed_add(connector, mode);
-   /* remember the last custom size for mode validation */
-   qdev->monitors_config_width = mode->hdisplay;
-   qdev->monitors_config_height = mode->vdisplay;
-   return 1;
+   return qxl_add_mode(connector, head->width, head->height, true);
 }
 
 static struct mode_size {
@@ -267,22 +276,16 @@ static struct mode_size {
{1920, 1200}
 };
 
-static int qxl_add_common_modes(struct drm_connector *connector,
-unsigned int pwidth,
-unsigned int pheight)
+static int qxl_add_common_modes(struct drm_connector *connector)
 {
-   struct drm_device *dev = connector->dev;
-   struct drm_display_mode *mode = NULL;
-   int i;
+   int i, ret = 0;
 
-   for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
-   mode = drm_cvt_mode(dev, common_modes[i].w, common_modes[i].h,
-   60, false, false, false);
-   if (common_modes[i].w == pwidth && common_modes[i].h == pheight)
-   mode->type |= DRM_MODE_TYPE_PREFERRED;
-   drm_mode_probed_add(connector, mode);
-   }
-   return i - 1;
+   for (i = 0; i < ARRAY_SIZE(common_modes); i++)
+   ret += qxl_add_mode(connector,
+   common_modes[i].w,
+   common_modes[i].h,
+   false);
+   return ret;
 }
 
 static void qxl_send_monitors_config(struct qxl_device *qdev)
@@ -935,14 +938,25 @@ static int qdev_crtc_init(struct drm_device *dev, int 
crtc_id)
 
 static int qxl_conn_get_modes(struct drm_connector *connector)
 {
+   struct drm_device *dev = connector->dev;
+   struct qxl_device *qdev = dev->dev_private;
+   struct qxl_output *output = drm_connector_to_qxl_output(connector);
unsigned int pwidth = 1024;
unsigned int pheight = 768;
int ret = 0;
 
-   ret = qxl_add_monitors_config_modes(connector, , );
-   if (ret < 0)
-   return ret;
-   ret += qxl_add_common_modes(connector, pwidth, pheight);
+   if (qdev->client_monitors_config) {
+   struct qxl_head *head;
+   head = >client_monitors_config->heads[output->index];
+   if (head->width)
+   pwidth = head->width;
+   if (head->height)
+   pheight = head->height;

[Spice-devel] [PATCH 1/3] drm/qxl: add mode/framebuffer check functions

[Gerd Hoffmann]

Add a helper functions to check video modes.  Also add a helper to check
framebuffer buffer objects, using the former for consistency.  That way
we should not fail in qxl_primary_atomic_check() because video modes
which are too big will not be added to the mode list in the first place.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 37 +++--
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 1f8fddcc34..07a37d52c4 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -191,6 +191,21 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev)
}
 }
 
+static int qxl_check_mode(struct qxl_device *qdev,
+ unsigned int width,
+ unsigned int height)
+{
+   if (width * height * 4 > qdev->vram_size)
+   return -ENOMEM;
+   return 0;
+}
+
+static int qxl_check_framebuffer(struct qxl_device *qdev,
+struct qxl_bo *bo)
+{
+   return qxl_check_mode(qdev, bo->surf.width, bo->surf.height);
+}
+
 static int qxl_add_monitors_config_modes(struct drm_connector *connector,
  unsigned *pwidth,
  unsigned *pheight)
@@ -466,12 +481,7 @@ static int qxl_primary_atomic_check(struct drm_plane 
*plane,
 
bo = gem_to_qxl_bo(state->fb->obj[0]);
 
-   if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
-   DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
-   return -EINVAL;
-   }
-
-   return 0;
+   return qxl_check_framebuffer(qdev, bo);
 }
 
 static int qxl_primary_apply_cursor(struct drm_plane *plane)
@@ -941,20 +951,11 @@ static enum drm_mode_status qxl_conn_mode_valid(struct 
drm_connector *connector,
 {
struct drm_device *ddev = connector->dev;
struct qxl_device *qdev = ddev->dev_private;
-   int i;
 
-   /* TODO: is this called for user defined modes? (xrandr --add-mode)
-* TODO: check that the mode fits in the framebuffer */
+   if (qxl_check_mode(qdev, mode->hdisplay, mode->vdisplay) != 0)
+   return MODE_BAD;
 
-   if (qdev->monitors_config_width == mode->hdisplay &&
-   qdev->monitors_config_height == mode->vdisplay)
-   return MODE_OK;
-
-   for (i = 0; i < ARRAY_SIZE(common_modes); i++) {
-   if (common_modes[i].w == mode->hdisplay && common_modes[i].h == 
mode->vdisplay)
-   return MODE_OK;
-   }
-   return MODE_BAD;
+   return MODE_OK;
 }
 
 static struct drm_encoder *qxl_best_encoder(struct drm_connector *connector)
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 3/3] drm/qxl: use kernel mode db

[Gerd Hoffmann]

Add all standard modes from the kernel's video mode data base.
Keep a few non-standard modes in the qxl mode list.

Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/qxl/qxl_display.c | 27 +++
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index b87f72f59e..9e49472872 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -256,34 +256,20 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector)
 static struct mode_size {
int w;
int h;
-} common_modes[] = {
-   { 640,  480},
+} extra_modes[] = {
{ 720,  480},
-   { 800,  600},
-   { 848,  480},
-   {1024,  768},
{1152,  768},
-   {1280,  720},
-   {1280,  800},
{1280,  854},
-   {1280,  960},
-   {1280, 1024},
-   {1440,  900},
-   {1400, 1050},
-   {1680, 1050},
-   {1600, 1200},
-   {1920, 1080},
-   {1920, 1200}
 };
 
-static int qxl_add_common_modes(struct drm_connector *connector)
+static int qxl_add_extra_modes(struct drm_connector *connector)
 {
int i, ret = 0;
 
-   for (i = 0; i < ARRAY_SIZE(common_modes); i++)
+   for (i = 0; i < ARRAY_SIZE(extra_modes); i++)
ret += qxl_add_mode(connector,
-   common_modes[i].w,
-   common_modes[i].h,
+   extra_modes[i].w,
+   extra_modes[i].h,
false);
return ret;
 }
@@ -954,7 +940,8 @@ static int qxl_conn_get_modes(struct drm_connector 
*connector)
pheight = head->height;
}
 
-   ret += qxl_add_common_modes(connector);
+   ret += drm_add_modes_noedid(connector, 8192, 8192);
+   ret += qxl_add_extra_modes(connector);
ret += qxl_add_monitors_config_modes(connector);
drm_set_preferred_mode(connector, pwidth, pheight);
return ret;
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH vd_agent_linux 1/7] file-xfers: Initialise correctly AgentFileXferTask::file_fd field

[Victor Toso]

Hi,

For some reason I miss in my inbox the 6/7 patch. Looking at,
seems that mailman got it.

https://lists.freedesktop.org/archives/spice-devel/2019-January/047268.html

My only suggestion is to move the test to root directory instead
of srcdir. I'd prefer also to use the glib test framework [0] but
tbh, from basically zero tests to 1, we can improve later if
needed :)

[0] https://developer.gnome.org/glib/stable/glib-Testing.html

Besides the location for the test, for the series
Acked-by: Victor Toso 

On Tue, Jan 15, 2019 at 06:49:56PM +, Frediano Ziglio wrote:
> Correct invalid value for a file descriptor is -1, not 0.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/vdagent/file-xfers.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
> index b5aedd0..78d5db3 100644
> --- a/src/vdagent/file-xfers.c
> +++ b/src/vdagent/file-xfers.c
> @@ -133,6 +133,7 @@ static AgentFileXferTask *vdagent_parse_start_msg(
>  goto error;
>  }
>  task = g_new0(AgentFileXferTask, 1);
> +task->file_fd = -1;
>  task->id = msg->id;
>  task->file_name = g_key_file_get_string(
>  keyfile, "vdagent-file-xfer", "name", );
> -- 
> 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