Re: [Spice-devel] irc

[Hans de Goede]

Hi,

On 8/1/22 16:13, Victor Toso wrote:
> Hi,
> 
> CC'ing the list and Hans.
> 
> On Sat, Jul 30, 2022 at 02:55:53PM +0100, James Miller wrote:
>> Hi Victor, thanks for getting back to me.
>> Currently I have to echo 0 to the path under sys, and then
>> manually select the device to redirect in virtmanager and then
>> return to the terminal on the host and echo 1 to the same path.
> 
> Ah, okay, so it is a three-step process: echo 0 to $path, then
> SPICE redirection, then echo 1 $path  if you want to do it
> for several VMs for example, it should be very annoying.
> 
>> I don't mind this so much, but I have added the alias to the
>> fedora help page on yubikeys, and was wondering if there is any
>> programmatic way of achieving the redirection in the alias
>> itself.  ie the alias would echo 0, handle the redirection, and
>> then echo 1...
> 
> I don't have such device to test but I wonder why that is needed
> at all. What is the path you have to echo? Perhaps that can be
> achievable in a different way with with libusb or some specific
> driver's config that will get called when SPICE's redirection
> takes place.
> 
> Hans, have you tried this kind of device before?

No not really, also I have the feeling that I'm missing a lot
of context here. Echo 1/0 to which sysfs file for example ?

Also is this using the usbredirection built into the SPICE client(s),
or is this using QEMU's host usbredirection?

Regards,

Hans


> 
>> I wondered if spice exposed a useful set of commands to achieve
>> this with, perhaps via the spice-vdagentd socket... (I
>> obviously know virtually nothing about how spice works...)...
> 
> No, no API like that is exposed because for the set of devices
> we redirect, it isn't needed. Let's see if we can workaround this
> in the client OS first (e.g: configs) otherwise we can think
> something in spice-gtk (probably QEMU too if you try to usb
> pass through?)
> 
> Cheers,
> Victor
> 
>> On Fri, 29 Jul 2022, 10:48 Victor Toso,  wrote:
>>
>>> Hi,
>>>
>>> On Fri, Jul 29, 2022 at 09:56:21AM +0100, James Miller wrote:
 Hi,  I wonder if the irc address on the website contact page (
 https://www.spice-space.org/contact.html) needs updating?

 I can't find irc.gnome.org - I think they now use matrix.

 I want to know if there is anyway to manage usb-redirection
 to a libvirt kvm vm from the command line.  In particular,
 I want to redirect a usb key (yubikey) to the virt-manager
 managed vm, from the command line, as I need to deauthorise
 the key (echoing 0 to a file under a path below sys) before
 redirecting it.
>>>
>>> Ah, interesting, never tried this kind of device.
>>>
>>> Wouldn't it simply work if you do echo 0 and the do the
>>> redirection with virt-manager or remote-viewer's UI?
>>>
>>> Cheers,
>>> Victor

Re: [Spice-devel] [PATCH 0/5] Clean up TTM mmap offsets

[Hans de Goede]

Hi,

On 07-02-19 09:59, Thomas Zimmermann wrote:

Almost all TTM-based drivers use the same values for the mmap-able
range of BO addresses. Each driver therefore duplicates the
DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
configurable by drivers.

This patch set replaces driver-specific configuration with a single
setup. All code is located within TTM. TTM and GEM share the same
range for mmap-able BOs.

Thomas Zimmermann (5):
   staging/vboxvideo: Use same BO mmap offset as other drivers
   drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
   drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
   drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
   drm: Use the same mmap-range offset and size for GEM and TTM


Note I'm about to push a patch-series to drm-misc-next which moves
vboxvideo out of staging and I see that this series has not landed
in drm-misc-next yet, so it will needs to be rebased.

Regards,

Hans

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

Re: [Spice-devel] [PATCH 0/5] Clean up TTM mmap offsets

[Hans de Goede]

Hi,

On 11-03-19 17:51, Christian König wrote:

Am 11.03.19 um 17:39 schrieb Hans de Goede:

Hi,

On 07-02-19 09:59, Thomas Zimmermann wrote:

Almost all TTM-based drivers use the same values for the mmap-able
range of BO addresses. Each driver therefore duplicates the
DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
configurable by drivers.

This patch set replaces driver-specific configuration with a single
setup. All code is located within TTM. TTM and GEM share the same
range for mmap-able BOs.

Thomas Zimmermann (5):
   staging/vboxvideo: Use same BO mmap offset as other drivers
   drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
   drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
   drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
   drm: Use the same mmap-range offset and size for GEM and TTM


Note I'm about to push a patch-series to drm-misc-next which moves
vboxvideo out of staging and I see that this series has not landed
in drm-misc-next yet, so it will needs to be rebased.


Mhm, TTM is usually not pushed upstream through drm-misc-next, so that will 
certainly collide with the next TTM pull request.


Ugh, I didn't realize that this series would not be going through drm-misc-next.


So can you wait with that or should I make an exception and merge this change 
though drm-misc-next?


I've already pushed it now :| My mail was more intended as a headsup then
that I expected an objection, sorry.

I see 2 possible solutions:

1) Merge drm-misc-next into the ttm tree (probably the cleanest)
2) Push your series through drm-misc-next

Regards,

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

Re: [Spice-devel] [PATCH 0/5] Clean up TTM mmap offsets

[Hans de Goede]

Hi,

On 2/7/19 9:59 AM, Thomas Zimmermann wrote:

Almost all TTM-based drivers use the same values for the mmap-able
range of BO addresses. Each driver therefore duplicates the
DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
configurable by drivers.

This patch set replaces driver-specific configuration with a single
setup. All code is located within TTM. TTM and GEM share the same
range for mmap-able BOs.

Thomas Zimmermann (5):
   staging/vboxvideo: Use same BO mmap offset as other drivers
   drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
   drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
   drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
   drm: Use the same mmap-range offset and size for GEM and TTM


The first patch looks good to me:

Reviewed-by: Hans de Goede 

The vboxvideo bits in the other patches look good to me to:

Acked-by: Hans de Goede 

For the other patches in the series.

Regards,

Hans

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

Re: [Spice-devel] [qxl] Xspice: Replace malloc/strdup use with xnfalloc/xnfstrdup

[Hans de Goede]

Hi,

On 15-12-16 10:57, Christophe Fergeau wrote:

spiceqxl_*.c files are Xspice-only code. They contain a few uses of
malloc/strdup, and none of these are checked for failure. It's better to
replace these with xfnalloc/xnfstrdup which are provided by the X server
and cannot fail (aborts on failure).

Signed-off-by: Christophe Fergeau <cferg...@redhat.com>


LGTM:

Reviewed-by: Hans de Goede <hdego...@redhat.com>

Regards,

Hans



---
 src/spiceqxl_audio.c|  2 +-
 src/spiceqxl_main_loop.c|  4 ++--
 src/spiceqxl_spice_server.c | 12 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/spiceqxl_audio.c b/src/spiceqxl_audio.c
index 52a45f0..ec9260d 100644
--- a/src/spiceqxl_audio.c
+++ b/src/spiceqxl_audio.c
@@ -405,7 +405,7 @@ static void handle_one_change(qxl_screen_t *qxl, struct 
inotify_event *e)
 return;
 }

-fname = malloc(strlen(e->name) + strlen(qxl->playback_fifo_dir) + 1 + 
1);
+fname = xnfalloc(strlen(e->name) + strlen(qxl->playback_fifo_dir) + 1 
+ 1);
 strcpy(fname, qxl->playback_fifo_dir);
 strcat(fname, "/");
 strcat(fname, e->name);
diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index c9894bb..669e116 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -209,7 +209,7 @@ int watch_count = 0;

 static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void 
*opaque)
 {
-SpiceWatch *watch = malloc(sizeof(SpiceWatch));
+SpiceWatch *watch = xnfalloc(sizeof(SpiceWatch));

 DPRINTF(0, "adding %p, fd=%d at %d", watch,
 fd, watch_count);
@@ -376,7 +376,7 @@ static int watch_update_mask_internal(SpiceWatch *watch, 
int event_mask)

 static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void 
*opaque)
 {
-SpiceWatch *watch = malloc(sizeof(SpiceWatch));
+SpiceWatch *watch = xnfalloc(sizeof(SpiceWatch));

 DPRINTF(0, "adding %p, fd=%d", watch, fd);

diff --git a/src/spiceqxl_spice_server.c b/src/spiceqxl_spice_server.c
index 6e8cf50..38257d0 100644
--- a/src/spiceqxl_spice_server.c
+++ b/src/spiceqxl_spice_server.c
@@ -200,23 +200,23 @@ void xspice_set_spice_server_options(OptionInfoPtr 
options)
 len = strlen(x509_dir) + 32;

 if (x509_key_file_base) {
-x509_key_file = strdup(x509_key_file_base);
+x509_key_file = xnfstrdup(x509_key_file_base);
 } else {
-x509_key_file = malloc(len);
+x509_key_file = xnfalloc(len);
 snprintf(x509_key_file, len, "%s/%s", x509_dir, 
X509_SERVER_KEY_FILE);
 }

 if (x509_cert_file_base) {
-x509_cert_file = strdup(x509_cert_file_base);
+x509_cert_file = xnfstrdup(x509_cert_file_base);
 } else {
-x509_cert_file = malloc(len);
+x509_cert_file = xnfalloc(len);
 snprintf(x509_cert_file, len, "%s/%s", x509_dir, 
X509_SERVER_CERT_FILE);
 }

 if (x509_cacert_file_base) {
-x509_cacert_file = strdup(x509_cert_file_base);
+x509_cacert_file = xnfstrdup(x509_cert_file_base);
 } else {
-x509_cacert_file = malloc(len);
+x509_cacert_file = xnfalloc(len);
 snprintf(x509_cacert_file, len, "%s/%s", x509_dir, 
X509_CA_CERT_FILE);
 }
 }


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

Re: [Spice-devel] [qxl] xspice: Adjust to X.org 1.19 changes

[Hans de Goede]

Hi,

On 14-12-16 11:51, Christophe Fergeau wrote:

In newer X.org versions, it's no longer supported to modify the set of
FDs passed to a BlockHandler method to get notified when the FD has data
to be read. This was limited anyway as we could only get read events
this way, and had to do our own polling to get notified about socket
writeability.

Starting from xserver 1.19, the supported way of doing this is to use
the SetNotifyFd/RemoveNotifyFd API, which is actually a much better way
as it matches very well the 'watch' API spice-server expects Xspice to
implement.

This commit switches to that new API, which removes the need for
RegisterBlockAndWakeupHandlers().


Thank you for doing this, one small comment inline, otherwise looks
good:

Reviewed-by: Hans de Goede <hdego...@redhat.com>

(with the comment fixed).


---
I've lightly (xeyes/rxvt) tested this on f25, and tested this still builds on
f24.
This should fix the issues Hans mentioned in
https://lists.freedesktop.org/archives/spice-devel/2016-October/032501.html


 configure.ac |  5 ---
 src/spiceqxl_main_loop.c | 90 
 2 files changed, 84 insertions(+), 11 deletions(-)

diff --git a/configure.ac b/configure.ac
index e865e75..b737d0d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -68,8 +68,6 @@ PKG_CHECK_EXISTS(xfont2,

 # Obtain compiler/linker options for the driver dependencies
 PKG_CHECK_MODULES(XORG, [xorg-server >= 1.0.99.901] xproto fontsproto 
$xfont_pc $REQUIRED_MODULES)
-# Check for xorg 1.19 as XSpice is currently not working with it
-PKG_CHECK_EXISTS([xorg-server >= 1.18.99], [has_xorg119=yes])

 save_CFLAGS="$CFLAGS"
 CFLAGS="$XORG_CFLAGS"
@@ -141,9 +139,6 @@ if test "x$enable_xspice" = "xyes"; then
 AC_SUBST(SPICE_LIBS)
 ],
 )
-if test x"${enable_xspice}" = "xyes" && test x"${has_xorg119}" = "xyes"; 
then
-AC_MSG_ERROR("XSpice cannot currently work against X.Org 1.19")
-fi
 else
 enable_xspice=no
 fi
diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index 49b715a..81bc418 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -193,6 +193,7 @@ static void timer_remove(SpiceTimer *timer)
 free(timer);
 }

+#if GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 23
 struct SpiceWatch {
 RingItem link;
 int fd;
@@ -236,11 +237,6 @@ static void watch_remove(SpiceWatch *watch)
 watch_count--;
 }

-static void channel_event(int event, SpiceChannelEventInfo *info)
-{
-NOT_IMPLEMENTED
-}
-
 static int set_watch_fds(fd_set *rfds, fd_set *wfds)
 {
 SpiceWatch *watch;
@@ -276,7 +272,7 @@ static void xspice_block_handler(pointer data, OSTimePtr 
timeout, pointer readma
 /*
  * xserver only calls wakeup_handler with the read fd_set, so we
  * must either patch it or do a polling select ourselves, this is the
- * later approach. Since we are already doing a polling select, we
+ * latter approach. Since we are already doing a polling select, we
  * already select on all (we could avoid selecting on the read since
  * that *is* actually taken care of by the wakeup handler).
  */
@@ -338,9 +334,88 @@ static void xspice_wakeup_handler(pointer data, int nfds, 
pointer readmask)
 select_and_check_watches();
 }

+#else /* GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 23 */
+
+struct SpiceWatch {
+int fd;
+int event_mask;
+SpiceWatchFunc func;
+void *opaque;
+};
+
+static void watch_fd_notified(int fd, int xevents, void *data)
+{
+SpiceWatch *watch = (SpiceWatch *)data;
+
+if ((watch->event_mask & SPICE_WATCH_EVENT_READ) && (xevents & 
X_NOTIFY_READ)) {
+watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
+}
+
+if ((watch->event_mask & SPICE_WATCH_EVENT_WRITE) && (xevents & 
X_NOTIFY_WRITE)) {
+watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
+}
+}
+
+static int watch_update_mask2(SpiceWatch *watch, int event_mask)
+{
+SetNotifyFd(watch->fd, NULL, X_NOTIFY_NONE, NULL);
+watch->event_mask = 0;
+
+if (event_mask & SPICE_WATCH_EVENT_READ) {
+SetNotifyFd(watch->fd, watch_fd_notified, X_NOTIFY_READ, watch);
+} else if (event_mask & SPICE_WATCH_EVENT_READ) {
+SetNotifyFd(watch->fd, watch_fd_notified, X_NOTIFY_WRITE, watch);
+} else {
+DPRINTF(0, "Unexpected watch event_mask: %i", event_mask);
+return -1;
+}
+watch->event_mask = event_mask;
+
+return 0;
+}
+
+static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void 
*opaque)
+{
+SpiceWatch *watch = malloc(sizeof(SpiceWatch));


This malloc may fail, please replace malloc with
xnfalloc which stands for X no fail alloc, it works like
the glib malloc functions.


+
+DPRINTF(0, "adding %p, fd=%d",

Re: [Spice-devel] [PATCH xf86-video-qxl] Adjust xspice_wakeup_handler() prototype for building xspice with server 1.19

[Hans de Goede]

Hi,

On 18-11-16 14:04, Timo Aaltonen wrote:

On 04.10.2016 14:41, Hans de Goede wrote:

Hi,

On 03-10-16 12:04, Christophe Fergeau wrote:



On Thu, Sep 29, 2016 at 01:03:01PM +0200, Hans de Goede wrote:

Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 src/spiceqxl_main_loop.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index db89b6d..0ac1f3e 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -330,7 +330,11 @@ static int no_write_watches(Ring *w)
 return 1;
 }

+#if ABI_VIDEODRV_VERSION >= SET_ABI_VERSION(23, 0)


We have an occurrence of
#if GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 6
I'd use this here too to stay consistent (I assume they are equivalent
here).

Acked-by: Christophe Fergeau <cferg...@redhat.com>


Sorry, but this patch turns out to be incomplete, self NACK.

I just saw that spiceqxl_main_loop also uses a BlockHandler
(xspice_block_handler) and expects to be able to add
fds to watch for read activity through the xserver mainloop
by treating the 3th argument as a FD_SET.

This is no longer supported as of xserver 1.19, instead
the new NotifyFD functionality should be used. The advantage
of this is that it can also properly watch fds for them
becoming ready for writing.

For an example patch of how to use the new NotifyFD
functionality see the recent tigervnc patch to make
tigervnc work with 1.19:

https://lists.x.org/archives/xorg-devel/2016-October/051482.html


Any update here?


Nope, sorry.


Looks like you still have the original patch in Fedora ;)


Yes, because it fixes the build. Xspice is not widely used,
so no-one has noticed it is broken yet...

Regards,

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

Re: [Spice-devel] spice-gtk updates for Fedora 25 need to be added to bodhi

[Hans de Goede]

Hi,

On 10/10/2016 02:38 PM, Marc-André Lureau wrote:

Hi

- Original Message -

Hi,

On 10/10/2016 11:49 AM, Marc-André Lureau wrote:

Hi

- Original Message -

Hi All,

I noticed that there have been a few spice-gtk builds fixing
the bugs I was hitting, thank you for that.

I had to install them manually from koji, since they are
not yet listed in bodhi. Can you please create bodhi
updates for these ?  :

https://bodhi.fedoraproject.org/updates/?packages=spice-gtk


f24:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-fea866fa85
f25:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-c1bf3f1059

Is that fine?


Yes those are fine.


Btw, since you are experienced Fedora packager, could you provide some
hints on solving the errors in
https://taskotron.fedoraproject.org/artifacts/all/886bb18a-8c8d-11e6-be50-525400120b80/task_output/spice-gtk-0.33-1.fc25.log


You should be able to reproduce those by doing:

fedpkg local
rpmlint *.src.rpm x86_64

in a fedpkg spice-gtk clone.

Then go through them 1:1, fix them rinse-repeat until all (most?) are fixed.


Yes, that I know of, I was wondering if you (or someone on the list) could help 
with the actual errors.


Well some of them are quite trivial to fix:

spice-gtk.i686: W: obsolete-not-provided spice-gtk-python

Can be fixed by adding:

Provides: spice-gtk-python = %{version}-%{release} to the list
of main Requires / Provides.

And this one:

spice-gtk3-devel.armv7hl: W: obsolete-not-provided spice-gtk-devel

Can be fixed by adding a similar provides to the spice-gtk3-devel subpkg, etc.

Likewise all the:

spice-gtk-tools.i686: W: no-manual-page-for-binary spicy-screenshot

Errors can be fixed by actually adding man-pages for those binaries.

Note these 2:

spice-gtk.src: W: invalid-url URL: http://spice-space.org/page/Spice-Gtk 
spice-gtk.src: W: invalid-url Source0: 
http://www.spice-space.org/download/gtk/spice-gtk-0.33.tar.bz2 

Seem to be false positives caused by the system doing the checks not having 
network access.

So I would start with fixing all the trivial ones and then see from there. TBH 
I do not
think you can fix them all, rpmlint is not  very bright tool.

Regards,

Hans




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

Re: [Spice-devel] spice-gtk updates for Fedora 25 need to be added to bodhi

[Hans de Goede]

Hi,

On 10/10/2016 11:49 AM, Marc-André Lureau wrote:

Hi

- Original Message -

Hi All,

I noticed that there have been a few spice-gtk builds fixing
the bugs I was hitting, thank you for that.

I had to install them manually from koji, since they are
not yet listed in bodhi. Can you please create bodhi
updates for these ?  :

https://bodhi.fedoraproject.org/updates/?packages=spice-gtk


f24:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-fea866fa85
f25:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-c1bf3f1059

Is that fine?


Yes those are fine.


Btw, since you are experienced Fedora packager, could you provide some hints on 
solving the errors in 
https://taskotron.fedoraproject.org/artifacts/all/886bb18a-8c8d-11e6-be50-525400120b80/task_output/spice-gtk-0.33-1.fc25.log


You should be able to reproduce those by doing:

fedpkg local
rpmlint *.src.rpm x86_64

in a fedpkg spice-gtk clone.

Then go through them 1:1, fix them rinse-repeat until all (most?) are fixed.

Unfortunately AFAIK there is no way yet to filter out some of the 
false-positives.

Regards,

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

[Spice-devel] spice-gtk updates for Fedora 25 need to be added to bodhi

[Hans de Goede]

Hi All,

I noticed that there have been a few spice-gtk builds fixing
the bugs I was hitting, thank you for that.

I had to install them manually from koji, since they are
not yet listed in bodhi. Can you please create bodhi
updates for these ?  :

https://bodhi.fedoraproject.org/updates/?packages=spice-gtk

Regards,

Hans

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

[Spice-devel] [PATCH xf86-video-qxl] Fix crash caused by attempting to access the screen pixmap before it is created

[Hans de Goede]

qxl_resize_primary_to_virtual() was using pScrn->pScreen != NULL to check
if createScreenResources has been called. But starting with xserver 1.19
pScrn->pScreen is non NULL even before createScreenResources is called,
causing an invalid access to the screenPixmap in
qxl_resize_primary_to_virtual().

This commit fixes this.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1381045
Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 src/qxl.h| 1 +
 src/qxl_driver.c | 6 +++---
 src/qxl_kms.c| 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/qxl.h b/src/qxl.h
index 5cc8d05..885048c 100644
--- a/src/qxl.h
+++ b/src/qxl.h
@@ -234,6 +234,7 @@ struct _qxl_screen_t
 struct qxl_ring *  cursor_ring;
 struct qxl_ring *  release_ring;
 
+Boolscreen_resources_created;
 int device_primary;
 struct qxl_bo * primary_bo;
 intnum_modes;
diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index fc1b629..8aecf3c 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -530,7 +530,6 @@ qxl_create_primary(qxl_screen_t *qxl)
 Bool
 qxl_resize_primary_to_virtual (qxl_screen_t *qxl)
 {
-ScreenPtr pScreen;
 long new_surface0_size;
 
 if ((qxl->primary_mode.x_res == qxl->virtual_x &&
@@ -566,9 +565,9 @@ qxl_resize_primary_to_virtual (qxl_screen_t *qxl)
 qxl->primary = qxl_create_primary(qxl);
 qxl->bytes_per_pixel = (qxl->pScrn->bitsPerPixel + 7) / 8;
 
-pScreen = qxl->pScrn->pScreen;
-if (pScreen)
+if (qxl->screen_resources_created)
 {
+ScreenPtr pScreen = qxl->pScrn->pScreen;
PixmapPtr root = pScreen->GetScreenPixmap (pScreen);
 
 if (qxl->deferred_fps <= 0)
@@ -645,6 +644,7 @@ qxl_create_screen_resources (ScreenPtr pScreen)
 qxl_create_desired_modes (qxl);
 qxl_update_edid (qxl);
 
+qxl->screen_resources_created = TRUE;
 return TRUE;
 }
 
diff --git a/src/qxl_kms.c b/src/qxl_kms.c
index fe37af0..d11b20e 100644
--- a/src/qxl_kms.c
+++ b/src/qxl_kms.c
@@ -235,6 +235,7 @@ qxl_create_screen_resources_kms(ScreenPtr pScreen)
 if (!uxa_resources_init (pScreen))
return FALSE;
 
+qxl->screen_resources_created = TRUE;
 return TRUE;
 }
 
-- 
2.9.3

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

Re: [Spice-devel] [PATCH xf86-video-qxl] Adjust xspice_wakeup_handler() prototype for building xspice with server 1.19

[Hans de Goede]

Hi,

On 03-10-16 12:04, Christophe Fergeau wrote:



On Thu, Sep 29, 2016 at 01:03:01PM +0200, Hans de Goede wrote:

Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 src/spiceqxl_main_loop.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index db89b6d..0ac1f3e 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -330,7 +330,11 @@ static int no_write_watches(Ring *w)
 return 1;
 }

+#if ABI_VIDEODRV_VERSION >= SET_ABI_VERSION(23, 0)


We have an occurrence of
#if GET_ABI_MAJOR(ABI_VIDEODRV_VERSION) < 6
I'd use this here too to stay consistent (I assume they are equivalent
here).

Acked-by: Christophe Fergeau <cferg...@redhat.com>


Sorry, but this patch turns out to be incomplete, self NACK.

I just saw that spiceqxl_main_loop also uses a BlockHandler
(xspice_block_handler) and expects to be able to add
fds to watch for read activity through the xserver mainloop
by treating the 3th argument as a FD_SET.

This is no longer supported as of xserver 1.19, instead
the new NotifyFD functionality should be used. The advantage
of this is that it can also properly watch fds for them
becoming ready for writing.

For an example patch of how to use the new NotifyFD
functionality see the recent tigervnc patch to make
tigervnc work with 1.19:

https://lists.x.org/archives/xorg-devel/2016-October/051482.html

Regards,

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

[Spice-devel] Issues with xf86-video-xql with xorg-1.19

[Hans de Goede]

Hi All,

See: https://bugzilla.redhat.com/show_bug.cgi?id=1381045

Also why is Alon still the maintainer of xorg-x11-drv-qxl in
Fedora ?  It would IMHO be better if the spice-team would
maintain it.

Regards,

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

[Spice-devel] [PATCH xf86-video-qxl] Adjust xspice_wakeup_handler() prototype for building xspice with server 1.19

[Hans de Goede]

Signed-off-by: Hans de Goede <hdego...@redhat.com>
---
 src/spiceqxl_main_loop.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
index db89b6d..0ac1f3e 100644
--- a/src/spiceqxl_main_loop.c
+++ b/src/spiceqxl_main_loop.c
@@ -330,7 +330,11 @@ static int no_write_watches(Ring *w)
 return 1;
 }
 
+#if ABI_VIDEODRV_VERSION >= SET_ABI_VERSION(23, 0)
+static void xspice_wakeup_handler(void *data, int nfds)
+#else
 static void xspice_wakeup_handler(pointer data, int nfds, pointer readmask)
