Re: [Spice-devel] need advice

[Uri Lublin]

On 01/26/2016 02:11 PM, Baurzhan Konurbayev wrote:

Thanks for the reply.

We would like to try hardware acceleration for our thin clients and
therefore, tried to apply the patch called "Add GStreamer support for
video streaming", https://patchwork.freedesktop.org/series/1874/



Hi,

There is a newer patch-set -- V9
https://patchwork.freedesktop.org/series/2591

Uri



On 01/26/2016 05:55 PM, Frediano Ziglio wrote:

Hello, guys!

I am trying to apply one of the patch series from
https://patchwork.freedesktop.org/project/Spice/series/

However, I am getting some compilation issues.

Could you, please, suggest if I took right sources?

I cloned base sources from :
   - git://git.freedesktop.org/git/spice/spice-protocol for spice protocol
   - git://anongit.freedesktop.org/spice/spice for spice server
   - git://anongit.freedesktop.org/xorg/driver/xf86-video-qxl for qxl

and applied the mbox patch then.
Is it correct to do so?

Thanks in advance.


Is correct. Probably however changes to master made the patch obsolete.
Which patches are you trying?

Frediano


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

Re: [Spice-devel] [PATCH v2] qxl: inline documentation for get_command and req_cmd_notification

[Uri Lublin]

On 01/26/2016 10:26 PM, Jonathon Jongsma wrote:

From: Frediano Ziglio 

Signed-off-by: Frediano Ziglio 
Signed-off-by: Jonathon Jongsma 
---

Good idea. Here's a proposed re-wording of the documentation.

Changes:
  - for get_command(), I changed the verb from 'request' to 'retrieve' to make
it (I think) a bit more clear that it's getting the command and not simply
making a request. Also tweaked the wording a bit.
  - for req_cmd_notification(), I dropped the part about "spice-server handled
all commands" since I think the documentation should just focus on the
function implementation, not about when it is called. I also added some more
details about exactly how the server gets notified after studying the code
for a while (thanks Uri!).


Hi,

Looks good to me with the some comments below.



  server/spice-qxl.h | 8 
  1 file changed, 8 insertions(+)