+#endif
 {
 if (!nfds && no_write_watches()) {
 return;
-- 
2.9.3

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

[Spice-devel] spice-gtk is broken in Fedora 25

[Hans de Goede]

Hi,

I needed a vm to test something and I noticed that spice-gtk
appears to be broken in F25. As long as scaling between the
guest resolution and the window size is needed everything
works fine, but as soon as the agent adjusts the guest
resolution to exactly match the window size, the display
stops updating.

Regards,

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

[Spice-devel] Fwd: where do usbredir patches go nowadays?

[Hans de Goede]

 Forwarded Message 
Subject: where do usbredir patches go nowadays?
Date: Mon, 18 Jul 2016 17:23:44 +0300
From: Alon Levy <a...@pobox.com>
To: Hans de Goede <hdego...@redhat.com>

Well, ok, so I'm taking this time to say hi - you seem to be having good
GPU fun these days.

>From 132286d0972914cd36c586be6b9b6fa3d0ee982c Mon Sep 17 00:00:00 2001
From: Alon Levy <a...@pobox.com>
Date: Mon, 18 Jul 2016 17:13:27 +0300
Subject: [PATCH] usbredirparser: prevent endless recursion if interface_count
 == 0

On fedora 24 this function is tail optimized, resulting in a busy wait.

usbredir-0.7.1-2.fc24.x86_64
---
 usbredirparser/usbredirfilter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/usbredirparser/usbredirfilter.c b/usbredirparser/usbredirfilter.c
index 02184ef..bdfbfc2 100644
--- a/usbredirparser/usbredirfilter.c
+++ b/usbredirparser/usbredirfilter.c
@@ -205,7 +205,7 @@ int usbredirfilter_check(
  * by recursively calling this function with a flag that forbids
  * skipping (usbredirfilter_fl_dont_skip_non_boot_hid)
  */
-if (num_skipped == interface_count) {
+if (interface_count > 0 && num_skipped == interface_count) {
 rc = usbredirfilter_check(rules, rules_count,
   device_class, device_subclass, device_protocol,
   interface_class, interface_subclass,
-- 
2.7.4

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

Re: [Spice-devel] [spice-gtk v3 4/4] usb-device-manager: Avoid USB event thread leak

[Hans de Goede]

Hi,

On 30-06-16 18:02, Christophe Fergeau wrote:

On Thu, Jun 30, 2016 at 04:54:53PM +0200, Hans de Goede wrote:

Hi,

The entire series looks good to me now, one remark wrt this patch,
with that fixed this series is:

Reviewed-by: Hans de Goede <hdego...@redhat.com>


On 30-06-16 15:40, Christophe Fergeau wrote:

This is a follow-up of the previous commit ('usb-channel: Really stop
listening for USB events on disconnection').

Since the USB event thread has to be stopped when we destroy the
associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
should force event_thread_run to FALSE even if
spice_usb_device_manager_stop_event_listening() was not enough. When
this happens, this means that there is a bug in the internal users of
spice_usb_device_manager_start_event_listening(), but with this change,
we'll at least warn about it, and avoid a thread leak/potential future
crash.
---
Unchanged since v2 apart from the commit log.


 src/usb-device-manager.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 808ec6c..33818c2 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -375,6 +375,15 @@ static void spice_usb_device_manager_dispose(GObject 
*gobject)
 #ifdef USE_LIBUSB_HOTPLUG
 if (priv->hp_handle) {
 spice_usb_device_manager_stop_event_listening(self);
+if (g_atomic_int_get(>event_thread_run)) {
+/* Force termination of the event thread even if there were some
+ * mismatched spice_usb_device_manager_{start,stop}_event_listening
+ * calls. Otherwise, the usb event thread will be leaked, and will
+ * try to use the libusb context we destroy in finalize(), which 
would
+ * cause a crash */
+ g_warn_if_reached();
+ g_atomic_int_set(>event_thread_run, FALSE);
+}
 /* This also wakes up the libusb_handle_events() in the event_thread */
 libusb_hotplug_deregister_callback(priv->context, priv->hp_handle);
 priv->hp_handle = 0;



I would move this to outside the #ifdef USE_LIBUSB_HOTPLUG / if 
(priv->hp_handle) {}

You will also want the warn_if_reached and g_atomic_int_set(..., FALSE) when
these are not true.


We need to set event_thread_run to FALSE before calling
libusb_hotplug_deregister_callback() otherwise the thread will not exit
properly libusb_handle_events() will return because
deregister_callback() was called, but event_thread_run is still TRUE, so
the thread does not exit, and we'll be back to waiting on
libusb_handle_events()...


Ah right, good point.


You should also remove the !g_atomic_int_get(>event_thread_run) from the
if below the #endif since you force that to false now, so that part of the
if is useless.


However, for the !USE_LIBUSB_HOTPLUG case, we don't need to force set
event_thread_run to FALSE if we remove the event_thread_run test from
the 'if' condition. We can just add a warning there if event_thread_run
was not FALSE. However, if this triggers, the g_thread_join() is likely
to hang as we don't have anything in dispose() which would force
libusb_handle_events() to return.

Proposed change to squash in this patch:


diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 806af74..aa48a01 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -390,7 +391,8 @@ static void spice_usb_device_manager_dispose(GObject 
*gobject)
 priv->hp_handle = 0;
 }
 #endif
-if (priv->event_thread && !g_atomic_int_get(>event_thread_run)) {
+if (priv->event_thread) {
+g_warn_if_fail(g_atomic_int_get(>event_thread_run) == FALSE);
 g_thread_join(priv->event_thread);
 priv->event_thread = NULL;
 }


Yeah that is probably the best we can do.

So with this squashed in the entire series is:

Reviewed-by: Hans de Goede <hdego...@redhat.com>

Regards,

Hans

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

Re: [Spice-devel] [spice-gtk v3 4/4] usb-device-manager: Avoid USB event thread leak

[Hans de Goede]

Hi,

The entire series looks good to me now, one remark wrt this patch,
with that fixed this series is:

Reviewed-by: Hans de Goede <hdego...@redhat.com>


On 30-06-16 15:40, Christophe Fergeau wrote:

This is a follow-up of the previous commit ('usb-channel: Really stop
listening for USB events on disconnection').

Since the USB event thread has to be stopped when we destroy the
associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
should force event_thread_run to FALSE even if
spice_usb_device_manager_stop_event_listening() was not enough. When
this happens, this means that there is a bug in the internal users of
spice_usb_device_manager_start_event_listening(), but with this change,
we'll at least warn about it, and avoid a thread leak/potential future
crash.
---
Unchanged since v2 apart from the commit log.


 src/usb-device-manager.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 808ec6c..33818c2 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -375,6 +375,15 @@ static void spice_usb_device_manager_dispose(GObject 
*gobject)
 #ifdef USE_LIBUSB_HOTPLUG
 if (priv->hp_handle) {
 spice_usb_device_manager_stop_event_listening(self);
+if (g_atomic_int_get(>event_thread_run)) {
+/* Force termination of the event thread even if there were some
+ * mismatched spice_usb_device_manager_{start,stop}_event_listening
+ * calls. Otherwise, the usb event thread will be leaked, and will
+ * try to use the libusb context we destroy in finalize(), which 
would
+ * cause a crash */
+ g_warn_if_reached();
+ g_atomic_int_set(>event_thread_run, FALSE);
+}
 /* This also wakes up the libusb_handle_events() in the event_thread */
 libusb_hotplug_deregister_callback(priv->context, priv->hp_handle);
 priv->hp_handle = 0;



I would move this to outside the #ifdef USE_LIBUSB_HOTPLUG / if 
(priv->hp_handle) {}

You will also want the warn_if_reached and g_atomic_int_set(..., FALSE) when
these are not true.

You should also remove the !g_atomic_int_get(>event_thread_run) from the
if below the #endif since you force that to false now, so that part of the
if is useless.

Regards,

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

Re: [Spice-devel] [spice-gtk 3/3] usb-device-manager: Fix USB event thread leak

[Hans de Goede]

Hi,

On 30-06-16 13:08, Hans de Goede wrote:

Hi,

On 30-06-16 09:08, Christophe Fergeau wrote:

On Wed, Jun 29, 2016 at 07:29:18PM +0200, Hans de Goede wrote:

Erm no, it is the caller's responsibility to make sure that all
spice_usbredir_channel's are properly torn down before the
usbdevicemanager gets torn down.

Otherwise you're not just having the problem of the thread continuing
to run, but also you still have open usbfs file-descriptors and any
redirected devices will not become available to the client-os again
(think e.g. virt-manager with multiple spice ontexts).

The proper fix is to fix the channel already being set to NULL,
without first calling spice_usbredir_channel_disconnect_device().


Yeah, I agree with most of this, however if the API is misused (and
actually it has been for years...), it still much better to cleanup the
thread rather than leaving it hanging around until it triggers a crash.

However, I should have made this commit more explicit that something bad
happened first, ie:

spice_usb_device_manager_stop_event_listening(self);

if (g_atomic_int_get(>event_thread_run)) {
/* Force termination of the event thread even if there were some 
mismatched
 * spice_usb_device_manager_{start,stop}_event_listening
 * calls. This should not happen if the API is properly
 * used, but if it's not, this will avoid a crash/thread
 * leak */
g_warn_if_reached();
g_atomic_int_set(>event_thread_run, FALSE);
}
(I'll send a v2 doing that, and add more precisions to the commit log
that it's not a real fix)


In this case spice_usbredir_channel_disconnect_device() is called with a
non-NULL channel:

#0  0x7fc80b449dc4 in spice_usbredir_channel_disconnect_device
(channel=0x55aa2bc55060 [SpiceUsbredirChannel]) at
channel-usbredir.c:459
#1  0x7fc80b449fb3 in _disconnect_device_thread (task=0x55aa2c7ad910
[GTask], object=0x55aa2bc55060, task_data=0x0, cancellable=0x0) at
channel-usbredir.c:508
#2  0x7fc83f0fab10 in g_task_thread_pool_thread
(thread_data=0x55aa2c7ad910, pool_data=) at gtask.c:1288
#3  0x7fc83f875735 in g_thread_pool_thread_proxy (data=) at gthreadpool.c:307
#4  0x7fc83f874d38 in g_thread_proxy (data=0x55aa2c93fed0) at
gthread.c:780
#5  0x7fc84244f5ca in start_thread (arg=0x7fc8193b3700) at
pthread_create.c:333
#6  0x7fc841a77ead in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109


but SpiceChannel::session has already been set to NULL, so this specific
code block is not going to run:

SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(channel));
if (session != NULL)

spice_usb_device_manager_stop_event_listening(spice_usb_device_manager_get(session,
 NULL));

The session is set to NULL through:

#0  0x7fc80b4221ed in spice_session_channel_destroy (session=0x55aa2ba69fa0 
[SpiceSession], channel=0x55aa2bc55060 [SpiceUsbredirChannel]) at 
spice-session.c:2280
#1  0x7fc80b41c70e in session_disconnect (self=0x55aa2ba69fa0 
[SpiceSession], keep_main=0)
at spice-session.c:314
#2  0x7fc80b420a3b in session_disconnect_idle (self=0x55aa2ba69fa0 
[SpiceSession])
at spice-session.c:1908
#3  0x7fc83f84e703 in g_main_context_dispatch (context=0x55aa2b09d320) at 
gmain.c:3154


I"m a bit wary of touching the device disconnection/channel teardown code paths
though as I'm not familiar with them :((


So it seems there are 2 problems:

1) The existence of the "if (session)" in:

   if (session != NULL)
  
spice_usb_device_manager_stop_event_listening(spice_usb_device_manager_get(session,
 NULL));

git blame gives me:

https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=e3932bfebbfec7637f3d03d90e8f9b75e3223236

Which says this was done to fix a segfault. Which clearly is a bad idea,
just adding if (pointer) checks to avoid segfaults is not the way to fix
bugs people!

So to fix this we need to root cause said segfault, fix it and then revert this
commit.

2) spice_usbredir_channel_disconnect_device now running in a thread, rather
then being a synchronous part of the tear-down sequence, this can either be
due to someone calling spice_usbredir_channel_disconnect_device_async() or
someone calling spice_usbredir_channel_reset(). Note that if either happens
just before session_disconnect() gets called we've a problem as then
the channel will be torn down while disconnect is running async, looking
at your backtraces that is exactly what is happening. This all seems to be
fallout from making usb device connect / disconnect async because windows
is oh so terribly slow with this.  Since you cannot fix windows, you at
least not to make sure that this all gets to run its course properly
on shutdown / session close.  Note that things being able to run their
course properly is also important to get the windows filter driver reconfigured
to give the redirected usb device back to windows proper / to rebind th

Re: [Spice-devel] [spice-gtk 3/3] usb-device-manager: Fix USB event thread leak

[Hans de Goede]

Hi,

On 30-06-16 09:08, Christophe Fergeau wrote:

On Wed, Jun 29, 2016 at 07:29:18PM +0200, Hans de Goede wrote:

Erm no, it is the caller's responsibility to make sure that all
spice_usbredir_channel's are properly torn down before the
usbdevicemanager gets torn down.

Otherwise you're not just having the problem of the thread continuing
to run, but also you still have open usbfs file-descriptors and any
redirected devices will not become available to the client-os again
(think e.g. virt-manager with multiple spice ontexts).

The proper fix is to fix the channel already being set to NULL,
without first calling spice_usbredir_channel_disconnect_device().


Yeah, I agree with most of this, however if the API is misused (and
actually it has been for years...), it still much better to cleanup the
thread rather than leaving it hanging around until it triggers a crash.

However, I should have made this commit more explicit that something bad
happened first, ie:

spice_usb_device_manager_stop_event_listening(self);

if (g_atomic_int_get(>event_thread_run)) {
/* Force termination of the event thread even if there were some 
mismatched
 * spice_usb_device_manager_{start,stop}_event_listening
 * calls. This should not happen if the API is properly
 * used, but if it's not, this will avoid a crash/thread
 * leak */
g_warn_if_reached();
g_atomic_int_set(>event_thread_run, FALSE);
}
(I'll send a v2 doing that, and add more precisions to the commit log
that it's not a real fix)


In this case spice_usbredir_channel_disconnect_device() is called with a
non-NULL channel:

#0  0x7fc80b449dc4 in spice_usbredir_channel_disconnect_device
(channel=0x55aa2bc55060 [SpiceUsbredirChannel]) at
channel-usbredir.c:459
#1  0x7fc80b449fb3 in _disconnect_device_thread (task=0x55aa2c7ad910
[GTask], object=0x55aa2bc55060, task_data=0x0, cancellable=0x0) at
channel-usbredir.c:508
#2  0x7fc83f0fab10 in g_task_thread_pool_thread
(thread_data=0x55aa2c7ad910, pool_data=) at gtask.c:1288
#3  0x7fc83f875735 in g_thread_pool_thread_proxy (data=) at gthreadpool.c:307
#4  0x7fc83f874d38 in g_thread_proxy (data=0x55aa2c93fed0) at
gthread.c:780
#5  0x7fc84244f5ca in start_thread (arg=0x7fc8193b3700) at
pthread_create.c:333
#6  0x7fc841a77ead in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109


but SpiceChannel::session has already been set to NULL, so this specific
code block is not going to run:

SpiceSession *session = spice_channel_get_session(SPICE_CHANNEL(channel));
if (session != NULL)

spice_usb_device_manager_stop_event_listening(spice_usb_device_manager_get(session,
 NULL));

The session is set to NULL through:

#0  0x7fc80b4221ed in spice_session_channel_destroy (session=0x55aa2ba69fa0 
[SpiceSession], channel=0x55aa2bc55060 [SpiceUsbredirChannel]) at 
spice-session.c:2280
#1  0x7fc80b41c70e in session_disconnect (self=0x55aa2ba69fa0 
[SpiceSession], keep_main=0)
at spice-session.c:314
#2  0x7fc80b420a3b in session_disconnect_idle (self=0x55aa2ba69fa0 
[SpiceSession])
at spice-session.c:1908
#3  0x7fc83f84e703 in g_main_context_dispatch (context=0x55aa2b09d320) at 
gmain.c:3154


I"m a bit wary of touching the device disconnection/channel teardown code paths
though as I'm not familiar with them :((


So it seems there are 2 problems:

1) The existence of the "if (session)" in:

   if (session != NULL)
  
spice_usb_device_manager_stop_event_listening(spice_usb_device_manager_get(session,
 NULL));

git blame gives me:

https://cgit.freedesktop.org/spice/spice-gtk/commit/?id=e3932bfebbfec7637f3d03d90e8f9b75e3223236

Which says this was done to fix a segfault. Which clearly is a bad idea,
just adding if (pointer) checks to avoid segfaults is not the way to fix
bugs people!

So to fix this we need to root cause said segfault, fix it and then revert this
commit.

2) spice_usbredir_channel_disconnect_device now running in a thread, rather
then being a synchronous part of the tear-down sequence, this can either be
due to someone calling spice_usbredir_channel_disconnect_device_async() or
someone calling spice_usbredir_channel_reset(). Note that if either happens
just before session_disconnect() gets called we've a problem as then
the channel will be torn down while disconnect is running async, looking
at your backtraces that is exactly what is happening. This all seems to be
fallout from making usb device connect / disconnect async because windows
is oh so terribly slow with this.  Since you cannot fix windows, you at
least not to make sure that this all gets to run its course properly
on shutdown / session close.  Note that things being able to run their
course properly is also important to get the windows filter driver reconfigured
to give the redirected usb device back to windows proper / to rebind the
linux kernel driver to the usb device. Witho

Re: [Spice-devel] [spice-gtk 2/3] usbredir: Use atomic for UsbDeviceManager::event_thread_run

[Hans de Goede]

Hi,

On 30-06-16 09:13, Christophe Fergeau wrote:

On Wed, Jun 29, 2016 at 07:20:16PM +0200, Hans de Goede wrote:

Hi,

On 29-06-16 17:42, Christophe Fergeau wrote:

This variable is accessed from 2 different threads (main thread and USB
event thread), so some care must be taken to read/write it.


The event-thread only reads it, so I believe there is no need for this.


https://en.wikipedia.org/wiki/Memory_ordering#Runtime_memory_ordering
says that on most architectures stores can be reordered after loads


Right, the "bible" on this is:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/memory-barriers.txt

As this mostly is a kernel problem, as such I'm not aware of any
userspace code dealing with this, and the while(run) syntax is quite
normal.

Given that libusb_handle_events takes multiple locks it will act
as a full barrier and you do not really need atomics.

> I'd

read this as: main thread sets event_thread_run to FALSE, calls
libusb_hotplug_deregister_callback(), usb event thread returns from
libusb_handle_events(), reads event_thread_run, but reads the old value
(TRUE) rather than the FALSE causing it to exit. I agree that it's
likely that either libusb_hotplug_deregister_callback() and/or
libusb_handle_events() are going to trigger some barrier, but it feels
safer to me to handle the concurrency ourselves.


It certainly cannot hurt but it is not necessary.

>> event_thread_run is a bool, you should make it a gint, probably even 
volatile.
>
> I've changed it to gint in v2, I don't know about the volatile, are
> there cases where not having it as a volatile is going to cause issues?
> Code using GAtomic in glib does not seem to be using volatile.

If you use atomic accesses you definitely do not need to use volatile.

Volatile is needed for something like this (typical cs example):

int event_ready;

thread_run_function()
{
while (true) {
while (!event_ready) {}
handle_event();
}
}

Which the compile will typically optimize too:

thread_run_function()
{
while (true) {
if (!event_ready)
while (true) {}
handle_event();
}
}

At which point the thread will just hang
if event_ready is false once, marking
event_ready volatile fixes this as it tells
the compiler to not optimize away any
event_ready accesses (be it reads or writes).

Note that this example is a bit microcontroller
oriented under a modern os one would typically
do e.g.:

thread_run_function()
{
while (true) {
while (!event_ready)
msleep(10);
handle_event();
}
}

To relax the cpu, and the compile does cannot know
if the msleep() call does or does not touch the
event_ready global, so it will not do the troublesome
optimization, assuming that msleep() may modify it.


Another textbook example is:

u8 *iomem;

iomem[5] = 7;
iomem[5] = 8;

Which the compiler will optimize to:

iomem[5] = 8;

Which when talking to actually memory mapped io
which you're trying to run through various internal
states is bad, again volatile fixes this.

Regards,

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

Re: [Spice-devel] [spice-gtk 3/3] usb-device-manager: Fix USB event thread leak

[Hans de Goede]

Hi,

On 29-06-16 17:42, Christophe Fergeau wrote:

When using USB redirection, it's fairly easy to leak the thread handling
USB events, which will eventually cause problems in long lived apps.
In particular, in virt-manager, one can:
- start a VM
- connect to it with SPICE
- open the USB redirection window
- redirect a device
- close the SPICE window
-> the SpiceUsbDeviceManager instance will be destroyed (including the
USB context it owns), but the associated event thread will keep running.
Since it's running a loop blocking on libusb_handle_events(priv->context),
the loop will eventually try to use the USB context we just destroyed
causing a crash.

We can get in this situation when redirecting a USB device because we
will call spice_usb_device_manager_start_event_listening() in
spice_usbredir_channel_open_device(). The matching
spice_usb_device_manager_stop_event_listening() call is supposed to
happen in spice_usbredir_channel_disconnect_device(), however by the
time it's called in the scenario described above, the session associated
with the channel will already have been set to NULL in
spice_session_channel_destroy().

Since the USB event thread has to be stopped when we destroy the
associated SpiceUsbDeviceManager, spice_usb_device_manager_dispose()
should force event_thread_run to FALSE


Erm no, it is the caller's responsibility to make sure that all
spice_usbredir_channel's are properly torn down before the
usbdevicemanager gets torn down.

Otherwise you're not just having the problem of the thread continuing
to run, but also you still have open usbfs file-descriptors and any
redirected devices will not become available to the client-os again
(think e.g. virt-manager with multiple spice ontexts).

The proper fix is to fix the channel already being set to NULL,
without first calling spice_usbredir_channel_disconnect_device().

Regards,

Hans




 rather than calling

spice_usb_device_manager_stop_event_listening() which will only set it
to FALSE if this was the only user of the event thread.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1217202
(virt-manager) and most
likely https://bugzilla.redhat.com/show_bug.cgi?id=1337007 (gnome-boxes)
as well.
---
 src/usb-device-manager.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 1912b62..2b10be2 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -374,7 +374,9 @@ static void spice_usb_device_manager_dispose(GObject 
*gobject)

 #ifdef USE_LIBUSB_HOTPLUG
 if (priv->hp_handle) {
-spice_usb_device_manager_stop_event_listening(self);
+/* Force termination of the event thread even if there were some 
mismatched
+ * spice_usb_device_manager_{start,stop}_event_listening calls */
+g_atomic_int_set(>event_thread_run, FALSE);
 /* This also wakes up the libusb_handle_events() in the event_thread */
 libusb_hotplug_deregister_callback(priv->context, priv->hp_handle);
 priv->hp_handle = 0;


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

Re: [Spice-devel] [spice-gtk 2/3] usbredir: Use atomic for UsbDeviceManager::event_thread_run

[Hans de Goede]

Hi,

On 29-06-16 17:42, Christophe Fergeau wrote:

This variable is accessed from 2 different threads (main thread and USB
event thread), so some care must be taken to read/write it.


The event-thread only reads it, so I believe there is no need for this.

Regards,

Hans



---
 src/usb-device-manager.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 1fc8fc1..1912b62 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -1280,7 +1280,7 @@ static gpointer 
spice_usb_device_manager_usb_ev_thread(gpointer user_data)
 SpiceUsbDeviceManagerPrivate *priv = self->priv;
 int rc;

-while (priv->event_thread_run) {
+while (g_atomic_int_get(>event_thread_run)) {
 rc = libusb_handle_events(priv->context);
 if (rc && rc != LIBUSB_ERROR_INTERRUPTED) {
 const char *desc = spice_usbutil_libusb_strerror(rc);
@@ -1310,7 +1310,7 @@ gboolean spice_usb_device_manager_start_event_listening(
  g_thread_join(priv->event_thread);
  priv->event_thread = NULL;
 }
-priv->event_thread_run = TRUE;
+g_atomic_int_set(>event_thread_run, TRUE);
 priv->event_thread = g_thread_new("usb_ev_thread",
   spice_usb_device_manager_usb_ev_thread,
   self);
@@ -1326,7 +1326,7 @@ void spice_usb_device_manager_stop_event_listening(

 priv->event_listeners--;
 if (priv->event_listeners == 0)
-priv->event_thread_run = FALSE;
+g_atomic_int_set(>event_thread_run, FALSE);
 }

 static void spice_usb_device_manager_check_redir_on_connect(


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

Re: [Spice-devel] [usbredir][PATCH] usbredirhost: Fix -Wformat warning

[Hans de Goede]

Hi,

On 03-11-15 01:52, Fabiano Fidêncio wrote:

On Tue, Nov 3, 2015 at 1:51 AM, Fabiano Fidêncio  wrote:

Cast uint64_t to long unsigned on printfs in order to avoid warnings
like:
usbredirhost.c: In function 'usbredirhost_can_write_iso_package':
usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
  DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
  ^
usbredirhost.c:1023:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
  DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
  ^
usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
  DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
  ^
usbredirhost.c:1028:19: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
  DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
  ^
usbredirhost.c: In function 'usbredirhost_set_iso_threshold':
usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 4 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)
  ^
usbredirhost.c:1162:11: warning: format '%lu' expects argument of type 'long 
unsigned int', but argument 5 has type 'uint64_t {aka long long unsigned int}' 
[-Wformat=]
  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
^
usbredirhost.c:181:57: note: in definition of macro 'DEBUG'
  #define DEBUG(...)   va_log(host, usbredirparser_debug, __VA_ARGS__)

A better way to solve the problem would be using %zu (C99 only) instead
of doing the cast, but then mingw32 complains about:
"warning: unknown conversion type character 'z' in format [-Wformat=]"

Signed-off-by: Fabiano Fidêncio 
---
  usbredirhost/usbredirhost.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
index adf9c5f..bfccfab 100644
--- a/usbredirhost/usbredirhost.c
+++ b/usbredirhost/usbredirhost.c
@@ -1021,12 +1021,14 @@ static int usbredirhost_can_write_iso_package(struct 
usbredirhost *host)
  if (size >= host->iso_threshold.higher) {
  if (!host->iso_threshold.dropping)
  DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
-  size, host->iso_threshold.higher);
+  (long unsigned) size,
+  (long unsigned) host->iso_threshold.higher);
  host->iso_threshold.dropping = true;
  } else if (size < host->iso_threshold.lower) {
  if (host->iso_threshold.dropping)
  DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
-  size, host->iso_threshold.lower);
+  (long unsigned) size,
+  (long unsigned) host->iso_threshold.lower);

  host->iso_threshold.dropping = false;
  }
@@ -1160,7 +1162,8 @@ static void usbredirhost_set_iso_threshold(struct 
usbredirhost *host,
  host->iso_threshold.lower = reference / 2;
  host->iso_threshold.higher = reference * 3;
  DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
-   host->iso_threshold.higher, host->iso_threshold.lower);
+   (long unsigned) host->iso_threshold.higher,
+   (long unsigned) host->iso_threshold.lower);
  }

  /* Called from both parser read and packet complete callbacks */
--
2.5.0



Adding Hans to the loop ...


Thanks.

The 

Re: [Spice-devel] [spice-gtk PATCH v6 2/2] channel-usbredir: drop isoc packets on low bandwidth

[Hans de Goede]

Hi,

On 10/23/2015 10:59 AM, Fabiano Fidêncio wrote:

On Fri, Oct 23, 2015 at 9:53 AM, Victor Toso  wrote:

When channel wants to send much more data then the wire can handle, the
queue grows fast. This patch does not limit the queue growth but
introduces an internal API to check if queue size is too big.

This internal API is used in usbredir_buffered_output_size_callback
which is called before any isoc pacaket is queued in usbredir. The
usbredir implements the logic to:
  - only drop isoc packets
  - while dropping packtes does still give us video frames from and above
10fps streams

An easy way to test locally is sharing and webcam and with tc:
 tc qdisc add dev lo root netem delay 100ms
 tc qdisc change dev lo root netem delay 1000ms
 tc qdisc del dev lo root netem

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
---
  src/channel-usbredir.c   |  9 +
  src/spice-channel-priv.h |  2 ++
  src/spice-channel.c  | 19 ++-
  3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 89f5c9d..dbcf0af 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -91,6 +91,7 @@ static void usbredir_log(void *user_data, int level, const 
char *msg);
  static int usbredir_read_callback(void *user_data, uint8_t *data, int count);
  static int usbredir_write_callback(void *user_data, uint8_t *data, int count);
  static void usbredir_write_flush_callback(void *user_data);
+static uint64_t usbredir_buffered_output_size_callback(void *user_data);

  static void *usbredir_alloc_lock(void);
  static void usbredir_lock_lock(void *user_data);
@@ -224,6 +225,8 @@ void 
spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
 usbredirhost_fl_write_cb_owns_buffer);
  if (!priv->host)
  g_error("Out of memory allocating usbredirhost");