diff --git a/server/spice-qxl.h b/server/spice-qxl.h
index e1f14e7..caa2693 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -166,7 +166,15 @@ struct QXLInterface {
  void (*set_mm_time)(QXLInstance *qin, uint32_t mm_time) 
SPICE_GNUC_DEPRECATED;

  void (*get_init_info)(QXLInstance *qin, QXLDevInitInfo *info);


Consider adding an empty line here.


+/* Retrieve the next command to be processed
+ * This call should be non-blocking. If no are commands available, it


If no commands are available  ^^^


+ * should return NULL */


This function returns int (boolean really) so
"If no commands are available it should return 0"

and maybe rewrite, adding -- or 1 (non-zero) if a command was retrieved.


  int (*get_command)(QXLInstance *qin, struct QXLCommandExt *cmd);


Consider adding an empty line here



+/* Request notification when new commands are available
+ * When a new command becomes available, the spice server should be
+ * notified by calling spice_qxl_wakeup(). If commands are already
+ * available, this function should return FALSE and no notification
+ * triggered */
  int (*req_cmd_notification)(QXLInstance *qin);
  void (*release_resource)(QXLInstance *qin, struct QXLReleaseInfoExt 
release_info);
  int (*get_cursor_command)(QXLInstance *qin, struct QXLCommandExt *cmd);



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

Re: [Spice-devel] [PATCH v9 00/24] Add GStreamer support for video streaming

[Uri Lublin]

On 01/18/2016 01:03 PM, Francois Gouget wrote:


The source changed so here is the new revision of this patchset.

This patch series adds support for using GStreamer to encode and decode
the video streams, adding support for VP8 and h264 codecs. As before the
patches can also be grabbed from the repositories below:



Hi,

I've tried testing this patch-set with qemu-kvm.
I wrote a patch for qemu-kvm to enable this feature, see at the bottom.

I found it working with spice:mjpeg but hangs with gstreamer:vp8 codec.

I do not have x264 installed.
if I set video-codecs to "gstreamer:h264;spice:mjpeg" and also with
gstreamer:h264 it falls back to spice:mjpeg.

With gstreamer:mjpeg it also fallbacks to spice:mjpeg with the following
error:
Spice-Debug **: gstreamer-encoder.c:795:create_pipeline: GStreamer 
pipeline: appsrc name=src format=2 do-timestamp=true ! videoconvert ! 
avenc_mjpeg name=encoder ! appsink name=sink
Spice-Warning **: gstreamer-encoder.c:799:create_pipeline: GStreamer 
error: no element "avenc_mjpeg"




When running with "gstreamer:vp8;spice:mjpeg" I see the following 
spice-server warnings:
((null):17523): Spice-Debug **: gstreamer-encoder.c:795:create_pipeline: 
GStreamer pipeline: appsrc name=src format=2 do-timestamp=true ! 
videoconvert ! vp8enc name=encoder ! appsink name=sink
((null):17523): Spice-Debug **: 
stream.c:454:display_channel_create_stream: stream 49 511x20 (5, 703) 
(516, 723) 30 fps
((null):17523): Spice-Debug **: stream.c:333:before_reattach_stream: 
ignoring drop, same process_commands_generation as previous frame


and later
((null):17523): Spice-Debug **: 
gstreamer-encoder.c:1282:spice_gst_encoder_encode_frame: video format 
change: width 0 -> 516, height 0 -> 25, format 0 -> 8
((null):17523): Spice-Warning **: gstreamer-encoder.c:726:map_format: 
The 8 format has not been tested yet
((null):17523): Spice-Debug **: 
gstreamer-encoder.c:862:set_gstenc_bitrate: setting the GStreamer 
target-bitrate to 0
((null):17523): Spice-Debug **: 
gstreamer-encoder.c:939:configure_pipeline: setting state to PLAYING




Attaching with gdb: I see the following threads:
(gdb) info threads
  Id   Target Id Frame
  13   Thread 0x7f763d1bb700 (LWP 17524) "qemu-system-x86" 
0x7f76477cdc59 in syscall () from /lib64/libc.so.6
  12   Thread 0x7f763b5bb700 (LWP 17531) "qemu-system-x86" 
0x7f7647aa1045 in do_futex_wait () from /lib64/libpthread.so.0
  11   Thread 0x7f763aab7700 (LWP 17532) "qemu-system-x86" 
0x7f76477c9707 in ioctl () from /lib64/libc.so.6
  10   Thread 0x7f7639dff700 (LWP 17533) "qemu-system-x86" 
0x7f76477cdc59 in syscall () from /lib64/libc.so.6
  9Thread 0x7f759700 (LWP 17579) "src:src" 0x7f76477cdc59 
in syscall () from /lib64/libc.so.6
  8Thread 0x7f759f7fe700 (LWP 17580) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
  7Thread 0x7f759effd700 (LWP 17581) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
  6Thread 0x7f759e7fc700 (LWP 17582) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
  5Thread 0x7f759dffb700 (LWP 17583) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
  4Thread 0x7f759d7fa700 (LWP 17584) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
  3Thread 0x7f759cff9700 (LWP 17585) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
  2Thread 0x7f7597fff700 (LWP 17586) "src:src" 0x7f7647aa0e57 
in do_futex_wait.constprop () from /lib64/libpthread.so.0
* 1Thread 0x7f764d78cbc0 (LWP 17523) "qemu-system-x86" 
0x7f76477c80a1 in ppoll () from /lib64/libc.so.6

(gdb) thread 8
[Switching to thread 8 (Thread 0x7f759f7fe700 (LWP 17580))]
#0  0x7f7647aa0e57 in do_futex_wait.constprop () from 
/lib64/libpthread.so.0

(gdb) bt
#0  0x7f7647aa0e57 in do_futex_wait.constprop () at 
/lib64/libpthread.so.0
#1  0x7f7647aa0f04 in __new_sem_wait_slow.constprop.0 () at 
/lib64/libpthread.so.0

#2  0x7f7647aa0faa in sem_wait@@GLIBC_2.2.5 () at /lib64/libpthread.so.0
#3  0x7f75ac63ee41 in thread_encoding_proc () at /lib64/libvpx.so.2
#4  0x7f7647a9960a in start_thread () at /lib64/libpthread.so.0
#5  0x7f76477d3a4d in clone () at /lib64/libc.so.6
(gdb) detach



---

From 4a29ad1422971e10f560ef95a1646ffa4dbac7a4 Mon Sep 17 00:00:00 2001
From: Uri Lublin 
Date: Thu, 14 Jan 2016 17:23:08 +0200
Subject: [PATCH] spice: add video-codecs option

Starting with spice-0.12.7 some video codecs in addition
to mjpeg.

Signed-off-by: Uri Lublin 
---
 qemu-options.hx |  6 ++
 ui/spice-core.c | 22 ++
 2 files changed, 28 insertions(+)

diff --git a/qemu-options.hx b/qem

Re: [Spice-devel] [PATCH 1/5] common: constify some declarations

[Uri Lublin]

On 01/27/2016 06:09 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 
---
  common/log.c| 2 +-
  common/marshaller.c | 7 +--
  common/marshaller.h | 2 +-
  3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/common/log.c b/common/log.c
index 607aa82..a9bbebc 100644
--- a/common/log.c
+++ b/common/log.c
@@ -43,7 +43,7 @@ static int abort_level = -1;

  static GLogLevelFlags spice_log_level_to_glib(SpiceLogLevel level)
  {
-static GLogLevelFlags glib_levels[] = {
+static const GLogLevelFlags glib_levels[] = {
  [ SPICE_LOG_LEVEL_ERROR ] = G_LOG_LEVEL_ERROR,
  [ SPICE_LOG_LEVEL_CRITICAL ] = G_LOG_LEVEL_CRITICAL,
  [ SPICE_LOG_LEVEL_WARNING ] = G_LOG_LEVEL_WARNING,
diff --git a/common/marshaller.c b/common/marshaller.c
index c967371..c1d208e 100644
--- a/common/marshaller.c
+++ b/common/marshaller.c
@@ -347,9 +347,12 @@ uint8_t *spice_marshaller_add(SpiceMarshaller *m, const 
uint8_t *data, size_t si
  return ptr;
  }

-uint8_t *spice_marshaller_add_ref(SpiceMarshaller *m, uint8_t *data, size_t 
size)
+uint8_t *spice_marshaller_add_ref(SpiceMarshaller *m, const uint8_t *data, 
size_t size)
  {
-return spice_marshaller_add_ref_full(m, data, size, NULL, NULL);
+/* the cast to no-const here is safe as data is used for writing only if
+ * free_data pointer is not NULL
+ */
+return spice_marshaller_add_ref_full(m, (uint8_t *) data, size, NULL, 
NULL);
  }


Hi Frediano,

Isn't there a potential problem with data being returned as non-const ?
I see currently no caller looks at the data returned.

Regards,
Uri.



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

Re: [Spice-devel] Some Grammar patches

[Uri Lublin]

On 02/27/2016 03:22 PM, Victor Toso wrote:

Hi,

On Fri, Feb 26, 2016 at 01:00:35PM -0500, Ryan wrote:

While reading the spec, some small grammar issues made it sounds
funky — I fixed some of them here:
https://github.com/Fuzion24/usbredir/commit/b3593f0f464aa9dc5150d38ff3e401cd25488f1b.patch


Thanks! Could you please send the patch to this mailing list with
git send-mail ?


Also, if the only change is replacing "send" with "sent"
please mention it.


And this part seems wrong:

---
-Control packets are handled synchroneously inside the usb-host, it will 
hand
+sControl packets are handled synchroneously inside the usb-host, it 
will hand

---

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

[Spice-devel] [server PATCH 1/3] replay: learn how to count commands

[Uri Lublin]

---
 server/tests/replay.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index f3b670f..83411d3 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -47,6 +47,8 @@ static QXLWorker *qxl_worker = NULL;
 static gboolean started = FALSE;
 static QXLInstance display_sin = { 0, };
 static gint slow = 0;
+static gboolean do_count = 0;
+static guint ncommands = 0;
 static pid_t client_pid;
 static GMainLoop *loop = NULL;
 static GAsyncQueue *aqueue = NULL;
@@ -117,6 +119,10 @@ static gboolean fill_queue_idle(gpointer user_data)
 goto end;
 }
 
+if (do_count) {
+ncommands++;
+}
+
 if (slow)
 g_usleep(slow);
 
@@ -297,6 +303,7 @@ int main(int argc, char **argv)
 { "port", 'p', 0, G_OPTION_ARG_INT, &port, "Server port (default 
5000)", "PORT" },
 { "wait", 'w', 0, G_OPTION_ARG_NONE, &wait, "Wait for client", NULL },
 { "slow", 's', 0, G_OPTION_ARG_INT, &slow, "Slow down replay. Delays 
USEC microseconds before each command", "USEC" },
+{ "count", 0, 0, G_OPTION_ARG_NONE, &do_count, "count the number of 
commands", NULL },
 { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &file, 
"replay file", "FILE" },
 { NULL }
 };
@@ -402,6 +409,9 @@ int main(int argc, char **argv)
 loop = g_main_loop_new(basic_event_loop_get_context(), FALSE);
 g_main_loop_run(loop);
 
+if (do_count)
+g_print("Counted %d commands\n", ncommands);
+
 end_replay();
 g_async_queue_unref(aqueue);
 
-- 
2.5.0

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

[Spice-devel] [server PATCH 0/3] replay: add count and skip

[Uri Lublin]

This patch-set touches only server/tests/replay utility.

It adds 2 commands and fixes an access to a NULL pointer.

With count and skip new commands, it's possible
to fast-forward the replay to an interesting point.

How to use it:
First run only with --count: spice-server-replay --count $spice-recorded-file
It counts the number of commands processed by spice-server.

Than run with skip and the number of commands to skip.
For example if there are 10 commands one can skip to 5
spice-server-replay --slow=1000 --skip=5  $spice-recorded-file

Note, that all commands are processed by spice-server, the "--skip"
only skips the sleep of "--slow".

Uri Lublin (3):
  replay: learn how to count commands
  replay: learn how to skip the first N (slow) commands
  replay: do not use argv after g_option_context_parse

 server/tests/replay.c | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

-- 
2.5.0

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

[Spice-devel] [server PATCH 3/3] replay: do not use argv after g_option_context_parse

[Uri Lublin]

Apparently, after using g_option_context_parse with G_OPTION_REMAINING
argv is modified and should not be used.
This patch uses "file" instead of "argv" and makes sure
file is freed later.
No free is called upon error - exit takes care of it.
---
 server/tests/replay.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index 4fcbc3e..cfb8871 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -364,12 +364,12 @@ int main(int argc, char **argv)
 } else {
 fd = fopen(file[0], "r");
 }
-g_strfreev(file);
-file = NULL;
 if (fd == NULL) {
-g_printerr("error opening %s\n", argv[1]);
-return 1;
+g_printerr("error opening %s\n", file[0]);
+exit(1);
 }
+g_strfreev(file);
+file = NULL;
 if (fcntl(fileno(fd), FD_CLOEXEC) < 0) {
 perror("fcntl failed");
 exit(1);
-- 
2.5.0

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

[Spice-devel] [server PATCH 2/3] replay: learn how to skip the first N (slow) commands

[Uri Lublin]

Note that the commands are executed by spice-server.
The "skip" is only done on the "sleep" part of the
"slow" command-line option.

This is helpful to run quickly through uninsteresting commands
in a beginning of a recorded file and going slowly when
interesting parts appear
---
 server/tests/replay.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index 83411d3..4fcbc3e 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -47,6 +47,7 @@ static QXLWorker *qxl_worker = NULL;
 static gboolean started = FALSE;
 static QXLInstance display_sin = { 0, };
 static gint slow = 0;
+static gint skip = 0;
 static gboolean do_count = 0;
 static guint ncommands = 0;
 static pid_t client_pid;
@@ -123,8 +124,13 @@ static gboolean fill_queue_idle(gpointer user_data)
 ncommands++;
 }
 
-if (slow)
-g_usleep(slow);
+if (slow) {
+if (skip > 0) {
+skip--;
+} else {
+g_usleep(slow);
+}
+}
 
 wakeup = TRUE;
 g_async_queue_push(aqueue, cmd);
@@ -303,6 +309,7 @@ int main(int argc, char **argv)
 { "port", 'p', 0, G_OPTION_ARG_INT, &port, "Server port (default 
5000)", "PORT" },
 { "wait", 'w', 0, G_OPTION_ARG_NONE, &wait, "Wait for client", NULL },
 { "slow", 's', 0, G_OPTION_ARG_INT, &slow, "Slow down replay. Delays 
USEC microseconds before each command", "USEC" },
+{ "skip", 0, 0, G_OPTION_ARG_INT, &skip, "skip 'slow' for the first n 
commands", NULL },
 { "count", 0, 0, G_OPTION_ARG_NONE, &do_count, "count the number of 
commands", NULL },
 { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &file, 
"replay file", "FILE" },
 { NULL }
-- 
2.5.0

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

Re: [Spice-devel] Hotkeys are disable when use spice-usbredir-redirect-on-connect option

[Uri Lublin]

On 03/02/2016 03:00 PM, Christophe Fergeau wrote:

Hey,

On Wed, Feb 17, 2016 at 01:54:48PM +0330, Hamid Mazrae Mollaie wrote:

Hi,
Tank you for replay.
yes, my mouse and keyboard are usb and redirect to vm... and all pressed
key sent to vm
all hotkeys and Ctrl+Alt+F[n] does not work for example. but if i avoid
from spice-usbredir-redirect-on-connect option, all hotkeys work perfect.
my question is how can i avoid from redirect mouse and keyboard always and
dynamically but i can use spice-usbredir-redirect-on-connect option for
other usb hardware?


I think you can tweak the filter you use with
--spice-usbredir-redirect-on-connect to exclude your USB keyboard/mouse
spice-gtk documentation says

 /**
  * The default setting filters out HID (class 0x03) USB devices from auto
  * connect and auto connects anything else. Note the explicit allow rule at
  * the end, this is necessary since by default all devices without a
  * matching filter rule will not auto-connect.
  */

and this default setting is "0x03,-1,-1,-1,0|-1,-1,-1,-1,1"

So I would try 
--spice-usbredir-redirect-on-connect="0x03,-1,-1,-1,0|-1,-1,-1,-1,1"


Or specify the specific device you want the guest to access and use
a rule to allow this specific device (or class) and reject all
other devices, such as:

-1,VID,PID,-1,1|-1,-1,-1,-1,0

Uri

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

[Spice-devel] [server PATCH v2 0/3] replay: add count and skip

[Uri Lublin]

This patch-set touches only server/tests/replay utility.

It adds 2 commands and fixes an access to a NULL pointer.

With count and skip new commands, it's possible
to fast-forward the replay to an interesting point.

How to use it:
First run only with --count: spice-server-replay --count $spice-recorded-file
It counts the number of commands processed by spice-server.

Then run with skip and the number of commands to skip.
For example if there are 10 commands one can skip to 5
spice-server-replay --slow=1000 --skip=5  $spice-recorded-file

Note, that all commands are processed by spice-server, the "--skip"
only skips the sleep of "--slow".

v1->v2 : Made some changes according to Frediano's comments:
 - The number of commands is always counted, and --count only prints it.
   = variable name changed accordingly
   = skip logic is a bit simplified
 - Help string for count starts with a capital letter.
 - ++ncommands instead of ncommands++ (same in this scenario)


Uri Lublin (3):
  replay: learn how to count commands
  replay: learn how to skip the first N (slow) commands
  replay: do not use argv after g_option_context_parse

 server/tests/replay.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

-- 
2.5.0

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

[Spice-devel] [server PATCH v2 3/3] replay: do not use argv after g_option_context_parse

[Uri Lublin]

Apparently, after using g_option_context_parse with G_OPTION_REMAINING
argv is modified and should not be used.
This patch uses "file" instead of "argv" and makes sure
file is freed later.
No free is called upon error - exit takes care of it.

Signed-off-by: Uri Lublin 
Acked-by: Frediano Ziglio 
---
 server/tests/replay.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index 3642f2d..7e4659b 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -358,12 +358,12 @@ int main(int argc, char **argv)
 } else {
 fd = fopen(file[0], "r");
 }
-g_strfreev(file);
-file = NULL;
 if (fd == NULL) {
-g_printerr("error opening %s\n", argv[1]);
-return 1;
+g_printerr("error opening %s\n", file[0]);
+exit(1);
 }
+g_strfreev(file);
+file = NULL;
 if (fcntl(fileno(fd), FD_CLOEXEC) < 0) {
 perror("fcntl failed");
 exit(1);
-- 
2.5.0

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

[Spice-devel] [server PATCH v2 2/3] replay: learn how to skip the first N (slow) commands

[Uri Lublin]

Note that the commands are executed by spice-server.
The "skip" is only done on the "sleep" part of the
"slow" command-line option.

This is helpful to run quickly through uninsteresting commands
in a beginning of a recorded file and going slowly when
interesting parts appear
---
 server/tests/replay.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index 0451b9a..3642f2d 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -47,6 +47,7 @@ static QXLWorker *qxl_worker = NULL;
 static gboolean started = FALSE;
 static QXLInstance display_sin = { 0, };
 static gint slow = 0;
+static gint skip = 0;
 static gboolean print_count = FALSE;
 static guint ncommands = 0;
 static pid_t client_pid;
@@ -121,8 +122,9 @@ static gboolean fill_queue_idle(gpointer user_data)
 
 ++ncommands;
 
-if (slow)
+if (slow && (ncommands > skip)) {
 g_usleep(slow);
+}
 
 wakeup = TRUE;
 g_async_queue_push(aqueue, cmd);
@@ -301,6 +303,7 @@ int main(int argc, char **argv)
 { "port", 'p', 0, G_OPTION_ARG_INT, &port, "Server port (default 
5000)", "PORT" },
 { "wait", 'w', 0, G_OPTION_ARG_NONE, &wait, "Wait for client", NULL },
 { "slow", 's', 0, G_OPTION_ARG_INT, &slow, "Slow down replay. Delays 
USEC microseconds before each command", "USEC" },
+{ "skip", 0, 0, G_OPTION_ARG_INT, &skip, "skip 'slow' for the first n 
commands", NULL },
 { "count", 0, 0, G_OPTION_ARG_NONE, &print_count, "Print the number of 
commands processed", NULL },
 { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &file, 
"replay file", "FILE" },
 { NULL }
-- 
2.5.0

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

[Spice-devel] [server PATCH v2 1/3] replay: learn how to count commands

[Uri Lublin]

---
 server/tests/replay.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/server/tests/replay.c b/server/tests/replay.c
index f3b670f..0451b9a 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -47,6 +47,8 @@ static QXLWorker *qxl_worker = NULL;
 static gboolean started = FALSE;
 static QXLInstance display_sin = { 0, };
 static gint slow = 0;
+static gboolean print_count = FALSE;
+static guint ncommands = 0;
 static pid_t client_pid;
 static GMainLoop *loop = NULL;
 static GAsyncQueue *aqueue = NULL;
@@ -117,6 +119,8 @@ static gboolean fill_queue_idle(gpointer user_data)
 goto end;
 }
 
+++ncommands;
+
 if (slow)
 g_usleep(slow);
 
@@ -297,6 +301,7 @@ int main(int argc, char **argv)
 { "port", 'p', 0, G_OPTION_ARG_INT, &port, "Server port (default 
5000)", "PORT" },
 { "wait", 'w', 0, G_OPTION_ARG_NONE, &wait, "Wait for client", NULL },
 { "slow", 's', 0, G_OPTION_ARG_INT, &slow, "Slow down replay. Delays 
USEC microseconds before each command", "USEC" },
+{ "count", 0, 0, G_OPTION_ARG_NONE, &print_count, "Print the number of 
commands processed", NULL },
 { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &file, 
"replay file", "FILE" },
 { NULL }
 };
@@ -402,6 +407,9 @@ int main(int argc, char **argv)
 loop = g_main_loop_new(basic_event_loop_get_context(), FALSE);
 g_main_loop_run(loop);
 
+if (print_count)
+g_print("Counted %d commands\n", ncommands);
+
 end_replay();
 g_async_queue_unref(aqueue);
 
-- 
2.5.0

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

Re: [Spice-devel] [spice-gtk 3/4] coverity: avoid out of bounds access

[Uri Lublin]

On 04/04/2016 12:13 PM, Christophe Fergeau wrote:

On Mon, Apr 04, 2016 at 10:02:08AM +0200, Fabiano Fidêncio wrote:

---
  src/controller/test.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/controller/test.c b/src/controller/test.c
index 649aca5..2909b06 100644
--- a/src/controller/test.c
+++ b/src/controller/test.c
@@ -262,7 +262,7 @@ int main (int argc, char *argv[])
  send_data (CONTROLLER_PASSWORD, (uint8_t*)PWD, strlen(PWD) + 1);
  send_data (CONTROLLER_SECURE_CHANNELS, (uint8_t*)SECURE_CHANNELS, 
strlen(SECURE_CHANNELS) + 1);
  send_data (CONTROLLER_DISABLE_CHANNELS, (uint8_t*)DISABLED_CHANNELS, 
strlen(DISABLED_CHANNELS) + 1);
-send_data (CONTROLLER_TLS_CIPHERS, (uint8_t*)TLS_CIPHERS, 
sizeof(TLS_CIPHERS) + 1);
+send_data (CONTROLLER_TLS_CIPHERS, (uint8_t*)TLS_CIPHERS, 
strlen(TLS_CIPHERS) + 1);


sizeof("TLS_CIPHER") is not doing the right thing?
This is consistent with the other constants anyways, so fine with me.


sizeof() includes the ending '\0', so data_size is larger by 1
than actual data.

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

Re: [Spice-devel] [PATCH] remove dandling pointer for RedCharDeviceVDIPort

[Uri Lublin]

On 05/02/2016 11:25 AM, Frediano Ziglio wrote:

This was caused by commit 1cec1c5118b65124de6bc6f984f376ff4e297bfb
("reds: Make VDIPortState a GObject") as the lifespan of RedCharDevice
was changed.

This could be reproduced with:
- start rhel7 machine
- connect remote viewer (RV)
- RV: login
- connect ssh
- SSH: stop agent
- disconnect RV
- SSH: start agent
- connect to RV

and caused (using address sanitizer):

(snipped)


Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index efd1429..67c262a 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -603,12 +603,10 @@ void reds_client_disconnect(RedsState *reds, RedClient 
*client)
 reds_mig_remove_wait_disconnect_client(reds, client);
 }



Hi Frediano,

Shouldn't the patch fix "agent_attached"  value instead of ignoring it ?
Why ignore its value here and not in other places ?

Regards,
Uri.


-if (reds->agent_dev->priv->agent_attached) {
-/* note that vdagent might be NULL, if the vdagent was once
- * up and than was removed */
-if (red_char_device_client_exists(RED_CHAR_DEVICE(reds->agent_dev), 
client)) {
-red_char_device_client_remove(RED_CHAR_DEVICE(reds->agent_dev), 
client);
-}
+/* note that client might be NULL, if the vdagent was once
+ * up and than was removed */
+if (red_char_device_client_exists(RED_CHAR_DEVICE(reds->agent_dev), 
client)) {
+red_char_device_client_remove(RED_CHAR_DEVICE(reds->agent_dev), 
client);
 }

 ring_remove(&client->link);



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

Re: [Spice-devel] [spice-devel]How to reduce the image resolution or definition ?

[Uri Lublin]

On 04/12/2016 12:18 PM, hongzhen_...@sina.com wrote:

 Hello

   Could you tell  me how to reduce the image resolution or definition ?

   I plan to reduce the image definition so that  decrease data
transmission from server to client .

   For example ,I assume if I have a low bandwidth in my network
environment,but it's stable , I want to reduce the image definition  to
ensure a small quantity of data were sent  between the server and
client  ,for the client can be worked normally (such as basic operation).

  But I don't know how to modify and control it (which functions or
attributes),  if you know that ,please help  me . Thank you very much ...


Hi,

For Windows guests when the spice-vdagent is running and network
is slow, by default when Spice recognizes its a slow network
it disables some features on the guest to lower the
bandwidth, such as animations, color depth, etc.

You can ask for that specifically with the spice client command line:
remote-viewer --spice-color-depth=16 --spice-disable-effects=all ...

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

Re: [Spice-devel] [PATCH] Make asciidoc a hard requirement

[Uri Lublin]

On 04/26/2016 12:23 AM, Eduardo Lima (Etrunko) wrote:

The problem happens when you run 'make dist' in a system without
asciidoc installed. Even though in configure time there is a check for
building the manual, it is required to be built for distribution.


Hi,

I think we should not force building the manual nor require asciidoc
to build spice-server.

We better find another way to make 'make dist' work

Regards,
Uri.




Signed-off-by: Eduardo Lima (Etrunko) 
---
 configure.ac| 25 ++---
 docs/Makefile.am|  2 --
 docs/manual/Makefile.am |  8 +---
 3 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/configure.ac b/configure.ac
index 8419508..18b907a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -155,24 +155,12 @@ if test "x$enable_automated_tests" = "xyes"; then
 fi


-AC_ARG_ENABLE([manual],
-   AS_HELP_STRING([--enable-manual=@<:@auto/yes/no@:>@],
-  [Build SPICE manual]),
-   [],
-   [enable_manual="auto"])
-if test "x$enable_manual" != "xno"; then
-AC_PATH_PROG([ASCIIDOC], [asciidoc])
-AS_IF([test -z "$ASCIIDOC" && test "x$enable_manual" = "xyes"],
-  [AC_MSG_ERROR([asciidoc is missing and build of manual was 
requested])])
-AC_PATH_PROG([A2X], [a2x])
-AS_IF([test -z "$A2X" && test "x$enable_manual" = "xyes"],
-  [AC_MSG_ERROR([a2x is missing and build of manual was requested])])
-fi
-AS_IF([test -n "$ASCIIDOC"], [have_asciidoc=yes], [have_asciidoc=no])
-AM_CONDITIONAL([BUILD_MANUAL], [test -n "$ASCIIDOC" || test -n "$A2X"])
-AM_CONDITIONAL([BUILD_HTML_MANUAL], [test -n "$ASCIIDOC"])
-AM_CONDITIONAL([BUILD_CHUNKED_MANUAL], [test -n "$A2X"])
-
+AC_PATH_PROG([ASCIIDOC], [asciidoc])
+AS_IF([test -z "$ASCIIDOC"],
+  [AC_MSG_ERROR([asciidoc is missing])])
+AC_PATH_PROG([A2X], [a2x])
+AS_IF([test -z "$A2X"],
+  [AC_MSG_ERROR([a2x is missing])])

 dnl ===
 dnl check compiler flags
@@ -245,7 +233,6 @@ AC_MSG_NOTICE([
 Smartcard:${have_smartcard}
 SASL support: ${have_sasl}
 Automated tests:  ${enable_automated_tests}
-Manual:   ${have_asciidoc}

 Now type 'make' to build $PACKAGE
 ])
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 18e785f..e76efaf 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -1,3 +1 @@
-if BUILD_MANUAL
 SUBDIRS = manual
-endif
diff --git a/docs/manual/Makefile.am b/docs/manual/Makefile.am
index 24a11fe..beda615 100644
--- a/docs/manual/Makefile.am
+++ b/docs/manual/Makefile.am
@@ -16,13 +16,7 @@ EXTRA_DIST = \
 manual.chunked: manual.txt
$(AM_V_GEN) $(A2X) -f chunked -D $(builddir) $(ASCIIDOC_FLAGS) $<

-docfiles =
-if BUILD_HTML_MANUAL
-docfiles += manual.html
-endif
-if BUILD_CHUNKED_MANUAL
-docfiles += manual.chunked
-endif
+docfiles = manual.html manual.chunked

 all-local: $(docfiles)




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

Re: [Spice-devel] [PATCH] Make asciidoc a hard requirement

[Uri Lublin]

On 05/02/2016 03:32 PM, Eduardo Lima (Etrunko) wrote:

On 05/02/2016 07:39 AM, Uri Lublin wrote:

On 04/26/2016 12:23 AM, Eduardo Lima (Etrunko) wrote:

The problem happens when you run 'make dist' in a system without
asciidoc installed. Even though in configure time there is a check for
building the manual, it is required to be built for distribution.


Hi,

I think we should not force building the manual nor require asciidoc
to build spice-server.



I disagree. The output of make dist should be the same no matter what
system the command is run. Users should have the required dependencies
installed if they are interested in building spice-server from the
source code tarball.


We better find another way to make 'make dist' work

Regards,
Uri.



The only other option I see is to keep track of the generated files in
git, and if there is any modifications, generate it again manually.


For example, one can configure --enable-manual before running make dist.

Or install asciidoc before running configure.

Uri.







Signed-off-by: Eduardo Lima (Etrunko) 
---
 configure.ac| 25 ++---
 docs/Makefile.am|  2 --
 docs/manual/Makefile.am |  8 +---
 3 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/configure.ac b/configure.ac
index 8419508..18b907a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -155,24 +155,12 @@ if test "x$enable_automated_tests" = "xyes"; then
 fi


-AC_ARG_ENABLE([manual],
-   AS_HELP_STRING([--enable-manual=@<:@auto/yes/no@:>@],
-  [Build SPICE manual]),
-   [],
-   [enable_manual="auto"])
-if test "x$enable_manual" != "xno"; then
-AC_PATH_PROG([ASCIIDOC], [asciidoc])
-AS_IF([test -z "$ASCIIDOC" && test "x$enable_manual" = "xyes"],
-  [AC_MSG_ERROR([asciidoc is missing and build of manual was
requested])])
-AC_PATH_PROG([A2X], [a2x])
-AS_IF([test -z "$A2X" && test "x$enable_manual" = "xyes"],
-  [AC_MSG_ERROR([a2x is missing and build of manual was
requested])])
-fi
-AS_IF([test -n "$ASCIIDOC"], [have_asciidoc=yes], [have_asciidoc=no])
-AM_CONDITIONAL([BUILD_MANUAL], [test -n "$ASCIIDOC" || test -n "$A2X"])
-AM_CONDITIONAL([BUILD_HTML_MANUAL], [test -n "$ASCIIDOC"])
-AM_CONDITIONAL([BUILD_CHUNKED_MANUAL], [test -n "$A2X"])
-
+AC_PATH_PROG([ASCIIDOC], [asciidoc])
+AS_IF([test -z "$ASCIIDOC"],
+  [AC_MSG_ERROR([asciidoc is missing])])
+AC_PATH_PROG([A2X], [a2x])
+AS_IF([test -z "$A2X"],
+  [AC_MSG_ERROR([a2x is missing])])

 dnl
===

 dnl check compiler flags
@@ -245,7 +233,6 @@ AC_MSG_NOTICE([
 Smartcard:${have_smartcard}
 SASL support: ${have_sasl}
 Automated tests:  ${enable_automated_tests}
-Manual:   ${have_asciidoc}

 Now type 'make' to build $PACKAGE
 ])
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 18e785f..e76efaf 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -1,3 +1 @@
-if BUILD_MANUAL
 SUBDIRS = manual
-endif
diff --git a/docs/manual/Makefile.am b/docs/manual/Makefile.am
index 24a11fe..beda615 100644
--- a/docs/manual/Makefile.am
+++ b/docs/manual/Makefile.am
@@ -16,13 +16,7 @@ EXTRA_DIST =\
 manual.chunked: manual.txt
 $(AM_V_GEN) $(A2X) -f chunked -D $(builddir) $(ASCIIDOC_FLAGS) $<

-docfiles =
-if BUILD_HTML_MANUAL
-docfiles += manual.html
-endif
-if BUILD_CHUNKED_MANUAL
-docfiles += manual.chunked
-endif
+docfiles = manual.html manual.chunked

 all-local: $(docfiles)









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

Re: [Spice-devel] [PATCH] remove dandling pointer for RedCharDeviceVDIPort

[Uri Lublin]

On 05/03/2016 01:53 PM, Frediano Ziglio wrote:


On 05/02/2016 11:25 AM, Frediano Ziglio wrote:

This was caused by commit 1cec1c5118b65124de6bc6f984f376ff4e297bfb
("reds: Make VDIPortState a GObject") as the lifespan of RedCharDevice
was changed.

This could be reproduced with:
- start rhel7 machine
- connect remote viewer (RV)
- RV: login
- connect ssh
- SSH: stop agent
- disconnect RV
- SSH: start agent
- connect to RV

and caused (using address sanitizer):

(snipped)


Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index efd1429..67c262a 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -603,12 +603,10 @@ void reds_client_disconnect(RedsState *reds,
RedClient *client)
 reds_mig_remove_wait_disconnect_client(reds, client);
 }



Hi Frediano,

Shouldn't the patch fix "agent_attached"  value instead of ignoring it ?
Why ignore its value here and not in other places ?

Regards,
 Uri.



Honestly more I look at the patch and this fix and more I think it's all
a big bug...
What I know for sure is that this patch fix a dandling pointer.


That's true.
But I think we should have a better fix that does not ignore the value
of agent_attached, or at least we should verify that other uses of this
variable are correct and this is the only place needs fixing.



Coming to your question. agent_attached. Should be a well defined things,
there is an agent in the guest attached to spice-server. Now... can we have
more agents or this "agent_attached" is referring to the agent daemon instead
of the an agent? All code seems to not to handle multiple agents. I don't
understand if this is a bug/missing feature or agent here refers to the daemon
which handle multiple agents.


The "agent" here refers to a program on the guest that opened the serial 
device (port). On Linux guests it's spice-vdagentd.



Handling the agent_attached. Previous code delete the RedCharDevice before
setting agent_attached to FALSE. This does not make much sense to me! The
RedCharDevice represents a device created by Qemu (in the normal usage of
spice-server) so there is no sense in deleting it when an agent is detached.


I think the Qemu device is always there. Qemu lets spice-server know
when the something (agent) is attached (opened) the device.


Also looking at the if:

if (red_channel_test_remote_cap(&reds->main_channel->base,
 SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS)) {
-red_char_device_destroy(dev->priv->base);
-dev->priv->base = NULL;
+dev->priv->agent_attached = FALSE;
 } else {
-red_char_device_reset(dev->priv->base);
+red_char_device_reset(RED_CHAR_DEVICE(dev));
 }

sorry.. this code is really messy. Is supposed to reset the vdp (actually it's
one of the very few function using "vdp" which is quite confusing). It's using
some RedChannel state (reds->main_channel) which raise some questions:
- having one main_channel... how we are supposed to handle multiple clients??


Note that the calling function (reds_agent_remove) has a comment saying:
"TODO: agent is broken with multiple clients."


- why vdp (which is device which is usually supposed to be always present) have
  to check a RedChannel (which is supposed to be present only when a client is
  connected) ?
- if there isn't the SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS behavior is quite
  different, why?


Backwards compatibility -- see comment above that code:
" resetting and not destroying the dev as a workaround for a bad
  tokens management in the vdagent protocol"
The code assumes this is not an issue when the client supports
SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS and the char-device can be destroyed.

There is only a single main-channel, but there may be several
clients connected to it (each has its own MainChannelClient).



Looking at http://picpaste.com/channels_devices.png and char-device.c one 
question
came into my mind. Who is supposed to handle the RedClient pointer inside
RedCharDeviceClient? IMHO this is quite bad programming style:
- the pointer is on a base class but not handled by the base class;
- there are no checks on the base class that the derived classes handle it;
- RedCharDevice has a red_char_device_client_add but it's not documented that
  derived classes MUST call red_char_device_client_remove.
Looks like other RedCharDevice derived classes handle these stuff in a 
completely
different way (for instance this reference is handled by on_disconnect 
callbacks).


I'm not sure, but maybe the vdagent is the oldest one, and is
the only one that needs to be compatible with old clients.


Thanks,
Uri.




Frediano



-if (reds->agent_dev->priv->agent_attached) {
-/* note that vdagent might be NULL, if the vdagent was once
- * up and than was removed */
-if
(red_char_device_client_exists(RED_CHAR_DEVICE(reds->agent_dev), clie

Re: [Spice-devel] [PATCH] Make asciidoc a hard requirement

[Uri Lublin]

On 05/03/2016 06:36 PM, Frediano Ziglio wrote:


On 05/02/2016 07:39 AM, Uri Lublin wrote:

On 04/26/2016 12:23 AM, Eduardo Lima (Etrunko) wrote:

The problem happens when you run 'make dist' in a system without
asciidoc installed. Even though in configure time there is a check for
building the manual, it is required to be built for distribution.


Hi,

I think we should not force building the manual nor require asciidoc
to build spice-server.



I disagree. The output of make dist should be the same no matter what
system the command is run. Users should have the required dependencies
installed if they are interested in building spice-server from the
source code tarball.



I agree with Eduardo. The make dist should build a tarball containing
everything and be the same.

However users should also have the option to build spice-server, install
it on their system without having to build the documentation.

So asciidoc should be required for "make dist" but not "make all" or
"make install", despite any possible --enable/disable-manual.

Frediano


I do not see any problem with having to configure with --enable-manual.
I tried make dist  with --enable-manual and without and the result
is the same.

However, --enable-manual is not even needed, as long as asciidoc is 
installed. So it's only required to install asciidoc before running

make dist (and if you did not and make dist fails just install
the package and reconfigure).

I have no problem accepting patches that make "make dist" require
asciidoc, but I think we should not build the manual by default.

Regards,
Uri.


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

Re: [Spice-devel] [PATCH] remove dandling pointer for RedCharDeviceVDIPort

[Uri Lublin]

On 05/04/2016 11:51 AM, Frediano Ziglio wrote:


On 05/03/2016 01:53 PM, Frediano Ziglio wrote:


On 05/02/2016 11:25 AM, Frediano Ziglio wrote:

This was caused by commit 1cec1c5118b65124de6bc6f984f376ff4e297bfb
("reds: Make VDIPortState a GObject") as the lifespan of RedCharDevice
was changed.

This could be reproduced with:
- start rhel7 machine
- connect remote viewer (RV)
- RV: login
- connect ssh
- SSH: stop agent
- disconnect RV
- SSH: start agent
- connect to RV

and caused (using address sanitizer):

(snipped)


Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index efd1429..67c262a 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -603,12 +603,10 @@ void reds_client_disconnect(RedsState *reds,
RedClient *client)
 reds_mig_remove_wait_disconnect_client(reds, client);
 }



Hi Frediano,

Shouldn't the patch fix "agent_attached"  value instead of ignoring it ?
Why ignore its value here and not in other places ?

Regards,
 Uri.



Honestly more I look at the patch and this fix and more I think it's all
a big bug...
What I know for sure is that this patch fix a dandling pointer.


That's true.
But I think we should have a better fix that does not ignore the value
of agent_attached, or at least we should verify that other uses of this
variable are correct and this is the only place needs fixing.



Don't be confused by the check removal. Previously (before the offending
commit) the check was checking the existence of RedCharDevice. Now
RedCharDevice is always present so the check should be removed.

I don't see any reason why this patch should not be merged (beside the
comment typo fix).

All other reasoning came from looking the current state of the related
code.



The specific fix looks fine to me -- when a client disconnects remove it
from the list of clients connected to the spice char-device.

My concern is that earlier patch(es) replaced
(reds->agent_state.base == NULL) with the boolean agent_attached.

This patch ignores the value of agent_attached in this specific
case of client disconnection, so we found a place where the
earlier patch's assumption does not hold.

I'm fine with applying this patch.

We should audit the code to find if there are more cases
when agent_attached value is incorrect/irrelevant.

Thanks,
Uri.






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

Re: [Spice-devel] [PATCH 0/2] Fix some regression introduced by recent patches

[Uri Lublin]

On 05/05/2016 03:54 PM, Eduardo Lima (Etrunko) wrote:

On 05/05/2016 09:33 AM, Frediano Ziglio wrote:

Frediano Ziglio (2):
  remove dangling pointer for RedCharDeviceVDIPort
  Revert "Remove use of opaque from vdi_port_read_one_msg_from_device"

 server/reds.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)



Tested and confirmed to fix the issues. Uri, any objections?

Acked-by: Eduardo Lima (Etrunko) 



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

[Spice-devel] [spice-streaming-agent PATCH] mjpeg: get_time: fix off-by-0

[Uri Lublin]

Only 9 0's are required

Signed-off-by: Uri Lublin 
---
 src/mjpeg-fallback.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 09f3769..1b263f2 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -26,7 +26,7 @@ static inline uint64_t get_time()
 
 clock_gettime(CLOCK_MONOTONIC, &now);
 
-return (uint64_t)now.tv_sec * 100u + (uint64_t)now.tv_nsec;
+return (uint64_t)now.tv_sec * 10u + (uint64_t)now.tv_nsec;
 }
 
 namespace {
-- 
2.20.1

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

[Spice-devel] [spice-streaming-agent PATCH v2] mjpeg: get_time: fix conversion of seconds to nanaseconds

[Uri Lublin]

1 second is 10^9 nanosecond, not 10^10.

Signed-off-by: Uri Lublin 
Acked-by: Victor Toso 
---

changes since V1:
 - Subject and log message reworded (Frediano, Victor)

---
 src/mjpeg-fallback.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 09f3769..1b263f2 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -26,7 +26,7 @@ static inline uint64_t get_time()
 
 clock_gettime(CLOCK_MONOTONIC, &now);
 
-return (uint64_t)now.tv_sec * 100u + (uint64_t)now.tv_nsec;
+return (uint64_t)now.tv_sec * 10u + (uint64_t)now.tv_nsec;
 }
 
 namespace {
-- 
2.20.1

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

Re: [Spice-devel] Blank screen on SPICE when using vGPU

[Uri Lublin]

On 4/3/19 7:33 PM, James Freeman wrote:

Hi all,

Conscious this might not be a SPICE issue but trying to debug where the 
issue is right now. I'm working with someone who is using RHV 4.2 with a 
vGPU configuration. At present we are stuck on a problem whereby the 
vGPU can be successfully assigned to a given VM (either Windows or 
Linux) and then the VM booted. However as soon as the closed source 
nVidia drivers are installed and the VM rebooted, the SPICE display is 
blank (all black).


Trying to determine where the issue is - with the Windows VM we have 
been able to RDP in and see that the nVidia driver was loaded and the 
nVidia control panel loads - this would seem to indicate the driver is 
loaded and working. Still running some additional tests on the Linux VM 
- however we're trying to figure out where the problem lies. Is it 
possible this is an interaction with SPICE, or is this likely an issue 
at a lower level? Is there a suggested route to debug this issue?


Hi James,

The VM has 2 GPUs, 1 is QXL(*) + 1 is NVIDIA.
(*) QXL or another emulated device.

SPICE connects to the QXL, but the guest is using NVIDIA
which is why the display is black.

There are 2 things you can try (I'm not sure how well they work):
1. Try to run the VM only with the NVIDIA vGPU (no QXL) and
   make add display=on to qemu-kvm command line mdev-device option.
2. Currently only for Linux -- add a spice.stream virtio-serial port
   and run spice-streaming-agent on the guest.

What OS is running on your host/guest ?
Do you actually use both QXL and NVIDIA ?

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

[Spice-devel] [spice-common PATCH] codegen Makefile: add common/ to --include client_marshallers.h

[Uri Lublin]

Fixes out-of-tree builds.

For example spice-gtk out-of-tree build fails with:
  ../subprojects/spice-common/common/generated_client_marshallers.h:19:10:
  fatal error: client_marshallers.h: No such file or directory
   #include "client_marshallers.h"
^~

Signed-off-by: Uri Lublin 
---
 common/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/Makefile.am b/common/Makefile.am
index 2dd6d04..a904dae 100644
--- a/common/Makefile.am
+++ b/common/Makefile.am
@@ -114,7 +114,7 @@ generated_client_demarshallers.c generated_messages.h: 
$(top_srcdir)/spice.proto
--generated-declaration-file generated_messages.h $< $@ >/dev/null
 
 generated_client_marshallers.c generated_client_marshallers.h: 
$(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
-   $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py 
--generate-marshallers -P --include client_marshallers.h --client \
+   $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py 
--generate-marshallers -P --include common/client_marshallers.h --client \
--generate-header $< $@ >/dev/null
 
 generated_server_demarshallers.c: $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
-- 
2.20.1

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

[Spice-devel] [Patch spice-gtk 1/2] spice-common: update submodule

[Uri Lublin]

Fix out-of-tree builds.

Uri Lublin (1):
  codegen Makefile: add common/ to --include client_marshallers.h
---
 subprojects/spice-common | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/subprojects/spice-common b/subprojects/spice-common
index 47ba8e0..5bcfa71 16
--- a/subprojects/spice-common
+++ b/subprojects/spice-common
@@ -1 +1 @@
-Subproject commit 47ba8e0f25ddba110fe77ba55ad5da570f4582bd
+Subproject commit 5bcfa711d90839c7c4c50310db67f0d5912a3824
-- 
2.20.1

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

[Spice-devel] [Patch spice-gtk 2/2] gitlab-ci: build out-of-tree too

[Uri Lublin]

Signed-off-by: Uri Lublin 
---
 .gitlab-ci.yml | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index adf53e1..dbfb7ad 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -30,10 +30,13 @@ fedora-autotools:
 - (cd spice-protocol && ./autogen.sh --prefix=/usr && make install)
 
   script:
-# Run with default options
-- ./autogen.sh --enable-static
+# Run with default options + out-of-tree
+- mkdir build
+- cd build
+- ../autogen.sh --enable-static
 - make -j4
 - make check
+- cd ..
 # Run without features
 - git clean -xfd
 - ./autogen.sh --enable-static
-- 
2.20.1

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

[Spice-devel] [Patch spice-gtk 0/2] Fix out-of-tree builds

[Uri Lublin]

There are two patches:
  The first picks the fixed spice-common
  The second adds a gitlab-ci test for out-of-tree builds

Uri Lublin (2):
  spice-common: update submodule
  gitlab-ci: build out-of-tree too

 .gitlab-ci.yml   | 7 +--
 subprojects/spice-common | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

-- 
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 0/2] Fix out-of-tree builds

[Uri Lublin]

On 4/23/19 6:24 PM, Uri Lublin wrote:

There are two patches:
   The first picks the fixed spice-common
   The second adds a gitlab-ci test for out-of-tree builds


gitlab-ci pipeline with both patches:
https://gitlab.freedesktop.org/uril/spice-gtk/pipelines/32682

gitlab-ci pipeline with only the second patch (fails as expected):
https://gitlab.freedesktop.org/uril/spice-gtk/pipelines/32558





Uri Lublin (2):
   spice-common: update submodule
   gitlab-ci: build out-of-tree too

  .gitlab-ci.yml   | 7 +--
  subprojects/spice-common | 2 +-
  2 files changed, 6 insertions(+), 3 deletions(-)



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

[Spice-devel] [PATCH] gitlab-ci: build out-of-tree too

[Uri Lublin]

Signed-off-by: Uri Lublin 
---

The relevant job of the pipeline is:
https://gitlab.freedesktop.org/uril/spice/-/jobs/269735

---
 .gitlab-ci.yml | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 9ce95c00e..43cdb45ac 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -14,12 +14,15 @@ before_script:
 
 makecheck:
   script:
+  - mkdir builddir
+  - cd builddir
   - >
 CFLAGS='-O2 -pipe -g -fsanitize=address -fno-omit-frame-pointer 
-Wframe-larger-than=40920'
 LDFLAGS='-fsanitize=address -lasan'
-./autogen.sh --enable-celt051
+../autogen.sh --enable-celt051
   - make
   - make -C server check || (cat server/tests/test-suite.log && exit 1)
+  - cd ..
 
 meson-makecheck:
   script:
-- 
2.20.1

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

Re: [Spice-devel] [PATCH] gitlab-ci: build out-of-tree too

[Uri Lublin]

On 4/30/19 1:15 PM, Frediano Ziglio wrote:

This is for SPICE server (missing in the subject).


Yes, sorry.





Signed-off-by: Uri Lublin 
---

The relevant job of the pipeline is:
https://gitlab.freedesktop.org/uril/spice/-/jobs/269735

---
  .gitlab-ci.yml | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 9ce95c00e..43cdb45ac 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -14,12 +14,15 @@ before_script:
  
  makecheck:

script:
+  - mkdir builddir


I would put a comment before this "mkdir" like in spice-gtk.
Other tests (IMHO correctly) run also in-tree so both conditions
are tested.


OK. I'll send a V2.

Thanks,
Uri.




+  - cd builddir
- >
  CFLAGS='-O2 -pipe -g -fsanitize=address -fno-omit-frame-pointer
  -Wframe-larger-than=40920'
  LDFLAGS='-fsanitize=address -lasan'
-./autogen.sh --enable-celt051
+../autogen.sh --enable-celt051
- make
- make -C server check || (cat server/tests/test-suite.log && exit 1)
+  - cd ..
  
  meson-makecheck:

script:


Otherwise fine with it.

Frediano



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

[Spice-devel] [PATCH spice-server V2] gitlab-ci: build out-of-tree too

[Uri Lublin]

One test is enough -- do it in makecheck.

Must git clean first any previous builds.

Signed-off-by: Uri Lublin 
---

Since V1:
   - Add a comment about out-of-tree
   - cleanup previous builds using git clean

pipeline task:
https://gitlab.freedesktop.org/uril/spice/-/jobs/272659

BTW, should we cleanup before/after every task ?
 should we rm -rf builddir ? spice-protocol ?

---
 .gitlab-ci.yml | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 9ce95c00e..2c1f46adf 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -14,12 +14,18 @@ before_script:
 
 makecheck:
   script:
+  # Also check out-of-tree build
+  - git clean -fdx # cleanup after previous builds
+  - git submodule foreach --recursive git clean -fdx
+  - mkdir builddir
+  - cd builddir
   - >
 CFLAGS='-O2 -pipe -g -fsanitize=address -fno-omit-frame-pointer 
-Wframe-larger-than=40920'
 LDFLAGS='-fsanitize=address -lasan'
-./autogen.sh --enable-celt051
+../autogen.sh --enable-celt051
   - make
   - make -C server check || (cat server/tests/test-suite.log && exit 1)
+  - cd ..
 
 meson-makecheck:
   script:
-- 
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-server V2] gitlab-ci: build out-of-tree too

[Uri Lublin]

On 5/1/19 2:30 PM, Frediano Ziglio wrote:


One test is enough -- do it in makecheck.

Must git clean first any previous builds.


No, not at the beginning.
spice-gtk CI script execute multiple test in a single job
so between jobs clean everything.


Well, it fails without it, with the following error messages:
  configure: error: source directory already configured; run "make
 distclean" there first
  configure: error: ../../../subprojects/spice-common/configure failed
 for subprojects/spice-common

https://gitlab.freedesktop.org/uril/spice/-/jobs/271029





Signed-off-by: Uri Lublin 
---

Since V1:
- Add a comment about out-of-tree
- cleanup previous builds using git clean

pipeline task:
https://gitlab.freedesktop.org/uril/spice/-/jobs/272659

BTW, should we cleanup before/after every task ?
  should we rm -rf builddir ? spice-protocol ?


Well, we should clean between, not at the beginning, at
the beginning repository is supposed to came from a git clone.


I see "Fetching changes...", which seems to me like there is no
new clone, but a git fetch.

I'm not sure yet why it works in spice-gtk.

Thanks,
Uri.





---
  .gitlab-ci.yml | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 9ce95c00e..2c1f46adf 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -14,12 +14,18 @@ before_script:
  
  makecheck:

script:
+  # Also check out-of-tree build
+  - git clean -fdx # cleanup after previous builds
+  - git submodule foreach --recursive git clean -fdx


There 2 lines here are useless.


+  - mkdir builddir
+  - cd builddir
- >
  CFLAGS='-O2 -pipe -g -fsanitize=address -fno-omit-frame-pointer
  -Wframe-larger-than=40920'
  LDFLAGS='-fsanitize=address -lasan'
-./autogen.sh --enable-celt051
+../autogen.sh --enable-celt051
- make
- make -C server check || (cat server/tests/test-suite.log && exit 1)
+  - cd ..
  
  meson-makecheck:

script:


Otherwise,

Acked-by: Frediano Ziglio 

Frediano



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

Re: [Spice-devel] [PATCH spice-streaming-agent v2 1/2] Build agent object not statically

[Uri Lublin]

On 5/1/19 4:53 PM, Frediano Ziglio wrote:

Allows to catch possible exception building the object.
Also will allow to more safely handle logger dependency.


The subject confused me a bit, as I thought about Makefile/Linking.
Perhaps change to something like - make agent object not static

Also see below for some comments.



Signed-off-by: Frediano Ziglio 


Ack.


---
  src/concrete-agent.cpp| 11 +++
  src/concrete-agent.hpp|  4 +---
  src/spice-streaming-agent.cpp | 12 +++-
  3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index f94aead..fb1412b 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -25,9 +25,10 @@ static inline unsigned MinorVersion(unsigned version)
  return version & 0xffu;
  }
  
-ConcreteAgent::ConcreteAgent()

+ConcreteAgent::ConcreteAgent(const std::vector 
&options):
+options(options)


Isn't it common to use different names (e.g. opts for the parameter) ?


  {
-options.push_back(ConcreteConfigureOption(nullptr, nullptr));
+this->options.push_back(ConcreteConfigureOption(nullptr, nullptr));


Nit - This too can also be done in main when creating options.

Uri.


  }
  
  bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const

@@ -49,12 +50,6 @@ const ConfigureOption* ConcreteAgent::Options() const
  return static_cast(&options[0]);
  }
  
-void ConcreteAgent::AddOption(const char *name, const char *value)

-{
-// insert before the last {nullptr, nullptr} value
-options.insert(--options.end(), ConcreteConfigureOption(name, value));
-}
-
  void ConcreteAgent::LoadPlugins(const std::string &directory)
  {
  std::string pattern = directory + "/*.so";
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 99dcf54..2c2ebc8 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -26,12 +26,10 @@ struct ConcreteConfigureOption: ConfigureOption
  class ConcreteAgent final : public Agent
  {
  public:
-ConcreteAgent();
+ConcreteAgent(const std::vector &options);
  void Register(const std::shared_ptr& plugin) override;
  const ConfigureOption* Options() const override;
  void LoadPlugins(const std::string &directory);
-// pointer must remain valid
-void AddOption(const char *name, const char *value);
  FrameCapture *GetBestFrameCapture(const std::set& 
codecs);
  private:
  bool PluginVersionIsCompatible(unsigned pluginVersion) const;
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 9507a54..039d628 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -41,8 +41,6 @@
  
  using namespace spice::streaming_agent;
  
-static ConcreteAgent agent;

-
  class FormatMessage : public OutboundMessage
  {
  public:
@@ -231,7 +229,7 @@ static void usage(const char *progname)
  }
  
  static void

-do_capture(StreamPort &stream_port, FrameLog &frame_log)
+do_capture(StreamPort &stream_port, FrameLog &frame_log, ConcreteAgent &agent)
  {
  unsigned int frame_count = 0;
  while (!quit_requested) {
@@ -353,6 +351,8 @@ int main(int argc, char* argv[])
  
  setlogmask(LOG_UPTO(LOG_NOTICE));
  
+std::vector options;

+
  while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL)) != 
-1) {
  switch (opt) {
  case 0:
@@ -371,7 +371,7 @@ int main(int argc, char* argv[])
  usage(argv[0]);
  }
  *p++ = '\0';
-agent.AddOption(optarg, p);
+options.push_back(ConcreteConfigureOption(optarg, p));
  break;
  }
  case OPT_LOG_BINARY:
@@ -401,6 +401,8 @@ int main(int argc, char* argv[])
  register_interrupts();
  
  try {

+ConcreteAgent agent(options);
+
  // register built-in plugins
  MjpegPlugin::Register(&agent);
  
@@ -418,7 +420,7 @@ int main(int argc, char* argv[])

  std::thread cursor_updater{CursorUpdater(&stream_port)};
  cursor_updater.detach();
  
-do_capture(stream_port, frame_log);

+do_capture(stream_port, frame_log, agent);
  }
  catch (std::exception &err) {
  syslog(LOG_ERR, "%s", err.what());



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

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/2] Add support log logging statistics from plugins

[Uri Lublin]

On 5/1/19 4:53 PM, Frediano Ziglio wrote:

Allows the plugins to add information to the log.


Hi,

Looks good.

Don't you need to bump the version ?

Some comments below.



Signed-off-by: Frediano Ziglio 
---
  include/spice-streaming-agent/plugin.hpp |  5 +
  src/concrete-agent.cpp   | 21 +
  src/concrete-agent.hpp   |  8 +++-
  src/frame-log.cpp|  9 +
  src/frame-log.hpp|  2 ++
  src/spice-streaming-agent.cpp|  6 +++---
  6 files changed, 43 insertions(+), 8 deletions(-)

Change since v1:
- better ConcreteAgent interface, removed ugly SetFrameLog

diff --git a/include/spice-streaming-agent/plugin.hpp 
b/include/spice-streaming-agent/plugin.hpp
index 3b265d9..a51f060 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -116,6 +116,11 @@ public:
   * \todo passing options to entry point instead?
   */
  virtual const ConfigureOption* Options() const = 0;
+/*!
+ * Write something in the log.
+ */
+__attribute__ ((format (printf, 2, 3)))
+virtual void LogStat(const char* format, ...) = 0;
  };
  
  typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index fb1412b..336bd09 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -5,13 +5,15 @@
   */
  
  #include 

+#include "concrete-agent.hpp"
+#include "frame-log.hpp"


Why move local .h files to the top ?


+
  #include 
  #include 
  #include 
  #include 
  #include 
-
-#include "concrete-agent.hpp"
+#include 
  
  using namespace spice::streaming_agent;
  
@@ -25,8 +27,9 @@ static inline unsigned MinorVersion(unsigned version)

  return version & 0xffu;
  }
  
-ConcreteAgent::ConcreteAgent(const std::vector &options):

-options(options)
+ConcreteAgent::ConcreteAgent(const std::vector 
&options, FrameLog *logger):
+options(options),
+logger(logger)
  {
  this->options.push_back(ConcreteConfigureOption(nullptr, nullptr));
  }
@@ -140,3 +143,13 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const 
std::setlog_statv(format, ap);
+va_end(ap);
+}
+}
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 2c2ebc8..6d1be35 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -14,6 +14,8 @@
  namespace spice {
  namespace streaming_agent {
  
+class FrameLog;

+
  struct ConcreteConfigureOption: ConfigureOption
  {
  ConcreteConfigureOption(const char *name, const char *value)
@@ -26,16 +28,20 @@ struct ConcreteConfigureOption: ConfigureOption
  class ConcreteAgent final : public Agent
  {
  public:
-ConcreteAgent(const std::vector &options);
+ConcreteAgent(const std::vector &options,
+  FrameLog *logger=nullptr);
  void Register(const std::shared_ptr& plugin) override;
  const ConfigureOption* Options() const override;
  void LoadPlugins(const std::string &directory);
  FrameCapture *GetBestFrameCapture(const std::set& 
codecs);
+__attribute__ ((format (printf, 2, 3)))
+void LogStat(const char* format, ...) override;
  private:
  bool PluginVersionIsCompatible(unsigned pluginVersion) const;
  void LoadPlugin(const std::string &plugin_filename);
  std::vector> plugins;
  std::vector options;
+FrameLog *const logger = nullptr;
  };
  
  }} // namespace spice::streaming_agent

diff --git a/src/frame-log.cpp b/src/frame-log.cpp
index 62fffc3..db6b652 100644
--- a/src/frame-log.cpp
+++ b/src/frame-log.cpp
@@ -52,6 +52,15 @@ void FrameLog::log_stat(const char* format, ...)
  }
  }
  
+void FrameLog::log_statv(const char* format, va_list ap)

+{
+if (log_file) {


Better also check that it's not binary like in log_stat above.

Also as a followup log_stat can call log_statv.

Uri.


+fprintf(log_file, "%" PRIu64 ": ", get_time());
+vfprintf(log_file, format, ap);
+fputc('\n', log_file);
+}
+}
+
  void FrameLog::log_frame(const void* buffer, size_t buffer_size)
  {
  if (log_file) {
diff --git a/src/frame-log.hpp b/src/frame-log.hpp
index 8503345..a104723 100644
--- a/src/frame-log.hpp
+++ b/src/frame-log.hpp
@@ -24,6 +24,8 @@ public:
  
  __attribute__ ((format (printf, 2, 3)))

  void log_stat(const char* format, ...);
+__attribute__ ((format (printf, 2, 0)))
+void log_statv(const char* format, va_list ap);
  void log_frame(const void* buffer, size_t buffer_size);
  
  static uint64_t get_time();

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 039d628..49f5dc4 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -401,15 +401,15 @@ int main(int argc, char* argv[])
  register_interrupts();
  
  try {

-ConcreteAgent agent(options);
+FrameLog frame_log(log_filename, log_binary, log_frames);
+
+

Re: [Spice-devel] [PATCH spice-streaming-agent v3 1/2] Make "agent" object not static

[Uri Lublin]

On 5/2/19 2:13 PM, Frediano Ziglio wrote:

Allows to catch possible exception building the object.
Also will allow to more safely handle logger dependency.

Signed-off-by: Frediano Ziglio 


Acked-by: Uri Lublin 

Uri.


---
  src/concrete-agent.cpp| 11 +++
  src/concrete-agent.hpp|  4 +---
  src/spice-streaming-agent.cpp | 12 +++-
  3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index f94aead..fb1412b 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -25,9 +25,10 @@ static inline unsigned MinorVersion(unsigned version)
  return version & 0xffu;
  }
  
-ConcreteAgent::ConcreteAgent()

+ConcreteAgent::ConcreteAgent(const std::vector 
&options):
+options(options)
  {
-options.push_back(ConcreteConfigureOption(nullptr, nullptr));
+this->options.push_back(ConcreteConfigureOption(nullptr, nullptr));
  }
  
  bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const

@@ -49,12 +50,6 @@ const ConfigureOption* ConcreteAgent::Options() const
  return static_cast(&options[0]);
  }
  
-void ConcreteAgent::AddOption(const char *name, const char *value)

-{
-// insert before the last {nullptr, nullptr} value
-options.insert(--options.end(), ConcreteConfigureOption(name, value));
-}
-
  void ConcreteAgent::LoadPlugins(const std::string &directory)
  {
  std::string pattern = directory + "/*.so";
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 99dcf54..2c2ebc8 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -26,12 +26,10 @@ struct ConcreteConfigureOption: ConfigureOption
  class ConcreteAgent final : public Agent
  {
  public:
-ConcreteAgent();
+ConcreteAgent(const std::vector &options);
  void Register(const std::shared_ptr& plugin) override;
  const ConfigureOption* Options() const override;
  void LoadPlugins(const std::string &directory);
-// pointer must remain valid
-void AddOption(const char *name, const char *value);
  FrameCapture *GetBestFrameCapture(const std::set& 
codecs);
  private:
  bool PluginVersionIsCompatible(unsigned pluginVersion) const;
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 9507a54..039d628 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -41,8 +41,6 @@
  
  using namespace spice::streaming_agent;
  
-static ConcreteAgent agent;

-
  class FormatMessage : public OutboundMessage
  {
  public:
@@ -231,7 +229,7 @@ static void usage(const char *progname)
  }
  
  static void

-do_capture(StreamPort &stream_port, FrameLog &frame_log)
+do_capture(StreamPort &stream_port, FrameLog &frame_log, ConcreteAgent &agent)
  {
  unsigned int frame_count = 0;
  while (!quit_requested) {
@@ -353,6 +351,8 @@ int main(int argc, char* argv[])
  
  setlogmask(LOG_UPTO(LOG_NOTICE));
  
+std::vector options;

+
  while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL)) != 
-1) {
  switch (opt) {
  case 0:
@@ -371,7 +371,7 @@ int main(int argc, char* argv[])
  usage(argv[0]);
  }
  *p++ = '\0';
-agent.AddOption(optarg, p);
+options.push_back(ConcreteConfigureOption(optarg, p));
  break;
  }
  case OPT_LOG_BINARY:
@@ -401,6 +401,8 @@ int main(int argc, char* argv[])
  register_interrupts();
  
  try {

+ConcreteAgent agent(options);
+
  // register built-in plugins
  MjpegPlugin::Register(&agent);
  
@@ -418,7 +420,7 @@ int main(int argc, char* argv[])

  std::thread cursor_updater{CursorUpdater(&stream_port)};
  cursor_updater.detach();
  
-do_capture(stream_port, frame_log);

+do_capture(stream_port, frame_log, agent);
  }
  catch (std::exception &err) {
  syslog(LOG_ERR, "%s", err.what());



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

Re: [Spice-devel] [PATCH spice-streaming-agent v3 2/2] Add support log logging statistics from plugins

[Uri Lublin]

On 5/2/19 2:13 PM, Frediano Ziglio wrote:

Allows the plugins to add information to the log.

Signed-off-by: Frediano Ziglio 


Acked-by: Uri Lublin 

Uri.


---
  include/spice-streaming-agent/plugin.hpp |  7 ++-
  src/concrete-agent.cpp   | 21 +
  src/concrete-agent.hpp   |  8 +++-
  src/frame-log.cpp| 11 ---
  src/frame-log.hpp|  2 ++
  src/spice-streaming-agent.cpp|  6 +++---
  6 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/include/spice-streaming-agent/plugin.hpp 
b/include/spice-streaming-agent/plugin.hpp
index 3b265d9..88cb7f2 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -28,7 +28,7 @@ class FrameCapture;
   * where MM is major and mm is the minor, can be easily expanded
   * using more bits in the future
   */
-enum Constants : unsigned { PluginVersion = 0x100u };
+enum Constants : unsigned { PluginVersion = 0x101u };
  
  enum Ranks : unsigned {

  /// this plugin should not be used
@@ -116,6 +116,11 @@ public:
   * \todo passing options to entry point instead?
   */
  virtual const ConfigureOption* Options() const = 0;
+/*!
+ * Write something in the log.
+ */
+__attribute__ ((format (printf, 2, 3)))
+virtual void LogStat(const char* format, ...) = 0;
  };
  
  typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index fb1412b..336bd09 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -5,13 +5,15 @@
   */
  
  #include 

+#include "concrete-agent.hpp"
+#include "frame-log.hpp"
+
  #include 
  #include 
  #include 
  #include 
  #include 
-
-#include "concrete-agent.hpp"
+#include 
  
  using namespace spice::streaming_agent;
  
@@ -25,8 +27,9 @@ static inline unsigned MinorVersion(unsigned version)

  return version & 0xffu;
  }
  
-ConcreteAgent::ConcreteAgent(const std::vector &options):

-options(options)
+ConcreteAgent::ConcreteAgent(const std::vector 
&options, FrameLog *logger):
+options(options),
+logger(logger)
  {
  this->options.push_back(ConcreteConfigureOption(nullptr, nullptr));
  }
@@ -140,3 +143,13 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const 
std::setlog_statv(format, ap);
+va_end(ap);
+}
+}
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 2c2ebc8..6d1be35 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -14,6 +14,8 @@
  namespace spice {
  namespace streaming_agent {
  
+class FrameLog;

+
  struct ConcreteConfigureOption: ConfigureOption
  {
  ConcreteConfigureOption(const char *name, const char *value)
@@ -26,16 +28,20 @@ struct ConcreteConfigureOption: ConfigureOption
  class ConcreteAgent final : public Agent
  {
  public:
-ConcreteAgent(const std::vector &options);
+ConcreteAgent(const std::vector &options,
+  FrameLog *logger=nullptr);
  void Register(const std::shared_ptr& plugin) override;
  const ConfigureOption* Options() const override;
  void LoadPlugins(const std::string &directory);
  FrameCapture *GetBestFrameCapture(const std::set& 
codecs);
+__attribute__ ((format (printf, 2, 3)))
+void LogStat(const char* format, ...) override;
  private:
  bool PluginVersionIsCompatible(unsigned pluginVersion) const;
  void LoadPlugin(const std::string &plugin_filename);
  std::vector> plugins;
  std::vector options;
+FrameLog *const logger = nullptr;
  };
  
  }} // namespace spice::streaming_agent

diff --git a/src/frame-log.cpp b/src/frame-log.cpp
index 62fffc3..f416cde 100644
--- a/src/frame-log.cpp
+++ b/src/frame-log.cpp
@@ -41,13 +41,18 @@ FrameLog::~FrameLog()
  }
  
  void FrameLog::log_stat(const char* format, ...)