+
+usbredirhost_set_buffered_output_size_cb(priv->host, 
usbredir_buffered_output_size_callback);
  }

  static gboolean spice_usbredir_channel_open_device(
@@ -461,6 +464,12 @@ void spice_usbredir_channel_get_guest_filter(
  /* -- */
  /* callbacks (any context)*/

+static uint64_t usbredir_buffered_output_size_callback(void *user_data)
+{
+g_return_val_if_fail(SPICE_IS_USBREDIR_CHANNEL(user_data), 0);
+return spice_channel_get_queue_size(SPICE_CHANNEL(user_data));
+}
+
  /* Note that this function must be re-entrant safe, as it can get called
 from both the main thread as well as from the usb event handling thread */
  static void usbredir_write_flush_callback(void *user_data)
diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
index 436a521..4b2d1e6 100644
--- a/src/spice-channel-priv.h
+++ b/src/spice-channel-priv.h
@@ -111,6 +111,7 @@ struct _SpiceChannelPrivate {
  gbooleanxmit_queue_blocked;
  STATIC_MUTEXxmit_queue_lock;
  guint   xmit_queue_wakeup_id;
+guint64 xmit_queue_size;

  charname[16];
  enum spice_channel_statestate;
@@ -171,6 +172,7 @@ void spice_channel_wakeup(SpiceChannel *channel, gboolean 
cancel);

  SpiceSession* spice_channel_get_session(SpiceChannel *channel);
  enum spice_channel_state spice_channel_get_state(SpiceChannel *channel);
+guint64 spice_channel_get_queue_size (SpiceChannel *channel);

  /* coroutine context */
  typedef void (*handler_msg_in)(SpiceChannel *channel, SpiceMsgIn *msg, 
gpointer data);
diff --git a/src/spice-channel.c b/src/spice-channel.c
index 2ce52c7..8b6125f 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -697,10 +697,12 @@ void spice_msg_out_send(SpiceMsgOut *out)
  {
  SpiceChannelPrivate *c;
  gboolean was_empty;
+guint32 size;

  g_return_if_fail(out != NULL);
  g_return_if_fail(out->channel != NULL);
  c = out->channel->priv;
+size = spice_marshaller_get_total_size(out->marshaller);

  STATIC_MUTEX_LOCK(c->xmit_queue_lock);
  if (c->xmit_queue_blocked) {
@@ -710,6 +712,7 @@ void spice_msg_out_send(SpiceMsgOut *out)

  was_empty = g_queue_is_empty(>xmit_queue);
  g_queue_push_tail(>xmit_queue, out);
+c->xmit_queue_size = (was_empty) ? size : c->xmit_queue_size + size;

  /* One wakeup is enough to empty the entire queue -> only do a wakeup
 if the queue was empty, and there isn't one pending already. */
@@ -2104,8 +2107,11 @@ static void spice_channel_iterate_write(SpiceChannel 
*channel)
  STATIC_MUTEX_LOCK(c->xmit_queue_lock);
  out = g_queue_pop_head(>xmit_queue);
  STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
-if (out)
+if (out) {
+guint32 size = spice_marshaller_get_total_size(out->marshaller);
+c->xmit_queue_size = (c->xmit_queue_size < 

Re: [Spice-devel] [PATCH 1/1] Allow missing capabilities from source host

[Hans de Goede]

Hi,

On 10/23/2015 03:09 PM, Christophe Fergeau wrote:

Hey,

On Mon, Sep 21, 2015 at 02:13:44PM +0100, Dr. David Alan Gilbert (git) wrote:

From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>

When loading a USB redirection stream during a qemu
migration, the source QEMU might be earlier and be missing
a bunch of capabilities that are now available in a more modern
version; allow this migration to work as long as the source
isn't claiming any capabilities that we don't have.

(We should be a bit more careful about this in future in qemu;
we could tie any new capabilities we ask for to machine types).


I don't think this was picked up? Hans, are you planning to pick this
up,



IIRC I gave my reviewed-by for this already, in case I did not here it is:

Reviewed-by: Hans de Goede <hdego...@redhat.com>

Anyone in the spice group should be able to push to the usbredir repo...

Regards,

Hans



or do you need help with the reviewing/testing?

Thanks,

Christophe



Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
---
  usbredirparser/usbredirparser.c | 22 ++
  1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/usbredirparser/usbredirparser.c b/usbredirparser/usbredirparser.c
index 8076b72..d1f9850 100644
--- a/usbredirparser/usbredirparser.c
+++ b/usbredirparser/usbredirparser.c
@@ -1682,10 +1682,24 @@ int usbredirparser_unserialize(struct usbredirparser 
*parser_pub,
  memcpy(orig_caps, parser->our_caps, i);
  if (unserialize_data(parser, , , , , "our_caps"))
  return -1;
-if (memcmp(parser->our_caps, orig_caps,
-   USB_REDIR_CAPS_SIZE * sizeof(int32_t)) != 0) {
-ERROR("error unserialize caps mismatch");
-return -1;
+for (i =0; i < USB_REDIR_CAPS_SIZE; i++) {
+if (parser->our_caps[i] != orig_caps[i]) {
+/* orig_caps is our original settings
+ * parser->our_caps is off the wire.
+ * We want to allow reception from an older
+ * usbredir that doesn't have all our features.
+ */
+if (parser->our_caps[i] & ~orig_caps[i]) {
+/* Source has a cap we don't */
+ERROR("error unserialize caps mismatch ours: %x recv: %x",
+  orig_caps[i], parser->our_caps[i]);
+return -1;
+} else {
+/* We've got a cap the source doesn't - that's OK */
+WARNING("unserialize missing some caps; ours: %x recv: %x",
+  orig_caps[i], parser->our_caps[i]);
+}
+}
  }

  data = (uint8_t *)parser->peer_caps;
--
2.5.0

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

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

Re: [Spice-devel] [usbredir PATCH v6 1/2] usbredirhost: new callback to drop isoc packets

[Hans de Goede]

/

+void usbredirhost_set_buffered_output_size_cb(struct usbredirhost *host,
+usbredirhost_buffered_output_size buffered_output_size_func)
+{
+if (!host) {
+ERROR("invalid usbredirhost");
+return;
+}
+
+host->buffered_output_size_func = buffered_output_size_func;
+}
+
  /* Return value:
  0 All ok
  1 Packet borked, continue with next packet / urb
diff --git a/usbredirhost/usbredirhost.h b/usbredirhost/usbredirhost.h
index c0042c9..042e084 100644
--- a/usbredirhost/usbredirhost.h
+++ b/usbredirhost/usbredirhost.h
@@ -33,6 +33,8 @@ struct usbredirhost;

  typedef void (*usbredirhost_flush_writes)(void *priv);

+typedef uint64_t (*usbredirhost_buffered_output_size)(void *priv);
+
  /* This function creates an usbredirhost instance, including its embedded
 libusbredirparser instance and sends the initial usb_redir_hello packet to
 the usb-guest.
@@ -114,6 +116,18 @@ void usbredirhost_close(struct usbredirhost *host);
  int usbredirhost_set_device(struct usbredirhost *host,
  libusb_device_handle *usb_dev_handle);

+/* Call this function to set a callback in usbredirhost.
+   The usbredirhost_buffered_output_size callback should return the
+   application's buffer size (in bytes) that are handling the isochronous data.


applications *pending writes* buffer size (in bytes).

(so add the pending writes, drop the "that are ... isoc data", since the 
application
is not aware which data is isoc data and which is not.

Otherwise looks good, no need to do a new version, just push it with the above 
change:

Reviewed-by: Hans de Goede <hdego...@redhat.com>


+
+   usbredirhost will set two levels of threshold based in the information
+   provided by the usb device. In case the application's buffer is increasing
+   too much then usbredirhost uses the threshold limits to drop isochronous
+   packages but still send full frames whenever is possible.
+*/
+void usbredirhost_set_buffered_output_size_cb(struct usbredirhost *host,
+usbredirhost_buffered_output_size buffered_output_size_func);
+
  /* Call this whenever there is data ready for the usbredirhost to read from
 the usb-guest
 returns 0 on success, or an error code from the below enum on error.



Regards,

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

Re: [Spice-devel] [usbredir PATCH v5 1/2] usbredirhost: new callback for iso streams

[Hans de Goede]

Hi,

On 22-10-15 17:41, Victor Toso wrote:

Hi,

On Thu, Oct 22, 2015 at 05:28:44PM +0200, Hans de Goede wrote:

Hi,

On 22-10-15 16:07, Victor Toso wrote:

For streaming devices it might be necessary from application to drop
data for different reasons. This patch provides a new callback that it
is called before queueing the most recent iso packages.

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
---
  usbredirhost/usbredirhost.c | 63 +++--
  usbredirhost/usbredirhost.h | 12 +
  2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
index ad30722..4c20bff 100644
--- a/usbredirhost/usbredirhost.c
+++ b/usbredirhost/usbredirhost.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -109,6 +110,7 @@ struct usbredirhost {
  usbredirparser_read read_func;
  usbredirparser_write write_func;
  usbredirhost_flush_writes flush_writes_func;
+usbredirhost_can_write_iso can_write_iso_func;


The can_write name no longer seems accurate, and from the
user of libusbredirhost there is nothing iso[c] specific
about this, so I would rename this to:

 usbredirhost_buffered_output_size buffered_output_size_func;



Sure!


  void *func_priv;
  int verbose;
  libusb_context *ctx;
@@ -130,6 +132,11 @@ struct usbredirhost {
  struct usbredirtransfer transfers_head;
  struct usbredirfilter_rule *filter_rules;
  int filter_rules_count;
+struct {
+uint64_t higher;
+uint64_t lower;
+bool dropping;
+} iso_threshold;
  };

  struct usbredirhost_dev_ids {
@@ -1003,6 +1010,31 @@ static void usbredirhost_send_stream_status(struct 
usbredirhost *host,
  }
  }

+static int usbredirhost_can_write_iso_package(struct usbredirhost *host,
+uint8_t ep)
+{
+uint64_t size;
+
+if (!host->can_write_iso_func)
+return true;
+
+size = host->can_write_iso_func(host->func_priv);


Which will change this to:

 size = host->buffered_output_size_func(host->func_priv);

Which seems to make more sense to me.



Agreed.


+if (size >= host->iso_threshold.higher) {
+if (!host->iso_threshold.dropping)
+DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
+  size, host->iso_threshold.higher);
+host->iso_threshold.dropping = true;
+} else if (size < host->iso_threshold.lower) {
+if (host->iso_threshold.dropping)
+DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
+  size, host->iso_threshold.lower);
+
+host->iso_threshold.dropping = false;
+}
+
+return !host->iso_threshold.dropping;
+}
+
  static void usbredirhost_send_stream_data(struct usbredirhost *host,
  uint64_t id, uint8_t ep, uint8_t status, uint8_t *data, int len)
  {
@@ -1028,8 +1060,10 @@ static void usbredirhost_send_stream_data(struct 
usbredirhost *host,
  .status   = status,
  .length   = len,
  };
-usbredirparser_send_iso_packet(host->parser, id, _packet,
-   data, len);
+
+if (usbredirhost_can_write_iso_package(host, ep))
+usbredirparser_send_iso_packet(host->parser, id, _packet,
+   data, len);
  break;
  }
  case usb_redir_type_bulk: {
@@ -1120,6 +1154,16 @@ static void usbredirhost_stop_stream(struct usbredirhost 
*host,
  FLUSH(host);
  }

+static void usbredirhost_set_iso_threshold(struct usbredirhost *host,
+uint8_t pkts_per_transfer, uint8_t transfer_count, uint16_t max_packetsize)
+{
+uint64_t reference = pkts_per_transfer * transfer_count * max_packetsize;


So this is based on the bufsize calculations in qemu, which aim for a buffer
of circa 60 ms. And you try to keep any buffered writes (which not have yet
moved to the os networkstack buffers) between 30 ms and 120 ms, this gives
you a window of aprox 90 ms once you re-start collecting isoc packets, assuming
the pipe is stalled. Those 90 ms may or may not contain a complete video frame,
depending on camera framerate which may be as low as 5 fps / aka 200ms per 
frame.

Now 5 fps is only seen in really low light conditions, so lets aim for 10 fps,
still we need to tweak these values a bit, maybe we should increase the higher 
limit
to reference * 3, giving us aprox 150 ms of continues data collection on a 
stalled
pipe, which should give us at least 1 complete video frame in there at 10 fps.



Many thanks for explaining, the values make much more sense now. I'll
include this explanation in the commit log if you don't mind.


Adding the above text to the commit msg is fine, also feel free to edit it
as needed to get a coherent commit msg.

Regards,

Hans



I believe
that 

Re: [Spice-devel] [usbredir PATCH v5 1/2] usbredirhost: new callback for iso streams

[Hans de Goede]

Hi,

On 22-10-15 16:07, Victor Toso wrote:

For streaming devices it might be necessary from application to drop
data for different reasons. This patch provides a new callback that it
is called before queueing the most recent iso packages.

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
---
  usbredirhost/usbredirhost.c | 63 +++--
  usbredirhost/usbredirhost.h | 12 +
  2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
index ad30722..4c20bff 100644
--- a/usbredirhost/usbredirhost.c
+++ b/usbredirhost/usbredirhost.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -109,6 +110,7 @@ struct usbredirhost {
  usbredirparser_read read_func;
  usbredirparser_write write_func;
  usbredirhost_flush_writes flush_writes_func;
+usbredirhost_can_write_iso can_write_iso_func;


The can_write name no longer seems accurate, and from the
user of libusbredirhost there is nothing iso[c] specific
about this, so I would rename this to:

usbredirhost_buffered_output_size buffered_output_size_func;


  void *func_priv;
  int verbose;
  libusb_context *ctx;
@@ -130,6 +132,11 @@ struct usbredirhost {
  struct usbredirtransfer transfers_head;
  struct usbredirfilter_rule *filter_rules;
  int filter_rules_count;
+struct {
+uint64_t higher;
+uint64_t lower;
+bool dropping;
+} iso_threshold;
  };

  struct usbredirhost_dev_ids {
@@ -1003,6 +1010,31 @@ static void usbredirhost_send_stream_status(struct 
usbredirhost *host,
  }
  }

+static int usbredirhost_can_write_iso_package(struct usbredirhost *host,
+uint8_t ep)
+{
+uint64_t size;
+
+if (!host->can_write_iso_func)
+return true;
+
+size = host->can_write_iso_func(host->func_priv);


Which will change this to:

size = host->buffered_output_size_func(host->func_priv);

Which seems to make more sense to me.


+if (size >= host->iso_threshold.higher) {
+if (!host->iso_threshold.dropping)
+DEBUG("START dropping isoc packets %lu buffer > %lu hi threshold",
+  size, host->iso_threshold.higher);
+host->iso_threshold.dropping = true;
+} else if (size < host->iso_threshold.lower) {
+if (host->iso_threshold.dropping)
+DEBUG("STOP dropping isoc packets %lu buffer < %lu low threshold",
+  size, host->iso_threshold.lower);
+
+host->iso_threshold.dropping = false;
+}
+
+return !host->iso_threshold.dropping;
+}
+
  static void usbredirhost_send_stream_data(struct usbredirhost *host,
  uint64_t id, uint8_t ep, uint8_t status, uint8_t *data, int len)
  {
@@ -1028,8 +1060,10 @@ static void usbredirhost_send_stream_data(struct 
usbredirhost *host,
  .status   = status,
  .length   = len,
  };
-usbredirparser_send_iso_packet(host->parser, id, _packet,
-   data, len);
+
+if (usbredirhost_can_write_iso_package(host, ep))
+usbredirparser_send_iso_packet(host->parser, id, _packet,
+   data, len);
  break;
  }
  case usb_redir_type_bulk: {
@@ -1120,6 +1154,16 @@ static void usbredirhost_stop_stream(struct usbredirhost 
*host,
  FLUSH(host);
  }

+static void usbredirhost_set_iso_threshold(struct usbredirhost *host,
+uint8_t pkts_per_transfer, uint8_t transfer_count, uint16_t max_packetsize)
+{
+uint64_t reference = pkts_per_transfer * transfer_count * max_packetsize;


So this is based on the bufsize calculations in qemu, which aim for a buffer
of circa 60 ms. And you try to keep any buffered writes (which not have yet
moved to the os networkstack buffers) between 30 ms and 120 ms, this gives
you a window of aprox 90 ms once you re-start collecting isoc packets, assuming
the pipe is stalled. Those 90 ms may or may not contain a complete video frame,
depending on camera framerate which may be as low as 5 fps / aka 200ms per 
frame.

Now 5 fps is only seen in really low light conditions, so lets aim for 10 fps,
still we need to tweak these values a bit, maybe we should increase the higher 
limit
to reference * 3, giving us aprox 150 ms of continues data collection on a 
stalled
pipe, which should give us at least 1 complete video frame in there at 10 fps.

Otherwise both patches look good, thanks for your work on this.

Regards,

Hans




+host->iso_threshold.lower = reference / 2;
+host->iso_threshold.higher = reference * 2;
+DEBUG("higher threshold is %lu bytes | lower threshold is %lu bytes",
+   host->iso_threshold.higher, host->iso_threshold.lower);
+}
+
  /* Called from both parser read and packet complete callbacks */
  static void usbredirhost_alloc_stream_unlocked(struct usbredirhost *host,
  uint64_t id, uint8_t 

Re: [Spice-devel] [usbredir PATCH v4 1/2] usbredirhost: new callback for iso streams

[Hans de Goede]

Hi,

On 21-10-15 12:38, Victor Toso wrote:

For streaming devices it might be necessary from application to drop
data for different reasons. This patch provides a new callback that it
is called before queueing the most recent iso packages.

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
---
  usbredirhost/usbredirhost.c | 70 +++--
  usbredirhost/usbredirhost.h | 13 +
  2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
index ad30722..1eaa13e 100644
--- a/usbredirhost/usbredirhost.c
+++ b/usbredirhost/usbredirhost.c
@@ -109,6 +109,7 @@ struct usbredirhost {
  usbredirparser_read read_func;
  usbredirparser_write write_func;
  usbredirhost_flush_writes flush_writes_func;
+usbredirhost_can_write_iso can_write_iso_func;
  void *func_priv;
  int verbose;
  libusb_context *ctx;
@@ -130,6 +131,11 @@ struct usbredirhost {
  struct usbredirtransfer transfers_head;
  struct usbredirfilter_rule *filter_rules;
  int filter_rules_count;
+struct {
+uint64_t higher;
+uint64_t lower;
+uint64_t packets;
+} iso_threshold;
  };

  struct usbredirhost_dev_ids {
@@ -1003,6 +1009,50 @@ static void usbredirhost_send_stream_status(struct 
usbredirhost *host,
  }
  }

+/* This function should always be called with LOCK in the host */
+static int usbredirhost_can_write_iso_package(struct usbredirhost *host,
+uint8_t ep)
+{
+uint64_t size;
+int can_write_packet = 1;
+
+if (!host->can_write_iso_func)
+return can_write_packet;
+
+size = host->can_write_iso_func(host->func_priv);
+if (size >= host->iso_threshold.higher) {
+/* Drop some packages, this one included */
+can_write_packet = 0;
+host->endpoint[EP2I(ep)].drop_packets =
+ (host->endpoint[EP2I(ep)].pkts_per_transfer *
+  host->endpoint[EP2I(ep)].transfer_count) * 2;
+goto end;
+}
+
+if (size >= host->iso_threshold.lower) {
+/* While we are in the lower threshold, we drop a fixed interval
+ * of iso packages so client application can keep streaming without
+ * buffering too fast */
+if (host->iso_threshold.packets == 0) {
+host->iso_threshold.packets =
+  (host->endpoint[EP2I(ep)].pkts_per_transfer *
+   host->endpoint[EP2I(ep)].transfer_count);
+host->endpoint[EP2I(ep)].drop_packets = 
host->iso_threshold.packets;
+can_write_packet = 0;
+} else {
+host->iso_threshold.packets--;
+}
+goto end;
+}
+
+/* In case we were above lower threshold, reset package counter */


Comment should say below lower threshold.


+if (host->iso_threshold.packets != 0) {
+host->iso_threshold.packets = 0;
+}
+end:
+return can_write_packet;
+}
+


This function seems over complicated, I would expect something like this:

   struct {
uint64_t higher;
uint64_t lower;
bool dropping;
} iso_threshold;

With dropping being initialized to false;

And then:

static bool usbredirhost_can_write_iso_package(struct usbredirhost *host,
uint8_t ep)

uint64_t size;

if (!host->can_write_iso_func)
return true;

size = host->can_write_iso_func(host->func_priv);
if (size >= host->iso_threshold.higher)
host->iso_threshold.dropping = true;
else if (size < host->iso_threshold.lower)
host->iso_threshold.dropping = false;

return !iso_threshold.dropping;
}

This way you make the lower threshold a proper hysteresis
and you stop the code from oscillating  around the higher limit
causing pretty much every video frame to become corrupted.

As an added bonus the code above is much easier to understand
then your original implementation.



  static void usbredirhost_send_stream_data(struct usbredirhost *host,
  uint64_t id, uint8_t ep, uint8_t status, uint8_t *data, int len)
  {
@@ -1028,8 +1078,10 @@ static void usbredirhost_send_stream_data(struct 
usbredirhost *host,
  .status   = status,
  .length   = len,
  };
-usbredirparser_send_iso_packet(host->parser, id, _packet,
-   data, len);
+
+if (usbredirhost_can_write_iso_package(host, ep))
+usbredirparser_send_iso_packet(host->parser, id, _packet,
+   data, len);
  break;
  }
  case usb_redir_type_bulk: {
@@ -1358,6 +1410,20 @@ static void usbredirhost_log_data(struct usbredirhost 
*host, const char *desc,

  /**/

+void usbredirhost_set_cb_can_write_iso(struct usbredirhost *host,
+usbredirhost_can_write_iso can_write_iso_func,
+uint64_t threshold_higher, uint64_t 

Re: [Spice-devel] [usbredir PATCH v4 1/2] usbredirhost: new callback for iso streams

[Hans de Goede]

Hi,

On 21-10-15 13:54, Victor Toso wrote:

Hi,

On Wed, Oct 21, 2015 at 01:40:01PM +0200, Hans de Goede wrote:

Hi,

On 21-10-15 12:38, Victor Toso wrote:

For streaming devices it might be necessary from application to drop
data for different reasons. This patch provides a new callback that it
is called before queueing the most recent iso packages.

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
---
  usbredirhost/usbredirhost.c | 70 +++--
  usbredirhost/usbredirhost.h | 13 +
  2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
index ad30722..1eaa13e 100644
--- a/usbredirhost/usbredirhost.c
+++ b/usbredirhost/usbredirhost.c
@@ -109,6 +109,7 @@ struct usbredirhost {
  usbredirparser_read read_func;
  usbredirparser_write write_func;
  usbredirhost_flush_writes flush_writes_func;
+usbredirhost_can_write_iso can_write_iso_func;
  void *func_priv;
  int verbose;
  libusb_context *ctx;
@@ -130,6 +131,11 @@ struct usbredirhost {
  struct usbredirtransfer transfers_head;
  struct usbredirfilter_rule *filter_rules;
  int filter_rules_count;
+struct {
+uint64_t higher;
+uint64_t lower;
+uint64_t packets;
+} iso_threshold;
  };

  struct usbredirhost_dev_ids {
@@ -1003,6 +1009,50 @@ static void usbredirhost_send_stream_status(struct 
usbredirhost *host,
  }
  }

+/* This function should always be called with LOCK in the host */
+static int usbredirhost_can_write_iso_package(struct usbredirhost *host,
+uint8_t ep)
+{
+uint64_t size;
+int can_write_packet = 1;
+
+if (!host->can_write_iso_func)
+return can_write_packet;
+
+size = host->can_write_iso_func(host->func_priv);
+if (size >= host->iso_threshold.higher) {
+/* Drop some packages, this one included */
+can_write_packet = 0;
+host->endpoint[EP2I(ep)].drop_packets =
+ (host->endpoint[EP2I(ep)].pkts_per_transfer *
+  host->endpoint[EP2I(ep)].transfer_count) * 2;
+goto end;
+}
+
+if (size >= host->iso_threshold.lower) {
+/* While we are in the lower threshold, we drop a fixed interval
+ * of iso packages so client application can keep streaming without
+ * buffering too fast */
+if (host->iso_threshold.packets == 0) {
+host->iso_threshold.packets =
+  (host->endpoint[EP2I(ep)].pkts_per_transfer *
+   host->endpoint[EP2I(ep)].transfer_count);
+host->endpoint[EP2I(ep)].drop_packets = 
host->iso_threshold.packets;
+can_write_packet = 0;
+} else {
+host->iso_threshold.packets--;
+}
+goto end;
+}
+
+/* In case we were above lower threshold, reset package counter */


Comment should say below lower threshold.


+if (host->iso_threshold.packets != 0) {
+host->iso_threshold.packets = 0;
+}
+end:
+return can_write_packet;
+}
+


This function seems over complicated, I would expect something like this:

struct {
 uint64_t higher;
 uint64_t lower;
 bool dropping;
 } iso_threshold;

With dropping being initialized to false;

And then:

static bool usbredirhost_can_write_iso_package(struct usbredirhost *host,
 uint8_t ep)

 uint64_t size;

 if (!host->can_write_iso_func)
 return true;

 size = host->can_write_iso_func(host->func_priv);
 if (size >= host->iso_threshold.higher)
 host->iso_threshold.dropping = true;
 else if (size < host->iso_threshold.lower)
 host->iso_threshold.dropping = false;

 return !iso_threshold.dropping;
}

This way you make the lower threshold a proper hysteresis
and you stop the code from oscillating  around the higher limit
causing pretty much every video frame to become corrupted.

As an added bonus the code above is much easier to understand
then your original implementation.



Yes, I started with this implementation at first but it seems to have
the same issue that we had in other versions: We endup dropping every
other package instead of dropping a bunch of packages at once.


That can only happen if the 2 thresholds are too close to each other
or if your code has a bug.

Lets assume that the isoc data is the only data filling the channel
transmit queue, and that the lower threshold is 1MB and the high one
3MB, then with my above example we will start dropping packets once
the queue reaches 3MB of data, the queue will over time drain
dowbn to 1MB, then (quickly) fill up to 3 MB again. But while filling
up we've queues dup 2 MB of continues (so no dropped packets) of
video isoc packets. This 2 MB should contain at least 1 complete
video frame which the guest os can actually p

Re: [Spice-devel] [usbredir PATCH v3] usbredir memory leak

[Hans de Goede]

p.s.

One more thing, when I reproduced this myself I noticed a
couple of other worrisome things:

1) When I stop the webcam-app inside the guest, the camera does
not stop streaming on the client side. As if the control request
being send from the client to stop the stream never reaches the
webcam. Now this may very well be related, and the cause may
be that there are multiple control requests involved and that
the 2nd or 3th one is actually the one stopping the stream,
and these never gets executed because of all the isoc data
being in the pipeline and thus in the way of completing the
first control transfer ...  IOW this will likely disappear
once the main issue is fixed

2) Likewise, and possibly more of a real issue, if I disconnect
the webcam from the usb-devices dialog to stop remote-viewer
from eating all my mem, the buffered memory does not seem to
get released, and the guest does not see a disconnect until I
close remote-viewer. This one is actually somewhat more worry
some. I would expect either the channel to reset, or for it
to slowly get drained over time. But after the unplug it seems
the channel just sits there with all its isoc data buffers
(and the disconnect packet queued after all those isoc buffers).

Regards,

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

Re: [Spice-devel] [usbredir PATCH v3] usbredir memory leak

[Hans de Goede]

Hi,

On 19-10-15 14:25, Fabiano Fidêncio wrote:

Adding Hans ...


Thanks, I missed the original patch, Victor, can you please
Cc me when sending this patches, and perhaps resend v3 with
me in the Cc ?


On Mon, Oct 19, 2015 at 12:07 PM, Victor Toso  wrote:

This is already better then v2 due the fact that we are only dropping
isochronous packets. Still need to find a way to drop the correct
amount of packages so usbredir always start sending data from start of
payload (not in the middle of a frame, for instance)


I'm afraid that dropping the right amount of packages is pretty
much impossible without adding support for every usb isoc protocol
out there. I would simply go for making sure that when you drop,
you drop a bunch of packets at once, rather then dropping every
other packets. This means that when dropping and the stream is
a video stream you will cause the frame currently being send +
how every many you drop + the frame which is in progress when
you stop dropping to get dropped. If you add smartness wrt
the amount of packets to drop, the only thing which will change
in that equation is the "+ the frame which is in progress when
you stop dropping" so without this you effectively end up
dropping 1 frame more (by causing it to be corrupted, at which
point the receiving side should drop it). I do not think that
that is a big deal.

What would be interesting is to change the callback from
a boolean to something returning how much data is buffered
by the channel code, then you can put the threshold when
to start / stop buffering inside the usbredirhost code and
make it depend on the isoc packet size + number of packets /
second. This way you can make it so that you always drop say
200ms of data (which will be a couple of frames), rather then
say always drop 2MB of data, which will be 1.5 frames at a
high resolution and many more at a low resolution.

Regards,

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

Re: [Spice-devel] [PATCH spice-vdagent] service: simplification and accessability - reversion

[Hans de Goede]

Hi,

On 19-10-15 16:55, poma wrote:


While testing, I came to the same conclusion as Marc-André - commit 1587063.

Hans, why the complexity of the "rules & target", isn't "ConditionPath*" 
sufficient?
Besides, the lack of "WantedBy=multi-user.target" - back and forth with 
'graphical.target' - 'spice-vdagentd' is "lost".

Obviously X session (e.g. w/ 'startxfce4') can also start within 
'multi-user.target',
so why start 'spice-vdagentd' manually, unnecessarily, when sufficient is to append 
"multi-user.target", i.e.
"WantedBy=spice-vdagentd.target multi-user.target"


Simplifies start-up scheme and makes 'spice-vdagentd' more accessible as 
service.

Tested-by: poma 


Nack, the spice-vdagent is a hardware service (even if it is virtual hardware),
systemd has a well-documented standard mechanism for hardware activation, and 
that is
what is being used. Using ConditionPathIsSymbolicLink is just a hack to achieve 
more
or less the same (mostly less, e.g. if the port ever becomes a hotplugable 
device
in the future the hack will not work).

Regards,

Hans








---
  Makefile.am  | 8 +---
  data/70-spice-vdagentd.rules | 1 -
  data/spice-vdagentd.service  | 3 ++-
  data/spice-vdagentd.target   | 2 --
  4 files changed, 3 insertions(+), 11 deletions(-)
  delete mode 100644 data/70-spice-vdagentd.rules
  delete mode 100644 data/spice-vdagentd.target

diff --git a/Makefile.am b/Makefile.am
index 8c55b43..de8159f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -65,11 +65,7 @@ endif
  if INIT_SCRIPT_SYSTEMD
  systemdunitdir = $(SYSTEMDSYSTEMUNITDIR)
  systemdunit_DATA = \
-   $(top_srcdir)/data/spice-vdagentd.service \
-   $(top_srcdir)/data/spice-vdagentd.target
-
-udevrulesdir = /lib/udev/rules.d
-udevrules_DATA = $(top_srcdir)/data/70-spice-vdagentd.rules
+   $(top_srcdir)/data/spice-vdagentd.service

  tmpfilesdir = $(prefix)/lib/tmpfiles.d
  tmpfiles_DATA = $(top_srcdir)/data/tmpfiles.d/spice-vdagentd.conf
@@ -82,11 +78,9 @@ manpage_DATA = data/spice-vdagent.1  \
  EXTRA_DIST =  \
NEWS\
README.RHEL-5   \
-   data/70-spice-vdagentd.rules\
data/spice-vdagent.desktop  \
data/spice-vdagentd \
data/spice-vdagentd.service \
-   data/spice-vdagentd.target  \
data/tmpfiles.d/spice-vdagentd.conf \
data/xorg.conf.RHEL-5   \
$(NULL)
diff --git a/data/70-spice-vdagentd.rules b/data/70-spice-vdagentd.rules
deleted file mode 100644
index a1785ba..000
--- a/data/70-spice-vdagentd.rules
+++ /dev/null
@@ -1 +0,0 @@
-ACTION=="add", SUBSYSTEM=="virtio-ports", 
ENV{DEVLINKS}=="/dev/virtio-ports/com.redhat.spice.0", ENV{SYSTEMD_WANTS}="spice-vdagentd.target"
diff --git a/data/spice-vdagentd.service b/data/spice-vdagentd.service
index 8c5effe..90eb1f3 100644
--- a/data/spice-vdagentd.service
+++ b/data/spice-vdagentd.service
@@ -1,6 +1,7 @@
  [Unit]
  Description=Agent daemon for Spice guests
  After=dbus.target
+ConditionPathIsSymbolicLink=/dev/virtio-ports/com.redhat.spice.0

  # TODO we should use:
  #Requires=spice-vdagentd.socket
@@ -14,4 +15,4 @@ PIDFile=/var/run/spice-vdagentd/spice-vdagentd.pid
  PrivateTmp=true

  [Install]
-WantedBy=spice-vdagentd.target
+WantedBy=multi-user.target
diff --git a/data/spice-vdagentd.target b/data/spice-vdagentd.target
deleted file mode 100644
index 1f74931..000
--- a/data/spice-vdagentd.target
+++ /dev/null
@@ -1,2 +0,0 @@
-[Unit]
-Description=Agent daemon for Spice guests


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

Re: [Spice-devel] [PATCH v4] usbredir: fix redirection of user-accesible device nodes.

[Hans de Goede]

Hi,

On 20-07-15 11:51, Christophe Fergeau wrote:

Hey,

Looks good te me now.
Hans, would you mind taking a quick look at that
patch in case you have objections on the change (if
spice_usb_acl_helper_open_acl_finish() fails, try to directly open the
device node anyway as it may be user-accessible).


I do not think that this is the right thing todo, this means e.g.
that if policykit / the admin explicitly denies redirection, but
the usb device node happens to be opened up (which happens with
e.g. scanners), then we will still redirect, this seems wrong to me.

Instead Michal should fixup his policykit so that the helper works
for him.

Regards,

Hans




Thanks,

Christophe


On Wed, Jul 15, 2015 at 10:07:37AM +0100, Michal Suchanek wrote:

When calling ACL helper fails also try to open the device node directly.

Otherwise user-accessible device nodes are rejected when policykit
support is compiled in and policy is not set up when in fact the device
could be accessed.

Signed-off-by: Michal Suchanek hramr...@gmail.com

--

v2:

  - clear errors properly

v4:

  - handle connection cancellation
---
  src/channel-usbredir.c | 15 ---
  1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
index 292b82f..c6f9e5e 100644
--- a/src/channel-usbredir.c
+++ b/src/channel-usbredir.c
@@ -276,13 +276,14 @@ static void spice_usbredir_channel_open_acl_cb(
  SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(user_data);
  SpiceUsbredirChannelPrivate *priv = channel-priv;
  GError *err = NULL;
+GError *acl_err = NULL;

  g_return_if_fail(acl_helper == priv-acl_helper);
  g_return_if_fail(priv-state == STATE_WAITING_FOR_ACL_HELPER ||
   priv-state == STATE_DISCONNECTING);

-spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, err);
-if (!err  priv-state == STATE_DISCONNECTING) {
+spice_usb_acl_helper_open_acl_finish(acl_helper, acl_res, acl_err);
+if (priv-state == STATE_DISCONNECTING) {
  err = g_error_new_literal(G_IO_ERROR, G_IO_ERROR_CANCELLED,
USB redirection channel connect 
cancelled);
  }
@@ -290,13 +291,21 @@ static void spice_usbredir_channel_open_acl_cb(
  spice_usbredir_channel_open_device(channel, err);
  }
  if (err) {
-g_simple_async_result_take_error(priv-result, err);
+if (acl_err) {
+g_simple_async_result_take_error(priv-result, acl_err);
+acl_err = NULL;
+} else {
+g_simple_async_result_take_error(priv-result, err);
+err = NULL;
+}
  libusb_unref_device(priv-device);
  priv-device = NULL;
  g_boxed_free(spice_usb_device_get_type(), priv-spice_device);
  priv-spice_device = NULL;
  priv-state  = STATE_DISCONNECTED;
  }
+g_clear_error(acl_err);
+g_clear_error(err);

  spice_usb_acl_helper_close_acl(priv-acl_helper);
  g_clear_object(priv-acl_helper);
--
2.1.4

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

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

Re: [Spice-devel] [usbredir PATCH v2] usbredirfilter_check: force check a device if all its interfaces are skipped

[Hans de Goede]

Hi,

On 14-07-15 17:30, Uri Lublin wrote:

See usbredirfilter.h for when interfaces are skipped.

Force filter check on such a device by calling recursively with
a flag that forbids skipping (usbredirfilter_fl_dont_skip_non_boot_hid)

Related rhbz#1179210


Looks good to me:

Reviewed-by: Hans de Goede hdego...@redhat.com

Regards,

Hans




---

v1-v2: In v1 such a case was handled by blocking the device.

---
  usbredirparser/usbredirfilter.c | 21 ++---
  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/usbredirparser/usbredirfilter.c b/usbredirparser/usbredirfilter.c
index ef9c63a..02184ef 100644
--- a/usbredirparser/usbredirfilter.c
+++ b/usbredirparser/usbredirfilter.c
@@ -172,7 +172,7 @@ int usbredirfilter_check(
  uint16_t vendor_id, uint16_t product_id, uint16_t device_version_bcd,
  int flags)
  {
-int i, rc;
+int i, rc, num_skipped=0;

  if (usbredirfilter_verify(rules, rules_count))
  return -EINVAL;
@@ -190,9 +190,10 @@ int usbredirfilter_check(
  for (i = 0; i  interface_count; i++) {
  if (!(flags  usbredirfilter_fl_dont_skip_non_boot_hid) 
  interface_count  1  interface_class[i] == 0x03 
-interface_subclass[i] == 0x00  interface_protocol[i] == 0x00)
+interface_subclass[i] == 0x00  interface_protocol[i] == 
0x00) {
+num_skipped++;
  continue;
-
+}
  rc = usbredirfilter_check1(rules, rules_count, interface_class[i],
 vendor_id, product_id, device_version_bcd,
 flags  usbredirfilter_fl_default_allow);
@@ -200,6 +201,20 @@ int usbredirfilter_check(
  return rc;
  }

+/* If all interfaces were skipped, then force check on that device,
+ * by recursively calling this function with a flag that forbids
+ * skipping (usbredirfilter_fl_dont_skip_non_boot_hid)
+ */
+if (num_skipped == interface_count) {
+rc = usbredirfilter_check(rules, rules_count,
+  device_class, device_subclass, 
device_protocol,
+  interface_class, interface_subclass,
+  interface_protocol, interface_count,
+  vendor_id, product_id, device_version_bcd,
+  flags | 
usbredirfilter_fl_dont_skip_non_boot_hid);
+return rc;
+}
+
  return 0;
  }



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

Re: [Spice-devel] [usbredir PATCH] usbredirfilter_check: block device if all its interfaces skipped

[Hans de Goede]

Hi,

On 09-07-15 16:51, Uri Lublin wrote:

On Thu, 2015-07-09 at 10:13 +0200, Hans de Goede wrote:

Hi,

On 09-07-15 09:19, Christophe Fergeau wrote:

On Wed, Jul 08, 2015 at 07:36:40PM +0200, Hans de Goede wrote:

Can you ask the reporter to provide lsusb -v output for the usb
interface
of the kvm in question, then we can better analyse what exactly
is going
wrong here. Perhaps my analyses of the problem is wrong.


Is https://bugzilla.redhat.com/attachment.cgi?id=980879 enough or
do you
need something even more detailed?


That is good enough, but all the hid devices in there have at least
one
interface which has:

bInterfaceSubClass  1 Boot Interface Subclass

So atleast 1 interface will not be skipped for the filter check and
the
Uri's patch will be a nop on these devices.



I've tested with a different KVM switch device, see below its
descriptors.

The patch fixes the problem I found with my KVM device.


Looking at the descriptors for your kvm, yes that makes sense
(and those descriptors ar weird btw, as said this kvm will likely
not work with many BIOS-es).


You are probably right it will not fix the problem in the bug.


Ok, so can you please respin the patch to re-call the filter function
with the usbredirfilter_fl_dont_skip_non_boot_hid flag or-ed
into the flags argument, rather then returning -ENOENT? This way e.g.
a vid:pid filter will still do what the user requested for devices
like your kvm, rather then always denying passthrough of them.

Regards,

Hans





---
Bus 002 Device 043: ID 10d5:55a4 Uni Class Technology Co., Ltd
Device Descriptor:
   bLength18
   bDescriptorType 1
   bcdUSB   1.10
   bDeviceClass0 (Defined at Interface level)
   bDeviceSubClass 0
   bDeviceProtocol 0
   bMaxPacketSize0 8
   idVendor   0x10d5 Uni Class Technology Co., Ltd
   idProduct  0x55a4
   bcdDevice1.00
   iManufacturer   1 No brand
   iProduct2 4 Port KVMSwicther
   iSerial 3 04’
   bNumConfigurations  1
   Configuration Descriptor:
 bLength 9
 bDescriptorType 2
 wTotalLength   59
 bNumInterfaces  2
 bConfigurationValue 1
 iConfiguration  4 HID Mouse
 bmAttributes 0x80
   (Bus Powered)
 MaxPower  100mA
 Interface Descriptor:
   bLength 9
   bDescriptorType 4
   bInterfaceNumber0
   bAlternateSetting   0
   bNumEndpoints   1
   bInterfaceClass 3 Human Interface Device
   bInterfaceSubClass  0 No Subclass
   bInterfaceProtocol  0 None
   iInterface  7 Keyboard
 HID Device Descriptor:
   bLength 9
   bDescriptorType33
   bcdHID   1.10
   bCountryCode0 Not supported
   bNumDescriptors 1
   bDescriptorType34 Report
   wDescriptorLength  65
  Report Descriptors:
** UNAVAILABLE **
   Endpoint Descriptor:
 bLength 7
 bDescriptorType 5
 bEndpointAddress 0x81  EP 1 IN
 bmAttributes3
   Transfer TypeInterrupt
   Synch Type   None
   Usage Type   Data
 wMaxPacketSize 0x0008  1x 8 bytes
 bInterval  10
 Interface Descriptor:
   bLength 9
   bDescriptorType 4
   bInterfaceNumber1
   bAlternateSetting   0
   bNumEndpoints   1
   bInterfaceClass 3 Human Interface Device
   bInterfaceSubClass  0 No Subclass
   bInterfaceProtocol  0 None
   iInterface  6 Mouse
 HID Device Descriptor:
   bLength 9
   bDescriptorType33
   bcdHID   1.10
   bCountryCode0 Not supported
   bNumDescriptors 1
   bDescriptorType34 Report
   wDescriptorLength  24
   Report Descriptor: (length is 24)
 Item(Global): Usage Page, data= [ 0x00 0xff ] 65280
 (null)
 Item(Local ): Usage, data= [ 0x01 ] 1
 (null)
 Item(Main  ): Collection, data= [ 0x01 ] 1
 Application
 Item(Global): Logical Minimum, data= [ 0x00 ] 0
 Item(Global): Logical Maximum, data= [ 0xff 0x00 ] 255
 Item(Local ): Usage Minimum, data= [ 0x00 ] 0
 (null)
 Item(Local ): Usage Maximum, data= [ 0xff 0x00 ] 255
 (null)
 Item(Global): Report Count, data= [ 0x08 ] 8
 Item(Global): Report Size, data= [ 0x08 ] 8

Re: [Spice-devel] [usbredir PATCH] usbredirfilter_check: block device if all its interfaces skipped

[Hans de Goede]

Hi,

On 09-07-15 09:19, Christophe Fergeau wrote:

On Wed, Jul 08, 2015 at 07:36:40PM +0200, Hans de Goede wrote:

Can you ask the reporter to provide lsusb -v output for the usb interface
of the kvm in question, then we can better analyse what exactly is going
wrong here. Perhaps my analyses of the problem is wrong.


Is https://bugzilla.redhat.com/attachment.cgi?id=980879 enough or do you
need something even more detailed?


That is good enough, but all the hid devices in there have at least one
interface which has:

bInterfaceSubClass  1 Boot Interface Subclass

So atleast 1 interface will not be skipped for the filter check and the
Uri's patch will be a nop on these devices.

Regards,

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

Re: [Spice-devel] [usbredir PATCH] usbredirfilter_check: block device if all its interfaces skipped

[Hans de Goede]

Hi,

On 08-07-15 15:26, Uri Lublin wrote:

See usbredirfilter.h for when interfaces are skipped.

Fixes rhbz#1179210


Sigh, the kvm in question is really messed up as it uses non bootclass hid 
interfaces,
that means it won't work in many BIOS' etc. What a mess ...


---
  usbredirparser/usbredirfilter.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/usbredirparser/usbredirfilter.c b/usbredirparser/usbredirfilter.c
index ef9c63a..5cbbfbf 100644
--- a/usbredirparser/usbredirfilter.c
+++ b/usbredirparser/usbredirfilter.c
@@ -172,7 +172,7 @@ int usbredirfilter_check(
  uint16_t vendor_id, uint16_t product_id, uint16_t device_version_bcd,
  int flags)
  {
-int i, rc;
+int i, rc, num_skipped=0;

  if (usbredirfilter_verify(rules, rules_count))
  return -EINVAL;
@@ -190,9 +190,10 @@ int usbredirfilter_check(
  for (i = 0; i  interface_count; i++) {
  if (!(flags  usbredirfilter_fl_dont_skip_non_boot_hid) 
  interface_count  1  interface_class[i] == 0x03 
-interface_subclass[i] == 0x00  interface_protocol[i] == 0x00)
+interface_subclass[i] == 0x00  interface_protocol[i] == 
0x00) {
+num_skipped++;
  continue;
-
+   }
  rc = usbredirfilter_check1(rules, rules_count, interface_class[i],
 vendor_id, product_id, device_version_bcd,
 flags  usbredirfilter_fl_default_allow);
@@ -200,6 +201,10 @@ int usbredirfilter_check(
  return rc;
  }

+/* If all interfaces were skipped, block the device */
+if (num_skipped == interface_count)
+   return -ENOENT;
+
  return 0;
  }


This seems wrong, this means that if a user wants to redirect some custom hid 
device,
with just a single hid interface that it will always be blocked because of this.

I suggest instead adding a vid/pid based list of devices on which to not skip 
non
boot compliant hid devices. This way if hid devices are allowed to be redirected
by the filter, the device can still be redirected, and in the default case where
hid devices are not allowed, we will skip the non-boot-hid skipping, do the 
regular
hid check, and block the device based on that.

Regards,

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

Re: [Spice-devel] [usbredir PATCH] usbredirfilter_check: block device if all its interfaces skipped

[Hans de Goede]

Hi,

On 08-07-15 18:22, Uri Lublin wrote:

Hi Hans,

Thanks for the quick review.
See answers below

On Wed, 2015-07-08 at 15:45 +0200, Hans de Goede wrote:

Hi,

On 08-07-15 15:26, Uri Lublin wrote:

See usbredirfilter.h for when interfaces are skipped.

Fixes rhbz#1179210


Sigh, the kvm in question is really messed up as it uses non
bootclass hid interfaces,
that means it won't work in many BIOS' etc. What a mess ...


---
   usbredirparser/usbredirfilter.c | 11 ---
   1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/usbredirparser/usbredirfilter.c
b/usbredirparser/usbredirfilter.c
index ef9c63a..5cbbfbf 100644
--- a/usbredirparser/usbredirfilter.c
+++ b/usbredirparser/usbredirfilter.c
@@ -172,7 +172,7 @@ int usbredirfilter_check(
   uint16_t vendor_id, uint16_t product_id, uint16_t
device_version_bcd,
   int flags)
   {
-int i, rc;
+int i, rc, num_skipped=0;

   if (usbredirfilter_verify(rules, rules_count))
   return -EINVAL;
@@ -190,9 +190,10 @@ int usbredirfilter_check(
   for (i = 0; i  interface_count; i++) {
   if (!(flags  usbredirfilter_fl_dont_skip_non_boot_hid)

   interface_count  1  interface_class[i] == 0x03

-interface_subclass[i] == 0x00 
interface_protocol[i] == 0x00)
+interface_subclass[i] == 0x00 
interface_protocol[i] == 0x00) {
+num_skipped++;
   continue;
-
+   }
   rc = usbredirfilter_check1(rules, rules_count,
interface_class[i],
  vendor_id, product_id,
device_version_bcd,
  flags 
usbredirfilter_fl_default_allow);
@@ -200,6 +201,10 @@ int usbredirfilter_check(
   return rc;
   }

+/* If all interfaces were skipped, block the device */
+if (num_skipped == interface_count)
+   return -ENOENT;
+
   return 0;
   }


This seems wrong, this means that if a user wants to redirect some
custom hid device,
with just a single hid interface that it will always be blocked
because of this.


In that case interface_count==1 and it will not be skipped.


Right, so make it a device with 2 custom hid interfaces ...


If that's not enough, see more below.



I suggest instead adding a vid/pid based list of devices on which to
not skip non
boot compliant hid devices. This way if hid devices are allowed to be
redirected
by the filter, the device can still be redirected, and in the default
case where
hid devices are not allowed, we will skip the non-boot-hid skipping,
do the regular
hid check, and block the device based on that.


That may work but I think it is complicated for users.
If they are going to use specific vid/pid, they might as
well add a filter-rule for those specific devices.


The skip in the filter is there so that for example a usb headset which
has both an audio interface and a usb=hid interface for the volume buttons
will not get filtered out as being a hid device.


Also assume the filter used is block-all-devices
(-1,-1,-1,-1, 0).


No by default for auto-redirect which is the problem here we use a filter
which filters out hid devices. Normally this works fine to not auto redirect
keyboards and mice since they have a hid interface which had an usb keyboard
boot interface subclass and as such will not trigger the skip checking for
this interface code. The problem here seems to be that a kvm is used which
only has non bootclass hid interfaces.


This will not be enough and the user
would have to provide the specific vid/uid list of such
weird devices (after failing the first time).


No the idea is to have a blacklist inside usbredir for devices for which
the skip code should be skipped, the user will not have to do anything.

Can you ask the reporter to provide lsusb -v output for the usb interface
of the kvm in question, then we can better analyse what exactly is going
wrong here. Perhaps my analyses of the problem is wrong.

Regards,

Hans

p.s.

I'm on vacation from July 11th - July 19th, so if I'm quiet that is why :)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [usbredir PATCH] usbredirfilter_check: block device if all its interfaces skipped

[Hans de Goede]

Hi,

Thinking more about this I think I've a good solution.

If we end up skipping all interfaces, then do not return
-ENOENT, but rerun the filter with the
usbredirfilter_fl_dont_skip_non_boot_hid flag or-ed
into the flags argument.

That shhold fix the use case on hand while not overriding
the users intent if the user actually wants hid devices
to pass the filter.

Regards,

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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Hans de Goede]

Hi,

On 07-07-15 18:47, Jeremy White wrote:




Well, the checkpatch.pl reports were all style (and mostly whitespace);
roughly 3000 of them against 3000 lines of code :-/.  I did review the
code, looking for areas where I thought it would badly cram into the
kernel, and I adjusted the few I found (and sent changes upstream).


style matters, as it's a thing with your brain.  You learn patterns and
if the patterns change, you have to do more work and don't see the real
issues involved.  So by ignoring our style you are saying you don't want
anyone else in the kernel community to ever review or work on the code,
which isn't ok.


Looks like I can't side step this unless Hans is willing to shift the
usbredir project entirely to using kernel style :-/.


I'm fine with moving the usbredir project to the kernel style, the question
is how to do this without causing any hidden breakage.

Can you create a gnu-indent invocation which will do most of the work?

And then a hopefully managable sized patch on top to fix the remaining
style errors in usbredirparser ?


I will plan to make changes so that checkpatch runs clean; I lay out my
concerns and my plan below to make sure I'm taking the best path.

My main concern with changing the ~2,500 lines of code from the upstream
usbredir project is that it will increase the odds that I will introduce
errors, both initially, and again later as I review and attempt to relay
patches from the upstream.

To summarize the checkpatch reports:  the biggest issue is whitespace,
which shouldn't be a problem; I should be able to automate that without
error.  There are also a fair number of one offs; FSF address, space
after '!', etc.  I hope to persuade Hans to take a few style only
patches upstream to address those.  That leaves a pack of about 60 brace
placement and line length issues.

I will plan to manually change those prior to submission.  Any upstream
changes that affect the same code will be manually corrected as well,
prior to submission.

Make sense?


Sounds good, note that as said I'm fine with moving over the usbredir(parser)
code to the kernel style, as long as the changes are reviewable.

I think it may be best to only convert the usbredirdparser files, as those
are the only ones you need for the kernel. Having a mixed style in usbredir
is not ideal, but something I can live with.

Regards,

Hans

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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Hans de Goede]

Hi,

On 02-07-15 10:45, Oliver Neukum wrote:

On Wed, 2015-07-01 at 10:06 +0100, Daniel P. Berrange wrote:


I don't really think it is sensible to be defining  implementing new
network services which can't support strong encryption and authentication.
Rather than passing the file descriptor to the kernel and having it do
the I/O directly, I think it would be better to dissassociate the kernel
from the network transport, and thus leave all sockets layer data I/O
to userspace daemons so they can layer in TLS or SASL or whatever else
is appropriate for the security need.


Hi,

this hits a fundamental limit. Block IO must be done entirely in kernel
space or the system will deadlock. The USB stack is part of the block
layer and the SCSI error handling. Thus if you involve user space you
cannot honor memory allocation with GFP_NOFS and you break all APIs
where we pass GFP_NOIO in the USB stack.

Supposed you need to reset a storage device for error handling.
Your user space programm does a syscall, which allocates memory
and needs to launder pages. It proceeds to write to the storage device
you wish to reset.

It is the same problem FUSE has with writable mmap. You cannot do
block devices in user space sanely.


So how is this dealt with for usbip ?

Regards,

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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Hans de Goede]

Hi,

On 01-07-15 20:31, Jeremy White wrote:

Assuming that's correct, then this seems to imply that the socket has raw
plain text data being sent/received, and thus precludes the possibility
of running any security protocol like TLS unless the kernel wants to have
an impl of the TLS protocol.


Good point.  For completeness, I'll note that, in a Spice use case, the
data would be encrypted by the normal Spice mechanisms.  And it would be
fairly straight forward to write a user space daemon that would accept
TLS and then relay to the unencrypted socket (of course, it would
rewrite everything, which would be inefficient).



I don't really think it is sensible to be defining  implementing new
network services which can't support strong encryption and authentication.
Rather than passing the file descriptor to the kernel and having it do
the I/O directly, I think it would be better to dissassociate the kernel
from the network transport, and thus leave all sockets layer data I/O
to userspace daemons so they can layer in TLS or SASL or whatever else
is appropriate for the security need.


And that would also eliminate the need to copy the parsing code, which
would be a nice improvement.

I considered this approach, but discarded it, perhaps wrongly, when my
google fu suggested that netlink sockets were the best way to connect
user space and a kernel module.  (Because I perceived netlink sockets to
be functionally equivalent to the relay daemon described, above).

 From the user space perspective, the usbredir parser has an interface
that exposes about 20 callback functions, which are invoked with
pointers to a variety of structures.  The ideal would be to have a
mechanism to 'call into' kernel space with those varying interfaces.

Would using ioctls be a reasonable way to achieve this?  Is there a
better way?

In the other direction, the usbredir hc provides a range of functions; I
think most interesting are the urb en/dequeue, hub control, and hub
status calls.  Some of that can be handled in the driver; some would
need to be passed on to user space.

My google fu did not lead me to an obvious way to pass this information
to user space.  The approach that comes to mind is to use a signal, or
woken socket, to instruct user space to poll.

I'd appreciate further comments and advice.


I think it makes sense to have the actual usbredir protocol parsing
in the kernel, and use a netlink interface, this will make it much
easier to deal with protocol extensions (although we have not had
any extensions to the usbredir proto in a while), and will be much
cleaner then an ioctl interface.

I think that Daniel's concern can easily be fixed by rather then
passing the fd of a socket into the kernel to simply forwarding the
data back and forth from a socket opened by userspace into the netlink
socket. This way SSL, SASL or whatever can be put in between, and
you can even built a nice test-suite this way :)

The downside of this is introducing an extra memcpy of all the data,
but an ioctl interface has the same problem and is going to be
unwieldy, so I advice against that.

As for the extra memcpy I would not worry about that, in all the
performance testing I've done it has almost always been all about
latency not throughput.

Regards,

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

Re: [Spice-devel] [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Hans de Goede]

Hi,

On 01-07-15 18:13, Greg KH wrote:

On Wed, Jul 01, 2015 at 10:55:49AM -0500, Jeremy White wrote:

On 07/01/2015 12:44 AM, Greg KH wrote:

On Tue, Jun 30, 2015 at 10:34:25PM -0500, Jeremy White wrote:

   1.  Is the basic premise reasonable?  Is Hans correct in asserting that an
alternate USB over IP module will be considered?


I have no idea, if it fully replaces the usbip functionality, I don't
see why that would be rejected.  But why can't you just fix up usbip for
the issues you find lacking?


This is what Hans said 5 years ago: [1]



3) The protocol itself is far from ideal.

Number 3 is the big deal breaker for me. I've looked at the
(undocumented) protocol by sifting through the source. And it is
very low level, all it does is shove usb packets back and forth
over the network. It has no concept of configuration
setting (the docs say make sure the device is in the right
configuration before sharing it). No concept of caching things
like descriptors, active configuration, per interface alt setting,
etc.

Besides missing a lot of useful smarts the whole one packet at a
time approach does not really fly when it comes to isoc endpoints.
As there paper states, the vm-host / guest os drivers need to
make sure enough packets are submitted / queued at all time
to gap the network delay / fill the network pipe.

For iso endpoints it makes much more sense to have a start / stop
stream model, where the usb-host pumpes the urb ringbuffer and
sends out data received from the usb device to the vm-host
(isoc input endp case), or sends data received from the vm-host
(through a buffer to deal with network jitter) to the isoc output
endpoint.

This also halves the number of packets which need to be
send over the network, as their is no need for the vm-host to send
a request for each packet once an input stream has started / for
the usb-host to send an ack for each delivered packet for an ouput
stream. It would still send an error when an error occurs, but their
is no reason to ack all delivered packets. Given the delay
caused by buffering, etc. not being able to match up the error to
an exact packet is not important, as from the vm-host emulated usb
hc (host controller), the packet has long been delivered already.

Instead we will simply report the error to the guest os for the
next packet enqueued by the guest after receiving notification of
the error from the usb-host.


The protocol is now documented, so that part is out of date.  I don't
see any evidence that the bulk of his other concerns have been
addressed, however.


Because no one has cared to.  Now it seems you care, so I'd prefer to
see someone fix this up instead of adding another protocol that does
much the same thing.


I understand where you are coming from, but usbip is unfixable as it
has no concept of capability negotiation, protocol versioning or some such.

What we need is an usbip v2, and usbredir was written as that, and has been
used in production for years now for redirection of usb devices from
virtual-machine-viewers into qemu based virtual-machines.

I understand that having 2 protocols for one thing is undesirable in
general, but think of this as usb-mass-storage bulk transport vs uas,
or ipv4 vs ipv6 in some cases it just is necessary to do a new
better protocol.

When I designed and implemented usbredir usbip was pretty much dead, but it
got ressurected later. I've never spoken up on this and never attempted to
block usbip's promotion out of staging, as that seemed unfair since no-one
was working on a virtual-hcd driver for the usbredir protocol.

Likewise I think it is unfair if my not speaking up back then now blocks
an usbredir virtual-hcd driver from entering the kernel.

Regards,

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

Re: [Spice-devel] Changes to usbredir to support a kernel module

[Hans de Goede]

Hi Jeremy,

On 17-06-15 21:06, Jeremy White wrote:

I have started work on a usbredir kernel module to enable USB
redirection for XSpice.

I'd like to reuse the usbredirparser.c code, but it needs changes to
compile as part of a kernel module.  These are those changes.


The changes look fine to me, but I was wondering if you are aware that
if you submit this upstream the kernel cannot rely on external sources /
libs ?

So you will have to copy this over into a dir of the kernel sources.

It probably still makes sense to do things the way you suggest, and
to consider the current usbredir git repo the canonical source, and
sync the in kernel copy with that every now and then.

For now I would like to first wait to see how this all develops though,
if you get something working and are ready to submit that to the upstream
kernel, then lets see if we can sell a file with the #ifdef-ery in it
to the upstream kernel devs. If we can I'm fine with taking these
changes for the usbredir repo.

Cool that you're working on this btw!

Regards,

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

Re: [Spice-devel] Questions on usbredir + XSpice redux, libusb anyone?

[Hans de Goede]

Hi,

On 13-04-15 21:40, Jeremy White wrote:

Hi folks,

I've done some work on a usbredir kernel module to bring support for USB
to XSpice.   In doing so, and talking with some colleagues, their
reaction was:  why don't you modify libusb instead?

That is, the current change is to modify XSpice to pipe the USBREDIR
protocol bytes out to a unix domain socket, and then into the kernel
module from there.  However, if you think about it, most Linux
applications (e.g. most apps running inside an XSpice session) are going
to use libusb to access the USB devices.

So then the question - why not modify libusb to detect devices from the
unix domain socket instead of making the round trip through the kernel?
  It has the benefit of letting you keep the traffic and the files in
userspace, and avoiding sharing devices system wide.  The main downside
would seem to be that non libusb devices (e.g. thumb drives et all)
would not be supported.  For our use case, that limitation is not an
issue, and the simplicity is compelling.

I wanted to share this idea to make sure there wasn't something else we
were missing and to see what others think.


most Linux applications ... are going to use libusb to access the USB devices.

That is not true, applications using libusb rather then a kernel driver
are the exception not the rule. Basically the only major applications using
libusb are sane (scanners) and libgphoto2 (still photo cameras), anything
else, webcams, usb audio, hid devices, usb-sticks, usb-harddisks, dvb receivers,
printers (*), etc. is all using kernel drivers.

Regards,

Hans

*) Yes some printer backends may use libusb, this is usually done to get
some extra info from the printer using vendor specific commands, the actual
printing is usually still done via /dev/lp and thus through a kernel driver.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Questions on usbredir + XSpice redux, libusb anyone?

[Hans de Goede]

Hi,

On 04/14/2015 09:10 PM, Jeremy White wrote:

Hey Hans,


most Linux applications ... are going to use libusb to access the USB
devices.

That is not true, applications using libusb rather then a kernel driver
are the exception not the rule. Basically the only major applications using
libusb are sane (scanners) and libgphoto2 (still photo cameras), anything
else, webcams, usb audio, hid devices, usb-sticks, usb-harddisks, dvb
receivers,
printers (*), etc. is all using kernel drivers.


Thanks for the reply; it's evident I don't have a vivid imagination for
the world of USB devices :-/.



*) Yes some printer backends may use libusb, this is usually done to get
some extra info from the printer using vendor specific commands, the actual
printing is usually still done via /dev/lp and thus through a kernel
driver.


Are you sure?  A (perhaps naive) look at the CUPS source code strongly
suggests that most usb device access now goes through libusb for CUPS.


Last time I was helping a printer driver developer with usb problems
what I wrote above was true, but I'm not 100% sure.

Regards,

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

Re: [Spice-devel] Dead lock in usbredir

[Hans de Goede]

Hi,

On 12/16/2014 04:55 PM, Christophe Fergeau wrote:

Hey,

Thanks to the detailed description. Adding Hans to cc: since he knows
USB redirection the best.


This is only a problem with libusb on windows, there is a patch-set at the
upstream libusb mailinglist to stop libusb from taking the eventlock on urb
submission, as that is something which it should not do.

Regards,

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

Re: [Spice-devel] libusbredir fixes

[Hans de Goede]

Hi,

On 11/20/2014 06:08 PM, Christophe Fergeau wrote:
 Hey,
 
 Here are 3 libusbredir patches. I'm not sure where they should go (here?
 bugzilla? someplace else?), just let me know if they should go elsewhere.

spice-devel (with me in the CC please as I'm no longer on spice-devel)
is fine.

All 3 patches look good to me, ACK series.

You should have push access as the usbredir git repo is part of the
spice repos, if not let me know and I'll push them.

Regards,

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

Re: [Spice-devel] [PATCH] [spice-gtk v2] usb-device: Expose libusb device

[Hans de Goede]

Hi,

On 11/19/2014 04:37 AM, Fabiano Fidêncio wrote:
 ping?

Looks good to me.

Regards,

Hans

 
 On Thu, 2014-11-13 at 02:32 +0100, Fabiano Fidêncio wrote:
 As we only can filter USB devices by their Classes and sometimes it is
 not enough (eg: I do not want to have Keyboard and Mouse, but I want to
 have Joysticks, being all of them part of HID Class), let's expose the
 libusb device associated to the SpiceUsbDevice, so the applications can
 have access to whatever information they need, directly from the libusb
 device, to refine their filters.
 ---
 Changes since v1:
 - The approach is completely different. In the first version of the patch
   I've added a new API to get, specifically, class, subclass and protocol
   of a libusb device. Discussing this approach with Marc-André and taking
   into consideration that, from Boxes, we could refine the filter once we
   have access to the libusb device I've decided to keep it simple in the
   spice-gtk and give more freedom to the applications access whatever
   they need from their side.
 ---
  gtk/map-file |  1 +
  gtk/spice-glib-sym-file  |  1 +
  gtk/usb-device-manager.c | 25 +
  gtk/usb-device-manager.h |  1 +
  4 files changed, 28 insertions(+)

 diff --git a/gtk/map-file b/gtk/map-file
 index 9f8d04e..3e9624f 100644
 --- a/gtk/map-file
 +++ b/gtk/map-file
 @@ -114,6 +114,7 @@ spice_uri_set_scheme;
  spice_uri_set_user;
  spice_uri_to_string;
  spice_usb_device_get_description;
 +spice_usb_device_get_libusb_device;
  spice_usb_device_get_type;
  spice_usb_device_manager_can_redirect_device;
  spice_usb_device_manager_connect_device_async;
 diff --git a/gtk/spice-glib-sym-file b/gtk/spice-glib-sym-file
 index 2189fa5..6ea8aeb 100644
 --- a/gtk/spice-glib-sym-file
 +++ b/gtk/spice-glib-sym-file
 @@ -91,6 +91,7 @@ spice_uri_set_scheme
  spice_uri_set_user
  spice_uri_to_string
  spice_usb_device_get_description
 +spice_usb_device_get_libusb_device
  spice_usb_device_get_type
  spice_usb_device_manager_can_redirect_device
  spice_usb_device_manager_connect_device_async
 diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
 index 5013b6c..824264c 100644
 --- a/gtk/usb-device-manager.c
 +++ b/gtk/usb-device-manager.c
 @@ -706,6 +706,31 @@ static gboolean 
 spice_usb_device_manager_get_device_descriptor(
  return TRUE;
  }
  
 +
 +/**
 + * spice_usb_device_get_libusb_device:
 + * @device: #SpiceUsbDevice to get the descriptor information of
 + *
 + * Returns: (transfer none): the %libusb_device associated to 
 %SpiceUsbDevice.
 + *
 + * Since: 0.27
 + **/
 +gconstpointer
 +spice_usb_device_get_libusb_device(const SpiceUsbDevice *device 
 G_GNUC_UNUSED)
 +{
 +#ifdef USE_USBREDIR
 +#ifndef G_OS_WIN32
 +const SpiceUsbDeviceInfo *info = (const SpiceUsbDeviceInfo *)device;
 +
 +g_return_val_if_fail(info != NULL, FALSE);
 +
 +return info-libdev;
 +#endif
 +#else
 +return NULL;
 +#endif
 +}
 +
  static gboolean spice_usb_device_manager_get_libdev_vid_pid(
  libusb_device *libdev, int *vid, int *pid)
  {
 diff --git a/gtk/usb-device-manager.h b/gtk/usb-device-manager.h
 index a7e3515..5b4cfbe 100644
 --- a/gtk/usb-device-manager.h
 +++ b/gtk/usb-device-manager.h
 @@ -89,6 +89,7 @@ GType spice_usb_device_get_type(void);
  GType spice_usb_device_manager_get_type(void);
  
  gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar 
 *format);
 +gconstpointer spice_usb_device_get_libusb_device(const SpiceUsbDevice 
 *device);
  
  SpiceUsbDeviceManager *spice_usb_device_manager_get(SpiceSession *session,
  GError **err);
 
 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Fwd: a question about Xspice

[Hans de Goede]

Hi,

Forwarding this to you guys from the xorg-devel list.

Regards,

Hans



 Forwarded Message 
Subject: a question about Xspice
Date: Tue, 21 Oct 2014 17:30:54 +0800 (CST)
From: cynthia cynthia_...@163.com
To: xorg-de...@lists.x.org, x...@lists.x.org

HI All,


  I am trying to build Xspice which is included in guest QXL driver, but one 
question confuses me a lot. As the Xspice's description in Readme, i get that 
Xspice is a X server and spice server in one, running in guest os, but our team 
has developed a set of windows virtual desktop in which the spiceserver is 
deployed in HOST os linked with QEMU, and i think that is coordinated with the 
spice protocol.,
  So, could anyone explain it for me if spiceserver is deployed in guest os or 
host os? I will be very very appreciated~



Best Regards
Cynthia

___
xorg-de...@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] hansg unsubscribing from spice-devel list

[Hans de Goede]

Hi all,

Since I'm not all that active on the spice front anymore I'm unsubscribing
from the spice-devel list.

If you want my input on anything spice related, please put me in the Cc,
or send me a direct mail.

Regards,

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

Re: [Spice-devel] Questions on usbredir + XSpice

[Hans de Goede]

Hi,

On 08/06/2014 01:29 PM, Alon Levy wrote:
 On 08/06/2014 02:02 PM, Hans de Goede wrote:
 Hi,

 On 08/06/2014 08:02 AM, Alon Levy wrote:
 On 08/05/2014 10:52 PM, Jeremy White wrote:
 While I'm researching, I thought I'd look into the next challenge for
 XSpice - usb device redirection.

 It faces similar challenges to the CAC card stuff.  That is, my sense is
 that the usbredir library and utility set is all about reading client
 machine USB device information and relaying it through to qemu, which
 then exposes it as a 'real' device to the guest OS.

 Again, with XSpice, we don't have qemu :-(.

 Ideally, we'd find some mechanism to inject a usb device into the system
 running XSpice.

 I've done some further research, and the usbip project would seem
 useful.  It was here:
http://usbip.sourceforge.net/
 but apparently is now in the kernel.

 If we use that, then the challenge would appear to be translating the
 protocols.

 Alternates:
   1.  Use usbip end-to-end
I'd like to avoid this; there are benefits to having all spice
 traffic go over the same channel
   2.  ???  Insight appreciated grin ???
   (I can't help but feel there are other methods for simulating a
 USB device, but I'm fairly ignorant of this space).


 2. Teach usbredir to use a kernel without a vm.

 Yes that would be the rigt solution to this, there are use-cases outside
 of Xspice for this too. The solution you're looking for here is to
 wtite a virtual hcd device (driver) for the kernel talking the
 
 So something like http://sourceforge.net/projects/usb-vhci/ ?

That sounds abour right (I did not take a look)

 Or if that is a bad starting point, which one is?
 ehci-hcd.c / ohci-hcd.c / other? (looking at kernel/drivers/usb/host)

The link you gave sounds right, and a better start then one of the existing
drivers for real hardware, but I did not actually look at it, sorry
-ENOTIME.

Regards,

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

Re: [Spice-devel] Questions on usbredir + XSpice

[Hans de Goede]

Hi,

On 08/06/2014 05:17 PM, Jeremy White wrote:
 I would advice against going for a protocol translater, your time would
 better spend on doing this properly right away.
 
 You don't think the kernel guys will push back, saying there is already an 
 existing ip-to-usb device, and we should just use that one?

No I don't expect push back, several people have shown interest in
having usbredir support in the kernel for cases like this.

The existing usb-ip which currently is in staging is a rather poor
protocol which lacks support for any smarts like pre-queuing
urbs for isoc modes so as to make sure those work reliable, etc.

Regards,

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

Re: [Spice-devel] Questions on usbredir + XSpice

[Hans de Goede]

Hi,

On 08/06/2014 08:02 AM, Alon Levy wrote:
 On 08/05/2014 10:52 PM, Jeremy White wrote:
 While I'm researching, I thought I'd look into the next challenge for
 XSpice - usb device redirection.

 It faces similar challenges to the CAC card stuff.  That is, my sense is
 that the usbredir library and utility set is all about reading client
 machine USB device information and relaying it through to qemu, which
 then exposes it as a 'real' device to the guest OS.

 Again, with XSpice, we don't have qemu :-(.

 Ideally, we'd find some mechanism to inject a usb device into the system
 running XSpice.

 I've done some further research, and the usbip project would seem
 useful.  It was here:
http://usbip.sourceforge.net/
 but apparently is now in the kernel.

 If we use that, then the challenge would appear to be translating the
 protocols.

 Alternates:
   1.  Use usbip end-to-end
I'd like to avoid this; there are benefits to having all spice
 traffic go over the same channel
   2.  ???  Insight appreciated grin ???
   (I can't help but feel there are other methods for simulating a
 USB device, but I'm fairly ignorant of this space).

 
 2. Teach usbredir to use a kernel without a vm.

Yes that would be the rigt solution to this, there are use-cases outside
of Xspice for this too. The solution you're looking for here is to
wtite a virtual hcd device (driver) for the kernel talking the
usbredir protocol, so basically do what the server side usbip bits
do, but then with a new protocol.

I would advice against going for a protocol translater, your time would
better spend on doing this properly right away.

I would certsainly be interested in testing and reviewing patches
for this and help getting them added to the upstream kernel.

Regards,

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

[Spice-devel] xorg-x11-drv-qxl in Fedora should be updated to 0.1.2

[Hans de Goede]

Hi,

$subject says it all, I've looked into doing this myself,
but there are quite a few patches, which likely can be all dropped,
but I believe it is better for the update to be done by someone who
knows the code better, and thus is a better judge of which patches
can be dropped.

Regards,

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

Re: [Spice-devel] Help on USB Sharing using Spice

[Hans de Goede]

Hi,

On 07/27/2014 07:49 AM, A.J.Pra$anth wrote:
 Hi all,
  Is it possible to use spice usbredir in non vm environment to
 share USB devices ? In the usb redirection protocol document i has been
 mentioned like
 
 The most significant use case for this is taking a usb device attached to
 some machine a which acts as a client / viewer to a virtual machine v
 hosted on another machine b, and make the usb device show up inside the
 virtual machine as if it were attached directly to the virtual machine v.
 
 So my doubt  is it possible to export an USB device (physically attached to
 a machine running linux) over IP and import the device in a  machine(Not
 VM) which runs windows . If possible please share some Howto links.
 
 Will it be right if i consider this as a replacement for the outdated
 http://usbip.sourceforge.net/ ?

This is possible through usbip, atm this is not possible through the spice
usbredir protocol as there are no usbredir-client implementations other then
qemu.

Regards,

Hans

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

Re: [Spice-devel] [PATCH] Fix leaks

[Hans de Goede]

Hi,

On 07/16/2014 01:24 AM, Fabiano Fidêncio wrote:
 ---
  usbredirhost/usbredirhost.c | 1 +
  usbredirtestclient/usbredirtestclient.c | 3 +++
  2 files changed, 4 insertions(+)
 
 diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
 index bbaafa4..6ab6e1b 100644
 --- a/usbredirhost/usbredirhost.c
 +++ b/usbredirhost/usbredirhost.c
 @@ -1191,6 +1191,7 @@ static void usbredirhost_alloc_stream_unlocked(struct 
 usbredirhost *host,
  host-endpoint[EP2I(ep)].transfer[i], INTERRUPT_TIMEOUT);
  break;
  }
 +free(buffer);
  }
  host-endpoint[EP2I(ep)].out_idx = 0;
  host-endpoint[EP2I(ep)].drop_packets = 0;

Erm, no this wrong, very very wrong, you're freeing the buffer were using to 
store the actual
usb data in here, while the usb transfers are still being used (are being 
created to get
used) not good.

This will get freed when the actual transfer gets free-ed through calling
usbredirhost_free_transfer(), specifically by this line in that function:

free(transfer-transfer-buffer);

 diff --git a/usbredirtestclient/usbredirtestclient.c 
 b/usbredirtestclient/usbredirtestclient.c
 index 42b16dc..32fcba2 100644
 --- a/usbredirtestclient/usbredirtestclient.c
 +++ b/usbredirtestclient/usbredirtestclient.c
 @@ -404,6 +404,9 @@ static int usbredirtestclient_cmdline_ctrl(void)
  }
  usbredirparser_send_control_packet(parser, id, control_packet,
 data, data_len);
 +if (data) {
 +free(data);
 +}
  printf(Send control packet with id: %u\n, id);
  id++;
  return 1;

This leak fix is correct, no need for the if though, free(NULL) is a nop. I've 
pushed this
to the usbredir git repo.

Regards,

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

Re: [Spice-devel] [xorg-x11-drv-qxl] add fix for invisible cursor after resize (#1116870)

[Hans de Goede]

Hi,

On 07/07/2014 06:34 PM, Marc-André Lureau wrote:
 commit 4a7b1661823eb94a7458941885e5edd4cb07ec32
 Author: Marc-André Lureau marcandre.lur...@gmail.com
 Date:   Mon Jul 7 18:28:27 2014 +0200
 
 add fix for invisible cursor after resize (#1116870)
 
  ...rm-restore-cursor-after-resolution-change.patch |   45 
 
  xorg-x11-drv-qxl.spec  |   11 -
  2 files changed, 54 insertions(+), 2 deletions(-)
 ---
 diff --git a/0014-drm-restore-cursor-after-resolution-change.patch 
 b/0014-drm-restore-cursor-after-resolution-change.patch
 new file mode 100644
 index 000..8f2eb06
 --- /dev/null
 +++ b/0014-drm-restore-cursor-after-resolution-change.patch
 @@ -0,0 +1,45 @@
 +From fe39d1621efec234864ad32690d9eeb10aa7444e Mon Sep 17 00:00:00 2001
 +From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= marcandre.lur...@gmail.com
 +Date: Wed, 2 Jul 2014 12:13:33 +0200
 +Subject: [PATCH xf86-qxl] drm: restore cursor after resolution change
 +
 +The Spice server  qemu reset the cursor state when
 +changing resolution. Although X does restore the
 +cursor on framebuffer changes, it doesn't for crtc
 +config. Restoring the cursor here is the simplest
 +way to solve the invisible cursor after resolution
 +change bug with DRM driver.
 +
 +https://bugzilla.redhat.com/show_bug.cgi?id=1030531
 +---
 + src/qxl_drmmode.c | 7 +++
 + 1 file changed, 7 insertions(+)
 +
 +diff --git a/src/qxl_drmmode.c b/src/qxl_drmmode.c
 +index af4b22d..f9eca5f 100644
 +--- a/src/qxl_drmmode.c
  b/src/qxl_drmmode.c
 +@@ -47,6 +47,9 @@
 + 
 + #include qxl.h
 + #include qxl_surface.h
 ++
 ++static void drmmode_show_cursor (xf86CrtcPtr crtc);
 ++
 + static void
 + drmmode_ConvertFromKMode(ScrnInfoPtrscrn,
 +  drmModeModeInfo *kmode,
 +@@ -248,6 +251,10 @@ done:
 + crtc-active = TRUE;
 + #endif
 + 
 ++CursorPtr cursor = xf86_config-cursor;
 ++if (cursor)
 ++drmmode_show_cursor(crtc);
 ++
 + return ret;
 + }
 + 
 +-- 
 +1.9.3
 +
 diff --git a/xorg-x11-drv-qxl.spec b/xorg-x11-drv-qxl.spec
 index 21f4960..4e8868a 100644
 --- a/xorg-x11-drv-qxl.spec
 +++ b/xorg-x11-drv-qxl.spec
 @@ -22,7 +22,7 @@ Name:  xorg-x11-drv-qxl
  
  Version:   0.1.1
  
 -Release:   11%{?gver}%{?dist}
 +Release:   12%{?gver}%{?dist}
  URL:   http://www.x.org
  Source0:   
 http://xorg.freedesktop.org/releases/individual/driver/%{tarball}-%{version}.tar.bz2
  
 @@ -47,6 +47,9 @@ Patch11:   
 0001-Add-support-for-XSERVER_PLATFORM_BUS.patch
  Patch12:   0002-Fix-qxl_driver_func-to-adhere-to-the-API.patch
  Patch13:   0003-Add-support-for-server-managed-fds.patch
  
 +Patch14: 0014-drm-restore-cursor-after-resolution-change.patch
 +
 +

Hmm 14 patches and not a new upstream release for 9 months, maybe it is
time for a new upstream release ?

Regards,

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

Re: [Spice-devel] [PATCH] qxl_kms: read virtual monitor size from xorg.conf

[Hans de Goede]

Hi,

On 06/09/2014 01:40 PM, Alon Levy wrote:
 Signed-off-by: Alon Levy al...@redhat.com

A better commit messages explaining the why, what and how of this patch
would be nice.

Regards,

Hans

 ---
  src/qxl_kms.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)
 
 diff --git a/src/qxl_kms.c b/src/qxl_kms.c
 index c31c62d..d952495 100644
 --- a/src/qxl_kms.c
 +++ b/src/qxl_kms.c
 @@ -164,8 +164,8 @@ Bool qxl_pre_init_kms(ScrnInfoPtr pScrn, int flags)
  if (drmmode_pre_init(pScrn, qxl-drmmode, pScrn-bitsPerPixel / 8) == 
 FALSE)
goto out;
  
 -qxl-virtual_x = 1024;
 -qxl-virtual_y = 768;
 +qxl-virtual_x = -1;
 +qxl-virtual_y = -1;
  
  pScrn-display-virtualX = qxl-virtual_x;
  pScrn-display-virtualY = qxl-virtual_y;
 @@ -335,6 +335,9 @@ Bool qxl_screen_init_kms(SCREEN_INIT_ARGS_DECL)
  if (!xf86CrtcScreenInit (pScreen))
   return FALSE;
  
 +qxl-virtual_x = pScrn-currentMode-HDisplay;
 +qxl-virtual_y = pScrn-currentMode-VDisplay;
 +
  if (!qxl_resize_primary_to_virtual (qxl))
   return FALSE;
  
 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Announcing usbredir-0.7

[Hans de Goede]

Hi,

On 05/23/2014 08:57 AM, Gerd Hoffmann wrote:
 
 usbredir-0.7   19 May 2014
 
 Any plans for package updates (for this and libusbx) in rawhide and
 maybe fedora 20?

rawhide is already done :)  I guess it is best to wait with doing
this for F-20 until libusb-1.0.19 goes final (rawhide has 1.0.19-rc1).

Regards,

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

[Spice-devel] [PATCH xf86-video-qxl 1/3] Add support for XSERVER_PLATFORM_BUS

[Hans de Goede]

This is a preparation patch for adding support for server managed fds.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 src/qxl.h|  6 ++
 src/qxl_driver.c | 48 ++--
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/src/qxl.h b/src/qxl.h
index a44875b..19555ba 100644
--- a/src/qxl.h
+++ b/src/qxl.h
@@ -50,6 +50,9 @@
 #ifdef XSERVER_PCIACCESS
 #include pciaccess.h
 #endif
+#ifdef XSERVER_PLATFORM_BUS
+#include xf86platformBus.h
+#endif
 #include fb.h
 #include vgaHW.h
 #endif /* XSPICE */
@@ -66,6 +69,8 @@ typedef struct list xorg_list_t;
 typedef struct xorg_list xorg_list_t;
 #endif
 
+struct xf86_platform_device;
+
 #include compat-api.h
 #define hidden _X_HIDDEN
 
@@ -271,6 +276,7 @@ struct _qxl_screen_t
 pciVideoPtrpci;
 PCITAG pci_tag;
 #endif
+struct xf86_platform_device *platform_dev;
 vgaRegRec   vgaRegs;
 #endif /* XSPICE */
 
diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index aa969e8..8aef838 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -1387,6 +1387,41 @@ qxl_pci_probe (DriverPtr drv, int entity, struct 
pci_device *dev, intptr_t match
 #define qxl_probe NULL
 
 #endif
+
+#ifdef XSERVER_PLATFORM_BUS
+static Bool
+qxl_platform_probe(DriverPtr driver, int entity, int flags,
+   struct xf86_platform_device *dev, intptr_t match_data)
+{
+qxl_screen_t *qxl;
+ScrnInfoPtr pScrn;
+int scrnFlag = 0;
+
+if (!dev-pdev)
+return FALSE;
+
+if (flags  PLATFORM_PROBE_GPU_SCREEN)
+scrnFlag = XF86_ALLOCATE_GPU_SCREEN;
+
+pScrn = xf86AllocateScreen(driver, scrnFlag);
+if (!pScrn)
+   return FALSE;
+
+if (xf86IsEntitySharable(entity))
+xf86SetEntityShared(entity);
+
+xf86AddEntityToScreen(pScrn, entity);
+
+qxl = pScrn-driverPrivate = xnfcalloc (sizeof (qxl_screen_t), 1);
+qxl-pci = dev-pdev;
+qxl-platform_dev = dev;
+
+qxl_init_scrn (pScrn, qxl_kernel_mode_enabled(pScrn, dev-pdev));
+
+return TRUE;
+}
+#endif /* XSERVER_PLATFORM_BUS */
+
 #endif /* XSPICE */
 
 static DriverRec qxl_driver = {
@@ -1400,12 +1435,21 @@ static DriverRec qxl_driver = {
 #ifdef XSPICE
 qxl_driver_func,
 NULL,
-NULL
+NULL,
+NULL,
 #else
 NULL,
 #ifdef XSERVER_LIBPCIACCESS
 qxl_device_match,
-qxl_pci_probe
+qxl_pci_probe,
+#else
+NULL,
+NULL,
+#endif
+#ifdef XSERVER_PLATFORM_BUS
+qxl_platform_probe,
+#else
+NULL,
 #endif
 #endif /* XSPICE */
 };
-- 
1.9.0

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

[Spice-devel] [PATCH xf86-video-qxl 3/3] Add support for server managed fds

[Hans de Goede]

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 src/qxl_driver.c |  4 
 src/qxl_kms.c| 32 +++-
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index c81b243..b9aa0e9 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -1432,6 +1432,10 @@ qxl_driver_func(ScrnInfoPtr pScrn, xorgDriverFuncOp op, 
void *data)
 *hw_flags = HW_IO | HW_MMIO;
 #endif
 return TRUE;
+#if XORG_VERSION_CURRENT = XORG_VERSION_NUMERIC(1,15,99,902,0)
+case SUPPORTS_SERVER_FDS:
+return TRUE;
+#endif
 default:
 return FALSE;
 }
diff --git a/src/qxl_kms.c b/src/qxl_kms.c
index d6dfcee..c31c62d 100644
--- a/src/qxl_kms.c
+++ b/src/qxl_kms.c
@@ -46,6 +46,17 @@ static Bool qxl_open_drm_master(ScrnInfoPtr pScrn)
 drmSetVersion sv;
 int err;
 
+#if defined(ODEV_ATTRIB_FD)
+if (qxl-platform_dev) {
+qxl-drm_fd = xf86_get_platform_device_int_attrib(qxl-platform_dev,
+  ODEV_ATTRIB_FD, -1);
+if (qxl-drm_fd != -1) {
+qxl-drmmode.fd = qxl-drm_fd;
+return TRUE;
+}
+}
+#endif
+
 #if XORG_VERSION_CURRENT = XORG_VERSION_NUMERIC(1,9,99,901,0)
 XNFasprintf(busid, pci:%04x:%02x:%02x.%d,
 dev-domain, dev-bus, dev-dev, dev-func);
@@ -218,11 +229,17 @@ qxl_enter_vt_kms (VT_FUNC_ARGS_DECL)
 qxl_screen_t *qxl = pScrn-driverPrivate;
 int ret;
 
-ret = drmSetMaster(qxl-drm_fd);
-if (ret) {
-   xf86DrvMsg(pScrn-scrnIndex, X_WARNING,
-  drmSetMaster failed: %s\n,
-  strerror(errno));
+#ifdef XF86_PDEV_SERVER_FD
+if (!(qxl-platform_dev 
+(qxl-platform_dev-flags  XF86_PDEV_SERVER_FD)))
+#endif
+{
+ret = drmSetMaster(qxl-drm_fd);
+if (ret) {
+xf86DrvMsg(pScrn-scrnIndex, X_WARNING,
+   drmSetMaster failed: %s\n,
+   strerror(errno));
+}
 }
 
 if (!xf86SetDesiredModes(pScrn))
@@ -241,6 +258,11 @@ qxl_leave_vt_kms (VT_FUNC_ARGS_DECL)
 xf86_hide_cursors (pScrn);
 //pScrn-EnableDisableFBAccess (XF86_SCRN_ARG (pScrn), FALSE);
 
+#ifdef XF86_PDEV_SERVER_FD
+if (qxl-platform_dev  (qxl-platform_dev-flags  XF86_PDEV_SERVER_FD))
+return;
+#endif
+
 ret = drmDropMaster(qxl-drm_fd);
 if (ret) {
xf86DrvMsg(pScrn-scrnIndex, X_WARNING,
-- 
1.9.0

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

[Spice-devel] [PATCH xf86-video-qxl 2/3] Fix qxl_driver_func to adhere to the API

[Hans de Goede]

The driverFunc callback MUST check the passed in operand and only return
TRUE it if understands it and has handled it. It must NOT blindly assume the
op is GET_REQUIRED_HW_INTERFACES.

While at also always define driverFunc, and welcome qxl to the 21st century.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 src/qxl_driver.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index 8aef838..c81b243 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -1302,12 +1302,6 @@ qxl_probe (struct _DriverRec *drv, int flags)
 return TRUE;
 }
 
-static Bool qxl_driver_func (ScrnInfoPtr screen_info_ptr, xorgDriverFuncOp 
xorg_driver_func_op, pointer hw_flags)
-{
-*(xorgHWFlags*)hw_flags = (xorgHWFlags)HW_SKIP_CONSOLE;
-return TRUE;
-}
-
 #else /* normal, not XSPICE */
 #ifndef XSERVER_LIBPCIACCESS
 static Bool
@@ -1424,6 +1418,25 @@ qxl_platform_probe(DriverPtr driver, int entity, int 
flags,
 
 #endif /* XSPICE */
 
+static Bool
+qxl_driver_func(ScrnInfoPtr pScrn, xorgDriverFuncOp op, void *data)
+{
+xorgHWFlags *hw_flags;
+
+switch (op) {
+case GET_REQUIRED_HW_INTERFACES:
+hw_flags = data;
+#ifdef XSPICE
+*hw_flags = HW_SKIP_CONSOLE;
+#else
+*hw_flags = HW_IO | HW_MMIO;
+#endif
+return TRUE;
+default:
+return FALSE;
+}
+}
+
 static DriverRec qxl_driver = {
 0,
 driver_name,
@@ -1432,13 +1445,12 @@ static DriverRec qxl_driver = {
 qxl_available_options,
 NULL,
 0,
-#ifdef XSPICE
 qxl_driver_func,
+#ifdef XSPICE
 NULL,
 NULL,
 NULL,
 #else
-NULL,
 #ifdef XSERVER_LIBPCIACCESS
 qxl_device_match,
 qxl_pci_probe,
-- 
1.9.0

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

Re: [Spice-devel] Releasing ownership of Spice components in Fedora

[Hans de Goede]

Hi all,

On 03/17/2014 10:08 AM, Hans de Goede wrote:
 Hi All,
 
 It seems I was still the owner of various Spice components in Fedora.
 
 I've just released the following components:
 
 https://admin.fedoraproject.org/pkgdb/acls/name/spice
 https://admin.fedoraproject.org/pkgdb/acls/name/spice-vdagent
 
 Someone from the team should go to the above urls and pick these up.

ping ? (for the spice-vdagent bits)

Regards,

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

[Spice-devel] Releasing ownership of Spice components in Fedora

[Hans de Goede]

Hi All,

It seems I was still the owner of various Spice components in Fedora.

I've just released the following components:

https://admin.fedoraproject.org/pkgdb/acls/name/spice
https://admin.fedoraproject.org/pkgdb/acls/name/spice-vdagent

Someone from the team should go to the above urls and pick these up.

Regards,

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

Re: [Spice-devel] does spicec support usbredir?

[Hans de Goede]

Hi,

On 11/13/2013 01:49 AM, Li Guang wrote:


Hi, Hans

I did strictly as you said at http://hansdegoede.livejournal.com/11084.html,
but, seems there's no effect in guest after selecting redirected USBdevice,
can you help to take a look if there are something wrong for my configuration?


The command line and xml look fine, no idea why this does not work for you
I'm afraid.

Regards,

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

Re: [Spice-devel] does spicec support usbredir?

[Hans de Goede]

Hi,

On 11/13/2013 08:31 AM, Tomáš Chaloupka wrote:

I guess that its because in spice-gtk-0.21 USB redir is not working?
Fixed by this patch: 
http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=fb469ef815d7e8396aee49ad4ca4e5d4f882ee26


Ah, yes that is likely the culprit, good one.

Regards,

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

Re: [Spice-devel] does spicec support usbredir?

[Hans de Goede]

Hi,

On 11/11/2013 04:55 AM, Li Guang wrote:

Hi, All

does spicec support usbredir?
can't figure out that by browsing source code of spice,
if it does, how to support usbredir?


No it does not, please use remote-viewer / virt-viewer for
usbredir use.

Regards,

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

Re: [Spice-devel] [PATCH spice-gtk] main: send max-clipboard to agent

[Hans de Goede]

Hi,

On 11/07/2013 01:03 AM, Marc-André Lureau wrote:

Send configured max-clipboard size to the agent, after receiving agent
capabilities.

See also spice-protocol patch description:
http://lists.freedesktop.org/archives/spice-devel/2013-November/015254.html


ACK, for both this patch as well as for the protocol patch.

Regards,

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

Re: [Spice-devel] Spice bug62033, Gnome bug 680195 rework: new inhibitors for desktop effects

[Hans de Goede]

Hi,

On 11/02/2013 05:50 PM, Fedor Lyakhov wrote:

Bastein, Hans,

We need an agreement on this topic so I can implement something - and
have it accepted in both Spice and Gnome eventually.

There are 2 possible approaches conflicting here:
(i) (spice-proposed) DEs to export API for toggling effects
(preferable inhibitor-styled). Spice to actively use this API as it
sees fit.
(ii) (gnome-proposed) Spice to export API about its internal state,
DEs to recognize Spice and use that API as they want (e.g. disable
effects).

Both approaches can work, and second one seems to be easier to
implement for Spice/Gnome stack.
Main arguments pro (i):
1. It seems right for Spice to be in an active position, deciding what
to do. DEs are merely environments providing APIs and means for
applications to achieve their goals.
2. Spice aims to support many DEs, not only Gnome (mainly under
freedesktop, ofc). Making other DEs to recognize Spice usage and
implement appropriate logic seems to be incorrect approach, which may
be not acceptable from their PoV.

To address Bastein's concern about new inhibitors: we want them to be
system ones, similar to existing idle and other inhibitors. Not
something in the user space of Spice. They should be useful for other
remoting applications like VNC, and maybe some other apps (cannot
think up other real use cases right now).


Either way works for me, with a slight preferences for having inhibitors.

Regards,

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

Re: [Spice-devel] [PATCH] qxl_image: fix build break once MIN/MAX switched to spice-protocol

[Hans de Goede]

ACK.

On 10/20/2013 02:37 PM, Alon Levy wrote:

In commit 5e122e4ab1ac35186cc610cd0d518cfd5e78d902
---
  src/qxl_image.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/qxl_image.c b/src/qxl_image.c
index 2349fca..0a0ca30 100644
--- a/src/qxl_image.c
+++ b/src/qxl_image.c
@@ -31,6 +31,9 @@
  #include string.h
  #include assert.h
  #include stdlib.h
+
+#include spice/macros.h
+
  #include qxl.h
  #include murmurhash3.h



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

Re: [Spice-devel] RFC: Spice in a 3D word presentation sheets

[Hans de Goede]

Hi,

Thanks for the feedback.

On 10/15/2013 09:45 PM, Fedor Lyakhov wrote:

Hans,

My 50 cents:

Slide 4: s/Areo/Aero s/Webbrowsers/Web browsers


Thanks, fixed.


Slide 8: I agree with Marc-André here


Well this is what all the experts say (ie Dave Airlie, Keith Packard),
and if you think a bit more about it it makes a lot of sense. Anyways
when creating the slides I already realized this would be controversial,
and I welcome a healty debate about this during the presentation.


Slide 10: a scheme of Virgil3D would help understand what it is


There is a separate presentation about Virgil3D, this single sheet
is to get people who have missed that one up to speed, which I hope
won't be too many people.


Slides 12-14: schemes of Virgil3D+Spice for local and remote clients
would be very useful.


I agree, but I'm not sure I'll be able to find the time to create these :|

Regards,

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

[Spice-devel] RFC: Spice in a 3D word presentation sheets

[Hans de Goede]

Hi All,

I've created sheets for my Spice in a 3D word presentation
at kvm-forum. Feedback much appreciated:

http://people.fedoraproject.org/~jwrdegoede/spice-3d.odp

Regards,

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

Re: [Spice-devel] [PATCH 1/2] vdagent-virtio-port: no need to log opening the port as a socket

[Hans de Goede]

Hi,

Looks good, ack series.

Regards,

Hans


On 10/14/2013 02:47 PM, Alon Levy wrote:

---
  src/vdagent-virtio-port.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/src/vdagent-virtio-port.c b/src/vdagent-virtio-port.c
index a5da35f..b04d55b 100644
--- a/src/vdagent-virtio-port.c
+++ b/src/vdagent-virtio-port.c
@@ -91,7 +91,6 @@ struct vdagent_virtio_port *vdagent_virtio_port_create(const 
char *portname,

  vport-fd = open(portname, O_RDWR);
  if (vport-fd == -1) {
-syslog(LOG_INFO, open %s: %m; trying as socket, portname);
  vport-fd = socket(PF_UNIX, SOCK_STREAM, 0);
  if (vport-fd == -1) {
  goto error;


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

Re: [Spice-devel] RFC: Integrating Virgil and Spice

[Hans de Goede]

Hi,

On 10/10/2013 01:25 PM, Gerd Hoffmann wrote:

   Hi,

Nice summary.


3) Virgil will render using the host gpu, using EGL to talk to
a drm render node. For non local displays the rendered contents
will be read back from the gpu and then passed as a pixmap to the
ui to transport over the network


Interesting in this context:  What is the status of 3d support for
qxl/spice?


Non existent AFAIK


Is it be possible to transform virgil 3d ops into spice 3d
ops, so you could offload the rendering to spice-client?  Does it make
sense to try?  Or would the transfer of the data needed to render more
expensive than transferring the rendered screen?


AFAIK, people more knowledgable people then me on 3d (ie Keith Packard)
all seem to agree on that transfering the commands to render would be
more expensive. IOW adding 3d support to Spice would be not really useful.

Regards,

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

Re: [Spice-devel] RFC: Integrating Virgil and Spice

[Hans de Goede]

Hi,

On 10/09/2013 10:44 AM, Gerd Hoffmann wrote:

snip


What is virtio-vga btw?  The virgil virtual vga device


Yes, see:

http://airlied.livejournal.com/78104.html

Regards,

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

[Spice-devel] RFC: Integrating Virgil and Spice

[Hans de Goede]

Hi All,

I realize that it may be a bit early to start this discussion,
given the somewhat preliminary state of Virgil, still I would
like to start a discussion about this now for 2 reasons:

1) I believe it would be good to start thinking about this earlier
rather then later.

2) I would like to present a general overview of a plan for this
at kvm-forum to get input on this from the wider kvm-forum community.

I've already had a quick discussion about this with Dave Airlie, and
our ideas on this aligned perfectly.

The basic idea is to use qemu's console layer (include/ui/console.h)
as an abstraction between the new virtio-vga device Dave has in mind
(which will include optional 3D rendering capability through VIRGIL),
and various display options, ie SDL, vnc and Spice.

The console layer would need some extensions for this:

1) Multi head support, a question which comes up here, is do we only
add support for multiple heads on a single card, or do we also want
to support multiple cards each driving a head here ? I myself tend
to go with the KISS solution for now and only support a single
card with multiple heads.

2) The ability for a video-card generating output to pass a dma-buf
context to the display (ui in qemu terms) to get the contents from,
rather then requiring the contents to be rendered to some memory
buffer. This way we can save the quite expensive read-back from gpu
memory of the rendered result and then copying that back to the
framebuffer of the gpu for local displays (ie gtk, SDL), we would
of course still need the read back of the rendered output for
vnc / spice.

For proper multi-head support in the ui layer for local displays,
we will need to use SDL-2, either by porting the current SDL ui code
to SDL-2, or by introducing a new SDL-2 ui component.

The changes needed to the gtk ui for multi-head support are not clear
at this moment (no-one has looked into this yet AFAIK).

Regards,

Hans


p.s.

Note that having multi-head support in qemu's console layer +
a multi-head capable SDL-2 ui, means that we could also use
a qxl device together with the SDL-2 ui to do multi-head
locally, which could be interesting for a variety of use-cases.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] RFC: Integrating Virgil and Spice

[Hans de Goede]

Hi,

On 10/08/2013 03:18 PM, Gerd Hoffmann wrote:

   Hi,


The basic idea is to use qemu's console layer (include/ui/console.h)
as an abstraction between the new virtio-vga device Dave has in mind
(which will include optional 3D rendering capability through VIRGIL),
and various display options, ie SDL, vnc and Spice.

The console layer would need some extensions for this:


snip


2) The ability for a video-card generating output to pass a dma-buf
context to the display (ui in qemu terms) to get the contents from,
rather then requiring the contents to be rendered to some memory
buffer. This way we can save the quite expensive read-back from gpu
memory of the rendered result and then copying that back to the
framebuffer of the gpu for local displays (ie gtk, SDL),


Hmm?  Not sure what you are asking for...

First, reading from gpu memory isn't expensive.  It's all virtual, no
slow read cycles as with real hardware.  There is almost no difference
between gpu memory and main memory for kvm guests.  It's not clear to me
why you are copying stuff from/to gpu memory.


This is mostly Dave's area of expertise, but let me try to explain things
a bit better here. The dma-buf pass-through is for the Virgil case, so
we're passing through 3D rendering commands from the guest to a real,
physcial GPU inside the host, which then renders the final image to show
inside the ui to its own, potentially on card, memory, reading from which
is expensive.

When displaying locally (so SDL-2 or gtk ui), we want to avoid the read by
passing a kernel dma_buf handle from the virtual card (in this case
virtio-vga with Virgil) to the ui (in this case SDL-2), so it can then
directly ask the GPU to blit from that dma_buf to its own visible surface.


Second, you can have your scanout framebuffer in main memory.  That
isn't a problem at all.  It only needs to be contiguous in guest
physical memory, scatter-gather for the framebuffer isn't going to fly.


This is not about the virtual gpu / virtual scanout buffer, this is
about a real GPU used to do the (final) rendering and about getting
that rendering shown to a local user (ie SDL or gtk ui).

So the rendered image is stored in memory owned by the real GPU, and
we need to get this copied to the window the user is viewing, without
using the CPU.


For proper multi-head support in the ui layer for local displays,
we will need to use SDL-2, either by porting the current SDL ui code
to SDL-2, or by introducing a new SDL-2 ui component.


/me votes for new SDL-2 ui component, the historical grown SDL code can
use a rewrite anyway ;)


I was already expecting you would prefer the new SDL-2 ui component
solution :)

Regards,

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

[Spice-devel] Current qemu-master hangs when used with qxl + linux guest

[Hans de Goede]

Hi All,

I'm having this weird problem with qemu master + spice/qxl using
guests. As soon as the guest starts Xorg, I get the following message
from qemu:

main-loop: WARNING: I/O thread spun for 1000 iterations

And from then on the guest hangs and qemu consumes 100% cpu. The qemu
console still works, and I can quit qemu that way.

Doing ctrl+c + a thread apply all bt in qemy shows one cpu thread waiting
for the iothread-lock, and all other threads waiting in poll.

This happens both with non kms guests (tried RHEL-6.5, older Fedoras)
as well as with kms guests (tried a fully up2date F-19).

Since I've not seen any similar reports, I assume it is something
with my setup ...

I've tried changing various things:
-removing the spice agent channel
-changing the number of virtual cpus (tried 1 and 2 virtual cpus)
-upgrading spice-server to the latest git master

But all to no avail.

This is with qemu-master build from source on a fully up2date
F-20 system, using the F-20 seabios files.

If someone has any clever ideas I'll happily try debugging this further.

Regards,

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

Re: [Spice-devel] Current qemu-master hangs when used with qxl + linux guest

[Hans de Goede]

Hi,

On 10/08/2013 04:30 PM, Daniel P. Berrange wrote:

On Tue, Oct 08, 2013 at 04:27:38PM +0200, Hans de Goede wrote:

Hi All,

I'm having this weird problem with qemu master + spice/qxl using
guests. As soon as the guest starts Xorg, I get the following message
from qemu:

main-loop: WARNING: I/O thread spun for 1000 iterations

And from then on the guest hangs and qemu consumes 100% cpu. The qemu
console still works, and I can quit qemu that way.

Doing ctrl+c + a thread apply all bt in qemy shows one cpu thread waiting
for the iothread-lock, and all other threads waiting in poll.

This happens both with non kms guests (tried RHEL-6.5, older Fedoras)
as well as with kms guests (tried a fully up2date F-19).

Since I've not seen any similar reports, I assume it is something
with my setup ...

I've tried changing various things:
-removing the spice agent channel
-changing the number of virtual cpus (tried 1 and 2 virtual cpus)
-upgrading spice-server to the latest git master

But all to no avail.

This is with qemu-master build from source on a fully up2date
F-20 system, using the F-20 seabios files.

If someone has any clever ideas I'll happily try debugging this further.


Does the QEMU build in F20 work correctly ?


Ah, yes it does, good dea.


If so that'd give you a
starting point from which to 'git bisect' the problem.


Yep, note I asked for a clever idea, iow not the big hammer of bisecting :)

But since 1.6.0 in F-20 worked it was not such a large bisect, so I went
ahead and bisected anyways, which pointed me to commit 7b595f35d8 :
aio / timers: Convert mainloop to use timeout

After careful review of that commit I had a hunch what might be wrong, and
it turned out to be right. So after this mail I'm going to send a patch
fixing this. If you want to know the details see the patch, titled:
main-loop: Don't lock starve io-threads when main_loop_tlg has pending events

Regards,

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

Re: [Spice-devel] [PATCH spice-gtk] spice-channel: Fix usbredir being broken since commit 159c6ebf

[Hans de Goede]

Hi,

On 09/30/2013 02:13 AM, Marc-André Lureau wrote:

ack,

can you add bug ref to commit msg?
https://bugs.freedesktop.org/show_bug.cgi?id=69935


Done and pushed.


thanks for tracking this down.


You're welcome.


I'll update the fedora packages.


Thanks.

Regards,

Hans




- Original Message -

The usbredir channel uses spice_msg_in_raw to get its data, which uses
in-dpos to determine the msg size and that was no longer being set for
non sub-messages.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
  gtk/spice-channel.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index b01b820..150636e 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1790,6 +1790,7 @@ void spice_channel_recv_msg(SpiceChannel *channel,
  spice_channel_read(channel, in-data, msg_size);
  if (c-has_error)
  goto end;
+in-dpos = msg_size;

  msg_type = spice_header_get_msg_type(in-header, c-use_mini_header);
  sub_list_offset = spice_header_get_msg_sub_list(in-header,
  c-use_mini_header);
--
1.8.3.1

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


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

[Spice-devel] [PATCH spice-gtk] spice-channel: Fix usbredir being broken since commit 159c6ebf

[Hans de Goede]

The usbredir channel uses spice_msg_in_raw to get its data, which uses
in-dpos to determine the msg size and that was no longer being set for
non sub-messages.

Signed-off-by: Hans de Goede hdego...@redhat.com
---
 gtk/spice-channel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
index b01b820..150636e 100644
--- a/gtk/spice-channel.c
+++ b/gtk/spice-channel.c
@@ -1790,6 +1790,7 @@ void spice_channel_recv_msg(SpiceChannel *channel,
 spice_channel_read(channel, in-data, msg_size);
 if (c-has_error)
 goto end;
+in-dpos = msg_size;
 
 msg_type = spice_header_get_msg_type(in-header, c-use_mini_header);
 sub_list_offset = spice_header_get_msg_sub_list(in-header, 
c-use_mini_header);
-- 
1.8.3.1

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

Re: [Spice-devel] [PATCH] mono cursor: increase contrast, better looking putty cursor (rhbz 998529)

[Hans de Goede]

Seems to make sense to me, ACK.

On 09/23/2013 03:10 PM, Alon Levy wrote:

Signed-off-by: Alon Levy al...@redhat.com
---
The previous fix is almost impossible to notice on putty, I'm assuming the
original values for the on/off pixels were taken from a different use case, but
so far since we do have a bug for putty and no bug for anything else I propose
we fix the putty case first.

  gtk/channel-cursor.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gtk/channel-cursor.c b/gtk/channel-cursor.c
index bbfb3c9..d1d2c34 100644
--- a/gtk/channel-cursor.c
+++ b/gtk/channel-cursor.c
@@ -269,15 +269,15 @@ static void mono_cursor(display_cursor *cursor, const 
guint8 *data)
   * the same contrast.
   */
  if ((x ^ y)  1) {
-dest[0] = 0x30;
-dest[1] = 0x30;
-dest[2] = 0x30;
-dest[3] = 0xc0;
+dest[0] = 0xff;
+dest[1] = 0xff;
+dest[2] = 0xff;
+dest[3] = 0xff;
  } else {
-dest[0] = 0x50;
-dest[1] = 0x50;
-dest[2] = 0x50;
-dest[3] = 0x30;
+dest[0] = 0x00;
+dest[1] = 0x00;
+dest[2] = 0x00;
+dest[3] = 0x00;
  }
  } else {
  /* unchanged - transparent */


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

Re: [Spice-devel] Bug62033 Add support for --spice-disable-effects client option to spice-vdagent on Linux guests with Gnome3

[Hans de Goede]

Hi,

On 09/15/2013 09:33 PM, Fedor Lyakhov wrote:

Hello Hans,

Finally I've found time to continue with this topic. Have sought for better 
solution and only come to improved previous one.

*Requirements*

Let's start with clarifying requirements.
req-1. As a user I want to have my desktop effect settings untouched if 
connected under normal conditions.
req-2. As a user I want to have a few desktop effect settings disabled if 
connected under bad conditions.
The list of settings shall be configurable (as of now - wallpaper, animations, 
font smoothing) - if this isn't true in oVirt, I'd need work on it as well, 
because disabling all of them may reduce user experience to unacceptable level, 
resulting in feature becoming useless for many users.
Bad conditions can be identified by the user himself (via UI), or automatically 
- to be implemented later.

Both these stories taken together implicitly require:
req-3. After connecting under bad conditions and having effects disabled and 
then connecting under good conditions, as a user I want my desktop effect 
settings restored to their normal (previous) values.

Non-functional requirements:
req-4. Robustness. Crash-proof solution: upon vdagent crash, qemu crash or 
forced guest shutdown, correct state of effect settings must be restored for 
next session.
req-5. Simplicity. KISS, Choose simplest possible implementation.

DEs to support:
req-6. Main: Gnome3, KDE4, Unity; Stretch goal: XFce, LXDE

digression {
Or do we need to talk about Window Managers here? Or display servers?
req-7. Solution shall be display server agnostic (X.Org, Wayland, XMir, Mir...).
req-8. Solution shall be window manager/shell agnostic (Mutter/Gnome Shell, 
KWin/Plasma, Compiz/Unity, Cinnamon probably - if we even bother of Ubutu, why 
don't respect Mint... etc).

Seems my lack of expertise in display system stacks in Linux is blocking me 
here; probably solution cannot be fully WM/shell agnostic. I'm going to try 
using DE abstraction level over WM specifics... if this makes sense.

All this profusion of DE/WM/Display servers nowadays results in a fact that 
without some standard implemented by them, it won't be possible to support all 
of them (and EWMH isn't even near the level of required interop...).
}

I know you'd like to weaken functional requirements if higher simplicity is 
possible. Could you please try specifying these weakened requirements - I 
cannot think up them myself.


My main reasons for weakening the functional requirements are not simplicity,
but keeping things in out own hand. IOW do this without making changes outside
of spice components.

However now that this has had some time to settle, I think that it is best to 
not
weaken the requirements, but instead do this right (which will likely turn out
simpler then trying to use existing API-s in ways they were not really 
intended).


Given the fact there are so many different display systems in Linux, it isn't 
practically possible to support all of them. On the other hand, supporting only 
few and not providing any means for developers of other systems to add similar 
support themselves doesn't make sense to me. So I suggest following 
architecture:
vdagent --DBus-- DE-specific display effects daemon -- DE API for effects
Basically vdagent (session, not system one) is to use DBus interface for 
toggling effects. The feature will be available only if the interface is 
implemented in the system.


Agreed.


And this implementation should be DE-specific and maybe even part of DE. But as 
we (Spice) are concerned with major DEs, sample implementation will be provided 
for Gnome3, KDE4 and Unity.


More likely patches will provided for, as this will likely be part of existing 
code in the DE.
Could be you already meant to say as much, just making sure, that this sample 
implementation
will not necessarily be a standalone thing.


Eventually I'm going to open tickets for these systems to include the new DBus 
interface implementation in their systems (like gnome-desktop-daemon). This 
should generate a good feedback upon interface itself (i.e. need to create an
interface acceptable for all these DEs).

Interface (subject to renames):

bool SetMinimumDesktopBackground() // if already minimum, do nothing; save 
current desktop background settings; set solid background, saved color (if 
none, use default)
bool RestoreDesktopBackground() // if nothing is saved, do nothing; if solid 
desktop background, save the color; restore desktop background settings

bool SetMinimumFontSmooth() // if already minimum, do nothing; save current 
font smoothing settings; set antialiasing and hinting to the minimum yet 
acceptable level
bool RestoreFontSmooth() // if nothing is saved, do nothing; restore font 
smoothing settings

bool SetMinimumAnimations() // if already minimum, do nothing; save current 
animation settings; disable all decorative window animation effects, but keep 
meaningful animations like spinnings enabled (if 

Re: [Spice-devel] [PATCH common 1/3] quic: compile with constant bpp, to help compiler

[Hans de Goede]

Looks good, ACK series (with the change you mentioned yourself already)

On 09/08/2013 09:07 PM, Marc-André Lureau wrote:

From: Marc-André Lureau marcandre.lur...@gmail.com

---
  common/quic_family_tmpl.c |  4 ++--
  common/quic_rgb_tmpl.c|  6 +++---
  common/quic_tmpl.c| 16 
  3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/common/quic_family_tmpl.c b/common/quic_family_tmpl.c
index cca2c05..287ff6d 100644
--- a/common/quic_family_tmpl.c
+++ b/common/quic_family_tmpl.c
@@ -74,13 +74,13 @@ static unsigned int FNAME(golomb_decoding)(const unsigned 
int l, const unsigned

  /* update the bucket using just encoded curval */
  static void FNAME(update_model)(CommonState *state, s_bucket * const bucket,
-const BYTE curval, unsigned int bpp)
+const BYTE curval)
  {
+const unsigned int bpp = BPC;
  COUNTER * const pcounters = bucket-pcounters;
  unsigned int i;
  unsigned int bestcode;
  unsigned int bestcodelen;
-//unsigned int bpp = encoder-bpp;

  /* update counters, find minimum */

diff --git a/common/quic_rgb_tmpl.c b/common/quic_rgb_tmpl.c
index 37c908c..19cc348 100644
--- a/common/quic_rgb_tmpl.c
+++ b/common/quic_rgb_tmpl.c
@@ -178,11 +178,11 @@

  #define UPDATE_MODEL(index)   
  \
  update_model(encoder-rgb_state, find_bucket(channel_r, 
correlate_row_r[index - 1]),   \
-correlate_row_r[index], bpc);  
 \
+correlate_row_r[index]);   
\
  update_model(encoder-rgb_state, find_bucket(channel_g, 
correlate_row_g[index - 1]),   \
-correlate_row_g[index], bpc);  
 \
+correlate_row_g[index]);   
\
  update_model(encoder-rgb_state, find_bucket(channel_b, 
correlate_row_b[index - 1]),   \
-correlate_row_b[index], bpc);
+correlate_row_b[index]);


  #ifdef RLE_PRED_1
diff --git a/common/quic_tmpl.c b/common/quic_tmpl.c
index b625daf..75f2ff0 100644
--- a/common/quic_tmpl.c
+++ b/common/quic_tmpl.c
@@ -173,7 +173,7 @@ static void FNAME(compress_row0_seg)(Encoder *encoder, 
Channel *channel, int i,
  } else {
  channel-state.waitcnt = (tabrand(channel-state.tabrand_seed)  
waitmask);
  update_model(channel-state, find_bucket(channel, 
decorelate_drow[-1]),
- decorelate_drow[i], bpc);
+ decorelate_drow[i]);
  }
  stopidx = ++i + channel-state.waitcnt;
  } else {
@@ -191,7 +191,7 @@ static void FNAME(compress_row0_seg)(Encoder *encoder, 
Channel *channel, int i,
  }

  update_model(channel-state, find_bucket(channel, 
decorelate_drow[stopidx - 1]),
- decorelate_drow[stopidx], bpc);
+ decorelate_drow[stopidx]);
  stopidx = i + (tabrand(channel-state.tabrand_seed)  waitmask);
  }

@@ -272,7 +272,7 @@ static void FNAME(compress_row_seg)(Encoder *encoder, 
Channel *channel, int i,
  } else {
  channel-state.waitcnt = (tabrand(channel-state.tabrand_seed)  
waitmask);
  update_model(channel-state, find_bucket(channel, 
decorelate_drow[-1]),
- decorelate_drow[0], bpc);
+ decorelate_drow[0]);
  }
  stopidx = ++i + channel-state.waitcnt;
  } else {
@@ -295,7 +295,7 @@ static void FNAME(compress_row_seg)(Encoder *encoder, 
Channel *channel, int i,
  }

  update_model(channel-state, find_bucket(channel, 
decorelate_drow[stopidx - 1]),
- decorelate_drow[stopidx], bpc);
+ decorelate_drow[stopidx]);
  stopidx = i + (tabrand(channel-state.tabrand_seed)  waitmask);
  }

@@ -406,7 +406,7 @@ static void FNAME(uncompress_row0_seg)(Encoder *encoder, 
Channel *channel, int i
  } else {
  channel-state.waitcnt = (tabrand(channel-state.tabrand_seed)  
waitmask);
  update_model(channel-state, find_bucket(channel, 
correlate_row[-1]),
- correlate_row[0], bpc);
+ correlate_row[0]);
  }
  stopidx = ++i + channel-state.waitcnt;
  } else {
@@ -426,7 +426,7 @@ static void FNAME(uncompress_row0_seg)(Encoder *encoder, 
Channel *channel, int i
  decode_eatbits(encoder, codewordlen);
  }

-update_model(channel-state, pbucket, correlate_row[stopidx], bpc);
+update_model(channel-state, pbucket, correlate_row[stopidx]);

  stopidx = i + (tabrand(channel-state.tabrand_seed)  waitmask);
  }