+{
+va_list ap;
+va_start(ap, format);
+log_statv(format, ap);
+va_end(ap);
+}
+
+void FrameLog::log_statv(const char* format, va_list ap)
  {
  if (log_file && !log_binary) {
  fprintf(log_file, "%" PRIu64 ": ", get_time());
-va_list ap;
-va_start(ap, format);
  vfprintf(log_file, format, ap);
-va_end(ap);
  fputc('\n', log_file);
  }
  }
diff --git a/src/frame-log.hpp b/src/frame-log.hpp
index 8503345..a104723 100644
--- a/src/frame-log.hpp
+++ b/src/frame-log.hpp
@@ -24,6 +24,8 @@ public:
  
  __attribute__ ((format (printf, 2, 3)))

  void log_stat(const char* format, ...);
+__attribute__ ((format (printf, 2, 0)))
+void log_statv(const char* format, va_list ap);
  void log_frame(const void* buffer, size_t buffer_size);
  
  static uint64_t get_time();

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 039d628..49f5dc4 100644
--- a/src/spi

[Spice-devel] [PATCH spice-server] build: do not warn about address-of-packed-member

[Uri Lublin]

The gcc warning address-of-packed-member is new and on by
default in gcc 9.

Many of the structures sent over the network are packed
and with unaligned fields.

This breaks the build -- due to -Werror.
Tell gcc to not warn about it.

Signed-off-by: Uri Lublin 
---
 m4/manywarnings.m4 | 1 +
 meson.build| 1 +
 2 files changed, 2 insertions(+)

diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 4f701f4ea..20543d4a4 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -174,6 +174,7 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
 -Wmultichar \
 -Wnarrowing \
 -Wnested-externs \
+-Wno-address-of-packed-member \
 -Wnonnull \
 -Wnonnull-compare \
 -Wnull-dereference \
diff --git a/meson.build b/meson.build
index 93fbdfff9..b8dde96a8 100644
--- a/meson.build
+++ b/meson.build
@@ -42,6 +42,7 @@ spice_server_global_cflags = ['-DSPICE_SERVER_INTERNAL',
   '-Wall',
   '-Wextra',
   '-Wno-sign-compare',
+  '-Wno-address-of-packed-member',
   '-Wno-unused-parameter']
 
 compiler = meson.get_compiler('c')
-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-server] build: do not warn about address-of-packed-member

[Uri Lublin]

On 5/7/19 12:53 PM, Frediano Ziglio wrote:


The gcc warning address-of-packed-member is new and on by
default in gcc 9.

Many of the structures sent over the network are packed
and with unaligned fields.

This breaks the build -- due to -Werror.
Tell gcc to not warn about it.

Signed-off-by: Uri Lublin 


What are the warning exactly for?


https://gcc.gnu.org/gcc-9/changes.html

  -Waddress-of-packed-member, enabled by default, warns about an
unaligned pointer value from the address of a packed member
of a struct or union.



Sure we don't want to fix them?


I think it would not be too hard to overcome this specific warning, by
copying the structures, or sending their content instead of a pointer.
We would still have unaligned access, but not for unaligned pointers.

It's not easy to change the structures themselves.
For example if we change SpiceMigrateDataDisplay to make it aligned,
we likely break migration from older versions.

Uri.




---
  m4/manywarnings.m4 | 1 +
  meson.build| 1 +
  2 files changed, 2 insertions(+)

diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 4f701f4ea..20543d4a4 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -174,6 +174,7 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
  -Wmultichar \
  -Wnarrowing \
  -Wnested-externs \
+-Wno-address-of-packed-member \
  -Wnonnull \
  -Wnonnull-compare \
  -Wnull-dereference \
diff --git a/meson.build b/meson.build
index 93fbdfff9..b8dde96a8 100644
--- a/meson.build
+++ b/meson.build
@@ -42,6 +42,7 @@ spice_server_global_cflags = ['-DSPICE_SERVER_INTERNAL',
'-Wall',
'-Wextra',
'-Wno-sign-compare',
+  '-Wno-address-of-packed-member',
'-Wno-unused-parameter']
  
  compiler = meson.get_compiler('c')


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

Re: [Spice-devel] [PATCH spice-server 2/2] dcc: Avoid usage of not aligned GlzEncDictRestoreData structure

[Uri Lublin]

On 5/8/19 4:06 PM, Frediano Ziglio wrote:

Copy to/from unaligned field to avoid potential unaligned access.
Although it adds a copy it's not in a hot path (migration) and
the structure is pretty small.

Signed-off-by: Frediano Ziglio 


Ack, but see one comment below.

I wrote a similar patch, you sent it faster :)


---
  server/dcc-send.c | 4 +++-
  server/dcc.c  | 3 ++-
  2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index e9b01b38..9fc54046 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -1845,8 +1845,10 @@ static void 
display_channel_marshall_migrate_data(RedChannelClient *rcc,
  memcpy(display_data.pixmap_cache_clients, dcc->priv->pixmap_cache->sync,
 sizeof(display_data.pixmap_cache_clients));
  
+GlzEncDictRestoreData glz_dict_data;


Perhaps move it to the beginning of the function like all the
other local variables.

Uri.


  image_encoders_glz_get_restore_data(encoders, &display_data.glz_dict_id,
-&display_data.glz_dict_data);
+&glz_dict_data);
+display_data.glz_dict_data = glz_dict_data;
  
  /* all data besided the surfaces ref */

  spice_marshaller_add(base_marshaller,
diff --git a/server/dcc.c b/server/dcc.c
index fdb0fbf1..271a466b 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1251,10 +1251,11 @@ bool dcc_handle_message(RedChannelClient *rcc, uint16_t 
type, uint32_t size, voi
  static int dcc_handle_migrate_glz_dictionary(DisplayChannelClient *dcc,
   SpiceMigrateDataDisplay *migrate)
  {
+GlzEncDictRestoreData glz_dict_data = migrate->glz_dict_data;
  return image_encoders_restore_glz_dictionary(&dcc->priv->encoders,
   
red_channel_client_get_client(RED_CHANNEL_CLIENT(dcc)),
   migrate->glz_dict_id,
- &migrate->glz_dict_data);
+ &glz_dict_data);
  }
  
  static bool restore_surface(DisplayChannelClient *dcc, uint32_t surface_id)




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

Re: [Spice-devel] [PATCH spice-server] build: do not warn about address-of-packed-member

[Uri Lublin]

On 5/7/19 12:57 PM, Daniel P. Berrangé wrote:

On Tue, May 07, 2019 at 12:48:55PM +0300, Uri Lublin wrote:

The gcc warning address-of-packed-member is new and on by
default in gcc 9.

Many of the structures sent over the network are packed
and with unaligned fields.

This breaks the build -- due to -Werror.
Tell gcc to not warn about it.


Note that unaligned accesses are genuine bugs so ignoring these
mean spice is broken on certain architectures. sparc, arm7, hppa,
mips in particular will either sigbus or cause traps to kernel space
which are super slow. Even on architectures where it is ok,
you often get a performance degradation.


We'll try to fix those warnings, so currently this patch is postponed.

Note that this is the current state -- it's just that gcc 9 warns
about it.



If you don't care about these architectures that's obviously fine,
but I think the commit message should at least acknowledge that
these are genuine bugs being intentionally ignored.


OK, if we can not fix them all, I'll add this information to the
commit message,

Thanks,
Uri.



Regards,
Daniel



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

Re: [Spice-devel] [PATCH spice-server 01/13] char-device: Remove unused red_char_device_destroy function

[Uri Lublin]

On 5/30/19 5:22 PM, Frediano Ziglio wrote:

g_object_unref is directly used.


OK.

I see there are some comments referring to red_char_device_destroy.
Consider removing those comments or rewriting them.

Uri.



Signed-off-by: Frediano Ziglio 
---
  server/char-device.c | 6 --
  server/char-device.h | 1 -
  2 files changed, 7 deletions(-)

diff --git a/server/char-device.c b/server/char-device.c
index 9ee255664..0f6a29d6f 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -697,12 +697,6 @@ void red_char_device_reset_dev_instance(RedCharDevice *dev,
  g_object_notify(G_OBJECT(dev), "sin");
  }
  
-void red_char_device_destroy(RedCharDevice *char_dev)

-{
-g_return_if_fail(RED_IS_CHAR_DEVICE(char_dev));
-g_object_unref(char_dev);
-}
-
  static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
 int do_flow_control,
 uint32_t 
max_send_queue_size,
diff --git a/server/char-device.h b/server/char-device.h
index 893d3e4b1..7d3ad8b3a 100644
--- a/server/char-device.h
+++ b/server/char-device.h
@@ -160,7 +160,6 @@ typedef struct RedCharDeviceWriteBuffer {
  
  void red_char_device_reset_dev_instance(RedCharDevice *dev,

  SpiceCharDeviceInstance *sin);
-void red_char_device_destroy(RedCharDevice *dev);
  
  /* only one client is supported */

  void red_char_device_migrate_data_marshall(RedCharDevice *dev,



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

Re: [Spice-devel] [PATCH spice-server 2/3] spicevmc: Do not use RedCharDevice pipe items handling

[Uri Lublin]

On 6/1/19 3:14 PM, Frediano Ziglio wrote:

As we don't use any token there's no reason to not queue
directly instead of passing through RedCharDevice.
This will make easier to limit the queue which currently is
unlimited.


Hi,

If we need flow control, how difficult would it be to add support
for tokens ?
( there is a "todo: add flow control" comment in spicevmc ).

Uri.



Signed-off-by: Frediano Ziglio 
---
  server/spicevmc.c | 15 +--
  1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 84bbb73c2..8c41573ae 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -360,29 +360,24 @@ static RedPipeItem 
*spicevmc_chardev_read_msg_from_dev(RedCharDevice *self,
  
  msg_item_compressed = try_compress_lz4(channel, n, msg_item);

  if (msg_item_compressed != NULL) {
-return &msg_item_compressed->base;
+red_channel_client_pipe_add_push(channel->rcc, 
&msg_item_compressed->base);
+return NULL;
  }
  #endif
  stat_inc_counter(channel->out_data, n);
  msg_item->uncompressed_data_size = n;
  msg_item->buf_used = n;
-return &msg_item->base;
-} else {
-channel->pipe_item = msg_item;
+red_channel_client_pipe_add_push(channel->rcc, &msg_item->base);
  return NULL;
  }
+channel->pipe_item = msg_item;
+return NULL;
  }
  
  static void spicevmc_chardev_send_msg_to_client(RedCharDevice *self,

  RedPipeItem *msg,
  RedClient *client)
  {
-RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(self);
-RedVmcChannel *channel = RED_VMC_CHANNEL(vmc->channel);
-
-spice_assert(red_channel_client_get_client(channel->rcc) == client);
-red_pipe_item_ref(msg);
-red_channel_client_pipe_add_push(channel->rcc, msg);
  }
  
  static void red_port_init_item_free(struct RedPipeItem *base)




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

Re: [Spice-devel] [PATCH] Fix compile errors on Linux 32bit system

[Uri Lublin]

On 6/5/19 6:18 AM, Hongzhi.Song wrote:

There are folowing compile errors on Linux 32bit system with -Werror
for gcc.

red-channel.c:207:73: error: format '%x' expects argument of type
'unsigned int', but argument 7 has type 'long unsigned int' [-Werror=format=]
|207| red_channel_debug(self, "thread_id 0x%" G_GSIZE_MODIFIER "x",
~^
self->priv->thread_id);
~^

On 32bit system, #define G_GSIZE_MODIFIER "". But the type of
'self->priv->thread_id' is 'unsigned long int', which should match '%lx'
not '%x'.

So we should recovery the <0x%" G_GSIZE_MODIFIER "x"> to <0x%lx">.
And others files modification are similar to G_GSIZE_MODIFIER.


Indeed in pthreadtypes.h appears:
  typedef unsigned long int pthread_t;

And in zlib.h (as part of struct z_stream_s):
uLongtotal_out; /* total number of bytes output so far */



Signed-off-by: Hongzhi.Song 


Ack, with a minor change below



---
  server/red-channel.c| 6 +++---
  server/red-client.c | 8 
  server/red-replay-qxl.c | 2 +-
  3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 82e5223..11644e4 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -202,7 +202,7 @@ red_channel_constructed(GObject *object)
  {
  RedChannel *self = RED_CHANNEL(object);
  
-red_channel_debug(self, "thread_id 0x%" G_GSIZE_MODIFIER "x", self->priv->thread_id);

+red_channel_debug(self, "thread_id 0x%lx", self->priv->thread_id);
  
  RedChannelClass *klass = RED_CHANNEL_GET_CLASS(self);
  
@@ -473,8 +473,8 @@ void red_channel_remove_client(RedChannel *channel, RedChannelClient *rcc)
  
  if (!pthread_equal(pthread_self(), channel->priv->thread_id)) {

  red_channel_warning(channel,
-"channel->thread_id (0x%" G_GSIZE_MODIFIER "x) != "
-"pthread_self (0x%" G_GSIZE_MODIFIER "x)."
+"channel->thread_id (0x%lx) != "
+"pthread_self (0x%lx)."
  "If one of the threads is != io-thread && != 
vcpu-thread, "
  "this might be a BUG",
  channel->priv->thread_id, pthread_self());
diff --git a/server/red-client.c b/server/red-client.c
index 961b497..81258e1 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -174,8 +174,8 @@ void red_client_migrate(RedClient *client)
  RedChannel *channel;
  
  if (!pthread_equal(pthread_self(), client->thread_id)) {

-spice_warning("client->thread_id (0x%" G_GSIZE_MODIFIER "x) != "
-  "pthread_self (0x%" G_GSIZE_MODIFIER "x)."
+spice_warning("client->thread_id (0x%lx) != "
+  "pthread_self (0x%lx)."
"If one of the threads is != io-thread && != 
vcpu-thread,"
" this might be a BUG",
client->thread_id, pthread_self());
@@ -193,8 +193,8 @@ void red_client_destroy(RedClient *client)
  RedChannelClient *rcc;
  
  if (!pthread_equal(pthread_self(), client->thread_id)) {

-spice_warning("client->thread_id (0x%" G_GSIZE_MODIFIER "x) != "
-  "pthread_self (0x%" G_GSIZE_MODIFIER "x)."
+spice_warning("client->thread_id (0x%lx) != "
+  "pthread_self (0x%lx)."
"If one of the threads is != io-thread && != 
vcpu-thread,"
" this might be a BUG",
client->thread_id,
diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 6d34818..0deb406 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -264,7 +264,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
  exit(1);
  }
  if ((ret = inflate(&strm, Z_NO_FLUSH)) != Z_STREAM_END) {
-spice_error("inflate error %d (disc: %" G_GSSIZE_FORMAT ")",
+spice_error("inflate error %d (disc: %li)",


Everywhere in the project "%[l]d" is used and not "%[l]i".
For consistency, I'd like to change this line to "(disc %ld)".

Thanks,
Uri.

  ret, *size - strm.total_out);
  if (ret == Z_DATA_ERROR) {
  /* last operation may be wrong. since we do the recording



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

Re: [Spice-devel] [PATCH spice-server 1/2] Fix compile warnings on Linux 32bit system

[Uri Lublin]

On 6/12/19 9:30 AM, Frediano Ziglio wrote:

Some typos/improves (just commit message)


Subject: [PATCH spice-server 1/2] Fix compile warnings on Linux 32bit system



Better "Remove compile warnings ..."


Both are OK.




Based on a patch from Hongzhi.Song .

There are following compile errors on Linux 32bit system with -Werror
for gcc.

red-channel.c:207:73: error: format '%x' expects argument of type
'unsigned int', but argument 7 has type 'long unsigned int' [-Werror=format=]
|207| red_channel_debug(self, "thread_id 0x%" G_GSIZE_MODIFIER "x",
~^
self->priv->thread_id);
~^

pthread_t is an opaque type so there it's not easy to get portably


s/portably//

maybe "... so there is no easy way to make the printf
format string portable"



Remove "there"


the printf format string. However the type must be comparable in C
so this (excluding floating point which does not make sense) means
an integral type or a pointer.
Under *BSD this is a pointer so can be converted without loosing
precision to void*.
Under Linux this is a "unsigned long int" type, being Linux LP32 or


It's ILP32, not LP32.


Since it's "unsigned long int" it's the same (IIUC).




LP64 this means that the size of pthread_t is the same as size_t >> so can be 
converted without loosing precision to void*.
Under MingW (the pthread port to Windows) this is a uintptr_t type
that is can be converted without loosing precision to void*.
On any potential future platforms if the integral type is smaller
than a uintptr_t type (which has the same size of void*) the cast
should trigger a warning and if not won't loose precision; the
integral type is unlikely to be bigger than a pointer and likely
the cast would trigger a warning.

The cast on read_binary (red-replay-qxl.c) is safe, "*size" is a
size_t while "strm.total_out" is the number of written bytes in
a buffer which cannot be bigger than a size_t.

Signed-off-by: Frediano Ziglio 


Ack.

Uri.


---
  server/red-channel.c|  8 
  server/red-client.c | 14 +++---
  server/red-replay-qxl.c |  2 +-
  3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 82e522395..e09edacf8 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -202,7 +202,7 @@ red_channel_constructed(GObject *object)
  {
  RedChannel *self = RED_CHANNEL(object);
  
-red_channel_debug(self, "thread_id 0x%" G_GSIZE_MODIFIER "x",

self->priv->thread_id);
+red_channel_debug(self, "thread_id %p", (void*) self->priv->thread_id);
  
  RedChannelClass *klass = RED_CHANNEL_GET_CLASS(self);
  
@@ -473,11 +473,11 @@ void red_channel_remove_client(RedChannel *channel,

RedChannelClient *rcc)
  
  if (!pthread_equal(pthread_self(), channel->priv->thread_id)) {

  red_channel_warning(channel,
-"channel->thread_id (0x%" G_GSIZE_MODIFIER "x)
!= "
-"pthread_self (0x%" G_GSIZE_MODIFIER "x)."
+"channel->thread_id (%p) != "
+"pthread_self (%p)."
  "If one of the threads is != io-thread && !=
  vcpu-thread, "
  "this might be a BUG",
-channel->priv->thread_id, pthread_self());
+(void*) channel->priv->thread_id, (void*)
pthread_self());
  }
  spice_return_if_fail(channel);
  link = g_list_find(channel->priv->clients, rcc);
diff --git a/server/red-client.c b/server/red-client.c
index 961b4970e..a4c79a174 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -174,11 +174,11 @@ void red_client_migrate(RedClient *client)
  RedChannel *channel;
  
  if (!pthread_equal(pthread_self(), client->thread_id)) {

-spice_warning("client->thread_id (0x%" G_GSIZE_MODIFIER "x) != "
-  "pthread_self (0x%" G_GSIZE_MODIFIER "x)."
+spice_warning("client->thread_id (%p) != "
+  "pthread_self (%p)."
"If one of the threads is != io-thread && !=
vcpu-thread,"
" this might be a BUG",
-  client->thread_id, pthread_self());
+  (void*) client->thread_id, (void*) pthread_self());
  }
  FOREACH_CHANNEL_CLIENT(client, rcc) {
  if (red_channel_client_is_connected(rcc)) {
@@ -193,12 +193,12 @@ void red_client_destroy(RedClient *client)
  RedChannelClient *rcc;
  
  if (!pthread_equal(pthread_self(), client->thread_id)) {

-spice_warning("client->thread_id (0x%" G_GSIZE_MODIFIER "x) != "
-  "pthread_self (0x%" G_GSIZE_MODIFIER "x)."
+spice_warning("client->thread_id (%p) != "
+  "pthread_self (%p)."
"If one of the th

Re: [Spice-devel] [PATCH spice-server 2/2] ci: Add test for 32 bit CentOS 7

[Uri Lublin]

On 6/11/19 9:43 PM, Frediano Ziglio wrote:

Make sure the project compile and pass tests without problems
on a 32 bit architecture.




Signed-off-by: Frediano Ziglio 


Ack.

Uri


---
  .gitlab-ci.yml | 22 ++
  1 file changed, 22 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 91ee47edc..7ec5ce0f0 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -94,6 +94,28 @@ makecheck-centos:
- make
- make -C server check || (cat server/tests/test-suite.log && exit 1)
  
+# Same as makecheck job but use a Centos image

+makecheck-centos32:
+  before_script:
+- >
+  yum install git libtool make libasan orc-devel glib-networking
+  yum-utils gcc glib2-devel spice-protocol celt051-devel
+  opus-devel pixman-devel openssl-devel libjpeg-devel
+  libcacard-devel cyrus-sasl-devel lz4-devel
+  gstreamer1-devel gstreamer1-plugins-base-devel
+  git-core pyparsing python-six
+  -y
+- git clone ${CI_REPOSITORY_URL/spice.git/spice-protocol.git}
+- (cd spice-protocol && ./autogen.sh --prefix=/usr && make install)
+  image: i386/centos:latest
+  script:
+  - >
+CFLAGS='-O2 -pipe -g -fsanitize=address -fno-omit-frame-pointer 
-Wframe-larger-than=40920'
+LDFLAGS='-fsanitize=address -lasan'
+./autogen.sh --enable-celt051
+  - make
+  - make -C server check || (cat server/tests/test-suite.log && exit 1)
+
  # Same as makecheck job but use Windows build
  makecheck-windows:
script:



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

Re: [Spice-devel] [PATCH spice-server] red-stream: Fix compilation on Fedora 30 for Windows using MingW

[Uri Lublin]

On 6/12/19 1:53 PM, Frediano Ziglio wrote:

On Windows Fedora 30 reports these errors:

In file included from 
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/crypto.h:29,
  from 
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/bio.h:20,
  from 
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/err.h:21,
  from red-stream.c:31:
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/x509.h:75:1: error: pasting 
"stack_st_" and "(" does not give a valid preprocessing token
  DEFINE_STACK_OF(X509_NAME)
  ^~~
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/x509.h:75:17: error: 
expected ')' before numeric constant
  DEFINE_STACK_OF(X509_NAME)
  ^
...

This is due to missing X509_NAME definition by Windows headers.
Incude missing header in order to make code compile again.

Signed-off-by: Frediano Ziglio 
---
  server/red-stream.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/server/red-stream.c b/server/red-stream.c
index 3057d8bbb..77fed097e 100644
--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -24,6 +24,8 @@
  #include 
  #include 
  #include 
+#else
+#include 


I see X509_NAME is defined in wincrypt.h

I did not follow the include-path but since
the Linux side includes network h-files it
makes sense the windows side will do the same.

Uri.


  #endif
  
  #include 




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

Re: [Spice-devel] [spice v2 2/2] utils: Remove the LL suffix from NSEC_PER_SEC

[Uri Lublin]

On 6/13/19 1:23 PM, Frediano Ziglio wrote:


This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.


Hi,

Wouldn't it be simpler to replace the LL suffix with
a prefix of (uint64_t) ?

-#define NSEC_PER_SEC  10LL
+#define NSEC_PER_SEC  (uint64_t)10

That way there is no worry about overflows.

Uri.




Signed-off-by: Francois Gouget 
---

v2: Cast COMMON_CLIENT_TIMEOUT to an int64_t instead of an uint64_t.


I would personally use signed for all timeouts, specifically also
DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT, MJPEG_WARMUP_TIME and 
RED_STREAM_INPUT_FPS_TIMEOUT.

Beside that patch is good.



v1: https://lists.freedesktop.org/archives/spice-devel/2019-May/049193.html

  server/common-graphics-channel.h | 2 +-
  server/dcc.h | 2 +-
  server/gstreamer-encoder.c   | 2 +-
  server/mjpeg-encoder.c   | 2 +-
  server/utils.h   | 4 ++--
  server/video-stream.c| 4 ++--
  server/video-stream.h| 2 +-
  7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/server/common-graphics-channel.h
b/server/common-graphics-channel.h
index d23f0c695..cccd24d44 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
  
  bool common_channel_client_config_socket(RedChannelClient *rcc);
  
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)

+#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
  
  #define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
  
diff --git a/server/dcc.h b/server/dcc.h

index 76c078bf5..da8697762 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void) G_GNUC_CONST;
  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
  #define CLIENT_PALETTE_CACHE_SIZE 128
  
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)

+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((uint64_t)NSEC_PER_SEC) * 10)
  #define DISPLAY_CLIENT_RETRY_INTERVAL 1 //micro
  
  /* Each drawable can refer to at most 3 images: src, brush and mask */

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5f39ed877..91dd475ac 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -556,7 +556,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
  /* Figure out how many frames to drop to not exceed the current bit
  rate.
   * Use nanoseconds to avoid precision loss.
   */
-uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 *
NSEC_PER_SEC / encoder->bit_rate;
  uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up */
  spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
  encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..d22e7d18b 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
   * are not necessarily related to mis-estimation of the bit rate, and we
   would
   * like to wait till the stream stabilizes.
   */
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((uint64_t)NSEC_PER_SEC) * 3)
  
  /* The compressed buffer initial size. */

  #define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
  
  typedef int64_t red_time_t;
  
-#define NSEC_PER_SEC  10LL

+#define NSEC_PER_SEC  10
  #define NSEC_PER_MILLISEC 100
  #define NSEC_PER_MICROSEC 1000
  
@@ -62,7 +62,7 @@ static inline red_time_t spice_get_monotonic_time_ns(void)

  struct timespec time;
  
  clock_gettime(CLOCK_MONOTONIC, &time);

-return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
+return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
  }
  
  #define MSEC_PER_SEC 1000

diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8b..7a2dca7dd 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -415,8 +415,8 @@ static void display_channel_create_stream(DisplayChannel
*display, Drawable *dra
   * the nearest integer, for instance 24 for 23.976.
   */
  uint64_t duration = drawable->creation_time -
  drawable->first_frame_time;
-if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
- 

Re: [Spice-devel] [PATCH spice-server] red-stream: Fix compilation on Fedora 30 for Windows using MingW

[Uri Lublin]

On 6/13/19 12:57 PM, Frediano Ziglio wrote:

On 6/12/19 1:53 PM, Frediano Ziglio wrote:

On Windows Fedora 30 reports these errors:

In file included from
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/crypto.h:29,
   from
   
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/bio.h:20,
   from
   
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/err.h:21,
   from red-stream.c:31:
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/x509.h:75:1: error:
pasting "stack_st_" and "(" does not give a valid preprocessing token
   DEFINE_STACK_OF(X509_NAME)
   ^~~
/usr/x86_64-w64-mingw32/sys-root/mingw/include/openssl/x509.h:75:17: error:
expected ')' before numeric constant
   DEFINE_STACK_OF(X509_NAME)
   ^
...

This is due to missing X509_NAME definition by Windows headers.
Incude missing header in order to make code compile again.

Signed-off-by: Frediano Ziglio 
---
   server/red-stream.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/server/red-stream.c b/server/red-stream.c
index 3057d8bbb..77fed097e 100644
--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -24,6 +24,8 @@
   #include 
   #include 
   #include 
+#else
+#include 


I see X509_NAME is defined in wincrypt.h

I did not follow the include-path but since
the Linux side includes network h-files it
makes sense the windows side will do the same.

Uri.



Windows and Unix (so Linux) include files for network are quite
different so the #ifdef. I included this header as I noted that
this was done in reds.c and the problem didn't happen so for
coherence I included that specific header.



Acked-by: Uri Lublin 

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

Re: [Spice-devel] [spice v2 2/2] utils: Remove the LL suffix from NSEC_PER_SEC

[Uri Lublin]

On 6/13/19 2:47 PM, Frediano Ziglio wrote:


On 6/13/19 1:23 PM, Frediano Ziglio wrote:


This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.


Hi,

Wouldn't it be simpler to replace the LL suffix with
a prefix of (uint64_t) ?

-#define NSEC_PER_SEC  10LL
+#define NSEC_PER_SEC  (uint64_t)10

That way there is no worry about overflows.

Uri.



You could also use the INT64_C macro for this, but Francois is trying to do
the opposite, not have the constant 64 bit in order to make the compiler able
to decide to use 32 bit arithmetic if it wants.


He tries "to avoid the long vs long long confusion"

I agree it's easier to printf an integer than an int64_t.



Note that "LL" is signed while uint64_t is unsigned, this could change some
math causing some nasty behaviour, I remember once I changed some code "easily"
like that and I got a nice buffer overflow for free.


Yes, you're right. Can be (int64) prefix.

And people say there is no such thing as a free nice buffer overflow :)

Uri.







Signed-off-by: Francois Gouget 
---

v2: Cast COMMON_CLIENT_TIMEOUT to an int64_t instead of an uint64_t.


I would personally use signed for all timeouts, specifically also
DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT, MJPEG_WARMUP_TIME and
RED_STREAM_INPUT_FPS_TIMEOUT.

Beside that patch is good.



v1:
https://lists.freedesktop.org/archives/spice-devel/2019-May/049193.html

   server/common-graphics-channel.h | 2 +-
   server/dcc.h | 2 +-
   server/gstreamer-encoder.c   | 2 +-
   server/mjpeg-encoder.c   | 2 +-
   server/utils.h   | 4 ++--
   server/video-stream.c| 4 ++--
   server/video-stream.h| 2 +-
   7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/server/common-graphics-channel.h
b/server/common-graphics-channel.h
index d23f0c695..cccd24d44 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
   
   bool common_channel_client_config_socket(RedChannelClient *rcc);
   
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)

+#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
   
   #define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
   
diff --git a/server/dcc.h b/server/dcc.h

index 76c078bf5..da8697762 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void)
G_GNUC_CONST;
   #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
   #define CLIENT_PALETTE_CACHE_SIZE 128
   
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)

+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((uint64_t)NSEC_PER_SEC) *
10)
   #define DISPLAY_CLIENT_RETRY_INTERVAL 1 //micro
   
   /* Each drawable can refer to at most 3 images: src, brush and mask */

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5f39ed877..91dd475ac 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -556,7 +556,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
   /* Figure out how many frames to drop to not exceed the current bit
   rate.
* Use nanoseconds to avoid precision loss.
*/
-uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 *
NSEC_PER_SEC / encoder->bit_rate;
   uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up
   */
   spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
   encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..d22e7d18b 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
* are not necessarily related to mis-estimation of the bit rate, and we
would
* like to wait till the stream stabilizes.
*/
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((uint64_t)NSEC_PER_SEC) * 3)
   
   /* The compressed buffer initial size. */

   #define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
   
   typedef int64_t red_time_t;
   
-#define NSEC_PER_SEC  10LL

+#define NSEC_PER_SEC  10
   #define NSEC_PER_MILLISEC 100
   #define NSEC_PER_MICROSEC 1000
   
@@ -62,7 +62,7 @@ static inline red_time_t

Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC

[Uri Lublin]

On 6/15/19 2:59 PM, Frediano Ziglio wrote:


This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.

Signed-off-by: Francois Gouget 


ack for me, waiting Uri to confirm


Honestly, I do not see the value of making NSEC_PER_SEC a 32bit integer.
Most of its usage is in 64bit expressions.

On the other hand I do not see anything wrong with it.

Uri.




---

v3: Turned all the timeout constants with casts to int64_t.

  server/common-graphics-channel.h | 2 +-
  server/dcc.h | 2 +-
  server/gstreamer-encoder.c   | 2 +-
  server/mjpeg-encoder.c   | 2 +-
  server/utils.h   | 4 ++--
  server/video-stream.c| 4 ++--
  server/video-stream.h| 2 +-
  7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/server/common-graphics-channel.h
b/server/common-graphics-channel.h
index d23f0c695..cccd24d44 100644
--- a/server/common-graphics-channel.h
+++ b/server/common-graphics-channel.h
@@ -27,7 +27,7 @@ G_BEGIN_DECLS
  
  bool common_channel_client_config_socket(RedChannelClient *rcc);
  
-#define COMMON_CLIENT_TIMEOUT (NSEC_PER_SEC * 30)

+#define COMMON_CLIENT_TIMEOUT (((int64_t)NSEC_PER_SEC) * 30)
  
  #define TYPE_COMMON_GRAPHICS_CHANNEL common_graphics_channel_get_type()
  
diff --git a/server/dcc.h b/server/dcc.h

index 76c078bf5..c35824d54 100644
--- a/server/dcc.h
+++ b/server/dcc.h
@@ -66,7 +66,7 @@ GType display_channel_client_get_type(void) G_GNUC_CONST;
  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
  #define CLIENT_PALETTE_CACHE_SIZE 128
  
-#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (NSEC_PER_SEC * 10)

+#define DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT (((int64_t)NSEC_PER_SEC) * 10)
  #define DISPLAY_CLIENT_RETRY_INTERVAL 1 //micro
  
  /* Each drawable can refer to at most 3 images: src, brush and mask */

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 6416b688d..f2f1cf61e 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -563,7 +563,7 @@ static void update_next_frame_mm_time(SpiceGstEncoder
*encoder)
  /* Figure out how many frames to drop to not exceed the current bit
  rate.
   * Use nanoseconds to avoid precision loss.
   */
-uint64_t delay_ns = -encoder->vbuffer_free * 8 * NSEC_PER_SEC /
encoder->bit_rate;
+uint64_t delay_ns = ((uint64_t)-encoder->vbuffer_free) * 8 *
NSEC_PER_SEC / encoder->bit_rate;
  uint32_t drops = (delay_ns + period_ns - 1) / period_ns; /* round up */
  spice_debug("drops=%u vbuffer %d/%d", drops, encoder->vbuffer_free,
  encoder->vbuffer_size);
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index 4a02e7c8b..56381b1c7 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -65,7 +65,7 @@ static const int
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
   * are not necessarily related to mis-estimation of the bit rate, and we
   would
   * like to wait till the stream stabilizes.
   */
-#define MJPEG_WARMUP_TIME (NSEC_PER_SEC * 3)
+#define MJPEG_WARMUP_TIME (((int64_t)NSEC_PER_SEC) * 3)
  
  /* The compressed buffer initial size. */

  #define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
diff --git a/server/utils.h b/server/utils.h
index 54bc9d490..4bbd45dff 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -52,7 +52,7 @@ static inline int test_bit(int index, uint32_t val)
  
  typedef int64_t red_time_t;
  
-#define NSEC_PER_SEC  10LL

+#define NSEC_PER_SEC  10
  #define NSEC_PER_MILLISEC 100
  #define NSEC_PER_MICROSEC 1000
  
@@ -62,7 +62,7 @@ static inline red_time_t spice_get_monotonic_time_ns(void)

  struct timespec time;
  
  clock_gettime(CLOCK_MONOTONIC, &time);

-return NSEC_PER_SEC * time.tv_sec + time.tv_nsec;
+return ((uint64_t)NSEC_PER_SEC) * time.tv_sec + time.tv_nsec;
  }
  
  #define MSEC_PER_SEC 1000

diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8b..7a2dca7dd 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -415,8 +415,8 @@ static void display_channel_create_stream(DisplayChannel
*display, Drawable *dra
   * the nearest integer, for instance 24 for 23.976.
   */
  uint64_t duration = drawable->creation_time -
  drawable->first_frame_time;
-if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {
-stream->input_fps = (NSEC_PER_SEC * drawable->frames_count +
duration / 2) / duration;
+if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count /
MAX_FPS) {
+stream->input_fps = (((uint64_t)NSEC_PER_SEC) *
drawable->frames_count + duration / 2) / duration

Re: [Spice-devel] [PATCH spice-server 1/3] Use SPICE_CONTAINEROF to avoid some possible alignment warnings on MIPS

[Uri Lublin]

On 6/3/19 2:22 PM, Frediano Ziglio wrote:

This patch came from some experiments using an emulated MIPS machine.
On such architecture due to not supporting alignment access the
compiler is more strict about conversion complaining with some
pointer casts. Use different conversion to avoid these warnings.

Signed-off-by: Frediano Ziglio 


Ack.

I'm not sure this patch really "fixes" alignment, especially
when the field is the first one == offset is 0, but it makes
the compiler happy and it's correct

Uri.


---
  server/cache-item.tmpl.c |  3 ++-
  server/dcc.c |  4 ++--
  server/image-cache.c |  5 +++--
  server/image-encoders.c  | 26 +-
  server/mjpeg-encoder.c   | 12 ++--
  server/pixmap-cache.c|  2 +-
  6 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/server/cache-item.tmpl.c b/server/cache-item.tmpl.c
index f119a9ee4..8e18f9b1b 100644
--- a/server/cache-item.tmpl.c
+++ b/server/cache-item.tmpl.c
@@ -89,7 +89,8 @@ static int FUNC_NAME(add)(CHANNELCLIENT *channel_client, 
uint64_t id, size_t siz
  channel_client->priv->VAR_NAME(available) -= size;
  SPICE_VERIFY(SPICE_OFFSETOF(RedCacheItem, u.cache_data.lru_link) == 0);
  while (channel_client->priv->VAR_NAME(available) < 0) {
-RedCacheItem *tail = (RedCacheItem 
*)ring_get_tail(&channel_client->priv->VAR_NAME(lru));
+RedCacheItem *tail = 
SPICE_CONTAINEROF(ring_get_tail(&channel_client->priv->VAR_NAME(lru)),
+ RedCacheItem, 
u.cache_data.lru_link);
  if (!tail) {
  channel_client->priv->VAR_NAME(available) += size;
  g_free(item);
diff --git a/server/dcc.c b/server/dcc.c
index acc1ca38c..71d09b77f 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -968,8 +968,8 @@ bool dcc_pixmap_cache_unlocked_add(DisplayChannelClient 
*dcc, uint64_t id,
  NewCacheItem **now;
  
  SPICE_VERIFY(SPICE_OFFSETOF(NewCacheItem, lru_link) == 0);

-if (!(tail = (NewCacheItem *)ring_get_tail(&cache->lru)) ||
-   tail->sync[dcc->priv->id] 
== serial) {
+if (!(tail = SPICE_CONTAINEROF(ring_get_tail(&cache->lru), 
NewCacheItem, lru_link)) ||
+ tail->sync[dcc->priv->id] 
== serial) {
  cache->available += size;
  g_free(item);
  return FALSE;
diff --git a/server/image-cache.c b/server/image-cache.c
index 2ca4d4b67..4881f4d92 100644
--- a/server/image-cache.c
+++ b/server/image-cache.c
@@ -78,7 +78,8 @@ static void image_cache_put(SpiceImageCache *spice_cache, 
uint64_t id, pixman_im
  #ifndef IMAGE_CACHE_AGE
  if (cache->num_items == IMAGE_CACHE_MAX_ITEMS) {
  SPICE_VERIFY(SPICE_OFFSETOF(ImageCacheItem, lru_link) == 0);
-ImageCacheItem *tail = (ImageCacheItem *)ring_get_tail(&cache->lru);
+ImageCacheItem *tail =
+SPICE_CONTAINEROF(ring_get_tail(&cache->lru), ImageCacheItem, 
lru_link);
  spice_assert(tail);
  image_cache_remove(cache, tail);
  }
@@ -133,7 +134,7 @@ void image_cache_reset(ImageCache *cache)
  ImageCacheItem *item;
  
  SPICE_VERIFY(SPICE_OFFSETOF(ImageCacheItem, lru_link) == 0);

-while ((item = (ImageCacheItem *)ring_get_head(&cache->lru))) {
+while ((item = SPICE_CONTAINEROF(ring_get_head(&cache->lru), 
ImageCacheItem, lru_link))) {
  image_cache_remove(cache, item);
  }
  #ifdef IMAGE_CACHE_AGE
diff --git a/server/image-encoders.c b/server/image-encoders.c
index 306c6dca6..d4d486c36 100644
--- a/server/image-encoders.c
+++ b/server/image-encoders.c
@@ -89,7 +89,7 @@ static void image_encoders_release_glz(ImageEncoders *enc);
  static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
  quic_usr_error(QuicUsrContext *usr, const char *fmt, ...)
  {
-EncoderData *usr_data = &(((QuicData *)usr)->data);
+EncoderData *usr_data = &(SPICE_CONTAINEROF(usr, QuicData, usr)->data);
  va_list ap;
  char message_buf[ENCODER_MESSAGE_SIZE];
  
@@ -104,7 +104,7 @@ quic_usr_error(QuicUsrContext *usr, const char *fmt, ...)

  static SPICE_GNUC_NORETURN SPICE_GNUC_PRINTF(2, 3) void
  lz_usr_error(LzUsrContext *usr, const char *fmt, ...)
  {
-EncoderData *usr_data = &(((LzData *)usr)->data);
+EncoderData *usr_data = &(SPICE_CONTAINEROF(usr, LzData, usr)->data);
  va_list ap;
  char message_buf[ENCODER_MESSAGE_SIZE];
  
@@ -233,25 +233,25 @@ static int encoder_usr_more_space(EncoderData *enc_data, uint8_t **io_ptr)
  
  static int quic_usr_more_space(QuicUsrContext *usr, uint32_t **io_ptr, int rows_completed)

  {
-EncoderData *usr_data = &(((QuicData *)usr)->data);
+EncoderData *usr_data = &(SPICE_CONTAINEROF(usr, QuicData, usr)->data);
  return encoder_usr_more_space(usr_data, (uint8_t **)io_ptr) / 
sizeof(uint32_t);
  }
  
  static int lz_usr_more_space(LzUsrContext *usr, uint8_t **io_ptr)

  {
-Encoder

Re: [Spice-devel] [PATCH spice-server 2/3] Remove a warning on MIPS machine

[Uri Lublin]

On 6/3/19 2:22 PM, Frediano Ziglio wrote:

The formula is here to make sure glyph is aligned to 4 bytes so
tell to the compiler to avoid a warning.


What's the warning ?
Is it that the pointer may be unaligned (which is imposible
if you start from an aligned address) ?

Looks complicated to me, especially since after the
SPICE_ALIGNED_CAST there is another (the "real") cast.

On the other hand it doesn't break anything.



Signed-off-by: Frediano Ziglio 


Ack.


---
  server/red-parse-qxl.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index afae94316..89b61c06f 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -941,9 +941,9 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, 
int group_id,
  spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]);
  memcpy(glyph->data, start->data, glyph_size);
  start = (QXLRasterGlyph*)(&start->data[glyph_size]);
-glyph = (SpiceRasterGlyph*)
+glyph = (SpiceRasterGlyph*) SPICE_ALIGNED_CAST(uint32_t*,
  (((uint8_t *)glyph) +
- SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4));
+ SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4)));
  }
  
  if (free_data) {




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

Re: [Spice-devel] [PATCH spice-server 2/3] Remove a warning on MIPS machine

[Uri Lublin]

On 6/18/19 8:03 PM, Frediano Ziglio wrote:




On 6/3/19 2:22 PM, Frediano Ziglio wrote:

The formula is here to make sure glyph is aligned to 4 bytes so
tell to the compiler to avoid a warning.


What's the warning ?


Trying to reproduce but the updated environment it's slow like
hell (it's more than 30 minutes that it's compiling 3 files).



The warning is

red-parse-qxl.c: In function ‘red_get_string’:
red-parse-qxl.c:949:17: error: cast increases required alignment of target type 
[-Werror=cast-align]
  glyph = (SpiceRasterGlyph*)
  ^



Is it that the pointer may be unaligned (which is imposible
if you start from an aligned address) ?



Yes, was an alignment warning


Looks complicated to me, especially since after the
SPICE_ALIGNED_CAST there is another (the "real") cast.



Maybe with an additional

@@ -941,7 +941,7 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots,
int group_id,
  spice_assert(glyph_size <= (char*) end - (char*) &start->data[0]);
  memcpy(glyph->data, start->data, glyph_size);
  start = (QXLRasterGlyph*)(&start->data[glyph_size]);
-glyph = (SpiceRasterGlyph*) SPICE_ALIGNED_CAST(uint32_t*,
+glyph = SPICE_ALIGNED_CAST(SpiceRasterGlyph*,
  (((uint8_t *)glyph) +
   SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4)));
  }


would work too. Not sure how long it's going to take to test it.



This works too.


That's better, thanks.



Signed-off-by: Frediano Ziglio 


Ack.


Uri.




---
   server/red-parse-qxl.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index afae94316..89b61c06f 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -941,9 +941,9 @@ static SpiceString *red_get_string(RedMemSlotInfo
*slots, int group_id,
   spice_assert(glyph_size <= (char*) end - (char*)
   &start->data[0]);
   memcpy(glyph->data, start->data, glyph_size);
   start = (QXLRasterGlyph*)(&start->data[glyph_size]);
-glyph = (SpiceRasterGlyph*)
+glyph = (SpiceRasterGlyph*) SPICE_ALIGNED_CAST(uint32_t*,
   (((uint8_t *)glyph) +
- SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4));
+ SPICE_ALIGN(sizeof(SpiceRasterGlyph) + glyph_size, 4)));
   }
   
   if (free_data) {






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


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

Re: [Spice-devel] [PATCH spice-server 3/3] jpeg: Support big endian machines

[Uri Lublin]

On 6/3/19 2:22 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 
---
  server/jpeg-encoder.c  |  7 +++
  server/mjpeg-encoder.c | 21 +++--
  2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/server/jpeg-encoder.c b/server/jpeg-encoder.c
index 269ed8aa7..1ff7e2506 100644
--- a/server/jpeg-encoder.c
+++ b/server/jpeg-encoder.c
@@ -118,6 +118,7 @@ static void convert_RGB16_to_RGB24(void *line, int width, 
uint8_t **out_line)
  
  for (x = 0; x < width; x++) {

 uint16_t pixel = *src_line++;
+   pixel = GUINT16_FROM_LE(pixel);
 *out_pix++ = ((pixel >> 7) & 0xf8) | ((pixel >> 12) & 0x7);
 *out_pix++ = ((pixel >> 2) & 0xf8) | ((pixel >> 7) & 0x7);
 *out_pix++ = ((pixel << 3) & 0xf8) | ((pixel >> 2) & 0x7);
@@ -153,9 +154,15 @@ static void convert_BGRX32_to_RGB24(void *line, int width, 
uint8_t **out_line)
  
  for (x = 0; x < width; x++) {

  uint32_t pixel = *src_line++;


If you want, you can keep it consistent with above with
pixel = GUINT32_FROM_LE(pixel);

(Another one below).

Otherwise, the patch looks good to me.

Uri.

+#ifndef WORDS_BIGENDIAN
  *out_pix++ = (pixel >> 16) & 0xff;
  *out_pix++ = (pixel >> 8) & 0xff;
  *out_pix++ = pixel & 0xff;
+#else
+*out_pix++ = (pixel >>  8) & 0xff;
+*out_pix++ = (pixel >> 16) & 0xff;
+*out_pix++ = (pixel >> 24) & 0xff;
+#endif
  }
  }
  
diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c

index e629adae4..e293b1b6e 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -70,6 +70,16 @@ static const int 
mjpeg_quality_samples[MJPEG_QUALITY_SAMPLE_NUM] = {20, 30, 40,
  /* The compressed buffer initial size. */
  #define MJPEG_INITIAL_BUFFER_SIZE (32 * 1024)
  
+#ifdef JCS_EXTENSIONS

+#  ifndef WORDS_BIGENDIAN
+#define JCS_EXT_LE_BGRX JCS_EXT_BGRX
+#define JCS_EXT_LE_BGR JCS_EXT_BGR
+#  else
+#define JCS_EXT_LE_BGRX JCS_EXT_XRGB
+#define JCS_EXT_LE_BGR JCS_EXT_RGB
+#  endif
+#endif
+
  enum {
  MJPEG_QUALITY_EVAL_TYPE_SET,
  MJPEG_QUALITY_EVAL_TYPE_UPGRADE,
@@ -232,15 +242,22 @@ static void pixel_rgb24bpp_to_24(void *src_ptr, uint8_t 
*dest)
  static void pixel_rgb32bpp_to_24(void *src, uint8_t *dest)
  {
  uint32_t pixel = *(uint32_t *)src > +#ifndef WORDS_BIGENDIAN
  *dest++ = (pixel >> 16) & 0xff;
  *dest++ = (pixel >>  8) & 0xff;
  *dest++ = (pixel >>  0) & 0xff;
+#else
+*dest++ = (pixel >>  8) & 0xff;
+*dest++ = (pixel >> 16) & 0xff;
+*dest++ = (pixel >> 24) & 0xff;
+#endif
  }
  #endif
  
  static void pixel_rgb16bpp_to_24(void *src, uint8_t *dest)

  {
  uint16_t pixel = *(uint16_t *)src;
+pixel = GUINT16_FROM_LE(pixel);
  *dest++ = ((pixel >> 7) & 0xf8) | ((pixel >> 12) & 0x7);
  *dest++ = ((pixel >> 2) & 0xf8) | ((pixel >> 7) & 0x7);
  *dest++ = ((pixel << 3) & 0xf8) | ((pixel >> 2) & 0x7);
@@ -753,7 +770,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
  case SPICE_BITMAP_FMT_RGBA:
  encoder->bytes_per_pixel = 4;
  #ifdef JCS_EXTENSIONS
-encoder->cinfo.in_color_space   = JCS_EXT_BGRX;
+encoder->cinfo.in_color_space   = JCS_EXT_LE_BGRX;
  encoder->cinfo.input_components = 4;
  #else
  encoder->pixel_converter = pixel_rgb32bpp_to_24;
@@ -766,7 +783,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder *encoder,
  case SPICE_BITMAP_FMT_24BIT:
  encoder->bytes_per_pixel = 3;
  #ifdef JCS_EXTENSIONS
-encoder->cinfo.in_color_space = JCS_EXT_BGR;
+encoder->cinfo.in_color_space = JCS_EXT_LE_BGR;
  #else
  encoder->pixel_converter = pixel_rgb24bpp_to_24;
  #endif



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

Re: [Spice-devel] [PATCH spice-server] dcc-send: Check some constants at compile time

[Uri Lublin]

On 6/21/19 12:45 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 


Ack.

Uri.


---
  server/dcc-send.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index ea2128e27..b14619a2b 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -1834,7 +1834,7 @@ static void 
display_channel_marshall_migrate_data(RedChannelClient *rcc,
  spice_marshaller_add_uint32(base_marshaller, 
SPICE_MIGRATE_DATA_DISPLAY_VERSION);
  
  spice_assert(dcc->priv->pixmap_cache);

-spice_assert(MIGRATE_DATA_DISPLAY_MAX_CACHE_CLIENTS == 4 &&
+SPICE_VERIFY(MIGRATE_DATA_DISPLAY_MAX_CACHE_CLIENTS == 4 &&
   MIGRATE_DATA_DISPLAY_MAX_CACHE_CLIENTS == MAX_CACHE_CLIENTS);
  
  display_data.message_serial = red_channel_client_get_message_serial(rcc);




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

Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC

[Uri Lublin]

On 6/25/19 7:38 AM, Francois Gouget wrote:

On Mon, 17 Jun 2019, Uri Lublin wrote:


On 6/15/19 2:59 PM, Frediano Ziglio wrote:


This constant fits in a 32 bit signed integer so it does not need the
suffix. However some of the derived constants don't so use an uint64_t
cast to avoid the long vs long long confusion (such as in print
statements).
Also some of the expressions these constants are used in may overflow so
perform the appropriate casts in those locations. This makes it clearer
that these operations are 64 bit.

Signed-off-by: Francois Gouget 


ack for me, waiting Uri to confirm


Honestly, I do not see the value of making NSEC_PER_SEC a 32bit integer.
Most of its usage is in 64bit expressions.


It's not a 32 bit vs. 64 bit issue. It's a long vs. long long one.

Whenever NSEC_PER_MILLISEC or NSEC_PER_SEC are used in a spice_debug()
parameter one cannot use %u or %lu as the format. But not so for
NSEC_PER_MICROSEC or MSEC_PER_SEC. This is inconsistent so that timing
traces require lots of guessing and recompilations.


When the variable is 64 bit, you can use a 64bit macro for printing,
like PRId64.

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

Re: [Spice-devel] [spice v3] utils: Remove the LL suffix from NSEC_PER_SEC

[Uri Lublin]

On 6/25/19 7:35 PM, Francois Gouget wrote:

On Tue, 25 Jun 2019, Frediano Ziglio wrote:
[...]

 uint64_t foo = 1234;
 spice_debug("foo=%" PRId64, foo / NSEC_PER_SEC);

[...]

If you assume long long == 64 bit should not be a big problem
although you can still have the warning.


So, we all agree that the LL suffix is not good.
And we all agree that this patch fixes this issue.

Also if NSEC_PER_SEC is changed to be of type int64_t, then
there is no warning here.

So, the discussion is if the issue can be fixed differently,
with a prefix of (int64_t) or with INT64_C.


[...]



I'd very much prefer the cast to be in the expression rather than hidden
in some far away macro.


Is that true even if the cast is needed in all the expressions
that use this constant ?

Thanks,
Uri.



For instance:

 int frames_count;
 ...

 if (duration > NSEC_PER_SEC * drawable->frames_count / MAX_FPS) {

Anyone checking this calculation would think that there is a risk of
overflow. And it's only when tracing NSEC_PER_SEC to utils.h that they
would discover that NSEC_PER_SEC forces 64 bit calculation.

At least this form is clear right away:

 if (duration > ((uint64_t)NSEC_PER_SEC) * drawable->frames_count / 
MAX_FPS) {


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

Re: [Spice-devel] [RFC spice-server 1/2] streaming: Restart streams on video-codec changes

[Uri Lublin]

On 6/27/19 11:40 AM, Frediano Ziglio wrote:


Interrupt/restart the video streams when the user changes the
preferred video-codecs (dcc_handle_preferred_video_codec_type) or when
the host admin updates the list of video-codecs allowed
(display_channel_set_video_codecs)


Hi,

This patch only stops the video stream.
A new stream will be started automatically according to spice rules
(video-stream detection).


---
  server/dcc.c | 2 ++
  server/display-channel.c | 2 ++
  server/video-stream.c| 5 +
  3 files changed, 9 insertions(+)

diff --git a/server/dcc.c b/server/dcc.c
index a94e27e8..0deeed88 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1201,6 +1201,8 @@ static int
dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,

  /* New client preference */
  dcc_update_preferred_video_codecs(dcc);
+video_stream_detach_and_stop(DCC_TO_DC(dcc));
+
  return TRUE;
  }

diff --git a/server/display-channel.c b/server/display-channel.c
index 071c0140..4b8e6e90 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -255,6 +255,8 @@ void display_channel_set_video_codecs(DisplayChannel
*display, GArray *video_cod
  g_clear_pointer(&display->priv->video_codecs, g_array_unref);
  display->priv->video_codecs = g_array_ref(video_codecs);
  g_object_notify(G_OBJECT(display), "video-codecs");
+
+video_stream_detach_and_stop(display);
  }

  GArray *display_channel_get_video_codecs(DisplayChannel *display)
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8..f227713b 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -925,6 +925,11 @@ void video_stream_detach_and_stop(DisplayChannel
*display)
  RingItem *stream_item;

  spice_debug("trace");
+
+if (!ring_is_initialized(&display->priv->streams)) {
+return;
+}
+


If this happens I would consider a program error and I would abort().


  while ((stream_item = ring_get_head(&display->priv->streams))) {


When the ring is empty, ring_get_head returns NULL, and the
while loop exits immediately. So this case is already covered.
(It also aborts (spice_assert) if the the ring is "not initialized")

Uri.


  VideoStream *stream = SPICE_CONTAINEROF(stream_item, VideoStream,
  link);



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



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

Re: [Spice-devel] [PATCH spice-server] display-channel: Initialise variable as soon as possible

[Uri Lublin]

Hi

Suggestions:
  Initialise -> Initialize  (?) (Is this British vs American ?)
  variable -> prev (?)

On 6/27/19 12:05 PM, Frediano Ziglio wrote:

Avoids to have not initialised variables before constructed is called.


I think this line can be removed, the next line explains what's needed.


This avoid potentially memory errors while setting properties.

Signed-off-by: Frediano Ziglio 


Ack.

Uri.


---
  server/display-channel.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 7ddd44c14..f99fd8faf 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2277,12 +2277,17 @@ display_channel_init(DisplayChannel *self)
  self->priv = g_new0(DisplayChannelPrivate, 1);
  self->priv->image_compression = SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
  self->priv->pub = self;
+self->priv->renderer = RED_RENDERER_INVALID;
+self->priv->stream_video = SPICE_STREAM_VIDEO_OFF;
  
  image_encoder_shared_init(&self->priv->encoder_shared_data);
  
  ring_init(&self->priv->current_list);

  drawables_init(self);
  self->priv->image_surfaces.ops = &image_surfaces_ops;
+
+image_cache_init(&self->priv->image_cache);
+display_channel_init_video_streams(self);
  }
  
  static void

@@ -2295,8 +2300,6 @@ display_channel_constructed(GObject *object)
  
  spice_assert(self->priv->video_codecs);
  
-self->priv->renderer = RED_RENDERER_INVALID;

-
  stat_init(&self->priv->add_stat, "add", CLOCK_THREAD_CPUTIME_ID);
  stat_init(&self->priv->exclude_stat, "exclude", CLOCK_THREAD_CPUTIME_ID);
  stat_init(&self->priv->__exclude_stat, "__exclude", 
CLOCK_THREAD_CPUTIME_ID);
@@ -2308,9 +2311,6 @@ display_channel_constructed(GObject *object)
"add_to_cache", TRUE);
  stat_init_counter(&self->priv->non_cache_counter, reds, stat,
"non_cache", TRUE);
-image_cache_init(&self->priv->image_cache);
-self->priv->stream_video = SPICE_STREAM_VIDEO_OFF;
-display_channel_init_video_streams(self);
  
  red_channel_set_cap(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);

  red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_COMPRESSION);



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

Re: [Spice-devel] [PATCH spice-server] display-channel: Remove some useless function declaration

[Uri Lublin]

On 6/27/19 12:05 PM, Frediano Ziglio wrote:

"image_surfaces_get" and "drawables_init" are already defined
in the same file earlier.

Signed-off-by: Frediano Ziglio 

Ack.

Uri.


---
  server/display-channel.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index f99fd8faf..4677c2619 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2263,8 +2263,6 @@ DisplayChannel* display_channel_new(RedsState *reds,
  return display;
  }
  
-static SpiceCanvas *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t surface_id);

-static void drawables_init(DisplayChannel *display);
  static void
  display_channel_init(DisplayChannel *self)
  {



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

Re: [Spice-devel] [RFC spice-server 1/2] streaming: Restart streams on video-codec changes

[Uri Lublin]

On 6/27/19 12:19 PM, Kevin Pouget wrote:

Hello Uri,

On Thu, Jun 27, 2019 at 11:04 AM Uri Lublin  wrote:


On 6/27/19 11:40 AM, Frediano Ziglio wrote:


Interrupt/restart the video streams when the user changes the
preferred video-codecs (dcc_handle_preferred_video_codec_type) or when
the host admin updates the list of video-codecs allowed
(display_channel_set_video_codecs)


Hi,

This patch only stops the video stream.
A new stream will be started automatically according to spice rules
(video-stream detection).


indeed, I wasn't sure about the wording, because the purpose of the
patch is to trigger a stream restart, it's not to stop it, so I don't
know which word to put in the commit title! (I'll add your sentence in
the body anyway to clarify)


Yes, the title is good -- it states the purpose of this patch.
Thanks for adding the additional sentence to the body.

Uri.




---
   server/dcc.c | 2 ++
   server/display-channel.c | 2 ++
   server/video-stream.c| 5 +
   3 files changed, 9 insertions(+)

diff --git a/server/dcc.c b/server/dcc.c
index a94e27e8..0deeed88 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1201,6 +1201,8 @@ static int
dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,

   /* New client preference */
   dcc_update_preferred_video_codecs(dcc);
+video_stream_detach_and_stop(DCC_TO_DC(dcc));
+
   return TRUE;
   }

diff --git a/server/display-channel.c b/server/display-channel.c
index 071c0140..4b8e6e90 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -255,6 +255,8 @@ void display_channel_set_video_codecs(DisplayChannel
*display, GArray *video_cod
   g_clear_pointer(&display->priv->video_codecs, g_array_unref);
   display->priv->video_codecs = g_array_ref(video_codecs);
   g_object_notify(G_OBJECT(display), "video-codecs");
+
+video_stream_detach_and_stop(display);
   }

   GArray *display_channel_get_video_codecs(DisplayChannel *display)
diff --git a/server/video-stream.c b/server/video-stream.c
index 4ac25af8..f227713b 100644
--- a/server/video-stream.c
+++ b/server/video-stream.c
@@ -925,6 +925,11 @@ void video_stream_detach_and_stop(DisplayChannel
*display)
   RingItem *stream_item;

   spice_debug("trace");
+
+if (!ring_is_initialized(&display->priv->streams)) {
+return;
+}
+


If this happens I would consider a program error and I would abort().


   while ((stream_item = ring_get_head(&display->priv->streams))) {


When the ring is empty, ring_get_head returns NULL, and the
while loop exits immediately. So this case is already covered.
(It also aborts (spice_assert) if the the ring is "not initialized")


yes, it was a bug that the stream was uninitialized at this stage;
hence ring_is_empty was aborting the execution

I will let video_stream_detach_and_stop untouched


thanks,

Kevin


   VideoStream *stream = SPICE_CONTAINEROF(stream_item, VideoStream,
   link);



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





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

Re: [Spice-devel] [PATCH spice-server] display-channel: Initialise variable as soon as possible

[Uri Lublin]

On 6/27/19 12:51 PM, Frediano Ziglio wrote:


Hi

Suggestions:
Initialise -> Initialize  (?) (Is this British vs American ?)


Yes, US/UK :-)


variable -> prev (?)



"Initialize prev as soon as possible" ?


Sorry, I meant "priv"

(Was "affected" by rings, after reviewing Kevin's patch ;-)

It's not important, just a minor suggestion.

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

Re: [Spice-devel] [PATCH spice-protocol 2/3] qxl_dev: Fix alignment for QXLReleaseInfo

[Uri Lublin]

On 5/13/19 12:45 PM, Frediano Ziglio wrote:

Do not declare the structure as aligned.
The start/end-packed.h headers affects only MingW or Microsoft
compilers.


They also define/undef SPICE_ATTR_PACKED which affects Linux.

BTW, if we define PACKED as ALIGNED(1) does that work for Windows
too (I mean instead of pragma pack) ?


To have unaligned structure with GCC compiler you have
to use SPICE_ATTR_PACKED. This way the definition are the same for
all compiler.



This structure is used in a lot of QXL structures which are not
aligned causing to have an aligned structure to be potentially
unaligned.
As this structure has no holes this change does not make any size
change using any compiler.


For this reason, this change looks safe to me.


The change will only change the alignment from 4/8 to 1.
This could affect structures containing this union however beside
packed structure in qxl_dev.h (which are not affected) there are no
other usages as such by spice-gtk, Qemu or spice-server.


It is used in Qemu, in hw/display/qxl.h, struct PCIQXLDevice.
Also it's used in spice-server and likely QXL drivers too.

Uri.



Signed-off-by: Frediano Ziglio 
---
  spice/qxl_dev.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spice/qxl_dev.h b/spice/qxl_dev.h
index a9cc4f4..659f930 100644
--- a/spice/qxl_dev.h
+++ b/spice/qxl_dev.h
@@ -275,7 +275,7 @@ typedef struct SPICE_ATTR_ALIGNED(4) SPICE_ATTR_PACKED 
QXLRam {
  
  } QXLRam;
  
-typedef union QXLReleaseInfo {

+typedef union SPICE_ATTR_PACKED QXLReleaseInfo {
  uint64_t id;  // in
  uint64_t next;// out
  } QXLReleaseInfo;



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

Re: [Spice-devel] [PATCH] gstreamer-encoder: fix compiler warning with Fedora 30

[Uri Lublin]

On 7/1/19 6:19 PM, Frediano Ziglio wrote:


Fedora 30 / gcc 9.1.1 20190503 (Red Hat 9.1.1-1) fails to build
because of this error/warning:


gstreamer-encoder.c: In function 'set_video_bit_rate':
gstreamer-encoder.c:518:17: error: taking the absolute value of unsigned
type 'uint64_t' {aka 'long unsigned int'} has no effect
[-Werror=absolute-value]
  518 | } else  if (abs(bit_rate - encoder->video_bit_rate) >
  encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
  | ^~~
gstreamer-encoder.c:518:17: error: absolute value function 'abs' given an
argument of type 'uint64_t' {aka 'long unsigned int'}


This patches solves these two warnings:

1) cast the substraction to a signed type (int64_t instead of
uint64_t) to preserve the operation meaning;

2) use the long int labs() instead of the int version abs() to avoid
data trunction.


Not working for all platform we support, maybe a

static inline int64_t i64abs(int64_t value)
{
 if (sizeof(int) == sizeof(value)) {
 return (int64_t) abs((int) value);
 }
 if (sizeof(long int) == sizeof(value)) {
 return (int64_t) labs((long int) value);
 }
 return (int64_t) llabs((long long int) value);
}


Or a simpler
return (value >= 0) ? value : -value;

Uri.




---

resending this patch with the comments addressed

---
  server/gstreamer-encoder.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 6416b688..da73c5ee 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -515,7 +515,7 @@ static void set_video_bit_rate(SpiceGstEncoder *encoder,
uint64_t bit_rate)
  encoder->video_bit_rate = bit_rate;
  set_gstenc_bitrate(encoder);

-} else  if (abs(bit_rate - encoder->video_bit_rate) >
encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
+} else  if (labs((int64_t)(bit_rate - encoder->video_bit_rate)) >
encoder->video_bit_rate * SPICE_GST_VIDEO_BITRATE_MARGIN) {
  encoder->video_bit_rate = bit_rate;
  set_pipeline_changes(encoder, SPICE_GST_VIDEO_PIPELINE_BITRATE);
  }


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



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

[Spice-devel] [spice-server PATCH 3/3] typo: fix the spelling of precede

[Uri Lublin]

---
 server/glz-encoder-dict.c  | 2 +-
 server/gstreamer-encoder.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/glz-encoder-dict.c b/server/glz-encoder-dict.c
index c3c4606c6..7bf16bfb2 100644
--- a/server/glz-encoder-dict.c
+++ b/server/glz-encoder-dict.c
@@ -410,7 +410,7 @@ static WindowImage 
*glz_dictionary_window_get_new_head(SharedDictionary *dict, i
 GLZ_ASSERT(dict->cur_usr, dict->window.used_segs_head != 
NULL_IMAGE_SEG_ID);
 GLZ_ASSERT(dict->cur_usr, dict->window.used_segs_tail != 
NULL_IMAGE_SEG_ID);
 
-// used_segs_head is the latest logical head (the physical head may 
preceed it)
+// used_segs_head is the latest logical head (the physical head may 
precede it)
 cur_head = dict->window.segs[dict->window.used_segs_head].image;
 cur_win_size = dict->window.segs[dict->window.used_segs_tail].pixels_num +
 dict->window.segs[dict->window.used_segs_tail].pixels_so_far -
diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 6416b688d..4718274d3 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -666,7 +666,7 @@ static void set_bit_rate(SpiceGstEncoder *encoder, uint64_t 
bit_rate)
 encoder->vbuffer_size = new_size;
 update_next_frame_mm_time(encoder);
 
-/* Frames preceeding the bit rate change are not relevant to the current
+/* Frames preceding the bit rate change are not relevant to the current
  * situation anymore.
  */
 encoder->stat_first = encoder->history_last;
-- 
2.21.0

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

[Spice-devel] [spice-server PATCH 1/3] typo: dcc: fix the spelling of unknown

[Uri Lublin]

---

Is  "Client has sent an unknown ..."  better ?
Or  "Client sent an unknown ... "

---
 server/dcc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/dcc.c b/server/dcc.c
index 71d09b77f..bd393e040 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1177,7 +1177,7 @@ static int 
dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
 
 if (video_codec < SPICE_VIDEO_CODEC_TYPE_MJPEG ||
 video_codec >= SPICE_VIDEO_CODEC_TYPE_ENUM_END) {
-spice_debug("Client has sent unknow video-codec (value %d at index 
%d). "
+spice_debug("Client has sent unknown video-codec (value %d at 
index %d). "
 "Ignoring as server can't handle it",
  video_codec, i);
 continue;
-- 
2.21.0

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

[Spice-devel] [spice-server PATCH 0/3] 3 spelling fixes

[Uri Lublin]

unknow-> unknown
garanteed -> guaranteed 
preceed   -> precede

Uri Lublin (3):
  typo: dcc: fix the spelling of unknown
  typo: image-encoders: fix the spelling of guaranteed
  typo: fix the spelling of precede

 server/dcc.c   | 2 +-
 server/glz-encoder-dict.c  | 2 +-
 server/gstreamer-encoder.c | 2 +-
 server/image-encoders.c| 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.21.0

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

[Spice-devel] [spice-server PATCH 2/3] typo: image-encoders: fix the spelling of guaranteed

[Uri Lublin]

---
 server/image-encoders.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/image-encoders.c b/server/image-encoders.c
index d4d486c36..81dc076a8 100644
--- a/server/image-encoders.c
+++ b/server/image-encoders.c
@@ -217,7 +217,7 @@ static void encoder_data_reset(EncoderData *data)
 }
 
 /* Allocate more space for compressed buffer.
- * The pointer returned in io_ptr is garanteed to be aligned to 4 bytes.
+ * The pointer returned in io_ptr is guaranteed to be aligned to 4 bytes.
  */
 static int encoder_usr_more_space(EncoderData *enc_data, uint8_t **io_ptr)
 {
-- 
2.21.0

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

[Spice-devel] [spice-server PATCH] dcc-send: remove useless pipe_item assignment pipe_item

[Uri Lublin]

In red_pipe_replace_rendered_drawables_with_images, the
value of pipe_item is re-written on the next iteration.

Since a78a7d251042892182b158650291d19a85bbd6b1 pipe_item
is no longer used to control the loop.

Found by Covscan.

Signed-off-by: Uri Lublin 
---
 server/dcc-send.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index b14619a2b..565a79f33 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -753,7 +753,6 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 
 spice_assert(image);
 
red_channel_client_pipe_remove_and_release_pos(RED_CHANNEL_CLIENT(dcc), l);
-pipe_item = &image->base;
 }
 }
 
-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-server] fixup! dcc-send: remove useless pipe_item assignment pipe_item

[Uri Lublin]

On 7/4/19 12:33 PM, Frediano Ziglio wrote:

Remove use-after-free introduced by a78a7d251042892182b158650291d19a85bbd6b1
---
  server/dcc-send.c | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 565a79f33..4582e3545 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -725,7 +725,6 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
  RedPipeItem *pipe_item = l->data;
  Drawable *drawable;
  RedDrawablePipeItem *dpi;
-RedImageItem *image;
  
  if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)

  continue;
@@ -745,14 +744,16 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
  continue;
  }
  
-image = dcc_add_surface_area_image(dcc, drawable->red_drawable->surface_id,

-   &drawable->red_drawable->bbox, l, 
TRUE);
+dcc_add_surface_area_image(dcc, drawable->red_drawable->surface_id,
+   &drawable->red_drawable->bbox, l, TRUE);
  resent_surface_ids[num_resent] = drawable->red_drawable->surface_id;
  resent_areas[num_resent] = drawable->red_drawable->bbox;
  num_resent++;
  
-spice_assert(image);

+GList *image_pos = l->next;


l may be the queue tail, and in that case l->next would be
NULL, wouldn't it ?


+spice_assert(image_pos);
  
red_channel_client_pipe_remove_and_release_pos(RED_CHANNEL_CLIENT(dcc), l);
+l = image_pos;
  }
  }
  



I solved it differently:

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 84fa1be72..255e893f7 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -713,7 +713,7 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient

 int resent_surface_ids[MAX_PIPE_SIZE];
 SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since 
drawables may be released

 int num_resent;
-GList *l;
+GList *l, *lprev;
 GQueue *pipe;

 resent_surface_ids[0] = first_surface_id;
@@ -723,12 +723,13 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient

 pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));

 // going from the oldest to the newest
-for (l = pipe->tail; l != NULL; l = l->prev) {
+for (l = pipe->tail; l != NULL; l = lprev) {
 RedPipeItem *pipe_item = l->data;
 Drawable *drawable;
 RedDrawablePipeItem *dpi;
 RedImageItem *image;

+lprev = l->prev;
 if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
 continue;
 dpi = SPICE_UPCAST(RedDrawablePipeItem, pipe_item);




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

Re: [Spice-devel] [PATCH spice-server] fixup! dcc-send: remove useless pipe_item assignment pipe_item

[Uri Lublin]

On 7/4/19 4:31 PM, Frediano Ziglio wrote:


On 7/4/19 12:33 PM, Frediano Ziglio wrote:

Remove use-after-free introduced by
a78a7d251042892182b158650291d19a85bbd6b1
---
   server/dcc-send.c | 9 +
   1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 565a79f33..4582e3545 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -725,7 +725,6 @@ static void
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
   RedPipeItem *pipe_item = l->data;
   Drawable *drawable;
   RedDrawablePipeItem *dpi;
-RedImageItem *image;
   
   if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)

   continue;
@@ -745,14 +744,16 @@ static void
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
   continue;
   }
   
-image = dcc_add_surface_area_image(dcc,

drawable->red_drawable->surface_id,
-
&drawable->red_drawable->bbox,
l, TRUE);
+dcc_add_surface_area_image(dcc,
drawable->red_drawable->surface_id,
+   &drawable->red_drawable->bbox, l,
TRUE);
   resent_surface_ids[num_resent] =
   drawable->red_drawable->surface_id;
   resent_areas[num_resent] = drawable->red_drawable->bbox;
   num_resent++;
   
-spice_assert(image);

+GList *image_pos = l->next;
+spice_assert(image_pos);
   
red_channel_client_pipe_remove_and_release_pos(RED_CHANNEL_CLIENT(dcc),
   l);
+l = image_pos;
   }
   }
   