@@ -511,7 +511,7 @@ static void FNAME(uncompress_row_seg)(Encoder *encoder, 
Channel 

Re: [Spice-devel] [PATCH v3 vdagent-linux] vdagentd: support fake uinput

[Hans de Goede]

ACK.

On 09/02/2013 11:09 PM, Alon Levy wrote:

This is used with Xspice. Fake means we open a pipe for write only, and
don't do any ioctls on it. Specifically it means the axis and buttons
have to be coordinated for now with Xspice (xf86-video-qxl).

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagentd-uinput.c | 11 +--
  src/vdagentd-uinput.h |  2 +-
  src/vdagentd.c| 14 +++---
  3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/vdagentd-uinput.c b/src/vdagentd-uinput.c
index a415f9c..47e1b45 100644
--- a/src/vdagentd-uinput.c
+++ b/src/vdagentd-uinput.c
@@ -42,12 +42,13 @@ struct vdagentd_uinput {
  struct vdagentd_guest_xorg_resolution *screen_info;
  int screen_count;
  VDAgentMouseState last;
+int fake;
  };

  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
  int width, int height,
  struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
-int debug)
+int debug, int fake)
  {
  struct vdagentd_uinput *uinput;

@@ -58,6 +59,7 @@ struct vdagentd_uinput *vdagentd_uinput_create(const char 
*devname,
  uinput-devname = devname;
  uinput-fd  = -1; /* Gets opened by vdagentd_uinput_update_size() */
  uinput-debug   = debug;
+uinput-fake= fake;

  vdagentd_uinput_update_size(uinput, width, height,
  screen_info, screen_count);
@@ -119,13 +121,18 @@ void vdagentd_uinput_update_size(struct vdagentd_uinput 
**uinputp,
  return;
  #endif

-uinput-fd = open(uinput-devname, O_RDWR);
+uinput-fd = open(uinput-devname, uinput-fake ? O_WRONLY : O_RDWR);
  if (uinput-fd == -1) {
  syslog(LOG_ERR, open %s: %m, uinput-devname);
  vdagentd_uinput_destroy(uinputp);
  return;
  }

+if (uinput-fake) {
+/* fake device doesn't understand any ioctls and only writes events */
+return;
+}
+
  rc = write(uinput-fd, device, sizeof(device));
  if (rc != sizeof(device)) {
  syslog(LOG_ERR, write %s: %m, uinput-devname);
diff --git a/src/vdagentd-uinput.h b/src/vdagentd-uinput.h
index 81f0376..b9bd9f1 100644
--- a/src/vdagentd-uinput.h
+++ b/src/vdagentd-uinput.h
@@ -30,7 +30,7 @@ struct vdagentd_uinput;
  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
  int width, int height,
  struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
-int debug);
+int debug, int fake);
  void vdagentd_uinput_destroy(struct vdagentd_uinput **uinputp);

  void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
diff --git a/src/vdagentd.c b/src/vdagentd.c
index 2288671..cfb7acc 100644
--- a/src/vdagentd.c
+++ b/src/vdagentd.c
@@ -58,6 +58,7 @@ static const char *portdev = 
/dev/virtio-ports/com.redhat.spice.0;
  static const char *vdagentd_socket = VDAGENTD_SOCKET;
  static const char *uinput_device = /dev/uinput;
  static int debug = 0;
+static int uinput_fake = 0;
  static struct udscs_server *server = NULL;
  static struct vdagent_virtio_port *virtio_port = NULL;
  static GHashTable *active_xfers = NULL;
@@ -317,7 +318,8 @@ int virtio_port_read_complete(
  agent_data-height,
  agent_data-screen_info,
  agent_data-screen_count,
-debug  1);
+debug  1,
+uinput_fake);
  if (!uinput) {
  syslog(LOG_CRIT, Fatal uinput error);
  retval = 1;
@@ -488,7 +490,8 @@ static void check_xorg_resolution(void)
  agent_data-height,
  agent_data-screen_info,
  agent_data-screen_count,
-debug  1);
+debug  1,
+uinput_fake);
  else
  vdagentd_uinput_update_size(uinput,
  agent_data-width,
@@ -867,6 +870,11 @@ int main(int argc, char *argv[])
  }
  }