I solved it differently:

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 84fa1be72..255e893f7 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -713,7 +713,7 @@ static void
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
   int resent_surface_ids[MAX_PIPE_SIZE];
   SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since
drawables may be released
   int num_resent;
-GList *l;
+GList *l, *lprev;


I would use just "prev" or "l_prev".


   GQueue *pipe;

   resent_surface_ids[0] = first_surface_id;
@@ -723,12 +723,13 @@ static void
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
   pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));

   // going from the oldest to the newest
-for (l = pipe->tail; l != NULL; l = l->prev) {
+for (l = pipe->tail; l != NULL; l = lprev) {
   RedPipeItem *pipe_item = l->data;
   Drawable *drawable;
   RedDrawablePipeItem *dpi;
   RedImageItem *image;

+lprev = l->prev;
   if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
   continue;
   dpi = SPICE_UPCAST(RedDrawablePipeItem, pipe_item);




Uri.



Fine with it.
Or maybe you can update "l" directly and use a "curr" instead with
limited scope (inside the "for") and always initialized?



Why not using GLIST_FOREACH_REVERSED ?


At first I thought it works, but how do you get
the location to add the image when calling
dcc_add_surface_area_image ?

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

Re: [Spice-devel] [PATCH spice-gtk 3/4] build: Do additional changes to Meson distribution

[Uri Lublin]

On 5/20/19 10:39 AM, Frediano Ziglio wrote:

- copy missing recorder files;
- generate THANKS file.

Signed-off-by: Frediano Ziglio 
---
  Makefile.am  |  1 +
  build-aux/meson-dist | 28 
  meson.build  |  2 +-
  3 files changed, 30 insertions(+), 1 deletion(-)
  create mode 100755 build-aux/meson-dist

diff --git a/Makefile.am b/Makefile.am
index 3c607c9a..6ba8c028 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -27,6 +27,7 @@ EXTRA_DIST =  \
meson_options.txt   \
po/meson.build  \
build-aux/git-version-gen   \
+   build-aux/meson-dist\
gtk-doc.make\
.version\
$(NULL)
diff --git a/build-aux/meson-dist b/build-aux/meson-dist
new file mode 100755
index ..9a18ff2d
--- /dev/null
+++ b/build-aux/meson-dist
@@ -0,0 +1,28 @@
+#!/bin/bash
+
+set -e
+set -o pipefail
+
+if test "$1" = ""; then
+echo "Version not provided" >&2
+exit 1
+fi
+if ! test -d "$2"; then
+echo "Source directory not provided" >&2
+exit 1
+fi
+
+# generate tarball version
+echo "$1" > "$MESON_DIST_ROOT/.tarball-version"
+
+# add missing recorder files
+(cd "$2" && ls -1 subprojects/spice-common/common/recorder/recorder.[ch] \
+subprojects/spice-common/common/recorder/recorder_ring.[ch] | \
+tar cf - -T -) | (cd "$MESON_DIST_ROOT" && exec tar xf -)


What happens when recorder is disabled and this scripts exits with
an error ?

Uri.


+
+# generate THANKS file
+{
+ echo "The spice-gtk team would like to thank the following 
contributors:"
+ echo
+ (cd "$2" && exec git log --format='%aN <%aE>') | sort -u
+} > "$MESON_DIST_ROOT/THANKS"
diff --git a/meson.build b/meson.build
index 8c6288f3..58961551 100644
--- a/meson.build
+++ b/meson.build
@@ -6,7 +6,7 @@ project('spice-gtk', 'c',
   license : 'LGPLv2.1',
   meson_version : '>= 0.49')
  
-meson.add_dist_script('sh', '-c', 'echo @0@>"$MESON_DIST_ROOT/.tarball-version"'.format(meson.project_version()))

+meson.add_dist_script('build-aux/meson-dist', meson.project_version(), 
meson.source_root())
  
  #

  # global C defines



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

Re: [Spice-devel] [PATCH spice-gtk 4/4] ci: Try Meson dist

[Uri Lublin]

On 5/20/19 10:39 AM, Frediano Ziglio wrote:

Make sure Meson is able to generate a correct tarball.

Signed-off-by: Frediano Ziglio 
---
  .gitlab-ci.yml | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a6cb2cda..5ddb4db8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -82,6 +82,12 @@ fedora-meson:
  
script:

  - meson --buildtype=release build-default
+# Meson does not update submodules recursively


Autotools do not do that either, it's done in the autogen.sh script


+- git submodule update --init --recursive
+# this fix an issue with Meson dist
+- if ! test -r ../spice-common.git; then DIR=`basename "$PWD"`; ln -s 
"$DIR/.git/modules/spice-common" ../spice-common.git; fi


That's weird. It does work for me locally.
It is expected that ../spice-common.git would be relative to 'origin'.


+- rm -rf meson-dist
+- ninja -C build-default dist


Note, that the generated tarball does not contain a configure script.
This is probably expected as autotools are not used, but I wanted
to emphasize that the two tarballs are different.

Uri.


  - ninja -C build-default
  - ninja -C build-default test
  



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

Re: [Spice-devel] [PATCH spice-gtk 0/4] Check distribution with CI

[Uri Lublin]

On 5/20/19 10:39 AM, Frediano Ziglio wrote:

Make sure we can build distribution.
Some small updates also to fix some minor issues.

CI results at https://gitlab.freedesktop.org/fziglio/spice-gtk/pipelines/37744


Ack series with some comments in 3/4 and 4/4



Frediano Ziglio (4):
   ci: Test we can create a no dirty package
   spice-client-gtk-module: Remove unused file
   build: Do additional changes to Meson distribution
   ci: Try Meson dist

  .gitlab-ci.yml| 20 
  Makefile.am   |  1 +
  build-aux/meson-dist  | 28 ++
  meson.build   |  2 +-
  src/spice-client-gtk-module.c | 45 ---
  5 files changed, 50 insertions(+), 46 deletions(-)
  create mode 100755 build-aux/meson-dist
  delete mode 100644 src/spice-client-gtk-module.c



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

Re: [Spice-devel] [PATCH spice-gtk 4/4] ci: Try Meson dist

[Uri Lublin]

On 7/8/19 4:10 PM, Frediano Ziglio wrote:


On 5/20/19 10:39 AM, Frediano Ziglio wrote:

Make sure Meson is able to generate a correct tarball.

Signed-off-by: Frediano Ziglio 
---
   .gitlab-ci.yml | 6 ++
   1 file changed, 6 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a6cb2cda..5ddb4db8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -82,6 +82,12 @@ fedora-meson:
   
 script:

   - meson --buildtype=release build-default
+# Meson does not update submodules recursively


Autotools do not do that either, it's done in the autogen.sh script



But Meson is supposed to deal with submodules, Autotools has nothing
to do with the repositories, it's quite a different mindset.


+- git submodule update --init --recursive
+# this fix an issue with Meson dist
+- if ! test -r ../spice-common.git; then DIR=`basename "$PWD"`; ln -s
"$DIR/.git/modules/spice-common" ../spice-common.git; fi


That's weird. It does work for me locally.


Maybe you already have the link. Or they fixed the issue on a later version.


Sorry. I meant that your fix works for me (and without it, the
build fails).




It is expected that ../spice-common.git would be relative to 'origin'.



What do you mean? ../spice-common.git is a directory link and origin
is a branch.


I mean I expect ../spice-common.git to not be a directory but a (remote)
git repository that it's location is based on remote 'origin' url




+- rm -rf meson-dist
+- ninja -C build-default dist


Note, that the generated tarball does not contain a configure script.
This is probably expected as autotools are not used, but I wanted
to emphasize that the two tarballs are different.



Yes, Meson "dist" is not powerful enough to generated such a tarball
and a manual script would be quite an hack.


OK, no problem, as long as we keep that in mind.

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

Re: [Spice-devel] [PATCH spice-gtk 0/2] Drop autotools

[Uri Lublin]

On 7/8/19 3:00 PM, Frediano Ziglio wrote:

This series is from Marc-André, I just rebased it on current master.
Not much left of the original series.
I think it's time to get back to it.



Does meson work well for mingw-spice-gtk ?

I think a safer path is to make a release with Meson
before removing autotools.

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

Re: [Spice-devel] [PATCH spice-gtk 0/2] Drop autotools

[Uri Lublin]

On 7/9/19 5:16 PM, Victor Toso wrote:

Hi,

On Tue, Jul 09, 2019 at 07:47:18AM -0400, Frediano Ziglio wrote:


On 7/8/19 3:00 PM, Frediano Ziglio wrote:

This series is from Marc-André, I just rebased it on current master.
Not much left of the original series.
I think it's time to get back to it.



Does meson work well for mingw-spice-gtk ?


It should, note tha mingw64-filesystem-106 provides mingw64-meson

 (wingsuit) spice-gtk (master 9a6ffbac) $ dnf whatprovides mingw64-meson
 Last metadata expiration check: 6:36:54 ago on Mon 08 Jul 2019 13:36:17 
CEST.
 mingw64-filesystem-106-1.fc30.noarch : MinGW cross compiler base 
filesystem and environment for the win64 target
 Repo: fedora
 Matched from:
 Filename: /usr/bin/mingw64-meson




I think a safer path is to make a release with Meson
before removing autotools.


I had that in mind with 0.37 release o/

https://gitlab.freedesktop.org/spice/spice-gtk/-/tags/v0.37


Note that this release was done with autotools and not meson.

https://www.spice-space.org/download/gtk/spice-gtk-0.37.tar.bz2

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

[Spice-devel] [spice-server PATCH v2 2/3] dcc-send: remove unused variable 'image'

[Uri Lublin]

From: Frediano Ziglio 

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 565a79f33..e0f3b8183 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -725,7 +725,6 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 RedPipeItem *pipe_item = l->data;
 Drawable *drawable;
 RedDrawablePipeItem *dpi;
-RedImageItem *image;
 
 if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
 continue;
@@ -745,13 +744,12 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 continue;
 }
 
-image = dcc_add_surface_area_image(dcc, 
drawable->red_drawable->surface_id,
-   &drawable->red_drawable->bbox, l, 
TRUE);
+dcc_add_surface_area_image(dcc, drawable->red_drawable->surface_id,
+   &drawable->red_drawable->bbox, l, TRUE);
 resent_surface_ids[num_resent] = drawable->red_drawable->surface_id;
 resent_areas[num_resent] = drawable->red_drawable->bbox;
 num_resent++;
 
-spice_assert(image);
 
red_channel_client_pipe_remove_and_release_pos(RED_CHANNEL_CLIENT(dcc), l);
 }
 }
-- 
2.21.0

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

[Spice-devel] [spice-server PATCH v2 1/3] dcc-send: remove useless pipe_item assignment pipe_item

[Uri Lublin]

In red_pipe_replace_rendered_drawables_with_images, the
value of pipe_item is re-written on the next iteration.

Since a78a7d251042892182b158650291d19a85bbd6b1 pipe_item
is no longer used to control the loop.

Found by Covscan.

Signed-off-by: Uri Lublin 
---
 server/dcc-send.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index b14619a2b..565a79f33 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -753,7 +753,6 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 
 spice_assert(image);
 
red_channel_client_pipe_remove_and_release_pos(RED_CHANNEL_CLIENT(dcc), l);
-pipe_item = &image->base;
 }
 }
 
-- 
2.21.0

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

[Spice-devel] [spice-server PATCH v2 3/3] dcc-send: fix use-after-free

[Uri Lublin]

'l' is being freed within the loop

Found-by: Frediano Ziglio 
Signed-off-by: Uri Lublin 
---
 server/dcc-send.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index e0f3b8183..4a92ce8cd 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -711,7 +711,7 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 int resent_surface_ids[MAX_PIPE_SIZE];
 SpiceRect resent_areas[MAX_PIPE_SIZE]; // not pointers since drawables may 
be released
 int num_resent;
-GList *l;
+GList *l, *prev;
 GQueue *pipe;
 
 resent_surface_ids[0] = first_surface_id;
@@ -721,11 +721,12 @@ static void 
red_pipe_replace_rendered_drawables_with_images(DisplayChannelClient
 pipe = red_channel_client_get_pipe(RED_CHANNEL_CLIENT(dcc));
 
 // going from the oldest to the newest
-for (l = pipe->tail; l != NULL; l = l->prev) {
+for (l = pipe->tail; l != NULL; l = prev) {
 RedPipeItem *pipe_item = l->data;
 Drawable *drawable;
 RedDrawablePipeItem *dpi;
 
+prev = l->prev;
 if (pipe_item->type != RED_PIPE_ITEM_TYPE_DRAW)
 continue;
 dpi = SPICE_UPCAST(RedDrawablePipeItem, pipe_item);
-- 
2.21.0

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

[Spice-devel] [spice-server PATCH v2 0/3] dcc-send: remove useless pipe_item assignment pipe_item

[Uri Lublin]

This patch-set fixes some problems in dcc-send's
red_pipe_replace_rendered_drawables_with_images().

v1->v2:
  - Two additional patches:
- remove unused variable 'image' (Frediano)
- fix use-after-free (Uri)

If people prefer I can squash all three into a single patch.


Frediano Ziglio (1):
  dcc-send: remove unused variable 'image'

Uri Lublin (2):
  dcc-send: remove useless pipe_item assignment pipe_item
  dcc-send: fix use-after-free

 server/dcc-send.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-streaming-agent] gst-plugin: receive encoder properties as command parameters

[Uri Lublin]

On 7/16/19 1:55 PM, Snir Sheriber wrote:

This allows to set plugin key=value properties on run time.
To add encoder plugin property use the following syntax:
-gst.prop="key=value"


Is this the correct syntax, or do you need a -c ?


Make sure syntax is accurate and that the property is supported by
the chosen plugin, wrong/invalid properties will be ignored silently.
Specific encoder available properties can be viewed by:
gst-inspect-1.0 
---
* This patch useful for encoders tuning and testing (later we can introduce
   fixed encoders setups), hence not checking for validity of input.
* I dropped sstream in previous patch but i found it useful here and added it
   again, alternative suggestions are welcome

---
  src/gst-plugin.cpp | 21 +
  1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
index 4e802f1..0d8773d 100644
--- a/src/gst-plugin.cpp
+++ b/src/gst-plugin.cpp
@@ -6,6 +6,7 @@
  
  #include 

  #include 
+#include 
  #include 
  #include 
  #include 
@@ -35,6 +36,7 @@ struct GstreamerEncoderSettings
  int fps = 25;
  SpiceVideoCodecType codec = SPICE_VIDEO_CODEC_TYPE_H264;
  std::string encoder;
+std::vector prop_strings;
  };
  
  template 

@@ -180,10 +182,15 @@ GstElement 
*GstreamerFrameCapture::get_encoder_plugin(const GstreamerEncoderSett
  
  encoder = factory ? gst_element_factory_create(factory, "encoder") : nullptr;

  if (encoder) { // Invalid properties will be ignored silently
-/* x264enc properties */
-gst_util_set_object_arg(G_OBJECT(encoder), "tune", "zerolatency");// 
stillimage, fastdecode, zerolatency
-gst_util_set_object_arg(G_OBJECT(encoder), "bframes", "0");
-gst_util_set_object_arg(G_OBJECT(encoder), "speed-preset", "1");// 
1-ultrafast, 6-med, 9-veryslow
+const char *key;
+const char *val;
+for (int i = 1; i < settings.prop_strings.size(); i += 2) {
+key = settings.prop_strings[i-1].c_str();
+val = settings.prop_strings[i].c_str();
+gst_util_set_object_arg(G_OBJECT(encoder), key, val);
+gst_syslog(LOG_NOTICE, "Trying to set encoder property: '%s = 
%s'", key, val);


If gst_util_set_object_arg returns something it would be nice to add
a status to the gst_syslog



+}
+
  }
  gst_plugin_feature_list_free(filtered);
  gst_plugin_feature_list_free(encoders);
@@ -449,6 +456,12 @@ void GstreamerPlugin::ParseOptions(const ConfigureOption 
*options)
  }
  } else if (name == "gst.encoder") {
  settings.encoder = value;
+} else if (name == "gst.prop") {
+std::stringstream ss(value);
+std::string item;
+while (std::getline(ss, item, '=')) {
+   settings.prop_strings.push_back(item);


Maybe better to get pairs.

Uri.


+}
  }
  }
  }



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

Re: [Spice-devel] [PATCH spice-server 1/2] websocket: Add header guards

[Uri Lublin]

On 6/27/19 6:03 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 

Ack.

Uri.


---
  server/websocket.h | 5 +
  1 file changed, 5 insertions(+)

diff --git a/server/websocket.h b/server/websocket.h
index 22120d939..7707e6804 100644
--- a/server/websocket.h
+++ b/server/websocket.h
@@ -15,6 +15,9 @@
   *  License along with this library; if not, see 
.
   */
  
+#ifndef WEBSOCKET_H_

+#define WEBSOCKET_H_
+
  typedef ssize_t (*websocket_read_cb_t)(void *opaque, void *buf, size_t nbyte);
  typedef ssize_t (*websocket_write_cb_t)(void *opaque, const void *buf, size_t 
nbyte);
  typedef ssize_t (*websocket_writev_cb_t)(void *opaque, struct iovec *iov, int 
iovcnt);
@@ -41,3 +44,5 @@ void websocket_free(RedsWebSocket *ws);
  int websocket_read(RedsWebSocket *ws, uint8_t *buf, size_t len, unsigned 
*flags);
  int websocket_write(RedsWebSocket *ws, const void *buf, size_t len, unsigned 
flags);
  int websocket_writev(RedsWebSocket *ws, const struct iovec *iov, int iovcnt, 
unsigned flags);
+
+#endif



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

Re: [Spice-devel] [PATCH spice-server 2/2] websocket: Make header self-independent

[Uri Lublin]

On 6/27/19 6:03 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 


Ack, with comment/suggestion below.


---
  server/websocket.h | 4 
  1 file changed, 4 insertions(+)

diff --git a/server/websocket.h b/server/websocket.h
index 7707e6804..ec452038b 100644
--- a/server/websocket.h
+++ b/server/websocket.h
@@ -18,6 +18,10 @@
  #ifndef WEBSOCKET_H_
  #define WEBSOCKET_H_
  
+#include 


This is for uint8_t, maybe
#include 


+
+#include "sys-socket.h"


(This is for struct iovec)

Uri.

+
  typedef ssize_t (*websocket_read_cb_t)(void *opaque, void *buf, size_t nbyte);
  typedef ssize_t (*websocket_write_cb_t)(void *opaque, const void *buf, size_t 
nbyte);
  typedef ssize_t (*websocket_writev_cb_t)(void *opaque, struct iovec *iov, int 
iovcnt);



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

Re: [Spice-devel] [PATCH spice-server] reds: Fix use-after-free

[Uri Lublin]

On 7/17/19 1:41 PM, Frediano Ziglio wrote:

video_codecs can be freed so use it before.

Signed-off-by: Frediano Ziglio 


Ack.

I had a similar patch, you sent your faster :)

Uri.


---
  server/reds.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 817fdd423..78bbe5a0f 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3851,6 +3851,10 @@ static int reds_set_video_codecs_from_string(RedsState 
*reds, const char *codecs
  codecs = c;
  }
  
+if (installed) {

+*installed = video_codecs->len;
+}
+
  if (video_codecs->len == 0) {
  spice_warning("Failed to set video codecs, input string: '%s'", 
codecs);
  g_array_unref(video_codecs);
@@ -3860,10 +3864,6 @@ static int reds_set_video_codecs_from_string(RedsState 
*reds, const char *codecs
  
  g_free(codecs_copy);
  
-if (installed) {

-*installed = video_codecs->len;
-}
-
  return invalid_codecs;
  }
  



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

Re: [Spice-devel] [PATCH spice-server] websocket: Include proper type header

[Uri Lublin]

On 7/17/19 2:32 PM, Frediano Ziglio wrote:

inttypes.h contains macro for format string while
stdint.h more specifically contains type definitions (like uint8_t).

Signed-off-by: Frediano Ziglio 


Ack :)


---
  server/websocket.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/websocket.h b/server/websocket.h
index ec452038b..4619836a6 100644
--- a/server/websocket.h
+++ b/server/websocket.h
@@ -18,7 +18,7 @@
  #ifndef WEBSOCKET_H_
  #define WEBSOCKET_H_
  
-#include 

+#include 
  
  #include "sys-socket.h"
  



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

Re: [Spice-devel] [PATCH spice-server] ci: Workaround an issue with GLib on Fedora 30

[Uri Lublin]

On 7/17/19 12:46 PM, Frediano Ziglio wrote:

This remove this error running test-listen test on a Fedora 30 docker
image:

(/builds/spice/spice/build/server/tests/test-listen:2233): GLib-GIO-CRITICAL 
**: 15:29:03.123: g_file_new_for_path: assertion 'path != NULL' failed

This error is due to some missing configuration on the image.
On a fully installed Desktop/Server machine these configuration
are usually in place so you won't note the issue but on recent
docker images these configuration are missing.

Running the dconf command add required configuration.



Nice. It seems it does indeed fix the weird failure of test-listen.
This is magic -- the patch resets 'location' (==GPS).

Thanks,
Uri.




Signed-off-by: Frediano Ziglio 
---
  .gitlab-ci.yml | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index b303d4656..3c5298f84 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -8,9 +8,11 @@ before_script:
  glib2-devel celt051-devel pixman-devel alsa-lib-devel openssl-devel 
libjpeg-turbo-devel
  libcacard-devel cyrus-sasl-devel lz4-devel opus-devel
  gstreamer1-devel gstreamer1-plugins-base-devel
+dconf
  -y
- git clone ${CI_REPOSITORY_URL/spice.git/spice-protocol.git}
- (cd spice-protocol && ./autogen.sh --prefix=/usr && make install)
+  - dconf reset /org/gnome/system/location/enabled || true
  
  makecheck:

script:



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

Re: [Spice-devel] [PATCH spice-server 3/3] ci: Add some Valgrind suppressions for Fedora 30