+if (strncmp(uinput_device, /dev, 4) != 0) {
+syslog(LOG_INFO, using fake uinput);
+uinput_fake = 1;
+}
+
  memset(act, 0, sizeof(act));
  act.sa_flags = SA_RESTART;
  act.sa_handler = quit_handler;
@@ -899,7 +907,7 @@ int main(int argc, char *argv[])

  #ifdef WITH_STATIC_UINPUT
  uinput = vdagentd_uinput_create(uinput_device, 1024, 768, NULL, 0,
-debug  1);
+debug  1, uinput_fake);
  if (!uinput) {
  udscs_destroy_server(server);
  return 1;


___
Spice-devel mailing list

Re: [Spice-devel] [PATCH vdagent-linux] vdagent-virtio-port: close socket on error

[Hans de Goede]

ACK.

On 09/03/2013 12:07 PM, Alon Levy wrote:

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagent-virtio-port.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/vdagent-virtio-port.c b/src/vdagent-virtio-port.c
index bdd22d9..bacfa42 100644
--- a/src/vdagent-virtio-port.c
+++ b/src/vdagent-virtio-port.c
@@ -116,6 +116,9 @@ struct vdagent_virtio_port 
*vdagent_virtio_port_create(const char *portname,

  error:
  syslog(LOG_ERR, open %s: %m, portname);
+if (vport-fd != -1) {
+close(vport-fd);
+}
  free(vport);
  return NULL;
  }


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

Re: [Spice-devel] [PATCH vdagent-linux 1/4] vdagent: add option to change vdagentd socket

[Hans de Goede]

ACK.

On 09/02/2013 05:02 PM, Alon Levy wrote:

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagent.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/vdagent.c b/src/vdagent.c
index 10ebf6e..f270615 100644
--- a/src/vdagent.c
+++ b/src/vdagent.c
@@ -43,6 +43,7 @@
  #include vdagent-file-xfers.h

  static const char *portdev = /dev/virtio-ports/com.redhat.spice.0;
+static const char *vdagentd_socket = VDAGENTD_SOCKET;
  static int debug = 0;
  static const char *fx_dir = NULL;
  static int fx_open_dir = -1;
@@ -117,7 +118,7 @@ void daemon_read_complete(struct udscs_connection **connp,
  int client_setup(int reconnect)
  {
  while (!quit) {
-client = udscs_connect(VDAGENTD_SOCKET, daemon_read_complete, NULL,
+client = udscs_connect(vdagentd_socket, daemon_read_complete, NULL,
 vdagentd_messages, VDAGENTD_NO_MESSAGES,
 debug);
  if (client || !reconnect || quit) {
@@ -137,6 +138,7 @@ static void usage(FILE *fp)
  -hprint this text\n
  -dlog debug messages\n
  -s port set virtio serial port\n
+-S filename set udcs socket\n
  -xdon't daemonize\n
  -f dir|xdg-desktop|xdg-download file xfer save dir\n
  -o 0|1  open dir on file xfer 
completion\n,
@@ -183,7 +185,7 @@ int main(int argc, char *argv[])
  struct sigaction act;

  for (;;) {
-if (-1 == (c = getopt(argc, argv, -dxhys:f:o:)))
+if (-1 == (c = getopt(argc, argv, -dxhys:f:o:S:)))
  break;
  switch (c) {
  case 'd':
@@ -207,6 +209,9 @@ int main(int argc, char *argv[])
  case 'o':
  fx_open_dir = atoi(optarg);
  break;
+case 'S':
+vdagentd_socket = optarg;
+break;
  default:
  fputs(\n, stderr);
  usage(stderr);


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

Re: [Spice-devel] [PATCH vdagent-linux 2/4] vdagentd: support configurable socket to vdagent

[Hans de Goede]

ACK.

On 09/02/2013 05:02 PM, Alon Levy wrote:

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagentd.c | 33 +++--
  1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/vdagentd.c b/src/vdagentd.c
index f4cea44..2288671 100644
--- a/src/vdagentd.c
+++ b/src/vdagentd.c
@@ -55,6 +55,7 @@ struct agent_data {
  /* variables */
  static const char *pidfilename = /var/run/spice-vdagentd/spice-vdagentd.pid;
  static const char *portdev = /dev/virtio-ports/com.redhat.spice.0;
+static const char *vdagentd_socket = VDAGENTD_SOCKET;
  static const char *uinput_device = /dev/uinput;
  static int debug = 0;
  static struct udscs_server *server = NULL;
@@ -721,18 +722,19 @@ static void usage(FILE *fp)
  Usage: spice-vdagentd [OPTIONS]\n\n
  Spice guest agent daemon, version %s.\n\n
  Options:\n
-  -h print this text\n
-  -d log debug messages (use twice for extra info)\n
-  -s port  set virtio serial port  [%s]\n
-  -u dev   set uinput device   [%s]\n
-  -x don't daemonize\n
+  -h print this text\n
+  -d log debug messages (use twice for extra info)\n
+  -s port  set virtio serial port  [%s]\n
+  -S filename  set udcs socket [%s]\n
+  -u dev   set uinput device   [%s]\n
+  -x don't daemonize\n
  #ifdef HAVE_CONSOLE_KIT
-  -X Disable console kit integration\n
+  -X Disable console kit integration\n
  #endif
  #ifdef HAVE_LIBSYSTEMD_LOGIN
-X Disable systemd-logind integration\n
  #endif
-,VERSION, portdev, uinput_device);
+,VERSION, portdev, vdagentd_socket, uinput_device);
  }

  void daemonize(void)
@@ -834,7 +836,7 @@ int main(int argc, char *argv[])
  struct sigaction act;

  for (;;) {
-if (-1 == (c = getopt(argc, argv, -dhxXs:u:)))
+if (-1 == (c = getopt(argc, argv, -dhxXs:u:S:)))
  break;
  switch (c) {
  case 'd':
@@ -843,6 +845,9 @@ int main(int argc, char *argv[])
  case 's':
  portdev = optarg;
  break;
+case 'S':
+vdagentd_socket = optarg;
+break;
  case 'u':
  uinput_device = optarg;
  break;
@@ -873,18 +878,18 @@ int main(int argc, char *argv[])
  openlog(spice-vdagentd, do_daemonize ? 0 : LOG_PERROR, LOG_USER);

  /* Setup communication with vdagent process(es) */
-server = udscs_create_server(VDAGENTD_SOCKET, agent_connect,
+server = udscs_create_server(vdagentd_socket, agent_connect,
   agent_read_complete, agent_disconnect,
   vdagentd_messages, VDAGENTD_NO_MESSAGES,
   debug);
  if (!server) {
  syslog(LOG_CRIT, Fatal could not create server socket %s,
-   VDAGENTD_SOCKET);
+   vdagentd_socket);
  return 1;
  }
-if (chmod(VDAGENTD_SOCKET, 0666)) {
+if (chmod(vdagentd_socket, 0666)) {
  syslog(LOG_CRIT, Fatal could not change permissions on %s: %m,
-   VDAGENTD_SOCKET);
+   vdagentd_socket);
  udscs_destroy_server(server);
  return 1;
  }
@@ -916,8 +921,8 @@ int main(int argc, char *argv[])
  vdagent_virtio_port_destroy(virtio_port);
  session_info_destroy(session_info);
  udscs_destroy_server(server);
-if (unlink(VDAGENTD_SOCKET) != 0)
-syslog(LOG_ERR, unlink %s: %s, VDAGENTD_SOCKET, strerror(errno));
+if (unlink(vdagentd_socket) != 0)
+syslog(LOG_ERR, unlink %s: %s, vdagentd_socket, strerror(errno));
  syslog(LOG_INFO, vdagentd quiting, returning status %d, retval);

  if (do_daemonize)


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

Re: [Spice-devel] [PATCH vdagent-linux 3/4] vdagentd: support virtio as uds for Xspice usage

[Hans de Goede]

ACK.

On 09/02/2013 05:02 PM, Alon Levy wrote:

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagent-virtio-port.c | 53 +--
  1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/src/vdagent-virtio-port.c b/src/vdagent-virtio-port.c
index 05bd344..bdd22d9 100644
--- a/src/vdagent-virtio-port.c
+++ b/src/vdagent-virtio-port.c
@@ -26,6 +26,9 @@
  #include unistd.h
  #include fcntl.h
  #include sys/select.h
+#include sys/socket.h
+#include sys/un.h
+
  #include vdagent-virtio-port.h

  #define VDP_LAST_PORT VDP_SERVER_PORT
@@ -51,6 +54,7 @@ struct vdagent_virtio_port_chunk_port_data {
  struct vdagent_virtio_port {
  int fd;
  int opening;
+int is_uds;

  /* Chunk read stuff, single buffer, separate header and data buffer */
  int chunk_header_read;
@@ -78,6 +82,8 @@ struct vdagent_virtio_port *vdagent_virtio_port_create(const 
char *portname,
  vdagent_virtio_port_disconnect_callback disconnect_callback)
  {
  struct vdagent_virtio_port *vport;
+struct sockaddr_un address;
+int c;

  vport = calloc(1, sizeof(*vport));
  if (!vport)
@@ -85,16 +91,33 @@ struct vdagent_virtio_port 
*vdagent_virtio_port_create(const char *portname,

  vport-fd = open(portname, O_RDWR);
  if (vport-fd == -1) {
-syslog(LOG_ERR, open %s: %m, portname);
-free(vport);
-return NULL;
-}
+syslog(LOG_INFO, open %s: %m; trying as socket, portname);
+vport-fd = socket(PF_UNIX, SOCK_STREAM, 0);
+if (vport-fd == -1) {
+goto error;
+}
+address.sun_family = AF_UNIX;
+snprintf(address.sun_path, sizeof(address.sun_path), %s, portname);
+c = connect(vport-fd, (struct sockaddr *)address, sizeof(address));
+if (c == 0) {
+vport-is_uds = 1;
+} else {
+goto error;
+}
+} else {
+vport-is_uds = 0;
+}
  vport-opening = 1;

  vport-read_callback = read_callback;
  vport-disconnect_callback = disconnect_callback;

  return vport;
+
+error:
+syslog(LOG_ERR, open %s: %m, portname);
+free(vport);
+return NULL;
  }

  void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
@@ -333,6 +356,15 @@ static void vdagent_virtio_port_do_chunk(struct 
vdagent_virtio_port **vportp)
  }
  }

+static int vport_read(struct vdagent_virtio_port *vport, char *buf, int len)
+{
+if (vport-is_uds) {
+return recv(vport-fd, buf, len, 0);
+} else {
+return read(vport-fd, buf, len);
+}
+}
+
  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
  {
  ssize_t n;
@@ -348,7 +380,7 @@ static void vdagent_virtio_port_do_read(struct 
vdagent_virtio_port **vportp)
  dest = vport-chunk_data + vport-chunk_data_pos;
  }

-n = read(vport-fd, dest, to_read);
+n = vport_read(vport, dest, to_read);
  if (n  0) {
  if (errno == EINTR)
  return;
@@ -412,6 +444,15 @@ static void vdagent_virtio_port_do_read(struct 
vdagent_virtio_port **vportp)
  }
  }

+static int vport_write(struct vdagent_virtio_port *vport, char *buf, int len)
+{
+if (vport-is_uds) {
+return send(vport-fd, buf, len, 0);
+} else {
+return write(vport-fd, buf, len);
+}
+}
+
  static void vdagent_virtio_port_do_write(struct vdagent_virtio_port **vportp)
  {
  ssize_t n;
@@ -430,7 +471,7 @@ static void vdagent_virtio_port_do_write(struct 
vdagent_virtio_port **vportp)
  }

  to_write = wbuf-size - wbuf-pos;
-n = write(vport-fd, wbuf-buf + wbuf-pos, to_write);
+n = vport_write(vport, wbuf-buf + wbuf-pos, to_write);
  if (n  0) {
  if (errno == EINTR)
  return;


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

Re: [Spice-devel] [PATCH vdagent-linux 4/4] vdagentd: support fake uinput

[Hans de Goede]

Comments inline.

On 09/02/2013 05:02 PM, Alon Levy wrote:

This is used with Xspice. Fake means we open a pipe for write only, and
don't do any ioctls on it. Specifically it means the axis and buttons
have to be coordinated for now with Xspice (xf86-video-qxl).

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagentd-uinput.c | 11 +--
  src/vdagentd-uinput.h |  2 +-
  src/vdagentd.c| 23 +--
  3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/src/vdagentd-uinput.c b/src/vdagentd-uinput.c
index a415f9c..47e1b45 100644
--- a/src/vdagentd-uinput.c
+++ b/src/vdagentd-uinput.c
@@ -42,12 +42,13 @@ struct vdagentd_uinput {
  struct vdagentd_guest_xorg_resolution *screen_info;
  int screen_count;
  VDAgentMouseState last;
+int fake;
  };

  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
  int width, int height,
  struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
-int debug)
+int debug, int fake)
  {
  struct vdagentd_uinput *uinput;

@@ -58,6 +59,7 @@ struct vdagentd_uinput *vdagentd_uinput_create(const char 
*devname,
  uinput-devname = devname;
  uinput-fd  = -1; /* Gets opened by vdagentd_uinput_update_size() */
  uinput-debug   = debug;
+uinput-fake= fake;

  vdagentd_uinput_update_size(uinput, width, height,
  screen_info, screen_count);
@@ -119,13 +121,18 @@ void vdagentd_uinput_update_size(struct vdagentd_uinput 
**uinputp,
  return;
  #endif

-uinput-fd = open(uinput-devname, O_RDWR);
+uinput-fd = open(uinput-devname, uinput-fake ? O_WRONLY : O_RDWR);
  if (uinput-fd == -1) {
  syslog(LOG_ERR, open %s: %m, uinput-devname);
  vdagentd_uinput_destroy(uinputp);
  return;
  }

+if (uinput-fake) {
+/* fake device doesn't understand any ioctls and only writes events */
+return;
+}
+
  rc = write(uinput-fd, device, sizeof(device));
  if (rc != sizeof(device)) {
  syslog(LOG_ERR, write %s: %m, uinput-devname);
diff --git a/src/vdagentd-uinput.h b/src/vdagentd-uinput.h
index 81f0376..b9bd9f1 100644
--- a/src/vdagentd-uinput.h
+++ b/src/vdagentd-uinput.h
@@ -30,7 +30,7 @@ struct vdagentd_uinput;
  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
  int width, int height,
  struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
-int debug);
+int debug, int fake);
  void vdagentd_uinput_destroy(struct vdagentd_uinput **uinputp);

  void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
diff --git a/src/vdagentd.c b/src/vdagentd.c
index 2288671..54765ce 100644
--- a/src/vdagentd.c
+++ b/src/vdagentd.c
@@ -58,6 +58,7 @@ static const char *portdev = 
/dev/virtio-ports/com.redhat.spice.0;
  static const char *vdagentd_socket = VDAGENTD_SOCKET;
  static const char *uinput_device = /dev/uinput;
  static int debug = 0;
+static int uinput_fake = 0;
  static struct udscs_server *server = NULL;
  static struct vdagent_virtio_port *virtio_port = NULL;
  static GHashTable *active_xfers = NULL;
@@ -317,11 +318,10 @@ int virtio_port_read_complete(
  agent_data-height,
  agent_data-screen_info,
  agent_data-screen_count,
-debug  1);
+debug  1,
+uinput_fake);
  if (!uinput) {
-syslog(LOG_CRIT, Fatal uinput error);
-retval = 1;
-quit = 1;
+syslog(LOG_CRIT, uinput error);
  }
  }
  break;


Making this not fatal is a bad idea in the non Xspice case (we keep the
virtio port open then, so all mouse events get send to the agent and then
dropped). Also if we want to make this non fatal (which IMHO we don't),
that should be done in a separate patch.


@@ -488,7 +488,8 @@ static void check_xorg_resolution(void)
  agent_data-height,
  agent_data-screen_info,
  agent_data-screen_count,
-debug  1);
+debug  1,
+uinput_fake);
  else
  vdagentd_uinput_update_size(uinput,
  agent_data-width,
@@ -496,10 +497,7 @@ static void check_xorg_resolution(void)
  agent_data-screen_info,
  agent_data-screen_count);
  if (!uinput) {
-syslog(LOG_CRIT, Fatal uinput error);
-retval = 1;
-quit = 1;
-   

Re: [Spice-devel] [PATCH v2 vdagent-linux] vdagentd: support fake uinput

[Hans de Goede]

Hi,

On 09/02/2013 05:12 PM, Alon Levy wrote:

This is used with Xspice. Fake means we open a pipe for write only, and
don't do any ioctls on it. Specifically it means the axis and buttons
have to be coordinated for now with Xspice (xf86-video-qxl).

Signed-off-by: Alon Levy al...@redhat.com
---
  src/vdagentd-uinput.c | 11 +--
  src/vdagentd-uinput.h |  2 +-
  src/vdagentd.c| 14 +++---
  3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/vdagentd-uinput.c b/src/vdagentd-uinput.c
index a415f9c..47e1b45 100644
--- a/src/vdagentd-uinput.c
+++ b/src/vdagentd-uinput.c
@@ -42,12 +42,13 @@ struct vdagentd_uinput {
  struct vdagentd_guest_xorg_resolution *screen_info;
  int screen_count;
  VDAgentMouseState last;
+int fake;
  };

  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
  int width, int height,
  struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
-int debug)
+int debug, int fake)
  {
  struct vdagentd_uinput *uinput;

@@ -58,6 +59,7 @@ struct vdagentd_uinput *vdagentd_uinput_create(const char 
*devname,
  uinput-devname = devname;
  uinput-fd  = -1; /* Gets opened by vdagentd_uinput_update_size() */
  uinput-debug   = debug;
+uinput-fake= fake;

  vdagentd_uinput_update_size(uinput, width, height,
  screen_info, screen_count);
@@ -119,13 +121,18 @@ void vdagentd_uinput_update_size(struct vdagentd_uinput 
**uinputp,
  return;
  #endif

-uinput-fd = open(uinput-devname, O_RDWR);
+uinput-fd = open(uinput-devname, uinput-fake ? O_WRONLY : O_RDWR);
  if (uinput-fd == -1) {
  syslog(LOG_ERR, open %s: %m, uinput-devname);
  vdagentd_uinput_destroy(uinputp);
  return;
  }

+if (uinput-fake) {
+/* fake device doesn't understand any ioctls and only writes events */
+return;
+}
+
  rc = write(uinput-fd, device, sizeof(device));
  if (rc != sizeof(device)) {
  syslog(LOG_ERR, write %s: %m, uinput-devname);
diff --git a/src/vdagentd-uinput.h b/src/vdagentd-uinput.h
index 81f0376..b9bd9f1 100644
--- a/src/vdagentd-uinput.h
+++ b/src/vdagentd-uinput.h
@@ -30,7 +30,7 @@ struct vdagentd_uinput;
  struct vdagentd_uinput *vdagentd_uinput_create(const char *devname,
  int width, int height,
  struct vdagentd_guest_xorg_resolution *screen_info, int screen_count,
-int debug);
+int debug, int fake);
  void vdagentd_uinput_destroy(struct vdagentd_uinput **uinputp);

  void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
diff --git a/src/vdagentd.c b/src/vdagentd.c
index 2288671..80726af 100644
--- a/src/vdagentd.c
+++ b/src/vdagentd.c
@@ -58,6 +58,7 @@ static const char *portdev = 
/dev/virtio-ports/com.redhat.spice.0;
  static const char *vdagentd_socket = VDAGENTD_SOCKET;
  static const char *uinput_device = /dev/uinput;
  static int debug = 0;
+static int uinput_fake = 0;
  static struct udscs_server *server = NULL;
  static struct vdagent_virtio_port *virtio_port = NULL;
  static GHashTable *active_xfers = NULL;
@@ -317,7 +318,8 @@ int virtio_port_read_complete(
  agent_data-height,
  agent_data-screen_info,
  agent_data-screen_count,
-debug  1);
+debug  1,
+uinput_fake);
  if (!uinput) {
  syslog(LOG_CRIT, Fatal uinput error);
  retval = 1;
@@ -488,7 +490,8 @@ static void check_xorg_resolution(void)
  agent_data-height,
  agent_data-screen_info,
  agent_data-screen_count,
-debug  1);
+debug  1,
+uinput_fake);
  else
  vdagentd_uinput_update_size(uinput,
  agent_data-width,
@@ -867,6 +870,11 @@ int main(int argc, char *argv[])
  }
  }

+if (strncmp(uinput_device, /dev, 4) != 0) {
+fprintf(stderr, %s: using fake uinput\n, __func__);
+uinput_fake = 1;
+}
+


This still needs to be changed to use syslog and to not use __func__

Regards,

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

Re: [Spice-devel] [PATCH] channel-main: Fix minor documentation issue

[Hans de Goede]

ACK.

On 08/27/2013 09:56 PM, Jonathon Jongsma wrote:

Documentation for spice_main_clipboard_selection_request() refers to deprecated
signal 'main-clipboard' instead of 'main-clipboard-selection'
---
  gtk/channel-main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index b9e0da2..fb1bc00 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -2650,7 +2650,7 @@ void spice_main_clipboard_request(SpiceMainChannel 
*channel, guint32 type)
   * @type: a #VD_AGENT_CLIPBOARD type
   *
   * Request clipboard data of @type from the guest. The reply is sent
- * through the #SpiceMainChannel::main-clipboard signal.
+ * through the #SpiceMainChannel::main-clipboard-selection signal.
   *
   * Since: 0.6
   **/


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

Re: [Spice-devel] [PATCH spice-gtk 1/4] gtk: remove dead code

[Hans de Goede]

Looks good, ACK.

On 08/23/2013 10:25 PM, Marc-André Lureau wrote:

---
  gtk/spice-gtk-session.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/gtk/spice-gtk-session.c b/gtk/spice-gtk-session.c
index 47c7d36..68777eb 100644
--- a/gtk/spice-gtk-session.c
+++ b/gtk/spice-gtk-session.c
@@ -367,7 +367,6 @@ static gint 
get_selection_from_clipboard(SpiceGtkSessionPrivate *s,
  static const struct {
  const char  *xatom;
  uint32_tvdagent;
-uint32_tflags;
  } atom2agent[] = {
  {
  .vdagent = VD_AGENT_CLIPBOARD_UTF8_TEXT,
@@ -660,7 +659,6 @@ static gboolean clipboard_grab(SpiceMainChannel *main, 
guint selection,
  found = TRUE;
  g_return_val_if_fail(i  SPICE_N_ELEMENTS(atom2agent), FALSE);
  targets[i].target = (gchar*)atom2agent[m].xatom;
-targets[i].flags = 0;
  targets[i].info = m;
  target_selected[m] = TRUE;
  i += 1;


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

Re: [Spice-devel] [PATCH spice-gtk 2/4] Revert channel-main: Convert text line-endings if necessary (rhbz#752350)

[Hans de Goede]

Looks good, ACK.

On 08/23/2013 10:25 PM, Marc-André Lureau wrote:

This implementation assumes that the toolkit doesn't do any conversion
on its own. This is actually not the case, gtk+ converts line endings
already on windows.  It would be pretty ugly to do conversions back and
forth at different levels. So the channel implementation shouldn't
try to do conversion, and implementation must be done at higher level,
where the actual system clipboard implementation is known (at
gtk-session for instance)

Since this code hasn't been officially released, let's revert it and
implement a better crlf conversion handling in gtk-session in the
following commits.

This reverts commit e45a446a9981ad4adaeff9c885962a8c6140333e.
---
  gtk/channel-main.c | 73 +++---
  1 file changed, 4 insertions(+), 69 deletions(-)

diff --git a/gtk/channel-main.c b/gtk/channel-main.c
index b9e0da2..b58af52 100644
--- a/gtk/channel-main.c
+++ b/gtk/channel-main.c
@@ -1181,24 +1181,6 @@ static void agent_announce_caps(SpiceMainChannel 
*channel)
  #define HAS_CLIPBOARD_SELECTION(c) \
  VD_AGENT_HAS_CAPABILITY((c)-agent_caps, G_N_ELEMENTS((c)-agent_caps), 
VD_AGENT_CAP_CLIPBOARD_SELECTION)

-#define GUEST_LINEEND_LF(c) \
-VD_AGENT_HAS_CAPABILITY((c)-agent_caps, G_N_ELEMENTS((c)-agent_caps), 
VD_AGENT_CAP_GUEST_LINEEND_LF)
-
-#define GUEST_LINEEND_CRLF(c) \
-VD_AGENT_HAS_CAPABILITY((c)-agent_caps, G_N_ELEMENTS((c)-agent_caps), 
VD_AGENT_CAP_GUEST_LINEEND_CRLF)
-
-#ifdef G_OS_UNIX
-#define CLIENT_LINEEND_LF 1
-#else
-#define CLIENT_LINEEND_LF 0
-#endif
-
-#ifdef G_OS_WIN32
-#define CLIENT_LINEEND_CRLF 1
-#else
-#define CLIENT_LINEEND_CRLF 0
-#endif
-
  /* any context: the message is not flushed immediately,
 you can wakeup() the channel coroutine or send_msg_queue() */
  static void agent_clipboard_grab(SpiceMainChannel *channel, guint selection,
@@ -1769,29 +1751,6 @@ static void file_xfer_handle_status(SpiceMainChannel 
*channel,
  file_xfer_completed(task, error);
  }

-/* any context */
-static guchar *convert_lineend(const guchar *in, gsize *size,
-   const gchar *from, const gchar *to)
-{
-gchar *nul_terminated, **split, *out;
-
-/* Nul-terminate */
-nul_terminated = g_malloc(*size + 1);
-memcpy(nul_terminated, in, *size);
-nul_terminated[*size] = 0;
-
-/* Convert */
-split = g_strsplit(nul_terminated, from, -1);
-out = g_strjoinv(to, split);
-*size = strlen(out);
-
-/* Clean-up */
-g_strfreev(split);
-g_free(nul_terminated);
-
-return (guchar *)out;
-}
-
  /* coroutine context */
  static void main_agent_handle_msg(SpiceChannel *channel,
VDAgentMessage *msg, gpointer payload)
@@ -1850,22 +1809,12 @@ static void main_agent_handle_msg(SpiceChannel *channel,
  case VD_AGENT_CLIPBOARD:
  {
  VDAgentClipboard *cb = payload;
-guchar *data = cb-data;
-gsize size = msg-size - sizeof(VDAgentClipboard);
-if (cb-type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
-if (GUEST_LINEEND_LF(c)  CLIENT_LINEEND_CRLF)
-data = convert_lineend(data, size, \n, \r\n);
-if (GUEST_LINEEND_CRLF(c)  CLIENT_LINEEND_LF)
-data = convert_lineend(data, size, \r\n, \n);
-}
  emit_main_context(channel, SPICE_MAIN_CLIPBOARD_SELECTION, selection,
-  cb-type, data, size);
+  cb-type, cb-data, msg-size - 
sizeof(VDAgentClipboard));

-if (selection == VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD)
+   if (selection == VD_AGENT_CLIPBOARD_SELECTION_CLIPBOARD)
  emit_main_context(channel, SPICE_MAIN_CLIPBOARD,
-  cb-type, data, size);
-if (data != cb-data)
-g_free(data);
+  cb-type, cb-data, msg-size - 
sizeof(VDAgentClipboard));
  break;
  }
  case VD_AGENT_CLIPBOARD_GRAB:
@@ -2605,27 +2554,13 @@ void spice_main_clipboard_notify(SpiceMainChannel 
*channel,
   * Since: 0.6
   **/
  void spice_main_clipboard_selection_notify(SpiceMainChannel *channel, guint 
selection,
-   guint32 type, const guchar *_data, 
size_t _size)
+   guint32 type, const guchar *data, 
size_t size)
  {
-const guchar *data = _data;
-gsize size = _size;
-
  g_return_if_fail(channel != NULL);
  g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));

-SpiceMainChannelPrivate *c = channel-priv;
-
-if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
-if (CLIENT_LINEEND_CRLF  GUEST_LINEEND_LF(c))
-data = convert_lineend(data, size, \r\n, \n);
-if (CLIENT_LINEEND_LF  GUEST_LINEEND_CRLF(c))
-data = convert_lineend(data, size, \n, \r\n);
-}
  agent_clipboard_notify(channel, selection, type, data, size);
  

Re: [Spice-devel] [PATCH spice-gtk 3/4] util: add unix2dos and dos2unix

[Hans de Goede]

Hi,

On 08/23/2013 10:25 PM, Marc-André Lureau wrote:

Convert line endings from/to LF/CRLF, in utf8.
---
  gtk/spice-util-priv.h |   2 +
  gtk/spice-util.c  | 122 ++
  2 files changed, 124 insertions(+)

diff --git a/gtk/spice-util-priv.h b/gtk/spice-util-priv.h
index ee5a42d..cc559dc 100644
--- a/gtk/spice-util-priv.h
+++ b/gtk/spice-util-priv.h
@@ -29,6 +29,8 @@ gboolean spice_strv_contains(const GStrv strv, const gchar 
*str);
  gchar* spice_uuid_to_string(const guint8 uuid[16]);
  const gchar* spice_yes_no(gboolean value);
  guint16 spice_make_scancode(guint scancode, gboolean release);
+gchar* spice_unix2dos(const gchar *str, gssize len, GError **error);
+gchar* spice_dos2unix(const gchar *str, gssize len, GError **error);

  #if GLIB_CHECK_VERSION(2,32,0)
  #define STATIC_MUTEXGMutex
diff --git a/gtk/spice-util.c b/gtk/spice-util.c
index 774a145..be10edc 100644
--- a/gtk/spice-util.c
+++ b/gtk/spice-util.c
@@ -19,6 +19,7 @@
  #ifdef HAVE_CONFIG_H
  # include config.h
  #endif
+
  #include stdlib.h
  #include string.h
  #include glib-object.h
@@ -245,3 +246,124 @@ guint16 spice_make_scancode(guint scancode, gboolean 
release)

  g_return_val_if_reached(0);
  }
+
+typedef enum {
+NEWLINE_TYPE_LF,
+NEWLINE_TYPE_CR_LF
+} NewlineType;
+
+static gssize get_line(const gchar *str, gsize len,
+   NewlineType type, gsize *nl_len,
+   GError **error)
+{
+const gchar *p = str;
+gsize nl = 0;
+
+if (type == NEWLINE_TYPE_CR_LF) {
+while ((p - str)  len) {
+p = g_utf8_strchr(p, len, '\r');
+if (!p)
+break;
+p = g_utf8_next_char(p);
+if (g_utf8_get_char(p) == '\n') {
+len = (p - str) - 1;
+nl = 2;
+break;
+}
+}
+} else {
+p = g_utf8_strchr(str, len, '\n');
+if (p) {
+len = p - str;
+nl = 1;
+}
+}


This looks way more complicated then it needs to be, in UTF-8
0x00 - 0x7f only are valid as a single-byte sequence. multi-byte
encoded characters will never contain 0x00 - 0x7f. UTF-8 was designed
this way, is so that existing string parsing code for non multi-byte
encodings, which make look for example for '  = or LF characters does
not break when parsing strings with multi-byte characters in there.

TL;DR: LF and CR will never be part of a multi byte character, so
you can simple do: strstr(str, \r\n) to find the CRLF.



+
+if (!g_utf8_validate(str, len, NULL)) {
+g_set_error_literal(error, G_CONVERT_ERROR,
+G_CONVERT_ERROR_ILLEGAL_SEQUENCE,
+Invalid byte sequence in conversion input);
+return -1;
+}


And once you simply treat this as a regular C-string without worrying
about multi-byte encodings you can also drop this.


+
+*nl_len = nl;
+return len;
+}
+
+
+static gchar* spice_convert_newlines(const gchar *str, gssize len,
+ NewlineType from,
+ NewlineType to,
+ GError **error)
+{
+GError *err = NULL;
+gssize length;
+gsize nl;
+GString *output;
+gboolean free_segment = FALSE;
+gint i;
+
+g_return_val_if_fail(str != NULL, NULL);
+g_return_val_if_fail(len = -1, NULL);
+g_return_val_if_fail(error == NULL || *error == NULL, NULL);
+/* only 2 supported combinations */
+g_return_val_if_fail((from == NEWLINE_TYPE_LF 
+  to == NEWLINE_TYPE_CR_LF) ||
+ (from == NEWLINE_TYPE_CR_LF 
+  to == NEWLINE_TYPE_LF), NULL);
+
+if (len == -1)
+len = strlen(str);
+/* sometime we get \0 terminated strings, skip that, or it fails
+   to utf8 validate line with \0 end */
+else if (str[len] == 0)
+len -= 1;
+
+/* allocate worst case, if it's small enough, we don't care much,
+ * if it's big, malloc will put us in mmap'd region, and we can
+ * over allocate.
+ */
+output = g_string_sized_new(len * 2 + 1);
+
+for (i = 0; i  len; i += length + nl) {
+length = get_line(str + i, len - i, from, nl, error);
+if (length  0)
+break;
+
+g_string_append_len(output, str + i, length);
+
+if (nl) {
+/* let's not double \r if it's already in the line */
+if (to == NEWLINE_TYPE_CR_LF 
+output-str[output-len - 1] != '\r')
+g_string_append_c(output, '\r');
+
+g_string_append_c(output, '\n');
+}
+}
+
+if (err) {
+g_propagate_error(error, err);
+free_segment = TRUE;
+}
+
+return g_string_free(output, free_segment);
+}
+
+G_GNUC_INTERNAL
+gchar* spice_dos2unix(const gchar *str, gssize len, GError **error)
+{
+return 

Re: [Spice-devel] [PATCH spice-gtk] tests: add some dos2unix tests

[Hans de Goede]

Looks good, ack.

On 08/24/2013 02:59 AM, Marc-André Lureau wrote:

This is probably not exhaustive enough, but better than nothing.
---
  Makefile.am   |  2 +-
  configure.ac  |  1 +
  gtk/spice-util.c  |  1 +
  tests/Makefile.am | 18 +++
  tests/util.c  | 89 +++
  5 files changed, 110 insertions(+), 1 deletion(-)
  create mode 100644 tests/Makefile.am
  create mode 100644 tests/util.c

diff --git a/Makefile.am b/Makefile.am
index ffa1649..ab10f5f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,7 +1,7 @@
  ACLOCAL_AMFLAGS = -I m4
  NULL =

-SUBDIRS = spice-common gtk po doc data
+SUBDIRS = spice-common gtk po doc data tests

  if HAVE_INTROSPECTION
  if WITH_VALA
diff --git a/configure.ac b/configure.ac
index 1235f4a..74738a3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -685,6 +685,7 @@ gtk/controller/Makefile
  doc/Makefile
  doc/reference/Makefile
  vapi/Makefile
+tests/Makefile
  ])

  dnl ==
diff --git a/gtk/spice-util.c b/gtk/spice-util.c
index e76320e..e04b5f5 100644
--- a/gtk/spice-util.c
+++ b/gtk/spice-util.c
@@ -22,6 +22,7 @@

  #include stdlib.h
  #include string.h
+#include glib.h
  #include glib-object.h
  #include spice-util-priv.h
  #include spice-util.h
diff --git a/tests/Makefile.am b/tests/Makefile.am
new file mode 100644
index 000..9510e2c
--- /dev/null
+++ b/tests/Makefile.am
@@ -0,0 +1,18 @@
+NULL =
+
+noinst_PROGRAMS = util
+TESTS = $(noinst_PROGRAMS)
+
+AM_CPPFLAGS =  \
+   $(GIO_CFLAGS) -I$(top_srcdir)/gtk   \
+   -DG_LOG_DOMAIN=\GSpice\ \
+   $(NULL)
+AM_LDFLAGS = $(GIO_LIBS)
+
+util_SOURCES = \
+   $(top_srcdir)/gtk/spice-util-priv.h \
+   $(top_srcdir)/gtk/spice-util.c  \
+   $(top_srcdir)/gtk/spice-util.h  \
+   util.c  \
+   $(NULL)
+
diff --git a/tests/util.c b/tests/util.c
new file mode 100644
index 000..86109aa
--- /dev/null
+++ b/tests/util.c
@@ -0,0 +1,89 @@
+#include glib.h
+#include stdio.h
+#include string.h
+#include stdlib.h
+
+#include spice-util-priv.h
+
+enum {
+DOS2UNIX = 1  0,
+UNIX2DOS = 1  1,
+};
+
+static const struct {
+const gchar *d;
+const gchar *u;
+glong flags;
+} dosunix[] = {
+{ , , DOS2UNIX|UNIX2DOS },
+{ a, a, DOS2UNIX|UNIX2DOS },
+{ \r\n, \n, DOS2UNIX|UNIX2DOS },
+{ \r\n\r\n, \n\n, DOS2UNIX|UNIX2DOS },
+{ a\r\n, a\n, DOS2UNIX|UNIX2DOS },
+{ a\r\n\r\n, a\n\n, DOS2UNIX|UNIX2DOS },
+{ \r\n\r\na\r\n\r\n, \n\na\n\n, DOS2UNIX|UNIX2DOS },
+{ 1\r\n\r\na\r\n\r\n2, 1\n\na\n\n2, DOS2UNIX|UNIX2DOS },
+{ \n, \n, DOS2UNIX },
+{ \n\n, \n\n, DOS2UNIX },
+{ \r\n, \r\n, UNIX2DOS },
+{ \r\r\n, \r\r\n, UNIX2DOS },
+{ é\r\né, é\né, DOS2UNIX|UNIX2DOS },
+{ \r\né\r\né\r\n, \né\né\n, DOS2UNIX|UNIX2DOS }
+/* TODO: add some utf8 test cases */
+};
+
+static void test_dos2unix(void)
+{
+GError *err = NULL;
+gchar *tmp;
+int i;
+
+for (i = 0; i  G_N_ELEMENTS(dosunix); i++) {
+if (!(dosunix[i].flags  DOS2UNIX))
+continue;
+
+tmp = spice_dos2unix(dosunix[i].d, -1, err);
+g_assert_cmpstr(tmp, ==, dosunix[i].u);
+g_assert_no_error(err);
+g_free(tmp);
+
+/* including ending \0 */
+tmp = spice_dos2unix(dosunix[i].d, strlen(dosunix[i].d) + 1, err);
+g_assert_cmpstr(tmp, ==, dosunix[i].u);
+g_assert_no_error(err);
+g_free(tmp);
+}
+}
+
+static void test_unix2dos(void)
+{
+GError *err = NULL;
+gchar *tmp;
+int i;
+
+for (i = 0; i  G_N_ELEMENTS(dosunix); i++) {
+if (!(dosunix[i].flags  UNIX2DOS))
+continue;
+
+tmp = spice_unix2dos(dosunix[i].u, -1, err);
+g_assert_cmpstr(tmp, ==, dosunix[i].d);
+g_assert_no_error(err);
+g_free(tmp);
+
+/* including ending \0 */
+tmp = spice_unix2dos(dosunix[i].u, strlen(dosunix[i].u) + 1, err);
+g_assert_cmpstr(tmp, ==, dosunix[i].d);
+g_assert_no_error(err);
+g_free(tmp);
+}
+}
+
+int main(int argc, char* argv[])
+{
+  g_test_init(argc, argv, NULL);
+
+  g_test_add_func(/util/dos2unix, test_dos2unix);
+  g_test_add_func(/util/unix2dos, test_unix2dos);
+
+  return g_test_run ();
+}


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

Re: [Spice-devel] [PATCH spice-gtk 4/4] gtk: handle clipboard CRLF conversion

[Hans de Goede]

Hi,

On 08/23/2013 10:25 PM, Marc-André Lureau wrote:

gtk+ internal text/utf8 is using LF conversion, on all platforms.
Even though the toolkit may only handle a single line ending type, we
may want to avoid the conversion for Spice use cases, gtk+ could learn a
new native utf8 target type, see also:
https://bugzilla.gnome.org/show_bug.cgi?id=706657

In the meantime, the only thing we need to convert, is to/from crlf
guest (from/to lf). This is what this change is about.

It has been tested successfully with the various guest/client OS
combinations.
---
  gtk/Makefile.am |  2 ++
  gtk/spice-gtk-session.c | 56 -
  2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/gtk/Makefile.am b/gtk/Makefile.am
index 82aa9a3..5af6642 100644
--- a/gtk/Makefile.am
+++ b/gtk/Makefile.am
@@ -117,6 +117,8 @@ SPICE_GTK_LIBADD_COMMON =   \

  SPICE_GTK_SOURCES_COMMON =\
glib-compat.h   \
+   spice-util.c\
+   spice-util-priv.h   \
spice-gtk-session.c \
spice-gtk-session-priv.h\
spice-widget.c  \
diff --git a/gtk/spice-gtk-session.c b/gtk/spice-gtk-session.c
index 68777eb..476af95 100644
--- a/gtk/spice-gtk-session.c
+++ b/gtk/spice-gtk-session.c
@@ -23,6 +23,7 @@
  #include spice-gtk-session.h
  #include spice-gtk-session-priv.h
  #include spice-session-priv.h
+#include spice-util-priv.h

  #define CLIPBOARD_LAST (VD_AGENT_CLIPBOARD_SELECTION_SECONDARY + 1)

@@ -548,6 +549,7 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,

  typedef struct
  {
+SpiceGtkSession *self;
  GMainLoop *loop;
  GtkSelectionData *selection_data;
  guint info;
@@ -555,21 +557,45 @@ typedef struct
  } RunInfo;

  static void clipboard_got_from_guest(SpiceMainChannel *main, guint selection,
- guint type, guchar *data, guint size,
+ guint type, const guchar *data, guint 
size,
   gpointer user_data)
  {
  RunInfo *ri = user_data;
+SpiceGtkSessionPrivate *s = ri-self-priv;
+gchar *conv = NULL;

  g_return_if_fail(selection == ri-selection);

  SPICE_DEBUG(clipboard got data);

-gtk_selection_data_set(ri-selection_data,
-gdk_atom_intern_static_string(atom2agent[ri-info].xatom),
-8, data, size);
+if (atom2agent[ri-info].vdagent == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
+/* on windows, gtk+ would already convert to LF endings, but
+   not on unix */
+if (spice_main_agent_test_capability(s-main, 
VD_AGENT_CAP_GUEST_LINEEND_CRLF)) {
+GError *err = NULL;
+
+conv = spice_dos2unix((gchar*)data, size, err);
+if (err) {
+g_warning(Failed to convert text line ending: %s, 
err-message);
+g_clear_error(err);
+goto end;
+}
+
+size = strlen(conv);
+}
+
+gtk_selection_data_set_text(ri-selection_data, conv ?: (gchar*)data, 
size);


Why the conv ?: (gchar*)data here ? Should conv not always be non NULL here, 
unless
size = 0, in which case conv == NULL is fine ?




+} else {
+gtk_selection_data_set(ri-selection_data,
+gdk_atom_intern_static_string(atom2agent[ri-info].xatom),
+8, data, size);
+}

+end:
  if (g_main_loop_is_running (ri-loop))
  g_main_loop_quit (ri-loop);
+
+g_free(conv);
  }

  static void clipboard_agent_connected(RunInfo *ri)
@@ -604,6 +630,7 @@ static void clipboard_get(GtkClipboard *clipboard,
  ri.info = info;
  ri.loop = g_main_loop_new(NULL, FALSE);
  ri.selection = selection;
+ri.self = self;

  clipboard_handler = g_signal_connect(s-main, main-clipboard-selection,
   G_CALLBACK(clipboard_got_from_guest),
@@ -740,8 +767,27 @@ static void clipboard_received_cb(GtkClipboard *clipboard,
  g_free(name);
  }

+const guchar *data = gtk_selection_data_get_data(selection_data);
+gpointer conv = NULL;
+
+/* gtk+ internal utf8 newline is always LF, even on windows */
+if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT 
+spice_main_agent_test_capability(s-main, 
VD_AGENT_CAP_GUEST_LINEEND_CRLF)) {
+GError *err = NULL;
+
+conv = spice_unix2dos((gchar*)data, len, err);
+if (err) {
+g_warning(Failed to convert text line ending: %s, err-message);
+g_clear_error(err);
+return;
+}
+
+len = strlen(conv);
+}
+
  spice_main_clipboard_selection_notify(s-main, selection, type,
-gtk_selection_data_get_data(selection_data), len);
+  conv ?: data, len);
+g_free(conv);


Same question here.

Otherwise this looks good.

Regards,

Hans