[Uri Lublin]

On 7/18/19 10:32 AM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 


Hi Frediano,

I've been playing with it too.
Had similar rules but different.

Your version works for me.
Some minor comments below.


---
  server/tests/valgrind/spice.supp | 25 +
  1 file changed, 25 insertions(+)

diff --git a/server/tests/valgrind/spice.supp b/server/tests/valgrind/spice.supp
index 1bfe81006..dd3663c68 100644
--- a/server/tests/valgrind/spice.supp
+++ b/server/tests/valgrind/spice.supp
@@ -36,3 +36,28 @@
...
fun:p11_kit_modules_load
  }
+
+{
+   gnutls_x509_ext_import_subject_alt_names
+   Memcheck:Cond
+   ...
+   fun:gnutls_x509_ext_import_subject_alt_names
+   fun:gnutls_x509_crt_import
+   fun:gnutls_x509_trust_list_iter_get_ca
+   ...
+   fun:g_initable_new_valist
+   fun:g_initable_new
+   ...


possibly you can remove the last 4 lines (not important)


+}
+
+{
+   glib_g_socket_client_class_init


Are all glib sockets leaking when initialized ?


+   Memcheck:Leak
+   fun:calloc
+   ...
+   fun:type_class_init_Wm
+   ...
+   fun:g_socket_client_class_init
+   ...
+   fun:type_class_init_Wm


You can probably remove the type_class_init_Wm and ...

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

Re: [Spice-devel] [PATCH spice-server 0/3] Update Valgrind suppression files

[Uri Lublin]

On 7/18/19 10:32 AM, Frediano Ziglio wrote:

This series split and update Valgrind suppression files in
order for make Gitlab CI pass.


Works for me.

Some minor comments for patch 3.

Ack series.

Uri.



Results at
https://gitlab.freedesktop.org/fziglio/spice/-/jobs/430006.
Note that to pass Valgrind checks some other patches are needed
but these series is complete for the suppression part.

Frediano Ziglio (3):
   ci: Separate SPICE specific Valgrind suppression
   ci: Update glib.supp file
   ci: Add some Valgrind suppressions for Fedora 30

  server/tests/Makefile.am |   2 +-
  server/tests/valgrind/glib.supp  | 207 ---
  server/tests/valgrind/spice.supp |  63 ++
  3 files changed, 257 insertions(+), 15 deletions(-)
  create mode 100644 server/tests/valgrind/spice.supp



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

Re: [Spice-devel] [PATCH x11spice 1/2] Bug fix: --config= did not work.

[Uri Lublin]

On 7/18/19 5:31 PM, Jeremy White wrote:

Signed-off-by: Jeremy White 


Hi,

The patch looks good to me.
See a comment below.


---
  src/options.c | 24 ++--
  1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/options.c b/src/options.c
index b7f487c5..0d3138d0 100644
--- a/src/options.c
+++ b/src/options.c
@@ -213,14 +213,34 @@ void options_handle_ssl_file_options(options_t *options,
  options->ssl.ciphersuite = string_option(userkey, systemkey, "ssl", 
"ciphersuite");
  }
  
+/* In general, we want to parse the config file options before the command line

+**  arguments.  However, the command line argument to specify a config file is
+**  the exception.  We manually parse this out now, so we can simplify the
+**  flow of control later. */
  void options_handle_user_config(int argc, char *argv[], options_t *options)
  {
  int i;
-for (i = 1; i < argc - 1; i++)
-if (strcmp(argv[i], "--config") == 0 || strcmp(argv[i], "-config") == 
0) {
+char *p, *q;
+
+/* getopt long is complex; it supports [-]-config[=]filename */


I too find it confusing.
Note that this is the behavior of getopt_long_only.
We can switch to using getopt_long (but you would still need to
look for '=').

Uri.


+for (i = 1; i < argc; i++) {
+p = strstr(argv[i], "--config");
+if (p != argv[i]) {
+p = strstr(argv[i], "-config");
+}
+if (p != argv[i]) {
+continue;
+}
+q = strstr(p, "=");
+if (q) {
+options->user_config_file = strdup(q + 1);
+continue;
+}
+if (i < argc - 1) {
  options->user_config_file = strdup(argv[i + 1]);
  i++;
  }
+}
  }
  
  int options_parse_arguments(int argc, char *argv[], options_t *options)




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

Re: [Spice-devel] [PATCH spice-server v3] ci: Workaround an issue with GLib on Fedora 30

[Uri Lublin]

On 7/17/19 3:06 PM, Frediano Ziglio wrote:

This remove this error running test-listen test on a Fedora 30 docker
image:

(/builds/spice/spice/build/server/tests/test-listen:2233): GLib-GIO-CRITICAL 
**: 15:29:03.123: g_file_new_for_path: assertion 'path != NULL' failed

This error is due to some missing configuration on the image.
On a fully installed Desktop/Server machine these configuration
are usually in place so you won't note the issue but on recent
docker images these configuration are missing.

Running the dconf command add required configuration.


This is indeed weird. But it works.

Thanks for adding the comment you added below.



Signed-off-by: Frediano Ziglio 

Ack.

Uri.


---
  .gitlab-ci.yml | 7 +++
  1 file changed, 7 insertions(+)

Changes since v2:
- more comments

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index b303d4656..316a860dd 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -8,9 +8,16 @@ before_script:
  glib2-devel celt051-devel pixman-devel alsa-lib-devel openssl-devel 
libjpeg-turbo-devel
  libcacard-devel cyrus-sasl-devel lz4-devel opus-devel
  gstreamer1-devel gstreamer1-plugins-base-devel
+dconf
  -y
- git clone ${CI_REPOSITORY_URL/spice.git/spice-protocol.git}
- (cd spice-protocol && ./autogen.sh --prefix=/usr && make install)
+  # This is a workaround for Fedora docker image, this will add some
+  # missing configuration
+  # '/org/gnome/system/location/enabled' is just the first key path
+  # I found, nothing special in it
+  # TODO remove when the image will fix this issue
+  - dconf reset /org/gnome/system/location/enabled || true
  
  makecheck:

script:



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

[Spice-devel] [PATCH 2/2] tests: rename video-encoders to test-video-encoders

[Uri Lublin]

---
 server/tests/Makefile.am | 6 +++---
 server/tests/meson.build | 2 +-
 server/tests/{video-encoders => test-video-encoders} | 0
 3 files changed, 4 insertions(+), 4 deletions(-)
 rename server/tests/{video-encoders => test-video-encoders} (100%)

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index ccaf5c87c..1e62557a8 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -139,13 +139,13 @@ libtest_stat3_a_CPPFLAGS = $(AM_CPPFLAGS) 
-DTEST_COMPRESS_STAT=1 -DTEST_RED_WORK
 libtest_stat4_a_SOURCES = stat-test.c
 libtest_stat4_a_CPPFLAGS = $(AM_CPPFLAGS) -DTEST_COMPRESS_STAT=1 
-DTEST_RED_WORKER_STAT=1 -DTEST_NAME=stat_test4
 
-## test-gst (helper) and video-encoders (test)
+## test-gst (helper) and test-video-encoders (test)
 
 if HAVE_GSTREAMER
 noinst_PROGRAMS += test-gst
 
 if ENABLE_EXTRA_CHECKS
-TESTS += video-encoders
+TESTS += test-video-encoders
 endif
 
 test_gst_SOURCES = test-gst.c \
@@ -157,7 +157,7 @@ test_gst_CPPFLAGS = \
$(NULL)
 endif
 
-EXTRA_DIST += video-encoders
+EXTRA_DIST += test-video-encoders
 
 if HAVE_SASL
 check_PROGRAMS += test-sasl
diff --git a/server/tests/meson.build b/server/tests/meson.build
index b6cf89894..c9377f1eb 100644
--- a/server/tests/meson.build
+++ b/server/tests/meson.build
@@ -74,7 +74,7 @@ endif
 if spice_server_has_gstreamer
   tests += [['test-gst', false]]
   if get_option('extra-checks')
-test('video-encoders', files('video-encoders'))
+test('test-video-encoders', files('test-video-encoders'))
   endif
 endif
 
diff --git a/server/tests/video-encoders b/server/tests/test-video-encoders
similarity index 100%
rename from server/tests/video-encoders
rename to server/tests/test-video-encoders
-- 
2.21.0

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

[Spice-devel] [PATCH 1/2] ci: pre-install "file"

[Uri Lublin]

This fixes the following warning:
  ./configure: line 7040: /usr/bin/file: No such file or directory

Signed-off-by: Uri Lublin 
---
 .gitlab-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 316a860dd..33210cb9d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -4,7 +4,7 @@ before_script:
   - >
 dnf install 'dnf-command(debuginfo-install)' git libtool make libasan 
orc-devel
 python3 python3-six python3-pyparsing glib-networking
-asciidoc bzip2 meson ninja-build
+asciidoc bzip2 meson ninja-build file
 glib2-devel celt051-devel pixman-devel alsa-lib-devel openssl-devel 
libjpeg-turbo-devel
 libcacard-devel cyrus-sasl-devel lz4-devel opus-devel
 gstreamer1-devel gstreamer1-plugins-base-devel
-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-server v2 1/2] red-replay-qxl: Fix some issue of alignment

[Uri Lublin]

On 7/19/19 12:18 PM, Frediano Ziglio wrote:

Do not pass unaligned QXLPHYSICAL but pass a valid pointer and
then cast.

Signed-off-by: Frediano Ziglio 


Ack.

Uri.


---
  server/red-replay-qxl.c | 18 +-
  1 file changed, 13 insertions(+), 5 deletions(-)

Changes since v1:
- remove unnecessary casts

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index fa44fa7c4..674feae2f 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -311,10 +311,12 @@ static ssize_t red_replay_data_chunks(SpiceReplay 
*replay, const char *prefix,
  data_size = cur->data_size;
  cur->next_chunk = cur->prev_chunk = 0;
  while (count_chunks-- > 0) {
-if (read_binary(replay, prefix, &next_data_size, 
(uint8_t**)&cur->next_chunk,
-sizeof(QXLDataChunk)) == REPLAY_ERROR) {
+uint8_t *data = NULL;
+if (read_binary(replay, prefix, &next_data_size, &data,
+sizeof(QXLDataChunk)) == REPLAY_ERROR) {
  return -1;
  }
+cur->next_chunk = QXLPHYSICAL_FROM_PTR(data);
  data_size += next_data_size;
  next = QXLPHYSICAL_TO_PTR(cur->next_chunk);
  next->prev_chunk = QXLPHYSICAL_FROM_PTR(cur);
@@ -472,7 +474,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
  if (qxl_flags & QXL_BITMAP_DIRECT) {
  qxl->bitmap.data = 
QXLPHYSICAL_FROM_PTR(red_replay_image_data_flat(replay, &bitmap_size));
  } else {
-size = red_replay_data_chunks(replay, "bitmap.data", 
(uint8_t**)&qxl->bitmap.data, 0);
+uint8_t *data = NULL;
+size = red_replay_data_chunks(replay, "bitmap.data", &data, 0);
+qxl->bitmap.data = QXLPHYSICAL_FROM_PTR(data);
  if (size != bitmap_size) {
  g_warning("bad image, %" G_GSIZE_FORMAT " != %" 
G_GSIZE_FORMAT, size, bitmap_size);
  return NULL;
@@ -710,7 +714,9 @@ static void red_replay_stroke_ptr(SpiceReplay *replay, 
QXLStroke *qxl, uint32_t
  size_t size;
  
  replay_fscanf(replay, "attr.style_nseg %d\n", &temp); qxl->attr.style_nseg = temp;

-read_binary(replay, "style", &size, (uint8_t**)&qxl->attr.style, 0);
+uint8_t *data = NULL;
+read_binary(replay, "style", &size, &data, 0);
+qxl->attr.style = QXLPHYSICAL_FROM_PTR(data);
  }
  red_replay_brush_ptr(replay, &qxl->brush, flags);
  replay_fscanf(replay, "fore_mode %d\n", &temp); qxl->fore_mode = temp;
@@ -1134,7 +1140,9 @@ static QXLSurfaceCmd *red_replay_surface_cmd(SpiceReplay 
*replay)
  }
  size = qxl->u.surface_create.height * 
abs(qxl->u.surface_create.stride);
  if ((qxl->flags & QXL_SURF_FLAG_KEEP_DATA) != 0) {
-read_binary(replay, "data", &read_size, 
(uint8_t**)&qxl->u.surface_create.data, 0);
+uint8_t *data = NULL;
+read_binary(replay, "data", &read_size, &data, 0);
+qxl->u.surface_create.data = QXLPHYSICAL_FROM_PTR(data);
  if (read_size != size) {
  g_warning("mismatch %" G_GSIZE_FORMAT " != %" G_GSIZE_FORMAT, 
size, read_size);
  }



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

Re: [Spice-devel] [PATCH spice-server v2] red-parse-qxl: Fix QUIC images from QXL

[Uri Lublin]

Hi Frediano,

On 7/23/19 11:21 PM, Frediano Ziglio wrote:

The decoding is wrong, the Red and QXL structures are different so
code is not doing what is expected. red-parse-qxl translate from QXL
to Red structures, red-record-qxl saves Red structure to file,
red-replay-qxl is supposed to read from file into QXL directly.

If a Quic image is stored inside QXL memory the layout of the QXLImage
in memory is:
- QXLImageDescriptor
- QXLQUICData
- QXLDataChunk
- first chunk data
and all remaining data in linked QXLDataChunk.
red_replay_image was reading the image as data was all contained in
QXLImage->quic.data however "data" shoule store the first QXLDataChunk


shoule -> should


followed by QXLDataChunk data.
Use proper base_size calling red_replay_data_chunks in order to
initialise the image with the first data chunk correctly.


I like this patch better then the first one.





Not easy to reproduce, the only driver is XDDM which means Windows XP
or similars, I got no recording with such images.

Signed-off-by: Frediano Ziglio 
---
  server/red-replay-qxl.c | 17 ++---
  1 file changed, 10 insertions(+), 7 deletions(-)

Changes v1:
- realloc the buffer inside read_binary with the proper size.
   The proper size if not available in red_replay_image.


Can we not just fix the above without moving the realloc call ?
Before only QUIC memory (which was malloc'ed) was realloc'ed.
With this other memory may be realloced.

Allocate more memory (add sizeof(QXLDataChunk))
   qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor)
+ sizeof(QXLQUICData)
+ sizeof(QXLDataChunk)
+ qxl->quic.data_size);

Call red_replay_data_chunks as you do in this patch
Call red_replay_data_chunks_free as you do in this patch.

Uri.



diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 674feae2f..1f1bd8c1d 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -114,6 +114,9 @@ static inline void replay_free(SpiceReplay *replay, void 
*mem)
  
  static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t n_bytes)

  {
+if (mem == NULL) {
+return replay_malloc(replay, n_bytes);
+}
  GList *elem = g_list_find(replay->allocated, mem);
  elem->data = g_realloc(mem, n_bytes);
  return elem->data;
@@ -231,9 +234,7 @@ static replay_t read_binary(SpiceReplay *replay, const char 
*prefix, size_t *siz
  return REPLAY_ERROR;
  }
  
-if (*buf == NULL) {

-*buf = replay_malloc(replay, *size + base_size);
-}
+*buf = replay_realloc(replay, *buf, *size + base_size);
  #if 0
  {
  int num_read = fread(*buf + base_size, *size, 1, fd);
@@ -497,9 +498,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
  if (replay->error) {
  return NULL;
  }
-qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
- qxl->quic.data_size);
-size = red_replay_data_chunks(replay, "quic.data", 
(uint8_t**)&qxl->quic.data, 0);
+size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&qxl,
+  sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
+  sizeof(QXLDataChunk));
  spice_assert(size == qxl->quic.data_size);
  break;
  default:
@@ -526,7 +527,9 @@ static void red_replay_image_free(SpiceReplay *replay, 
QXLPHYSICAL p, uint32_t f
  case SPICE_IMAGE_TYPE_SURFACE:
  break;
  case SPICE_IMAGE_TYPE_QUIC:
-red_replay_data_chunks_free(replay, qxl, 0);
+red_replay_data_chunks_free(replay, qxl,
+sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
+sizeof(QXLDataChunk));
  qxl = NULL;
  break;
  default:



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

Re: [Spice-devel] [PATCH spice-server v2] red-parse-qxl: Fix QUIC images from QXL

[Uri Lublin]

On 7/24/19 2:37 PM, Frediano Ziglio wrote:


Hi Frediano,

On 7/23/19 11:21 PM, Frediano Ziglio wrote:

The decoding is wrong, the Red and QXL structures are different so
code is not doing what is expected. red-parse-qxl translate from QXL
to Red structures, red-record-qxl saves Red structure to file,
red-replay-qxl is supposed to read from file into QXL directly.

If a Quic image is stored inside QXL memory the layout of the QXLImage
in memory is:
- QXLImageDescriptor
- QXLQUICData
- QXLDataChunk
- first chunk data
and all remaining data in linked QXLDataChunk.
red_replay_image was reading the image as data was all contained in
QXLImage->quic.data however "data" shoule store the first QXLDataChunk


shoule -> should



I'll fix, thanks


followed by QXLDataChunk data.
Use proper base_size calling red_replay_data_chunks in order to
initialise the image with the first data chunk correctly.


I like this patch better then the first one.





Not easy to reproduce, the only driver is XDDM which means Windows XP
or similars, I got no recording with such images.

Signed-off-by: Frediano Ziglio 
---
   server/red-replay-qxl.c | 17 ++---
   1 file changed, 10 insertions(+), 7 deletions(-)

Changes v1:
- realloc the buffer inside read_binary with the proper size.
The proper size if not available in red_replay_image.


Can we not just fix the above without moving the realloc call ?
Before only QUIC memory (which was malloc'ed) was realloc'ed.
With this other memory may be realloced.


I checked all calls, only for QUIC the initial pointer is NULL
so it will get reallocated.



Allocate more memory (add sizeof(QXLDataChunk))
 qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor)
  + sizeof(QXLQUICData)
  + sizeof(QXLDataChunk)
  + qxl->quic.data_size);



No, I rather prefer patch version 1 instead. The replay code should
attempt to allocate memory more like drivers. This will also help
using sanitizer tools. In this specific case allocating a larger
buffer will avoid some potential buffer overflow check.

Looking more at the code I noted that in many places the "header"
(information before chunks) are read into temporary variables
then raw data is allocated with the structure than the structure
header is filled (like red_replay_clip_rects for instance,
another more complicated one is red_replay_cursor).
The exception is red_replay_image where the "qxl" (in this case
a QXLImage type) is allocated at the beginning. One difference
between bitmap and quic is that "data" in QXLBitmap is a
QXLPHYSICAL, not a uint8_t[0].


Yes, you are right. I think your V1 indeed fits better to
the code around.

I was able to reproduce with running windows XP guest and
setting -spice image-compression=quic on qemu-kvm command line.

It indeed crashes on master and fixed by V1 and V2.




Call red_replay_data_chunks as you do in this patch
Call red_replay_data_chunks_free as you do in this patch.



Surely the offset is wrong, is expected to be the offset of raw
data to write.


Yes, the offset of original code is wrong but you fixed it in both
versions.


Uri.





diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 674feae2f..1f1bd8c1d 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -114,6 +114,9 @@ static inline void replay_free(SpiceReplay *replay,
void *mem)
   
   static inline void *replay_realloc(SpiceReplay *replay, void *mem, size_t

   n_bytes)
   {
+if (mem == NULL) {
+return replay_malloc(replay, n_bytes);
+}
   GList *elem = g_list_find(replay->allocated, mem);
   elem->data = g_realloc(mem, n_bytes);
   return elem->data;
@@ -231,9 +234,7 @@ static replay_t read_binary(SpiceReplay *replay, const
char *prefix, size_t *siz
   return REPLAY_ERROR;
   }
   
-if (*buf == NULL) {

-*buf = replay_malloc(replay, *size + base_size);
-}
+*buf = replay_realloc(replay, *buf, *size + base_size);
   #if 0
   {
   int num_read = fread(*buf + base_size, *size, 1, fd);
@@ -497,9 +498,9 @@ static QXLImage *red_replay_image(SpiceReplay *replay,
uint32_t flags)
   if (replay->error) {
   return NULL;
   }
-qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) +
sizeof(QXLQUICData) +
- qxl->quic.data_size);
-size = red_replay_data_chunks(replay, "quic.data",
(uint8_t**)&qxl->quic.data, 0);
+size = red_replay_data_chunks(replay, "quic.data",
(uint8_t**)&qxl,
+  sizeof(QXLImageDescriptor) +
sizeof(QXLQUICData) +
+  sizeof(QXLDataChunk));
   spice_assert(size == qxl->quic.data_size);
   break;
   default:
@@ -526,7 +527,9 @@ static void red_replay_image_free(SpiceReplay *replay,
QXLPHYSICAL p, uint32_t f
   case SPICE_IMAGE_TYPE_SURFACE:
   break;

Re: [Spice-devel] [PATCH spice-server v3] red-parse-qxl: Fix QUIC images from QXL

[Uri Lublin]

On 7/31/19 6:13 PM, Frediano Ziglio wrote:

The decoding is wrong, the Red and QXL structures are different so
code is not doing what is expected. red-parse-qxl translate from QXL
to Red structures, red-record-qxl saves Red structure to file,
red-replay-qxl is supposed to read from file into QXL directly.

If a Quic image is stored inside QXL memory the layout of the QXLImage
in memory is:
- QXLImageDescriptor
- QXLQUICData
- QXLDataChunk
- first chunk data
and all remaining data in linked QXLDataChunk.
red_replay_image was reading the image as data was all contained in
QXLImage->quic.data however "data" should store the first QXLDataChunk
followed by QXLDataChunk data.
Use proper base_size calling red_replay_data_chunks in order to
initialise the image with the first data chunk correctly.

Not easy to reproduce, the only driver is XDDM which means Windows XP
or similars. To enable QUIC encoding I added "image-compression=quic"
and "streaming-video=off" to "-spice" Qemu option in order to force
QUIC encoding in the guest driver (thanks to Uri Lublin for the help
reproducing it).

Signed-off-by: Frediano Ziglio 


Ack.

Uri.


---
  server/red-replay-qxl.c | 17 -
  1 file changed, 12 insertions(+), 5 deletions(-)

Changes from v2:
- back to v1 with reproduction and explanation.

diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 674feae2f..7104c81cd 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -416,7 +416,7 @@ static uint8_t *red_replay_image_data_flat(SpiceReplay 
*replay, size_t *size)
  
  static QXLImage *red_replay_image(SpiceReplay *replay, uint32_t flags)

  {
-QXLImage* qxl = NULL;
+QXLImage* qxl = NULL, *data;
  size_t bitmap_size;
  ssize_t size;
  uint8_t qxl_flags;
@@ -497,10 +497,15 @@ static QXLImage *red_replay_image(SpiceReplay *replay, 
uint32_t flags)
  if (replay->error) {
  return NULL;
  }
-qxl = replay_realloc(replay, qxl, sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
- qxl->quic.data_size);
-size = red_replay_data_chunks(replay, "quic.data", 
(uint8_t**)&qxl->quic.data, 0);
+data = NULL;
+size = red_replay_data_chunks(replay, "quic.data", (uint8_t**)&data,
+  sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
+  sizeof(QXLDataChunk));
  spice_assert(size == qxl->quic.data_size);
+data->descriptor = qxl->descriptor;
+data->quic.data_size = qxl->quic.data_size;
+replay_free(replay, qxl);
+qxl = data;
  break;
  default:
  spice_warn_if_reached();
@@ -526,7 +531,9 @@ static void red_replay_image_free(SpiceReplay *replay, 
QXLPHYSICAL p, uint32_t f
  case SPICE_IMAGE_TYPE_SURFACE:
  break;
  case SPICE_IMAGE_TYPE_QUIC:
-red_replay_data_chunks_free(replay, qxl, 0);
+red_replay_data_chunks_free(replay, qxl,
+sizeof(QXLImageDescriptor) + 
sizeof(QXLQUICData) +
+sizeof(QXLDataChunk));
  qxl = NULL;
  break;
  default:



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