from:"Daniel P. Berrange"

On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote:
> > One reason is that you may use a library dynamically, and you may dlclose() 
> > it, and then atexit() will likely crash. 
> 
> Is that a real or theoretical scenario? Who loads this library dynamically
> currently?

Anyone using SPICE from a non-C language will have dlopen'd it and potentially
later dlclose it. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

On Thu, Oct 19, 2017 at 08:53:08AM -0400, Frediano Ziglio wrote:
> > 
> > On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > 
> > > > In fact non-trivial shared libraries should generally never be unloaded,
> > > > even
> > > > if they were originally dlopend.  If the library has used a pthread 
> > > > local
> > > > with
> > > > a destructor function, then unloading the library will remove the code
> > > > that
> > > > contains the impl of the destructor. When the thread later exits and its
> > > > thread
> > > > locals are cleaned up, the application will crash & burn.
> > > > 
> > > > Many libraries, including libvirt, will link with '-z nodelete' to
> > > > prevent
> > > > unloading of the library even if dlclose() is called, to avoid these 
> > > > kind
> > > > of crashes.
> > > > 
> > > > IOW getting perfect "cleanup" is just a fools errand and will likely
> > > > create
> > > > obscure problems down the road that are worse than the problems the
> > > > cleanup
> > > > is trying to solve. Just accept normal process resource cleanup when the
> > > > application exits.
> > > 
> > > This is a point for Windows... they manage to have unloading working.
> > > Also you can unload Linux kernel modules.
> > > I honestly find these reasoning a lazy excuse to bad programming and
> > > design.
> > 
> > Calling it laziness is completely missing the point. There is *nothing*
> > in the pthreads API that lets you avoid the problem with thread local
> > destructors described above, no matter how much we want to fix it. The
> > only "solution" is to not use pthread locals at all, which is not practical.
> > 
> 
> If not lazy as I said is bad design, not? Somebody designed pthread too.

POSIX designed pthreads, but dlopen comes from Solaris, only after that
adopted as part of POSIX. The situation is just a result of the way the
standard has organically grown over time, not been designed top down to
deal with all possible interactions

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote:
> > 
> > 
> > In fact non-trivial shared libraries should generally never be unloaded, 
> > even
> > if they were originally dlopend.  If the library has used a pthread local
> > with
> > a destructor function, then unloading the library will remove the code that
> > contains the impl of the destructor. When the thread later exits and its
> > thread
> > locals are cleaned up, the application will crash & burn.
> > 
> > Many libraries, including libvirt, will link with '-z nodelete' to prevent
> > unloading of the library even if dlclose() is called, to avoid these kind
> > of crashes.
> > 
> > IOW getting perfect "cleanup" is just a fools errand and will likely create
> > obscure problems down the road that are worse than the problems the cleanup
> > is trying to solve. Just accept normal process resource cleanup when the
> > application exits.
> 
> This is a point for Windows... they manage to have unloading working.
> Also you can unload Linux kernel modules.
> I honestly find these reasoning a lazy excuse to bad programming and design.

Calling it laziness is completely missing the point. There is *nothing*
in the pthreads API that lets you avoid the problem with thread local
destructors described above, no matter how much we want to fix it. The
only "solution" is to not use pthread locals at all, which is not practical.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

On Thu, Oct 19, 2017 at 07:47:24AM -0400, Frediano Ziglio wrote:
> > > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
> > 
> 
> > > > From: Christophe de Dinechin < dinec...@redhat.com >
> > > 
> > 
> 
> > > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > 
> > 
> > > > that perform some of their operations within gst_deinit.
> > > 
> > 
> 
> > > > Without this patch, if you run spicy with
> > > 
> > 
> > > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > 
> > 
> > > > the leak tracer does not run at exit, because it runs in gst_deinit.
> > > 
> > 
> 
> > > > Signed-off-by: Christophe de Dinechin < dinec...@redhat.com >
> > > 
> > 
> > > > ---
> > > 
> > 
> > > > spice-common | 2 +-
> > > 
> > 
> > > > src/channel-display-gst.c | 1 +
> > > 
> > 
> > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > 
> 
> > > > diff --git a/spice-common b/spice-common
> > > 
> > 
> > > > index 429ad96..ba11de3 16
> > > 
> > 
> > > > --- a/spice-common
> > > 
> > 
> > > > +++ b/spice-common
> > > 
> > 
> > > > @@ -1 +1 @@
> > > 
> > 
> > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > 
> > 
> > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > 
> > 
> > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > 
> > 
> > > > index f978602..c9ab9bf 100644
> > > 
> > 
> > > > --- a/src/channel-display-gst.c
> > > 
> > 
> > > > +++ b/src/channel-display-gst.c
> > > 
> > 
> > > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > > 
> > 
> > > > GError *err = NULL;
> > > 
> > 
> > > > if (gst_init_check(NULL, NULL, )) {
> > > 
> > 
> > > > success = 1;
> > > 
> > 
> > > > + atexit(gst_deinit);
> > > 
> > 
> > > > } else {
> > > 
> > 
> > > > spice_warning("Disabling GStreamer video support: %s",
> > > 
> > 
> > > > err->message);
> > > 
> > 
> > > > g_clear_error();
> > > 
> > 
> 
> > > Calling atexit from a library is a bad idea.
> > 
> 
> > Could you elaborate?
> 
> > I do not really agree with this statement. I’d actually go as far as saying
> > that libraries are the reason atexit exists to start with.
> > Apparently, I’m not alone, see first three responses in
> > https://stackoverflow.com/questions/25115612/whats-the-scenario-to-use-atexit-function
> > that all mention libraries.
> 
> > Christophe
> 
> Shared libraries in theory can be unloaded before the atexit function is 
> called for instance. 
> 
> Calling gst_deinit from a library is a bad idea because other part of the 
> program could use 
> gstreamer too and call other gstreamer functions. Even after your atexit 
> function! 
> 
> The gcc way to catch shared library unload is to use the destructor attribute
> which in Unix usually chain the .deinit/deinit function. Also has the
> advantage of not using space (atexit function calls are usually limited as in
> many systems the buffer used to register  them is static).

In fact non-trivial shared libraries should generally never be unloaded, even
if they were originally dlopend.  If the library has used a pthread local with
a destructor function, then unloading the library will remove the code that
contains the impl of the destructor. When the thread later exits and its thread
locals are cleaned up, the application will crash & burn.

Many libraries, including libvirt, will link with '-z nodelete' to prevent
unloading of the library even if dlclose() is called, to avoid these kind
of crashes.

IOW getting perfect "cleanup" is just a fools errand and will likely create
obscure problems down the road that are worse than the problems the cleanup
is trying to solve. Just accept normal process resource cleanup when the
application exits.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win PATCH v6 2/5] Initial rewrite of image conversion code

2017-07-27 Thread Daniel P. Berrange

On Thu, Jul 27, 2017 at 12:39:32PM +0200, Christophe Fergeau wrote:
> On Thu, Jul 27, 2017 at 11:54:08AM +0200, Victor Toso wrote:
> > On Mon, Jul 24, 2017 at 10:47:34AM -0400, Frediano Ziglio wrote:
> > > Not really familiar with GitLab merge requests but on GitHub they
> > > remain open till closed so this would help with old ones.
> > > The big change on moving to full PR is the way of commenting patches.
> > > Unless PR are just used for tracking and are replicated to ML but
> > > maybe is hard to keep them consistent. I think is possible to configure
> > > PRs to send changes to ML. This would make the history persistent on
> > > the ML.
> > >
> > > Could we try for a period and see how does it go?
> > 
> > +1, but how should we approach that? I think we need to move from
> > freedesktop to either gitlab or github first otherwise we can get
> > confused on what's going on.
> > 
> 
> Just to be sure, you are both suggesting switching to pull requests, and
> doing the reviews in gitlab web UI?
> 
> The initial problem is that some reviews are not done in a timely
> manner. Being able to easily get a list of pending reviews was brought
> forward as a potential solution to this problem, and apparently, you
> both think that switching from email based reviews to a web based review
> system would help in getting more reviews faster? (iow, it would make
> you more efficient at reviewing code, and you vastly prefer that over
> email).
> 
> I'm not necessarily opposed to trying things out, I'm just trying to get
> a clear view of what we are expecting to get out of the change.

IME, if you're not getting fast enough reviews on patches, the problem
isn't usually the tools, it is the lack of reviewer bandwidth. On OpenStack
we used Gerrit for reviewers, had tonnes of pretty graphs, metrics, reports
on what reviews were stalled, how long reviews were waited for, etc, etc.
It didn't help getting reviews done in a timely manner. You still had to
go and manually "ping" people to get attention on your reviews they had
not looked at or forgotten about.

Added to that the web UI was so inefficient to deal with (particularly for
patch series), that people ended up writing command line tools to do code
review in a "email like" user interface and then posting the results back
to gerrit.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Use designated struct initializer

2017-07-26 Thread Daniel P. Berrange

On Fri, Jul 07, 2017 at 04:26:33PM +0200, Pavel Grunt wrote:
> Silence -Wmissing-field-initializers warnings.

This doesn't make sense for many of these cases you've changed

> diff --git a/src/channel-main.c b/src/channel-main.c
> index 4edd575..104b18a 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -1275,7 +1275,7 @@ static void agent_sync_audio_record(SpiceMainChannel 
> *main_channel)
>  static void agent_display_config(SpiceMainChannel *channel)
>  {
>  SpiceMainChannelPrivate *c = channel->priv;
> -VDAgentDisplayConfig config = { 0, };
> +VDAgentDisplayConfig config = { .flags = 0, .depth = 0};

eg "{ 0 }" is explicitly defined in the C spec as meaning initialize every
field to all zeros. 

GCC would complain about { 1 }, but should never complain about { 0 }, even
with -Wmissing-field-initializers.



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3 5/5] Use SPICE_ALIGNED_CAST to silence warning with ucontext on macOS

2017-05-17 Thread Daniel P. Berrange

On Wed, May 17, 2017 at 07:08:26AM +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  configure.ac|  3 ++-
>  src/channel-cursor.c|  6 +++---
>  src/channel-display-mjpeg.c |  2 +-
>  src/continuation.h  |  6 --
>  src/decode-glz-tmpl.c   |  2 +-
>  src/spice-channel.c | 10 +-
>  6 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index ecab365..8b433ba 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -481,7 +481,8 @@ if test "$with_coroutine" = "auto"; then
>if test "$os_win32" = "yes"; then
>  with_coroutine=winfiber
>elif test "$os_mac" = "yes"; then
> -with_coroutine=gthread
> +with_coroutine=ucontext
> +AC_DEFINE([_XOPEN_SOURCE], [1], [Define _XOPEN_SOURCE on macOS for 
> ucontext])
>else
>  with_coroutine=ucontext
>fi
> diff --git a/src/channel-cursor.c b/src/channel-cursor.c
> index 50de5ce..650d408 100644
> --- a/src/channel-cursor.c
> +++ b/src/channel-cursor.c
> @@ -340,7 +340,7 @@ static display_cursor *set_cursor(SpiceChannel *channel, 
> SpiceCursor *scursor)
>  memcpy(cursor->data, data, size);
>  for (i = 0; i < hdr->width * hdr->height; i++) {
>  pix_mask = get_pix_mask(data, size, i);
> -if (pix_mask && *(SPICE_ALIGNED_CAST(guint32*, data) + i) == 
> 0xff) {
> +if (pix_mask && *(SPICE_ALIGNED_CAST(guint32 *, data) + i) == 
> 0xff) {
>  cursor->data[i] = get_pix_hack(i, hdr->width);
>  } else {
>  cursor->data[i] |= (pix_mask ? 0 : 0xff00);
> @@ -350,7 +350,7 @@ static display_cursor *set_cursor(SpiceChannel *channel, 
> SpiceCursor *scursor)
>  case SPICE_CURSOR_TYPE_COLOR16:
>  for (i = 0; i < hdr->width * hdr->height; i++) {
>  pix_mask = get_pix_mask(data, size, i);
> -pix = *(SPICE_ALIGNED_CAST(guint16*,data) + i);
> +pix = *(SPICE_ALIGNED_CAST(guint16 *, data) + i);
>  if (pix_mask && pix == 0x7fff) {
>  cursor->data[i] = get_pix_hack(i, hdr->width);
>  } else {
> @@ -364,7 +364,7 @@ static display_cursor *set_cursor(SpiceChannel *channel, 
> SpiceCursor *scursor)
>  for (i = 0; i < hdr->width * hdr->height; i++) {
>  pix_mask = get_pix_mask(data, size + (sizeof(uint32_t) << 4), i);
>  int idx = (i & 1) ? (data[i >> 1] & 0x0f) : ((data[i >> 1] & 
> 0xf0) >> 4);
> -pix = *(SPICE_UNALIGNED_CAST(uint32_t*,(data + size)) + idx);
> +pix = *(SPICE_UNALIGNED_CAST(uint32_t *, (data + size)) + idx);
>  if (pix_mask && pix == 0xff) {
>  cursor->data[i] = get_pix_hack(i, hdr->width);
>  } else {
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 17c0f4f..ee33b01 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -151,7 +151,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer 
> video_decoder)
>  #ifndef JCS_EXTENSIONS
>  {
>  uint8_t *s = lines[0];
> -uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *,s);
> +uint32_t *d = SPICE_ALIGNED_CAST(uint32_t *, s);
>  
>  if (back_compat) {
>  for (unsigned int j = lines_read * width; j > 0; ) {

While these whitespace changes make sense, they are not related to the
commit message.

> diff --git a/src/continuation.h b/src/continuation.h
> index 675a257..d1fd137 100644
> --- a/src/continuation.h
> +++ b/src/continuation.h
> @@ -21,6 +21,7 @@
>  #ifndef _CONTINUATION_H_
>  #define _CONTINUATION_H_
>  
> +#include "spice-common.h"
>  #include 
>  #include 
>  #include 
> @@ -48,8 +49,9 @@ int cc_release(struct continuation *cc);
>  int cc_swap(struct continuation *from, struct continuation *to);
>  
>  #define offset_of(type, member) ((unsigned long)(&((type *)0)->member))
> -#define container_of(obj, type, member) \
> -(type *)(((char *)obj) - offset_of(type, member))
> +#define container_of(obj, type, member) \
> +SPICE_ALIGNED_CAST(type *,  \
> +   (((char *)obj) - offset_of(type, member)))
>  
>  #endif
>  /*

Yep, makes sense.


> diff --git a/src/decode-glz-tmpl.c b/src/decode-glz-tmpl.c
> index 7695a28..76d832c 100644
> --- a/src/decode-glz-tmpl.c
> +++ b/src/decode-glz-tmpl.c
> @@ -178,7 +178,7 @@ static size_t FNAME(decode)(SpiceGlzDecoderWindow *window,
>  uint64_t image_id, SpicePalette *plt)
>  {
>  uint8_t  *ip = in_buf;
> -OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *,out_buf);
> +OUT_PIXEL*out_pix_buf = SPICE_ALIGNED_CAST(OUT_PIXEL *, out_buf);
>  OUT_PIXEL*op = out_pix_buf;
>  OUT_PIXEL

Re: [Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)

2017-05-11 Thread Daniel P. Berrange

On Thu, May 11, 2017 at 07:08:13AM -0400, Frediano Ziglio wrote:
> > 
> > On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin 
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > >  configure.ac | 14 ++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index 74b5811..ecab365 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -62,6 +62,18 @@ esac
> > >  AC_MSG_RESULT([$os_win32])
> > >  AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"])
> > >  
> > > +AC_MSG_CHECKING([for native macOS])
> > > +case "$host_os" in
> > > + *darwin*)
> > > +os_mac=yes
> > > +;;
> > > + *)
> > > +os_mac=no
> > > +;;
> > > +esac
> > > +AC_MSG_RESULT([$os_mac])
> > > +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"])
> > > +
> > >  AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h])
> > >  AC_CHECK_HEADERS([termios.h])
> > >  AC_CHECK_HEADERS([epoxy/egl.h],
> > > @@ -468,6 +480,8 @@ esac
> > >  if test "$with_coroutine" = "auto"; then
> > >if test "$os_win32" = "yes"; then
> > >  with_coroutine=winfiber
> > > +  elif test "$os_mac" = "yes"; then
> > > +with_coroutine=gthread
> > >else
> > >  with_coroutine=ucontext
> > >fi
> > 
> > Despite ucontext being deprecated we are still better off using that &
> > ignoring the warnings, than using the gthread impl.
> > 
> > Regards,
> > Daniel
> 
> But Christophe reported that this is not compiling at all.
> Did you manage to compile under OsX with ucontext ?

Is that with or without -Werror though ?  I understand if -Werror makes
it fail due to the deprecation warnings, but AFAIK, the functionality
still exists & should be compatible.

> Why ucontext is better?

Better performance essentially. 

> According to 
> http://duriansoftware.com/joe/PSA:-avoiding-the-%22ucontext-routines-are-deprecated%22-error-on-Mac-OS-X-Snow-Leopard.html
> seems that defining _XOPEN_SOURCE should remove at least the error.

Better is to just use a GCC pragma to silence the compile warning for that
piece of code only.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v2 4/4] Add check for macOS, disable ucontext on macOS (deprecated)

2017-05-11 Thread Daniel P. Berrange

On Thu, May 11, 2017 at 12:47:08PM +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  configure.ac | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 74b5811..ecab365 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -62,6 +62,18 @@ esac
>  AC_MSG_RESULT([$os_win32])
>  AM_CONDITIONAL([OS_WIN32],[test "$os_win32" = "yes"])
>  
> +AC_MSG_CHECKING([for native macOS])
> +case "$host_os" in
> + *darwin*)
> +os_mac=yes
> +;;
> + *)
> +os_mac=no
> +;;
> +esac
> +AC_MSG_RESULT([$os_mac])
> +AM_CONDITIONAL([OS_MAC],[test "$os_mac" = "yes"])
> +
>  AC_CHECK_HEADERS([sys/socket.h netinet/in.h arpa/inet.h])
>  AC_CHECK_HEADERS([termios.h])
>  AC_CHECK_HEADERS([epoxy/egl.h],
> @@ -468,6 +480,8 @@ esac
>  if test "$with_coroutine" = "auto"; then
>if test "$os_win32" = "yes"; then
>  with_coroutine=winfiber
> +  elif test "$os_mac" = "yes"; then
> +with_coroutine=gthread
>else
>  with_coroutine=ucontext
>fi

Despite ucontext being deprecated we are still better off using that &
ignoring the warnings, than using the gthread impl.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/3] Add check for macOS, disable ucontext on macOS (deprecated)

2017-04-28 Thread Daniel P. Berrange

On Fri, Apr 28, 2017 at 07:48:01AM -0400, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > Subject: [PATCH 1/3] Add check for macOS, disable ucontext on macOS
> >  (deprecated)
> > 
> 
> It's also deprecated on Linux afaik, but it's still the preferred way in qemu 
> too.
>
> An alternative would be to implement sigaltstack I suppose (available in 
> qemu).

Until any OS actually remove ucontext, IMHO it is better to stick
with the ucontext impl as it is tried & tested and known to have
good performance. I certainly wouldn't recommend use of the
gthread impl as its performance is going to be worse & has very
little testing. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] build-sys: Add tests/pki to EXTRA_DIST

2017-03-10 Thread Daniel P. Berrange

On Fri, Mar 10, 2017 at 01:16:37PM +0100, Christophe Fergeau wrote:
> This fixes make distcheck
> ---
> This applies on top of the make check-valgrind patch I sent earlier, I can
> rebase on top of git master if needed
> 
> Christophe
> 
>  server/tests/Makefile.am | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> index 86977ab..8237138 100644
> --- a/server/tests/Makefile.am
> +++ b/server/tests/Makefile.am
> @@ -2,7 +2,12 @@ NULL =
>  
>  @VALGRIND_CHECK_RULES@
>  VALGRIND_SUPPRESSIONS_FILES = $(top_srcdir)/server/tests/valgrind/glib.supp
> -EXTRA_DIST = $(VALGRIND_SUPPRESSIONS_FILES)
> +EXTRA_DIST = \
> + $(VALGRIND_SUPPRESSIONS_FILES)  \
> + pki/ca-cert.pem \
> + pki/server-cert.pem \
> + pki/server-key.pem  \
> + $(NULL)

Unless i'm mistaken you have a timebomb there that will break the tests
in about 1 & 1/2 years time

$ certtool  -i --infile server/tests/pki/server-cert.pem  | grep --after 2 
Validity
Validity:
Not Before: Sun Mar 05 15:51:44 UTC 2017
Not After: Fri Nov 29 15:51:44 UTC 2019


The ca-cert.pem is valid until 2044 by which time y2038 will have destroyed
the world, so that's ok.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] doc: Remove perl(Text::CSV) references

2017-03-10 Thread Daniel P. Berrange

On Fri, Mar 10, 2017 at 12:29:55PM +0100, Christophe Fergeau wrote:
> It's no longer used since commit e271222e "Switch over to using
> keycodemapdb submodule"
> ---
>  README  | 2 +-
>  src/Makefile.am | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)

ACK


> diff --git a/README b/README
> index 43ee08a..12513c5 100644
> --- a/README
> +++ b/README
> @@ -53,4 +53,4 @@ gstreamer1-devel gstreamer1-plugins-base-devel 
> gstreamer1-plugins-good gstreamer
>  
>  . If you build from git, you'll also need:
>  
> -libtool automake vala vala-tools perl-Text-CSV
> +libtool automake vala vala-tools

This might want to mention python2 / python3, since new keymap tool
uses that. IIUC, though python is already a pre-existing dep for the
spice code generator, so that's an existing bug


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3] Switch over to using keycodemapdb submodule

2017-03-02 Thread Daniel P. Berrange

On Thu, Mar 02, 2017 at 07:59:42AM +0100, Pavel Grunt wrote:
> On Mon, 2017-02-27 at 10:44 +0000, Daniel P. Berrange wrote:
> > On Mon, Feb 27, 2017 at 11:37:44AM +0100, Pavel Grunt wrote:
> > > Hello Daniel,
> > > 
> > > On Mon, 2017-02-27 at 10:25 +, Daniel P. Berrange wrote:
> > > > Consume the keymaps.csv file from a git submodule instead of
> > > > having
> > > > a private copy. This makes it easier to ensure all users of the
> > > > keymap
> > > > (libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a
> > > > consistent
> > > > set of data.
> > > > 
> > > 
> > > besides that it also allow us to drop the dependency on perl (also
> > > perl-Text-CSV is not packaged in some distros)
> > 
> > True yes, I intentionally kept the python code so that it only
> > used modules base-python installs so we don't rely on external
> > modules from pypi.
> > 
> > > Are there contributing rules for the keycodemapdb (where to send
> > > the
> > > patches etc.)?
> > 
> > Just send pull requests to the repo is best I think. I don't think
> > we'd
> > have enough traffic to warrant creating a new mailing list. In any
> > case I would expect that most bug reports would start off with a
> > mail
> > and/or bug report to a project using the module. eg a mail on spice-
> > devel,
> > so there's little point trying to artificially move discussion to a
> > dedicated list.
> > 
> > I'm happy to add any of the people with experiance of this code to
> > the
> > admins/committers list of the gitlab project too, so I'm not a
> > potential
> > bottleneck.
> Feel free to add me (nickname "xerus")

Ok, I added you as co-owner to increase the bus factor

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4] Switch over to using keycodemapdb submodule

2017-03-02 Thread Daniel P. Berrange

Consume the keymaps.csv file from a git submodule instead of having
a private copy. This makes it easier to ensure all users of the keymap
(libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a consistent
set of data.

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
---

Changed in v4:

  - Use correct submodule commit to fix OSX keymaps

 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..594c0de 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdi

Re: [Spice-devel] [PATCH spice-gtk v3] Switch over to using keycodemapdb submodule

2017-03-02 Thread Daniel P. Berrange

On Thu, Mar 02, 2017 at 08:16:15AM +0100, Pavel Grunt wrote:
> On Thu, 2017-03-02 at 07:59 +0100, Pavel Grunt wrote:
> > On Mon, 2017-02-27 at 10:44 +0000, Daniel P. Berrange wrote:
> > > On Mon, Feb 27, 2017 at 11:37:44AM +0100, Pavel Grunt wrote:
> > > > Hello Daniel,
> > > > 
> > > > On Mon, 2017-02-27 at 10:25 +, Daniel P. Berrange wrote:
> > > > > Consume the keymaps.csv file from a git submodule instead of
> > > > > having
> > > > > a private copy. This makes it easier to ensure all users of
> > > > > the
> > > > > keymap
> > > > > (libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a
> > > > > consistent
> > > > > set of data.
> > > > > 
> > > > 
> > > > besides that it also allow us to drop the dependency on perl
> > > > (also
> > > > perl-Text-CSV is not packaged in some distros)
> > > 
> > > True yes, I intentionally kept the python code so that it only
> > > used modules base-python installs so we don't rely on external
> > > modules from pypi.
> > > 
> > > > Are there contributing rules for the keycodemapdb (where to send
> > > > the
> > > > patches etc.)?
> > > 
> > > Just send pull requests to the repo is best I think. I don't think
> > > we'd
> > > have enough traffic to warrant creating a new mailing list. In any
> > > case I would expect that most bug reports would start off with a
> > > mail
> > > and/or bug report to a project using the module. eg a mail on
> > > spice-
> > > devel,
> > > so there's little point trying to artificially move discussion to
> > > a
> > > dedicated list.
> > > 
> > > I'm happy to add any of the people with experiance of this code to
> > > the
> > > admins/committers list of the gitlab project too, so I'm not a
> > > potential
> > > bottleneck.
> > 
> > Feel free to add me (nickname "xerus")
> > 
> > > 
> > > > (For the future) Do you consider adding the
> > > > vncdisplaykeymap.[ch]
> > > > to
> > > > the repo ?
> > > 
> > > I'm unsure of the direction to take for that at this time. It
> > > would
> > > certainly be interesting to look at sharing that logic. For
> > > sharing
> > > code though, I wonder if its better to create a
> > > libgtkkeycodemap.so
> > > library rather than do a sub-module thing for that code too.
> > 
> > we have both ways in spice-common
> > 
> > About the patch - the generated file are not the same (I used git
> > diff
> > --word-diff)
> > 
> > xorgkbd2xtkbd - empty
> > xorgxquartz2xtkbd - some values are different
> > 
> > Pavel
> > 
> > https://paste.fedoraproject.org/paste/N9n1q2y3dZHDNhMOaIA5c15M1UNdIG
> > Yh
> > yRLivL9gydE=
> > 
> Ok, I see it is fixed in the current git master (this submodule is one
> commit behind)

Yes, just verified that accounts for the difference you see - i'll send
a new patch.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice v1] syntax-check: Fix missing AUTHORS

2017-02-28 Thread Daniel P. Berrange

On Tue, Feb 28, 2017 at 02:37:25PM +0100, Victor Toso wrote:
> From: Victor Toso <m...@victortoso.com>
> 
> Signed-off-by: Victor Toso <victort...@redhat.com>
> ---
>  AUTHORS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/AUTHORS b/AUTHORS
> index 2b0fe24..99f6c18 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -28,6 +28,7 @@ Patches also contributed by
>  소병철 <byungchul...@samsung.com>
>  Cédric Bosdonnat <cbosdon...@suse.com>
>  Christian Ruppert <id...@qasl.de>
> +Christophe de Dinechin <dinec...@redhat.com>
>  Cole Robinson <crobi...@redhat.com>
>  Daniel P. Berrange <berra...@redhat.com>
>  Dan Kenigsberg <dan...@redhat.com>

FWIW, git can trivially auto-fill an AUTHORS file at make dist time.
Libvirt does it as follows:

Rename AUTHORS to AUTHORS.in, replace the names beneath the
"Patches also contributed by" line, with "#authorslist#" then
add to Makefile.am

dist-hook: gen-AUTHORS

.PHONY: gen-AUTHORS
gen-AUTHORS:
$(AM_V_GEN)if test -d $(srcdir)/.git; then \
out="`cd $(srcdir) && git log --pretty=format:'%aN <%aE>' | sort 
-u`" && \
perl -p -e "s/#authorslist#// and print '$$out'" \
  < $(srcdir)/AUTHORS.in > $(distdir)/AUTHORS-tmp && \
mv -f $(distdir)/AUTHORS-tmp $(distdir)/AUTHORS ; \
fi

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] keymap detection under XWayland broken again ?

On Mon, Feb 27, 2017 at 02:37:37PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 27, 2017 at 01:13:25PM +0000, Daniel P. Berrange wrote:
> > A long while back, Pavel learnt that under XWayland we needed to
> > use XkbGetMap instead of XkbGetKeyboard:
> > 
> >   commit 95e322a6bbc29dc9c28724fb80706464e276b89f
> >   Author: Pavel Grunt <pgr...@redhat.com>
> >   Date:   Fri Feb 20 16:19:50 2015 +0100
> > 
> > vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of 
> > XkbGetKeyboard
> > 
> > XkbGetKeyboard does not work in XWayland (bfo#89240).
> > 
> > If I test spice-gtk today, forcing GTK to use X11 as the backend, the
> > keymap detection is broken for XWayland again :-(
> > 
> > $ GDK_BACKEND=x11  ./tools/spicy
> > 
> > (lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping 
> > '(unnamed)'.
> > Please report to gtk-vnc-l...@gnome.org
> > including the following information:
> > 
> >   - Operating system
> >   - GDK build
> >   - X11 Server
> >   - xprop -root
> >   - xdpyinfo
> > 
> > 
> > It seems, at least on my own Fedora 25 machine,  Xwayland is reporting
> > "unnamed" for the keymap instead of "evdev". I'm pretty sure this used
> > to work correctly when Pavel first wrote that changeset above, so I'm
> > wondering if anyone else sees this same behaviour, or if perhaps it is
> > something peculiar to my desktop install/setup. If its not jsut me, then
> > its a change in behaviour of Xwayland we'll need to adapt to. I really
> > hope we don't need to go back to my hack of checking if WAYLAND_DISPLAY
> > env variable exists :-(
> 
> I'm seeing these warnings too with GDK_BACKEND=x11, never tried to
> figure out what was going on, and I had forgotten that Pavel had fixed
> something similar in the past :( So no, it's not just you.

Ok, this problem just became alot more "fun". Initially when logging in
Xwayland correctly reports "evdev" as the keycode mapping. As soon as
you launch firefox, it starts to report "unknown".

So firefox is doing something to the Xwayland server that breaks it :-(

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] keymap detection under XWayland broken again ?

On Mon, Feb 27, 2017 at 02:24:16PM +0100, Pavel Grunt wrote:
> On Mon, 2017-02-27 at 13:13 +0000, Daniel P. Berrange wrote:
> > A long while back, Pavel learnt that under XWayland we needed to
> > use XkbGetMap instead of XkbGetKeyboard:
> > 
> >   commit 95e322a6bbc29dc9c28724fb80706464e276b89f
> >   Author: Pavel Grunt <pgr...@redhat.com>
> >   Date:   Fri Feb 20 16:19:50 2015 +0100
> > 
> > vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of
> > XkbGetKeyboard
> > 
> > XkbGetKeyboard does not work in XWayland (bfo#89240).
> > 
> > If I test spice-gtk today, forcing GTK to use X11 as the backend,
> > the
> > keymap detection is broken for XWayland again :-(
> > 
> > $ GDK_BACKEND=x11  ./tools/spicy
> > 
> > (lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping
> > '(unnamed)'.
> > Please report to gtk-vnc-l...@gnome.org
> > including the following information:
> > 
> >   - Operating system
> >   - GDK build
> >   - X11 Server
> >   - xprop -root
> >   - xdpyinfo
> > 
> > 
> > It seems, at least on my own Fedora 25 machine,  Xwayland is
> > reporting
> > "unnamed" for the keymap instead of "evdev". I'm pretty sure this
> > used
> > to work correctly when Pavel first wrote that changeset above, so
> > I'm
> > wondering if anyone else sees this same behaviour, or if perhaps it
> > is
> > something peculiar to my desktop install/setup. If its not jsut me,
> > then
> > its a change in behaviour of Xwayland we'll need to adapt to.
> 
> There is https://bugzilla.redhat.com/show_bug.cgi?id=1413814 , it is
> about ssh -X from the Wayland client to X11, in fact it does the same
> as your reproducer

Yeah, I'm not using SSH at all - just my local GNOME session with Wayland

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] keymap detection under XWayland broken again ?

A long while back, Pavel learnt that under XWayland we needed to
use XkbGetMap instead of XkbGetKeyboard:

  commit 95e322a6bbc29dc9c28724fb80706464e276b89f
  Author: Pavel Grunt 
  Date:   Fri Feb 20 16:19:50 2015 +0100

vncdisplaykeymap: Use XkbGetMap and XkbGetNames instead of XkbGetKeyboard

XkbGetKeyboard does not work in XWayland (bfo#89240).

If I test spice-gtk today, forcing GTK to use X11 as the backend, the
keymap detection is broken for XWayland again :-(

$ GDK_BACKEND=x11  ./tools/spicy

(lt-spicy:17512): vnc-keymap-WARNING **: Unknown keycode mapping '(unnamed)'.
Please report to gtk-vnc-l...@gnome.org
including the following information:

  - Operating system
  - GDK build
  - X11 Server
  - xprop -root
  - xdpyinfo


It seems, at least on my own Fedora 25 machine,  Xwayland is reporting
"unnamed" for the keymap instead of "evdev". I'm pretty sure this used
to work correctly when Pavel first wrote that changeset above, so I'm
wondering if anyone else sees this same behaviour, or if perhaps it is
something peculiar to my desktop install/setup. If its not jsut me, then
its a change in behaviour of Xwayland we'll need to adapt to. I really
hope we don't need to go back to my hack of checking if WAYLAND_DISPLAY
env variable exists :-(

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

On Mon, Feb 27, 2017 at 12:44:27PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 27, 2017 at 06:39:14AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Wed, Feb 15, 2017 at 11:24:37AM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > 
> > > > > > On 15 Feb 2017, at 12:45, Frediano Ziglio  
> > > > > > wrote:
> > > > > > 
> > > > > >> @@ -134,7 +134,7 @@ static void
> > > > > >> red_qxl_display_migrate(RedChannelClient
> > > > > >> *rcc)
> > > > > >> }
> > > > > >> g_object_get(channel, "channel-type", , "id", , NULL);
> > > > > >> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
> > > > > >> "dispatcher");
> > > > > >> -spice_printerr("channel type %u id %u", type, id);
> > > > > >> +spice_debug("channel type %u id %u", type, id);
> > > > > >> payload.rcc = rcc;
> > > > > >> dispatcher_send_message(dispatcher,
> > > > > >> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> > > > > > 
> > > > > > Looks like there's lot of debugging on migration.
> > > > > > Like we are not really sure the code is working.
> > > > > 
> > > > > Maybe someone once needed a lot of debug information, and kept it 
> > > > > around
> > > > > in
> > > > > case things start going south. That probably means we have annotations
> > > > > that
> > > > > may end up being annoying for everybody debugging anything else than
> > > > > migration.
> > > > > 
> > > > 
> > > > I wrote that consideration thinking about customer reports.
> > > > spice_printerr goes in the logs while spice_debug not (by default).
> > > > If we have lot of bugs in migration removing from client logs could
> > > > became an issue for us. It's hard to ask to the customer to enable all
> > > > debugging (too invasive) and even if it could be enabled separately
> > > > it could be too late (as hard to reproduce for the customer) or
> > > > complicated (think about huge setups).
> > > > 
> > > > (CCing Jasa)
> > > 
> > > In my opinion, a library really should not be displaying anything on
> > > stdout by default, and as little as possible on stderr. "We print this
> > > debugging code to stderr because we want it to show up in the log by
> > > default" does not sound very compelling. Why this debugging code and
> > > not eg qxl display debugging code? Next step is suggesting to always
> > > output all debugging code to stderr, but then we'd get too much :)
> > > 
> > > I'd just silence all the debugging code, or remove it, and work on a way
> > > of making it easy to enable/filter/... if what we have now is not good
> > > enough.
> > > 
> > > Christophe
> > > 
> > 
> > Sure, however your comment is going OT.
> > My comments was specific to migration problems.
> > 
> 
> And I don't think debug for migration problems should be anything
> special. It has no good reason to go to stderr.

Agreed, libraries should never unconditionally dump debug output to either
stdout or stderr. This is particularly true if any of the debug logs are
possible to trigger by the guest OS, as it could be used to flood the host
logs. So I agree with Christophe that there should be an explicit opt-in
to turn on debugging output, either via an environment variable, or via
an API call QEMU can make.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v3] Switch over to using keycodemapdb submodule

On Mon, Feb 27, 2017 at 11:37:44AM +0100, Pavel Grunt wrote:
> Hello Daniel,
> 
> On Mon, 2017-02-27 at 10:25 +0000, Daniel P. Berrange wrote:
> > Consume the keymaps.csv file from a git submodule instead of having
> > a private copy. This makes it easier to ensure all users of the
> > keymap
> > (libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a
> > consistent
> > set of data.
> > 
> besides that it also allow us to drop the dependency on perl (also
> perl-Text-CSV is not packaged in some distros)

True yes, I intentionally kept the python code so that it only
used modules base-python installs so we don't rely on external
modules from pypi.

> Are there contributing rules for the keycodemapdb (where to send the
> patches etc.)?

Just send pull requests to the repo is best I think. I don't think we'd
have enough traffic to warrant creating a new mailing list. In any
case I would expect that most bug reports would start off with a mail
and/or bug report to a project using the module. eg a mail on spice-devel,
so there's little point trying to artificially move discussion to a
dedicated list.

I'm happy to add any of the people with experiance of this code to the
admins/committers list of the gitlab project too, so I'm not a potential
bottleneck.

> (For the future) Do you consider adding the vncdisplaykeymap.[ch] to
> the repo ?

I'm unsure of the direction to take for that at this time. It would
certainly be interesting to look at sharing that logic. For sharing
code though, I wonder if its better to create a libgtkkeycodemap.so
library rather than do a sub-module thing for that code too.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk v3] Switch over to using keycodemapdb submodule

Consume the keymaps.csv file from a git submodule instead of having
a private copy. This makes it easier to ensure all users of the keymap
(libvirt, gtk-vnc, spice-gtk, and eventually QEMU) to have a consistent
set of data.

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
---

Changed in v3:

 - Fix makefile rules to find generator script in VPATH build

 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..594c0de 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(srcdir)/$(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GE

Re: [Spice-devel] [PATCH v2 spice-gtk 1/2] authentication: Handle failed SASL authentication separately

2017-02-22 Thread Daniel P. Berrange

On Wed, Feb 22, 2017 at 11:47:10AM +0100, Christophe Fergeau wrote:
> On Wed, Feb 22, 2017 at 11:50:21AM +0200, Snir Sheriber wrote:
> > Hi,
> > 
> > 
> > On 02/21/2017 06:37 PM, Christophe Fergeau wrote:
> > > On Sun, Feb 19, 2017 at 04:47:17PM +0200, Snir Sheriber wrote:
> > > > Remove handling with failures in the SASL authentication
> > > > process to separate function
> > > > ---
> > > >   src/spice-channel.c | 44 +++-
> > > >   1 file changed, 27 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > > > index af67931..cbf1291 100644
> > > > --- a/src/spice-channel.c
> > > > +++ b/src/spice-channel.c
> > > > @@ -1113,28 +1113,38 @@ static int spice_channel_read(SpiceChannel 
> > > > *channel, void *data, size_t length)
> > > >   return length;
> > > >   }
> > > > +#if HAVE_SASL
> > > >   /* coroutine context */
> > > > -static void spice_channel_failed_authentication(SpiceChannel *channel,
> > > > -gboolean 
> > > > invalidPassword)
> > > > +static void spice_channel_failed_sasl_authentication(SpiceChannel 
> > > > *channel)
> > > >   {
> > > >   SpiceChannelPrivate *c = channel->priv;
> > > > +gint err_code; /* Affects the authentication window activated 
> > > > fileds */
> > > >   if (c->auth_needs_username && c->auth_needs_password)
> > > > -g_set_error_literal(>error,
> > > > -SPICE_CLIENT_ERROR,
> > > > -
> > > > SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
> > > > -_("Authentication failed: password and 
> > > > username are required"));
> > > > +err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME;
> > > >   else if (c->auth_needs_username)
> > > > -g_set_error_literal(>error,
> > > > -SPICE_CLIENT_ERROR,
> > > > -SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME,
> > > > -_("Authentication failed: username is 
> > > > required"));
> > Is there a mechanism that allows only username ?
> 
> I guess in SSO setups, it makes sense to first ask for just a username,
> then check for a valid kerberos ticket for that username (or whatever
> you use for SSO), and if there is no such ticket, then ask for an
> additional authentication token.

If you want to correctly use SASL then you should not make any
assumptions about which credentials you'll be asked for. Even if a
mechanism wants the username *and* password, it is permitted to
ask for them in separate steps of the handshake. So you might need
to popup a dialog to ask for username, and then later ask for
password in a new dialog popup. It is upto the mechanism plugin
to decide which to ask for at which point, so the app can not
predict that.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk v5 6/6] usb-device-widget: Migrate to GtkGrid from GtkBox

2017-02-20 Thread Daniel P. Berrange

On Mon, Feb 20, 2017 at 12:44:52PM +0100, Victor Toso wrote:
> Hi,
> 
> On Fri, Feb 17, 2017 at 11:24:55AM +0100, Pavel Grunt wrote:
> > GtkVBox is deprecated since Gtk 3.2, GtkBox is going to be
> > deprecated. Just use the GtkGrid and GtkContainer api.
> 
> Sure
> 
> > ---
> > GtkContainer is an abstract class, so let's use the Grid with the
> > vertical orientation
> 
> At first I thought this was a ABI break but it was questioned in the
> public IRC and the SpiceUsbDeviceWidget should not be subclassed, so
> this should be fine. The chat is below for reference.

Saying on IRC that it shouldn't be subclassed doesn't mean that it hasn't
been sub-classed, particularly if this rule about not-subclassing has not
been documented anywhere for app developers to learn about it...

> pgrunt | i have this patch https://paste.fedoraproject.org/.. is it abi
>  break ? SpiceUsbDeviceWidget is publically a GtkWidget
>  
> https://www.spice-space.org/api/spice-gtk/spice-gtk-SpiceUsbDeviceWidget.html,
>  but internally I am changing it from GtkBox to GtkContainer, so
>  binary is different api is same
> teuf   | yeah, depends if we consider it a valid/likely use case to
>  derive from SpiceUsbDeviceWidget or not
> teuf   | (ie whether G_DECLARE_FINAL_TYPE could be used in the header or
>  not)
> pgrunt | hm, it should be final
> teuf   | note that it's glib 2.44+ only
> pgrunt | well i can keep the type GtkBox and use the gtk_container api
>  anyway

Even if you keep the type GtkBox in the struct, but change the
G_DEFINE_TYPE to use GTK_CONTAINER, instad of GTK_TYPE_BOX, that is
still technically an API break as you've changed the semantics of
the widget. ie if a subclass exists, and they are calling any of
the gtk_box_* APIs those will break, even though the struct still
contains the GtkBox content. ie while ABI refers to compiled type
sizes, API covers semantics of the types too, and you need to
preserve both ABI + API if you are not changing the soname.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v2] Switch over to using keycodemapdb submodule

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
---

Changed in v2:

 - Newer submodule hash to pull in python 3 portability fixes

 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..76c2755 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv win32 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_win322xtkbd code-map $(srcdir)/$(KEYMAP_CSV) win32 xtkbd > $@ || rm $@
 
 vncdisplaykeymap_x112xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdi

Re: [Spice-devel] bool or gboolean

On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> Hi,
>   question was raised recently on the ML and IRC.
> 
> Some time ago we decided to use gboolean but some of us would like
> to discuss again.
> 
> As any style changes there's no right or wrong, mainly personal
> opinions but I think consistency is quite important.
> 
> Some consideration (feel free to add/remove/comment)
> - gboolean is more used in the code (about 76%)
> - TRUE/FALSE are more used (96%)
> - bool is C99 convention, defined in stdbool.h
> - using gcc the bool type is a bit different from gboolean
>   (which basically is an int) catching some problems as
>   warnings (like cast between different function pointers
>   using bool instead of gboolean/int)
> - bool is easier to write (OT: and my vim is more happy too)

As noted above gboolean & bool are different types, in fact they
are different sizes too (4 bytes vs 1 byte).

If you're using glib2, you'll find various places where it wants
callbacks which have a gboolean return value, or parameters. You
can't pass in a callback which uses bool, as that's not type
compatible. So no matter what, you'll always need to use gboolean
in some portion of the code, even if you would rather have bool.

Thus if consistency is the goal, then gboolean is the winner as
you're more likely to be able to kill off all uses of bool, than
be able to kill off all uses of gboolean.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Switch over to using keycodemapdb submodule

On Wed, Feb 15, 2017 at 04:44:22PM +0100, Pavel Grunt wrote:
> nice, although the python script looks pretty heavy.

Different users of the keymap database have different output formats.
We don't want ever user re-implementing the same logic, as that's
just as bad what we have today. So the tool is designed to satisfy
current usage requireents from libvirt, gtk-vnc & spice-gtk

> Anyway, in spice-gtk we support python3, imo it cannot go in without
> the python3 support.

Ok, that's easy enough todo.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] Switch over to using keycodemapdb submodule

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
---
 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..76c2755 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv win32 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_win322xtkbd code-map $(srcdir)/$(KEYMAP_CSV) win32 xtkbd > $@ || rm $@
 
 vncdisplaykeymap_x112xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv x11 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON

Re: [Spice-devel] [PATCH spice-gtk] keymaps.csv: Add missing columns

2017-02-14 Thread Daniel P. Berrange

On Tue, Feb 14, 2017 at 08:33:09AM -0500, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > > 
> > > Allow easier processing by scripts and csv editors
> > > ---
> > > Also it renders nicely on github
> > >  https://github.com/xerus/spice-gtk/blob/keymap/src/keymaps.csv
> > > From that ^ table is obvious which values are missing
> > 
> > With
> > 
> >   git diff HEAD^ --word-diff --word-diff-regex='[^[:space:]]'
> > 
> > you can easily see that there's no difference beside the added
> > columns at the end and with a
> > 
> >   perl -pe 's.[^,\n]..sg' src/keymaps.csv | uniq
> > 
> > you can see all rows have the same amount of columns
> > 
> > Acked-by: Frediano Ziglio 
> 
> Are we still in sync with gtk-vnc? 
> https://git.gnome.org/browse/gtk-vnc/tree/src/keymaps.csv
> 
> An option would be to have a submodule for keymaps and related code,
> to be shared with gtk-vnc, spice-gtk, eventually qemu (and a freerdp
> gtk widget etc)

I had done some work in this respect, but I don't think I ever pushed
it anywhere. Originally I had been intending to put it on freedesktop
but their admins never responded to repo creation requests. gitlab
would be a better choice these days. Lemme try and find my previous
work...


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] Allow websockify path to be controlled

2017-01-26 Thread Daniel P. Berrange

The spice_auto.html file allows websockify path to be set via a
query parameter, but the standard spice.html does not provide
any mechanism for this. Add another form input field to allow
it to be set, defaulting to /websockify

Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
---
 spice.html | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/spice.html b/spice.html
index d4c9962..e341f13 100644
--- a/spice.html
+++ b/spice.html
@@ -74,8 +74,13 @@
 
 host = document.getElementById("host").value;
 port = document.getElementById("port").value;
+token = document.getElementById("token").value;
 password = document.getElementById("password").value;
+path = document.getElementById("path").value;
 
+if (window.location.protocol == 'https:') {
+scheme = "wss://";
+}
 
 if ((!host) || (!port)) {
 console.log("must set host and port");
@@ -86,7 +91,7 @@
 sc.stop();
 }
 
-uri = scheme + host + ":" + port;
+uri = scheme + host + ":" + port + "/" + path + "?token=" + 
token;
 
 document.getElementById('connectButton').innerHTML = "Stop";
 document.getElementById('connectButton').onclick = disconnect;
@@ -165,7 +170,9 @@
 SPICE
 Host:  
 Port: 
+Token: 
 Password: 
+Path: 
 Start
 
 
-- 
2.9.3

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

Re: [Spice-devel] Graceful failure when VNC client connects to SPICE server ?

On Wed, Jan 25, 2017 at 05:59:44PM +, Daniel P. Berrange wrote:
> On Wed, Jan 25, 2017 at 12:38:09PM -0500, Frediano Ziglio wrote:
> > > 
> > > On Wed, Jan 25, 2017 at 12:23:35PM -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > When used with QEMU at least, both SPICE and VNC live in the same port
> > > > > range (5900+). It is thus not entirely uncommon for a user to 
> > > > > mistakenly
> > > > > connect to a SPICE server with a VNC client, or vica-verca.
> > > > > 
> > > > > When connecting to VNC server with a SPICE client, you quickly get an
> > > > > error. This is because the VNC server sends its short greeting and 
> > > > > then
> > > > > sees the RedLinkMess from the SPICE client, realizes its garbage and
> > > > > drops the connection.
> > > > > 
> > > > > When connecting to a SPICE server with a VNC client though, you get an
> > > > > indefinite hang. The VNC client is waiting for the VNC greeting, which
> > > > > the SPICE server will never send. The SPICE server is waiting for the
> > > > > RedLinkMess which the VNC client will never send.
> > > > > 
> > > > > Since VNC is a server sends first protocol and SPICE is a client sends
> > > > > first protocol, it would seem this problem is impossible to fix, but
> > > > > I think it is possible to tweak things so it can be more gracefully
> > > > > handled.
> > > > > 
> > > > > In VNC the protocol starts with the follow data sent:
> > > > > 
> > > > >  Server: "RFB 003.008\n"
> > > > >  Client: "RFB 003.008\n"
> > > > > 
> > > > > What I'm thinking is that we could easily change the VNC client so 
> > > > > that
> > > > > it will send some bytes first. eg
> > > > > 
> > > > >  Client: "RFB "
> > > > >  Server: "RFB 003.008\n"
> > > > >  Client: "003.008\n"
> > > > > 
> > > > > From the VNC server POV, it'll still be receiving the same 12 bytes 
> > > > > from
> > > > > the client - it just happens that 4 of them might arrive a tiny bit
> > > > > earlier than the other 8 of them. IOW nothing should break in the VNC
> > > > > server from this change.
> > > > > 
> > > > > I tried this, but we still get a hang in the SPICE server. The problem
> > > > > is that the SPICE server code is waiting for the full RedLinkMess
> > > > > message to be received before checking its content.
> > > > > 
> > > > > Can we change the SPICE server so that it reads the 'uint32 magic' 
> > > > > first,
> > > > > validates that, and then reads the remainder of RedLinkMess ?
> > > > > 
> > > > > This would solve the problem, as the SPICE server would see 'RFB ' as 
> > > > > the
> > > > > magic, realize it was garbage and so close the connection. The VNC 
> > > > > client
> > > > > which is waiting for the VNC greeting will thus terminate rather than
> > > > > hanging
> > > > > forever.
> > > > > 
> > > > > Regards,
> > > > > Daniel
> > > > 
> > > > Perhaps another solution would be to add a timeout to spice server so
> > > > after, say, 2/3 seconds it would close the connection.
> > > > 
> > > > This would work even without changing the VNC client.
> > > 
> > > I'm not a fan of timeouts because this could negatively impact on
> > > genuine spice clients which hit a latency spike on the network
> > > for whatever reason. The incremental read of RedLinkMess by constrast
> > > is guaranteed to not have any chance of negative impact on spice clients.
> > > 
> > > Regards,
> > > Daniel
> > 
> > So you would like something like that:
> > 
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 8db70ee..50098df 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2264,12 +2264,6 @@ static void reds_handle_read_header_done(void 
> > *opaque)
> >  header->minor_version = GUINT32_FROM_LE(header->minor_version);
> >  header->size = GUINT32_FROM_LE(header->size);
> >  
> > -if

Re: [Spice-devel] Graceful failure when VNC client connects to SPICE server ?

On Wed, Jan 25, 2017 at 12:38:09PM -0500, Frediano Ziglio wrote:
> > 
> > On Wed, Jan 25, 2017 at 12:23:35PM -0500, Frediano Ziglio wrote:
> > > > 
> > > > When used with QEMU at least, both SPICE and VNC live in the same port
> > > > range (5900+). It is thus not entirely uncommon for a user to mistakenly
> > > > connect to a SPICE server with a VNC client, or vica-verca.
> > > > 
> > > > When connecting to VNC server with a SPICE client, you quickly get an
> > > > error. This is because the VNC server sends its short greeting and then
> > > > sees the RedLinkMess from the SPICE client, realizes its garbage and
> > > > drops the connection.
> > > > 
> > > > When connecting to a SPICE server with a VNC client though, you get an
> > > > indefinite hang. The VNC client is waiting for the VNC greeting, which
> > > > the SPICE server will never send. The SPICE server is waiting for the
> > > > RedLinkMess which the VNC client will never send.
> > > > 
> > > > Since VNC is a server sends first protocol and SPICE is a client sends
> > > > first protocol, it would seem this problem is impossible to fix, but
> > > > I think it is possible to tweak things so it can be more gracefully
> > > > handled.
> > > > 
> > > > In VNC the protocol starts with the follow data sent:
> > > > 
> > > >  Server: "RFB 003.008\n"
> > > >  Client: "RFB 003.008\n"
> > > > 
> > > > What I'm thinking is that we could easily change the VNC client so that
> > > > it will send some bytes first. eg
> > > > 
> > > >  Client: "RFB "
> > > >  Server: "RFB 003.008\n"
> > > >  Client: "003.008\n"
> > > > 
> > > > From the VNC server POV, it'll still be receiving the same 12 bytes from
> > > > the client - it just happens that 4 of them might arrive a tiny bit
> > > > earlier than the other 8 of them. IOW nothing should break in the VNC
> > > > server from this change.
> > > > 
> > > > I tried this, but we still get a hang in the SPICE server. The problem
> > > > is that the SPICE server code is waiting for the full RedLinkMess
> > > > message to be received before checking its content.
> > > > 
> > > > Can we change the SPICE server so that it reads the 'uint32 magic' 
> > > > first,
> > > > validates that, and then reads the remainder of RedLinkMess ?
> > > > 
> > > > This would solve the problem, as the SPICE server would see 'RFB ' as 
> > > > the
> > > > magic, realize it was garbage and so close the connection. The VNC 
> > > > client
> > > > which is waiting for the VNC greeting will thus terminate rather than
> > > > hanging
> > > > forever.
> > > > 
> > > > Regards,
> > > > Daniel
> > > 
> > > Perhaps another solution would be to add a timeout to spice server so
> > > after, say, 2/3 seconds it would close the connection.
> > > 
> > > This would work even without changing the VNC client.
> > 
> > I'm not a fan of timeouts because this could negatively impact on
> > genuine spice clients which hit a latency spike on the network
> > for whatever reason. The incremental read of RedLinkMess by constrast
> > is guaranteed to not have any chance of negative impact on spice clients.
> > 
> > Regards,
> > Daniel
> 
> So you would like something like that:
> 
> 
> diff --git a/server/reds.c b/server/reds.c
> index 8db70ee..50098df 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2264,12 +2264,6 @@ static void reds_handle_read_header_done(void *opaque)
>  header->minor_version = GUINT32_FROM_LE(header->minor_version);
>  header->size = GUINT32_FROM_LE(header->size);
>  
> -if (header->magic != SPICE_MAGIC) {
> -reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
> -reds_link_free(link);
> -return;
> -}
> -
>  if (header->major_version != SPICE_VERSION_MAJOR) {
>  if (header->major_version > 0) {
>  reds_send_link_error(link, SPICE_LINK_ERR_VERSION_MISMATCH);
> @@ -2296,13 +2290,31 @@ static void reds_handle_read_header_done(void *opaque)
> link);
>  }
>  
> +static void reds_handle_read_magic_done(void *opaque)
> +{
> +RedLinkInfo *link = (RedLinkInfo *)opaque;
> +const SpiceLinkHeader *header = >link_header;
> +
> +if (header->magic != SPICE_MAGIC) {
> +reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
> +reds_link_free(link);
> +return;
> +}
> +
> +reds_stream_async_read(link->stream,
> +   ((uint8_t *)>link_header) + 
> sizeof(header->magic),
> +   sizeof(SpiceLinkHeader) - sizeof(header->magic),
> +   reds_handle_read_header_done,
> +   link);
> +}
> +
>  static void reds_handle_new_link(RedLinkInfo *link)
>  {
>  reds_stream_set_async_error_handler(link->stream, 
> reds_handle_link_error);
>  reds_stream_async_read(link->stream,
> (uint8_t *)>link_header,
> -   sizeof(SpiceLinkHeader),
> -

Re: [Spice-devel] Graceful failure when VNC client connects to SPICE server ?

On Wed, Jan 25, 2017 at 12:23:35PM -0500, Frediano Ziglio wrote:
> > 
> > When used with QEMU at least, both SPICE and VNC live in the same port
> > range (5900+). It is thus not entirely uncommon for a user to mistakenly
> > connect to a SPICE server with a VNC client, or vica-verca.
> > 
> > When connecting to VNC server with a SPICE client, you quickly get an
> > error. This is because the VNC server sends its short greeting and then
> > sees the RedLinkMess from the SPICE client, realizes its garbage and
> > drops the connection.
> > 
> > When connecting to a SPICE server with a VNC client though, you get an
> > indefinite hang. The VNC client is waiting for the VNC greeting, which
> > the SPICE server will never send. The SPICE server is waiting for the
> > RedLinkMess which the VNC client will never send.
> > 
> > Since VNC is a server sends first protocol and SPICE is a client sends
> > first protocol, it would seem this problem is impossible to fix, but
> > I think it is possible to tweak things so it can be more gracefully
> > handled.
> > 
> > In VNC the protocol starts with the follow data sent:
> > 
> >  Server: "RFB 003.008\n"
> >  Client: "RFB 003.008\n"
> > 
> > What I'm thinking is that we could easily change the VNC client so that
> > it will send some bytes first. eg
> > 
> >  Client: "RFB "
> >  Server: "RFB 003.008\n"
> >  Client: "003.008\n"
> > 
> > From the VNC server POV, it'll still be receiving the same 12 bytes from
> > the client - it just happens that 4 of them might arrive a tiny bit
> > earlier than the other 8 of them. IOW nothing should break in the VNC
> > server from this change.
> > 
> > I tried this, but we still get a hang in the SPICE server. The problem
> > is that the SPICE server code is waiting for the full RedLinkMess
> > message to be received before checking its content.
> > 
> > Can we change the SPICE server so that it reads the 'uint32 magic' first,
> > validates that, and then reads the remainder of RedLinkMess ?
> > 
> > This would solve the problem, as the SPICE server would see 'RFB ' as the
> > magic, realize it was garbage and so close the connection. The VNC client
> > which is waiting for the VNC greeting will thus terminate rather than 
> > hanging
> > forever.
> > 
> > Regards,
> > Daniel
> 
> Perhaps another solution would be to add a timeout to spice server so
> after, say, 2/3 seconds it would close the connection.
> 
> This would work even without changing the VNC client.

I'm not a fan of timeouts because this could negatively impact on
genuine spice clients which hit a latency spike on the network
for whatever reason. The incremental read of RedLinkMess by constrast
is guaranteed to not have any chance of negative impact on spice clients.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Graceful failure when VNC client connects to SPICE server ?

When used with QEMU at least, both SPICE and VNC live in the same port
range (5900+). It is thus not entirely uncommon for a user to mistakenly
connect to a SPICE server with a VNC client, or vica-verca.

When connecting to VNC server with a SPICE client, you quickly get an
error. This is because the VNC server sends its short greeting and then
sees the RedLinkMess from the SPICE client, realizes its garbage and
drops the connection.

When connecting to a SPICE server with a VNC client though, you get an
indefinite hang. The VNC client is waiting for the VNC greeting, which
the SPICE server will never send. The SPICE server is waiting for the
RedLinkMess which the VNC client will never send.

Since VNC is a server sends first protocol and SPICE is a client sends
first protocol, it would seem this problem is impossible to fix, but
I think it is possible to tweak things so it can be more gracefully
handled.

In VNC the protocol starts with the follow data sent:

 Server: "RFB 003.008\n"
 Client: "RFB 003.008\n"

What I'm thinking is that we could easily change the VNC client so that
it will send some bytes first. eg

 Client: "RFB "
 Server: "RFB 003.008\n"
 Client: "003.008\n"

From the VNC server POV, it'll still be receiving the same 12 bytes from
the client - it just happens that 4 of them might arrive a tiny bit
earlier than the other 8 of them. IOW nothing should break in the VNC
server from this change.

I tried this, but we still get a hang in the SPICE server. The problem
is that the SPICE server code is waiting for the full RedLinkMess
message to be received before checking its content.

Can we change the SPICE server so that it reads the 'uint32 magic' first,
validates that, and then reads the remainder of RedLinkMess ?

This would solve the problem, as the SPICE server would see 'RFB ' as the
magic, realize it was garbage and so close the connection. The VNC client
which is waiting for the VNC greeting will thus terminate rather than hanging
forever.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-win RFC PATCH 0/5] Replace CxImage code

2016-11-09 Thread Daniel P. Berrange

On Wed, Nov 09, 2016 at 10:32:40AM +, Frediano Ziglio wrote:
> CxImage is used for image conversion for clipboard support.
> CxImage have currently some issue:
> - library is old and unsupported;
> - required an old libpng library.
> Currently the MingW binary we distribute due to some
> issue have PNG disabled (so no clipboard image support).
> Note that currently we support (before and after this patch)
> only BMP and PNG. PNG is required as the default agent format.
> However Windows programs wants to have BMP (specifically DIB)
> in the clipboard.
> This patch remove CxImage and use directly libpng (BMP is
> supported directly by Windows APIs).
> Tested with various formats and the compiled agent with a
> Linux client.
> The main concern is actually the possible problem to
> support some weird/old BMP file formats.

Is there any sense in using gdk-pixbuf for image processing to
enable support for formats beyond bmp/png ?  


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-protocol v2] Add ioctls structures to qxl_windows.h

2016-08-16 Thread Daniel P. Berrange

On Tue, Aug 16, 2016 at 03:58:50PM +0300, Sameeh Jubran wrote:
> This patch defines the structures of ioctls that are used between
> win-vdagent and the qxl-wddm-dod driver.
> 
> Signed-off-by: Sameeh Jubran 
> ---
>  spice/qxl_windows.h | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/spice/qxl_windows.h b/spice/qxl_windows.h
> index bc2ceff..89aeac9 100644
> --- a/spice/qxl_windows.h
> +++ b/spice/qxl_windows.h
> @@ -2,7 +2,7 @@
>  #define _H_QXL_WINDOWS
>  
>  #include 
> -
> +#include 
>  #include 
>  
>  enum {
> @@ -11,11 +11,18 @@ enum {
>  };
>  
>  typedef struct SPICE_ATTR_PACKED QXLEscapeSetCustomDisplay {
> -uint32_t xres;
> -uint32_t yres;
> -uint32_t bpp;
> +uint32_t _xres;
> +uint32_t _yres;
> +uint32_t _bpp;
>  } QXLEscapeSetCustomDisplay;

Renaming fields in structs breaks API for any app already using the
struct in previous releases of spice-protocol

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [protocol 0/3] Fixing the *_DEPRECATED set of macros

2016-08-11 Thread Daniel P. Berrange

On Thu, Aug 11, 2016 at 04:28:58PM +0200, Francois Gouget wrote:
> 
> The following patch broke compilation of xf86-video-qxl:
> 
> commit b41220b1441b8adea6db9a98e9da1b43a8f426bb
> Author: Christophe Fergeau 
> Date:   Thu Mar 5 15:28:22 2015 +0100
> 
> Mark unused public API methods/code as deprecated
> 
> Acked-by: Jonathon Jongsma 
> 
> 
> The reason is it introduces a #include  in spice-server.h which 
> is a *public* header! This means any application that needs to include 
> spice-server.h, like xf86-video-qxl, must now check for GLib even if 
> they don't use it, like xf86-video-qxl.

They shouldn't have to check for it explicitly. The spice-server.pc
pkgconfig file should have been updated in that commit so that glib-2.0
is listed under "Requires" instead of "Requires.private", then applications
using spice-server would not have broken, as pkg-config would have just
"done the right thing" and automatically added -I/path/to/glib/headers.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Virt manager

2016-06-08 Thread Daniel P. Berrange

On Mon, Jun 06, 2016 at 01:14:21PM -0600, Garret Acott wrote:
> Anybody know how to reach the https://www.virt-manager.org guys? The site is
> down with a DB error. Or do you have an alternative place to download those
> files from? Looking for the virt-viewer for windows in particular.

Hmm, it was down a few days ago, but its working fine AFAICT right now

FYI, the master download hosting site for virt-viewer is

  https://fedorahosted.org/released/virt-viewer/

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Support SASL GSSAPI

2016-06-06 Thread Daniel P. Berrange

On Mon, Jun 06, 2016 at 04:34:09PM +0300, Alexander Bokovoy wrote:
> On Mon, 06 Jun 2016, Daniel P. Berrange wrote:
> > On Mon, Jun 06, 2016 at 09:01:10AM -0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > - Original Message -
> > > > I'm sending Alexander Bokovoy's patch as it is, also here is some notes 
> > > > from
> > > > him:
> > > >
> > > > "I'd really like to find a way to do it with pure SASL properties so 
> > > > that the
> > > > code would work for both SPNEGO and Kerberos. SPNEGO NTLMSSP would make 
> > > > it
> > > > working for environments where you don't have Kerberos but what we have
> > > > right now should be fine for pure Kerberos environments like FreeIPA or
> > > > Active Directory."
> > > >
> > > > And also his blog post:
> > > > https://vda.li/en/posts/2016/05/30/Single-sign-on-to-virtual-machines/
> > > >
> > > > On one hand I think would be good to have this issue partially fixed 
> > > > (as per
> > > > Alexander's comment) for 0.32, on the other hand I don't like calling 
> > > > these
> > > > kerberos functions directly. Also, we probably would have to add a 
> > > > kerberos
> > > > check/option on configure, right? I can do that without any problems, 
> > > > but I
> > > > firstly would like to hear the opinions from other people in the 
> > > > project.
> > > 
> > > Yes, it will have to be optional (especially because compiling krb5 on 
> > > mingw is *hard* - last time I checked)
> > 
> > Even compiling cryus-sasl is hard - indeed last I looked fedora didn't
> > have any mingw packages for it.
> > 
> > > 
> > > > I'm willing to re-work this patch after the release and try to find an 
> > > > ideal
> > > > solution (if possible) and also spend some more time digging into the
> > > > differences on handling this between gtk-vnc and spice-gtk.
> > > 
> > > From his blog, I gathered that it worked with gtk-vnc but not with
> > > spice-gtk. Why do we need krb specific code when gtk-vnc doesn't need it?
> > 
> > It looks like the code is trying to set a default username based on the
> > current kerberos credential the user has. gtk-vnc doesn't bother trying
> > todo this - the user just always has to supply the username explicitly
> > IMHO it would be fine for spice-gtk todo the same and avoid the krb dep/
> I tried that. Let me get a bit deeper into details, though.
> 
> Cyrus SASL GSSAPI would work if you provide NULL username but the code
> in spice-gtk rejects such usernames:
> https://cgit.freedesktop.org/spice/spice-gtk/tree/src/spice-channel.c#n1390

Hmm, that code looks really rather wrong - it is clearly making a bogus
assumption that a NULL username will result in auth failure - it should
definitely be left upto the SASL library to decide that on the server
side.

> I tried to allow NULL username here but the problem is that we need
> eventually to set actual username so that SPICE communication can
> continue. And if SASL GSSAPI module did find default credentials, we
> need to pick up the username from them. This is possible theoretically
> but all my attempts to do so caused SPICE server side to drop actual
> SPICE connection.

I'm not sure what failure you just remove that check, but I think we
need to investigate that further, as I don't think that check for
NULL is right.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Support SASL GSSAPI

2016-06-06 Thread Daniel P. Berrange

On Mon, Jun 06, 2016 at 09:01:10AM -0400, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > I'm sending Alexander Bokovoy's patch as it is, also here is some notes from
> > him:
> > 
> > "I'd really like to find a way to do it with pure SASL properties so that 
> > the
> > code would work for both SPNEGO and Kerberos. SPNEGO NTLMSSP would make it
> > working for environments where you don't have Kerberos but what we have
> > right now should be fine for pure Kerberos environments like FreeIPA or
> > Active Directory."
> > 
> > And also his blog post:
> > https://vda.li/en/posts/2016/05/30/Single-sign-on-to-virtual-machines/
> > 
> > On one hand I think would be good to have this issue partially fixed (as per
> > Alexander's comment) for 0.32, on the other hand I don't like calling these
> > kerberos functions directly. Also, we probably would have to add a kerberos
> > check/option on configure, right? I can do that without any problems, but I
> > firstly would like to hear the opinions from other people in the project.
> 
> Yes, it will have to be optional (especially because compiling krb5 on mingw 
> is *hard* - last time I checked)

Even compiling cryus-sasl is hard - indeed last I looked fedora didn't
have any mingw packages for it.

> 
> > I'm willing to re-work this patch after the release and try to find an ideal
> > solution (if possible) and also spend some more time digging into the
> > differences on handling this between gtk-vnc and spice-gtk.
> 
> From his blog, I gathered that it worked with gtk-vnc but not with
> spice-gtk. Why do we need krb specific code when gtk-vnc doesn't need it?

It looks like the code is trying to set a default username based on the
current kerberos credential the user has. gtk-vnc doesn't bother trying
todo this - the user just always has to supply the username explicitly
IMHO it would be fine for spice-gtk todo the same and avoid the krb dep/

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Make sure g_object_new receive the correct data

2016-06-03 Thread Daniel P. Berrange

On Fri, Jun 03, 2016 at 07:16:57AM -0400, Frediano Ziglio wrote:

> > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > index b662d94..a863e39 100644
> > > --- a/server/spicevmc.c
> > > +++ b/server/spicevmc.c
> > > @@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance
> > > *sin,
> > >  return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> > >  "sin", sin,
> > >  "spice-server", reds,
> > > -"client-tokens-interval", 0ULL,
> > > -"self-tokens", ~0ULL,
> > > +"client-tokens-interval", (guint64) 0ULL,
> > > +"self-tokens", (guint64) ~0ULL,
> > >  "opaque", opaque,
> > >  NULL);
> > 
> > AFAICT, these are all redundant since the ULL suffice on the constant
> > should already ensure it is passed as a 64-bit type.
> > 
> 
> Not really portable assumption, in theory could be 128 bit or 256 in
> a remote future. I think Gcc already support 128 bit integers.

Haha, this will be the least of our worries if platforms actually
get created using 128 bit or larger integers. It'll make the
switch from 32-bit to 64-bit look like child's play. Pretty much
every piece of software I've seen will break horribly in multiple
ways.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] Make sure g_object_new receive the correct data

2016-06-03 Thread Daniel P. Berrange

On Fri, Jun 03, 2016 at 12:00:24PM +0100, Frediano Ziglio wrote:
> g_object_new is a variadic function which take property values.
> As compiler cannot check if these property values are correct
> make sure they are using casts.
> This actully fix a crash in reds.c for 32 bit architectures.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/main-dispatcher.c | 2 +-
>  server/reds.c| 4 ++--
>  server/smartcard.c   | 4 ++--
>  server/spicevmc.c| 4 ++--
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index bc0de24..93de440 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -292,7 +292,7 @@ MainDispatcher* main_dispatcher_new(RedsState *reds, 
> SpiceCoreInterfaceInternal
>  MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
>  "spice-server", reds,
>  "core-interface", core,
> -"max-message-type", 
> MAIN_DISPATCHER_NUM_MESSAGES,
> +"max-message-type", (guint) 
> MAIN_DISPATCHER_NUM_MESSAGES,
>  NULL);

The MAIN_DISPATCHER_NUM_MESSAGES message comes from an enum, so technically
it could be as small as a 'char', but in practice it'll always be an int.
In any case var-args will always promote types small that int, to be the
size of an int.

IOW, this cast shouldn't be needed, since it'll already be past as an
int by the compiler.


> diff --git a/server/reds.c b/server/reds.c
> index 4fd1d35..5ec73b7 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -4379,8 +4379,8 @@ static RedCharDeviceVDIPort 
> *red_char_device_vdi_port_new(RedsState *reds)
>  {
>  return g_object_new(RED_TYPE_CHAR_DEVICE_VDIPORT,
>  "spice-server", reds,
> -"client-tokens-interval", REDS_TOKENS_TO_SEND,
> -"self-tokens", REDS_NUM_INTERNAL_AGENT_MESSAGES,
> +"client-tokens-interval", (guint64) 
> REDS_TOKENS_TO_SEND,
> +"self-tokens", (guint64) 
> REDS_NUM_INTERNAL_AGENT_MESSAGES,
>  "opaque", reds,
>  NULL);
>  }

This all make total sense though, since you're forcing use of a larger type
than the compiler would otherwise use - it would use int by default,but you
want a 64-bit type.


> diff --git a/server/smartcard.c b/server/smartcard.c
> index 872aa1d..9b1b3d6 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -261,8 +261,8 @@ static RedCharDeviceSmartcard 
> *smartcard_device_new(RedsState *reds, SpiceCharDe
>  char_dev = g_object_new(RED_TYPE_CHAR_DEVICE_SMARTCARD,
>  "sin", sin,
>  "spice-server", reds,
> -"client-tokens-interval", 0ULL,
> -"self-tokens", ~0ULL,
> +"client-tokens-interval", (guint64) 0ULL,
> +"self-tokens", (guint64) ~0ULL,
>  NULL);
>  
>  g_object_set(char_dev, "opaque", char_dev, NULL);
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index b662d94..a863e39 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
>  return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
>  "sin", sin,
>  "spice-server", reds,
> -"client-tokens-interval", 0ULL,
> -"self-tokens", ~0ULL,
> +"client-tokens-interval", (guint64) 0ULL,
> +"self-tokens", (guint64) ~0ULL,
>  "opaque", opaque,
>  NULL);

AFAICT, these are all redundant since the ULL suffice on the constant
should already ensure it is passed as a 64-bit type.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Gtk-doc VS Doxygen

2016-05-20 Thread Daniel P. Berrange

On Fri, May 20, 2016 at 06:44:01AM -0400, Frediano Ziglio wrote:
> Hi,
>   I'd like to have some standard on the internal documentation of 
> spice-server.
> 
> The current situation is quite messy having different styles.
> I think the worst think is not having any documentation, so better messy
> than nothing.
> I really like Doxygen as it's really complete tool however my Gtk-doc 
> knowledge
> is very low and I cannot compare the two.
> I know we use quite a lot of "G" stuff so perhaps people feels more 
> comfortable
> with Gtk-doc.
> I'd like to see the documentation generated and possibly published on web
> so suggestion from the guys who maintain the website are really welcome.

IMHO i'd always use gtk-doc for API documentation for any codebase which is
based on glib, especially if you start to use GObject in any way, as it opens
the door to trivially support gobject introspection in the future as it uses
the same annotations.

From the POV of a developer consuming the API documentation I find the Doxygen
HTML fairly unpleasant to read - the GTK-DOC style/layout feels (subjectively)
more user friendly.

Ultimately though any documentation is better than no documentation and the
real hard work is writing the good quality docs, regardless of format :-)

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 14/14] Convert RedChannel heirarchy to GObject

2016-05-09 Thread Daniel P. Berrange

On Wed, May 04, 2016 at 04:30:35AM -0400, Frediano Ziglio wrote:
> A patch of this size cannot be accepted, no matter how good it is.
> We need to find a way to split it.
> 
> Any proposal would be good.

There's some simple things which could be done ahead of the main conversion
to GObject.

 - API renames - eg get_stream_id -> display_channel_get_stream_id
 - Macro renames - you can introduce DISPLAY_CHANNEL before the
   conversion by defining it in terms of SPICE_CONTAINEROF

When converting to GObject you can also avoid adding the the XXXPrivate
structs - just put all the fields in the public object struct. Then
introcuce the XXXPrivate structs as a second step.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] reds: constification

2016-02-04 Thread Daniel P. Berrange

On Thu, Feb 04, 2016 at 04:03:10PM +, Frediano Ziglio wrote:
> Make version_string a constant.
> Also there is no need to have a pointer but declare the buffer as static
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index edbdaad..4225847 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3299,7 +3299,7 @@ static void reds_init_vd_agent_resources(RedsState 
> *reds)
>  }
>  }
>  
> -const char *version_string = VERSION;
> +static const char version_string[] = VERSION;

The 'version_string' is only used once:

$ git grep version_string
reds.c:const char *version_string = VERSION;
reds.c:spice_info("starting %s", version_string);

So why not just kill the pointless variable and directly
use 'VERSION' in the call to spice_info()

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] RFC: drop spice-gtk gtk+ 2.0 support

2016-01-18 Thread Daniel P. Berrange

On Mon, Jan 18, 2016 at 02:07:49PM +0100, Fabiano Fidêncio wrote:
> On Mon, Dec 21, 2015 at 4:54 PM, Marc-André Lureau
>  wrote:
> > Hi,
> >
> > With virt-viewer now dropping gtk 2.0, I don't know anyone left using
> > the spice-gtk with gtk+ 2.0. Thus I'd like to propose that the next
> > spice-gtk release be the last with 2.0 support.
> >
> > Comments?
> 
> I completely agree with this.
> I already have been working on a patch dropping the support and I will
> submit it as soon we do the last release with gtk2 support.

virt-manager is already gtk3 only too, as is GNOME boxes, so I doubt
that there's any mainstream user of spice-gtk with gtk2, and if there
was they can stick on an older maint branch if really needed

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 4/4] worker: use glib main loop

2016-01-13 Thread Daniel P. Berrange

On Wed, Jan 13, 2016 at 12:18:45PM +, Daniel P. Berrange wrote:
> On Wed, Jan 13, 2016 at 11:39:09AM +, Frediano Ziglio wrote:
> > Use the glib mainloop instead of writing our own. The glib loop is both
> > cleaner to use and is more extensible. It is also very mature and
> > reduces the maintenance burden on the spice server.
> 
> Why not take it further and provide the option to do away
> with the spice event loop entirely. Having 2 threads both
> providing event loops (QEMU event loop thread + SPICE event
> loop thread) feels rather pointless. Obviously for backcompat
> you need to be able to run the event loop in SPICE thread,
> but We could provide an API which lets QEMU tell libspice
> that it can use the default GMainContext and not run its
> own.

Ignore this comment. After talking with Marc-Andre I realize that
spice needs its separate event thread because some of the callbacks
take non-negligble wall-clock time to run.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 4/4] worker: use glib main loop

2016-01-13 Thread Daniel P. Berrange

On Wed, Jan 13, 2016 at 11:39:09AM +, Frediano Ziglio wrote:
> Use the glib mainloop instead of writing our own. The glib loop is both
> cleaner to use and is more extensible. It is also very mature and
> reduces the maintenance burden on the spice server.

Why not take it further and provide the option to do away
with the spice event loop entirely. Having 2 threads both
providing event loops (QEMU event loop thread + SPICE event
loop thread) feels rather pointless. Obviously for backcompat
you need to be able to run the event loop in SPICE thread,
but We could provide an API which lets QEMU tell libspice
that it can use the default GMainContext and not run its
own.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v7 01/23] protocol: Add support for the VP8 and h264 video codecs

2015-12-16 Thread Daniel P. Berrange

On Wed, Dec 16, 2015 at 04:16:04PM +0100, Francois Gouget wrote:
> Clients that support multiple codecs must advertise the
> SPICE_DISPLAY_CAP_MULTI_CODEC capability and one
> SPICE_DISPLAY_CAP_CODEC_XXX per supported codec.
> 
> Signed-off-by: Francois Gouget 
> ---
>  configure.ac | 2 +-
>  spice.proto  | 2 ++
>  spice/enums.h| 2 ++
>  spice/protocol.h | 4 
>  4 files changed, 9 insertions(+), 1 deletion(-)
> 
> Note:
> * This increases the version to 0.12.12 so the spice server and 
>   client can check for it. Adjust as appropriate.
> 
> diff --git a/configure.ac b/configure.ac
> index e8d118d..7486d81 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2,7 +2,7 @@ AC_PREREQ([2.57])
>  
>  m4_define([SPICE_MAJOR], 0)
>  m4_define([SPICE_MINOR], 12)
> -m4_define([SPICE_MICRO], 11)
> +m4_define([SPICE_MICRO], 12)
>  
>  AC_INIT(spice-protocol, [SPICE_MAJOR.SPICE_MINOR.SPICE_MICRO], [], 
> spice-protocol)
>  
> diff --git a/spice.proto b/spice.proto
> index 3bca900..b03d0db 100644
> --- a/spice.proto
> +++ b/spice.proto
> @@ -329,6 +329,8 @@ flags8 path_flags { /* TODO: C enum names changes */
>  
>  enum8 video_codec_type {
>  MJPEG = 1,
> +VP8,
> +H264,
>  };

I understand the general desire to support something better than
MJPEG, but what is the compelling rationale for adding both VP8
and H264, as opposed to just VP8 ?

Support for the H264 codec is pretty difficult given the open
source / free software hostile patent licensing situation around
it, which will effectively limit the ability to support it in
many spice server/client impls.

VP8 by comparison is open source friendly as its patents were
placed under an irrevokable royalty free license.

If we add H264 to the SPICE protocol then we are inevitably
going to cause interoperability issues with some clients not
being able to work with some servers & vica-verca. I think
this would be bad for SPICE in general - one of its strengths
over VNC is that we don't have fragmented codec support across
impls.

As such I'd much prefer to see us *only* add VP8 which is
not going to cause implementation problems wrt licensing,
and so will not cause the same kind of interoperability
issues as H264.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Full featured (qxl compatible) spice web client released

2015-11-09 Thread Daniel P. Berrange

On Mon, Nov 09, 2015 at 01:30:15PM +0100, Jose Carlos Norte Fernandez wrote:
> Thanks for your comments Daniel!
> 
> It wasn’t our intention to add the badgeware paragraph in the first place,
> it was there because we recovered our header license text from our past
> open source components (from 4 years ago), that were in fact badgeware.
> 
> Since this is not our intention with spice-web-client we will be removing
> this parts from the headers.

That's great news. Thanks for addressing this confusion / problem.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice ] Add support for clients connecting with the WebSocket protocol.

2015-11-03 Thread Daniel P. Berrange

On Fri, Oct 30, 2015 at 03:52:56PM -0500, Jeremy White wrote:
> We do this by auto detecting the inbound http(s) 'GET' and probing
> for a well formulated WebSocket binary connection, such as used
> by the spice-html5 client.  If detected, we implement a set of
> cover functions that abstract the read/write/writev functions,
> in a fashion similar to the SASL implemented.

I'm not really a huge fan of overloading two protocols on the
same socket in this way.

I'd be rather inclined to have a separate port open for the
websockets protocol, in the same way that QEMU does the VNC
server.

Admins should be able to choose which protocol is available
to their clients. For example, they might launch QEMU with
both protocols available, but only wish to make one of the
protocols available to the public internet. By overloading
both protocols on the same port, you prevent them from
being able todo this in firewall rules.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Full featured (qxl compatible) spice web client released

2015-11-02 Thread Daniel P. Berrange

On Fri, Oct 30, 2015 at 10:05:23PM +0900, Daniel P. Berrange wrote:
> On Fri, Oct 30, 2015 at 01:24:36PM +0100, j...@eyeos.com wrote:
> >  
> > > I note the attribution clause in your license. As it stands now, I
> > > don't think that would cause any trouble (because of the 'however' of 5
> > > (d) of the agpl). But it would probably be useful to get a clear
> > > expression of your intent - is it the case that you are willing to
> > > contribute this only so long as the eyeos logo is prominently displayed
> > > by any one choosing to deploy it?
> > 
> > Our intentions are very simple. We want this to become the defacto html5
> > client for spice. Of course we want to maintain it and develop it
> > openly. Aside from our obvious personal preferences for FOSS the main
> > reason to opensource it is to develop it in community and of course, we
> > are very excited to discuss the next steps with you and anyone
> > interested in web support for spice :) 
> > 
> > And no, we are not willing to have an eyeos logo everywhere this client
> > is used. In fact there is no need for you to display an eyeos logo to
> > use this. 
> > 
> > We choosed AGPL because it protects the spice web client from the ASP
> > loophole. We don't want en eyeos logo or something like this displayed
> > every time this client is used, we just don't want for example a third
> > party company adding support for opus in a private product of its own
> > and not contributing it since this can be used from a service provider
> > (ASP loophole in gpl and similar licenses). 
> > 
> > However, we are open to discussions about the license if you feel this
> > can be a problem for reasonable scenarios.
> 
> Your rationale for using the AGPL as a core license makes sense to me.
> 
> Adding any additional terms to any license though, generally makes me
> concerned, as it is very easy to add seemingly innocuous text that in
> fact has non-obvious legal consequences. So I am a bit conerned about
> the additionl terms wrt attribution & logo display. IANAL though, so
> I will see if I can get any clarity / expert opinion on whether these
> terms are likely to cause any problems for acceptance in distros such
> as Fedora (or Debian) which are quite strict about interpretation of
> licensing.

Ok, so besides the standard AGPLv3 boilerplate text, there are two
paragraphs added to the license header at the top of the various .js
files in the repo

First is

[snip]
The interactive user interfaces in modified source and object code versions
of this program must display Appropriate Legal Notices, as required under
Section 5 of the GNU Affero General Public License version 3.
[/snip]

Since this is just re-stating section 5(d) of the LICENSE file it seems
pretty pointless, but at the same time, mostly harmless.

The second paragraph is the problematic one

[snip]
In accordance with Section 7(b) of the GNU Affero General Public License 
version 3,
these Appropriate Legal Notices must retain the display of the "Powered by
eyeos" logo and retain the original copyright notice. If the display of the 
logo is not reasonably feasible for technical reasons, the Appropriate Legal 
Notices
must display the words "Powered by eyeos" and retain the original copyright 
notice.
[/snip]

This is essentially adding a "badgeware" clause to the license.
Historically opinion has been divided as to whether "badgeware" /
"advertizing" clauses make a license non-free, or are merely an
undesirable attribution approach.

I was pointed at the SugarCRM community license which had an (somewhat
stricter) logo display requirement and there were strong opinions in
Debian that this made it non-free. Although your clause isn't as strict,
I think it is sufficiently similar that people would have the same
opinion about it being non-free.

  https://lists.debian.org/debian-legal/2005/11/msg00114.html

I was also referred to this FOSDEM presentation by a lawyer in the open
source space which puts across the case for all "badgeware" licenses
being considered non-free

  https://archive.fosdem.org/2014/schedule/event/trademark_licenses/

Since you say elsewhere in this thread, that it is not actually your
intention that everyone have to display a logo in their application,
I think it would be beneficial if you were able to re-consider the
terms used in the boilerplate text of the source files. To make the
licensing clear & simple to understand, IMHO, the easiest could be
to use the standard AGPL v3 boilerplate text "as-is" without any
custom additions.

Regards,
Daniel

[1] https://lists.debian.org/debian-legal/2005/11/msg00114.html
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/d

Re: [Spice-devel] Full featured (qxl compatible) spice web client released

2015-10-30 Thread Daniel P. Berrange

On Fri, Oct 30, 2015 at 01:24:36PM +0100, j...@eyeos.com wrote:
>  
> > I note the attribution clause in your license. As it stands now, I
> > don't think that would cause any trouble (because of the 'however' of 5
> > (d) of the agpl). But it would probably be useful to get a clear
> > expression of your intent - is it the case that you are willing to
> > contribute this only so long as the eyeos logo is prominently displayed
> > by any one choosing to deploy it?
> 
> Our intentions are very simple. We want this to become the defacto html5
> client for spice. Of course we want to maintain it and develop it
> openly. Aside from our obvious personal preferences for FOSS the main
> reason to opensource it is to develop it in community and of course, we
> are very excited to discuss the next steps with you and anyone
> interested in web support for spice :) 
> 
> And no, we are not willing to have an eyeos logo everywhere this client
> is used. In fact there is no need for you to display an eyeos logo to
> use this. 
> 
> We choosed AGPL because it protects the spice web client from the ASP
> loophole. We don't want en eyeos logo or something like this displayed
> every time this client is used, we just don't want for example a third
> party company adding support for opus in a private product of its own
> and not contributing it since this can be used from a service provider
> (ASP loophole in gpl and similar licenses). 
> 
> However, we are open to discussions about the license if you feel this
> can be a problem for reasonable scenarios.

Your rationale for using the AGPL as a core license makes sense to me.

Adding any additional terms to any license though, generally makes me
concerned, as it is very easy to add seemingly innocuous text that in
fact has non-obvious legal consequences. So I am a bit conerned about
the additionl terms wrt attribution & logo display. IANAL though, so
I will see if I can get any clarity / expert opinion on whether these
terms are likely to cause any problems for acceptance in distros such
as Fedora (or Debian) which are quite strict about interpretation of
licensing.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] build-sys:tests: Error out unless --enable-static is used

2015-10-14 Thread Daniel P. Berrange

On Tue, Oct 13, 2015 at 10:56:00AM +0200, Christophe Fergeau wrote:
> The binaries in tests/ need a static build of spice-gtk libraries in
> order to be built, but by default, we disable static libraries at
> LT_INIT() time.
> As the compile failure can be quite cryptic when someone
> tries to manually run 'make -C tests', this commit adds an explicit
> error message when trying to build the tests without --enable-static.
> ---
>  tests/Makefile.am | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 19c02b6..144bc38 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -1,3 +1,6 @@
> +if !BUILD_TESTS
> +$(error Building tests requires using --enable-static)
> +endif
>  NULL =

IIUC, the problem you have is that the tests try to link to the
libspice-client-glib-2.0.la, but when that is a share library
the symbols you need to use are hidden due to the linker script
whitelist.

One way to get around that is to build the sources you need into
a parallel convenience library and use that instead when running
tests. The main downside of this approach is that everything
ends up being compiled twice.

You would do something like this:

diff --git a/src/Makefile.am b/src/Makefile.am
index 0c40c48..9245bce 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -381,6 +381,17 @@ endif
 libspice_client_glib_2_0_la_LIBADD += -lws2_32 -lgdi32
 endif
 
+# Remove version script from convenience library
+test_LDFLAGS = \
+  $$(echo '$(libspice_client_glib_2_0_la_LDFLAGS)'  \
+ |sed 's!$(GLIB_SYMBOLS_LDFLAGS)!!')
+
+noinst_LTLIBRARIES = libspice-client-glib-test.la
+libspice_client_glib_test_la_SOURCES = $(libspice_client_glib_2_0_la_SOURCES)
+libspice_client_glib_test_la_LIBADD = $(libspice_client_glib_2_0_la_LIBADD)
+libspice_client_glib_test_la_LDFLAGS = $(test_LDFLAGS) $(AM_LDFLAGS)
+libspice_client_glib_test_la_CFLAGS = $(AM_CFLAGS)
+
 spicy_SOURCES =\
spicy.c \
spicy-connect.h \
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 19c02b6..1049ea3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -19,10 +19,10 @@ AM_CPPFLAGS =   \
-DG_LOG_DOMAIN=\"GSpice\"   \
$(NULL)
 
-AM_LDFLAGS = $(GIO_LIBS) -static
+AM_LDFLAGS = $(GIO_LIBS)
 
 LDADD =\
-   $(top_builddir)/src/libspice-client-glib-2.0.la \
+   $(top_builddir)/src/libspice-client-glib-test.la\
$(NULL)
 
 util_SOURCES = util.c


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-common PATCH] ssl_verify.c: Add IPv6 support

2015-09-07 Thread Daniel P. Berrange

On Mon, Sep 07, 2015 at 06:55:47PM +0200, Lukas Venhoda wrote:
> Add inet_pton and inet_ntop which supports IPv6 address.
> inet_aton left for compatibility.

You really should not use any of the inet_* functions,
even the ones which technically support IPv6.

Instead use getaddrinfo()/getnameinfo(), passing AI_NUMERIC
if you need to skip DNS forward/backward lookups and stick
to numeric ddresses

See this page for detailed information

  http://www.akkadia.org/drepper/userapi-ipv6.html


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [libvirt] [PATCH v4] qemu: Use heads parameter for QXL driver

2015-07-22 Thread Daniel P. Berrange

On Wed, Jul 22, 2015 at 09:59:00AM +0200, Martin Kletzander wrote:
 On Tue, Jul 21, 2015 at 04:36:45PM +0100, Daniel P. Berrange wrote:
 On Tue, Jul 21, 2015 at 11:34:08AM -0400, Laine Stump wrote:
 On 07/21/2015 09:41 AM, Daniel P. Berrange wrote:
  On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
  On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
  On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
  On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
  On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
  I spend all morning fixing this to be installed properly in the
  system.  Anyway, I finally managed to make this work and found out 
  the
  guest I used for it is not ready to have multiple monitors.  Anyway,
  looking at everything else this definitely works, so ACK, I'll push 
  it
  in a while.
  I'm still a bit worried that all existing guests will have heads='1' 
  in
  their XML as libvirt is adding that by default, but I don't really 
  see a
  way around it :-/ We should make sure libvirt stops adding it from now
  on though ;)
 
  Well, how would you then limit the outputs to 1?  Having said that, I
  have no idea why we started adding heads=1 automatically and playing
  with different changes, we have another bug in the parsing/formatting
  code.  You'll also won't be able to migrate from older libvirt with
  heads='1' to newer ones.  I haven't realized that.  I'm thinking about
  reverting the change in libvirt or even using heads  1.  Although
  that won't migrate either.  So the only other thing that we can do is
  to introduce new parameter, like maxHeads.  The sound you just heard
  is me slapping my face because again there will multiple parameters
  meaning similar things...
  Introducing a new parameter is not really viable IMHO, because as you
  say it'll leave us with two things meaning the same, and will not be
  compatible with existing apps that know about the current parameter.
 
  I think we need to figure out a way to drop the 'heads=1' from any
  existing configs in /etc/libvirt/qemu when we start up with the new
  libvirtd for the first time.
 
  I'm already working on an RFC that would differentiate between loading
  XMLs on daemon start-up and defining them.  The purpose of that is so
  that we can make incompatible XML changes without losing any domains,
  but that's not the point here.  If we were to drop heads='1' anywhere,
  this is the place.  The ABI change would be minimal for the guest.
  It isn't sufficient to just differentiate loading on startup
  vs defining them IIUC. We need to differentiate loading on
  startup from XML previously written by a libvirtd  X.Y.Z
  which is why I think we need to have a version number recorded
  in the XML ultimately.
 
 But a version number isn't sufficient to describe exactly  what the
 previous libvirt was capable of. Many times new features (externally
 visible only in the XML) are backported to earlier versions of libvirt
 downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x),
 so this still doesn't buy us perfection. Downstream maintainers could
 make it better by paying very close attention to any use of this version
 number when backporting new stuff, but there would still be problems if
 someone decided to build and install an upstream libvirt on a system
 that previously had some hybrid downstream build.
 
 (Not saying we shouldn't do it, just that it's no perfect :-)
 
 Yep, understood. I'm thinking purely in terms of upstream where we do
 not backport features to stable branches. Distros which get into the
 feature backport game have to deal with that pain themselves.
 
 
 How about we use some part of the XML to mark features there?  That
 part would only be parsed on loading and formatted into migration
 cookie and the XML saved on disk.  We can put it into another
 namespace.
 
 Each feature would have its handler that fixes whatever needs to be
 fixed in the postparse part.

I'm not sure what you are suggesting by features here, but I'd prefer
if we didn't introduce a chunk of XML which would contain an ever growing
set of hacks. It seems sufficient for us to just record the libvirt
version number which generated the XML document, so newer libvirt can
detect a previous buggy version and correct what's needed, so the
hacks are confined to the code and not spread across code + xml.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [libvirt] [PATCH v4] qemu: Use heads parameter for QXL driver

2015-07-21 Thread Daniel P. Berrange

On Tue, Jul 21, 2015 at 11:34:08AM -0400, Laine Stump wrote:
 On 07/21/2015 09:41 AM, Daniel P. Berrange wrote:
  On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
  On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
  On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
  On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
  On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
  I spend all morning fixing this to be installed properly in the
  system.  Anyway, I finally managed to make this work and found out the
  guest I used for it is not ready to have multiple monitors.  Anyway,
  looking at everything else this definitely works, so ACK, I'll push it
  in a while.
  I'm still a bit worried that all existing guests will have heads='1' in
  their XML as libvirt is adding that by default, but I don't really see a
  way around it :-/ We should make sure libvirt stops adding it from now
  on though ;)
 
  Well, how would you then limit the outputs to 1?  Having said that, I
  have no idea why we started adding heads=1 automatically and playing
  with different changes, we have another bug in the parsing/formatting
  code.  You'll also won't be able to migrate from older libvirt with
  heads='1' to newer ones.  I haven't realized that.  I'm thinking about
  reverting the change in libvirt or even using heads  1.  Although
  that won't migrate either.  So the only other thing that we can do is
  to introduce new parameter, like maxHeads.  The sound you just heard
  is me slapping my face because again there will multiple parameters
  meaning similar things...
  Introducing a new parameter is not really viable IMHO, because as you
  say it'll leave us with two things meaning the same, and will not be
  compatible with existing apps that know about the current parameter.
 
  I think we need to figure out a way to drop the 'heads=1' from any
  existing configs in /etc/libvirt/qemu when we start up with the new
  libvirtd for the first time.
 
  I'm already working on an RFC that would differentiate between loading
  XMLs on daemon start-up and defining them.  The purpose of that is so
  that we can make incompatible XML changes without losing any domains,
  but that's not the point here.  If we were to drop heads='1' anywhere,
  this is the place.  The ABI change would be minimal for the guest.
  It isn't sufficient to just differentiate loading on startup
  vs defining them IIUC. We need to differentiate loading on
  startup from XML previously written by a libvirtd  X.Y.Z
  which is why I think we need to have a version number recorded
  in the XML ultimately.
 
 But a version number isn't sufficient to describe exactly  what the
 previous libvirt was capable of. Many times new features (externally
 visible only in the XML) are backported to earlier versions of libvirt
 downstream (e.g. libvirt-0.10.2 that is used in RHEL6.x and CentOS 6.x),
 so this still doesn't buy us perfection. Downstream maintainers could
 make it better by paying very close attention to any use of this version
 number when backporting new stuff, but there would still be problems if
 someone decided to build and install an upstream libvirt on a system
 that previously had some hybrid downstream build.
 
 (Not saying we shouldn't do it, just that it's no perfect :-)

Yep, understood. I'm thinking purely in terms of upstream where we do
not backport features to stable branches. Distros which get into the
feature backport game have to deal with that pain themselves.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [libvirt] [PATCH v4] qemu: Use heads parameter for QXL driver

2015-07-21 Thread Daniel P. Berrange

On Tue, Jul 21, 2015 at 03:35:50PM +0200, Martin Kletzander wrote:
 On Tue, Jul 21, 2015 at 01:50:22PM +0100, Daniel P. Berrange wrote:
 On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
 On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
 On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
 I spend all morning fixing this to be installed properly in the
 system.  Anyway, I finally managed to make this work and found out the
 guest I used for it is not ready to have multiple monitors.  Anyway,
 looking at everything else this definitely works, so ACK, I'll push it
 in a while.
 
 I'm still a bit worried that all existing guests will have heads='1' in
 their XML as libvirt is adding that by default, but I don't really see a
 way around it :-/ We should make sure libvirt stops adding it from now
 on though ;)
 
 
 Well, how would you then limit the outputs to 1?  Having said that, I
 have no idea why we started adding heads=1 automatically and playing
 with different changes, we have another bug in the parsing/formatting
 code.  You'll also won't be able to migrate from older libvirt with
 heads='1' to newer ones.  I haven't realized that.  I'm thinking about
 reverting the change in libvirt or even using heads  1.  Although
 that won't migrate either.  So the only other thing that we can do is
 to introduce new parameter, like maxHeads.  The sound you just heard
 is me slapping my face because again there will multiple parameters
 meaning similar things...
 
 Introducing a new parameter is not really viable IMHO, because as you
 say it'll leave us with two things meaning the same, and will not be
 compatible with existing apps that know about the current parameter.
 
 I think we need to figure out a way to drop the 'heads=1' from any
 existing configs in /etc/libvirt/qemu when we start up with the new
 libvirtd for the first time.
 
 
 I'm already working on an RFC that would differentiate between loading
 XMLs on daemon start-up and defining them.  The purpose of that is so
 that we can make incompatible XML changes without losing any domains,
 but that's not the point here.  If we were to drop heads='1' anywhere,
 this is the place.  The ABI change would be minimal for the guest.

It isn't sufficient to just differentiate loading on startup
vs defining them IIUC. We need to differentiate loading on
startup from XML previously written by a libvirtd  X.Y.Z
which is why I think we need to have a version number recorded
in the XML ultimately.

 To make this work we probably need to write a magic attribute or
 comment into the XML configs to record which version of libvirt
 saved it to disk. That way we know whether this is the first time
 loading the config with the new libvirt. It will help us with
 similar situations in the future. In this case, we'd just look
 to see if the libvirt version number was missing from the XML,
 and if so, then drop heads=1. If a version number is present, then
 we're new enough, so we can keep it.  We'd also ned to pass this
 version number in the XML when migrating, or possibly via the
 migration cookie.
 
 
 And you're suggesting to differentiate this by a comment?  That's
 really, *really* hacky.  Anyway, do you agree on temporarily reverting
 the commit so we don't release another version with it?  The changes
 will also be clearer when they will not modify what this commit
 changed.

Actually we'd need to have a formal attribute to deal with the migration
case, so slightly less hacky.

Yeah, if we can't immediately fix it, we should revert it until we have
a decent fix without regression. Likewise probably fix it on stable
branch too.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [libvirt] [PATCH v4] qemu: Use heads parameter for QXL driver

2015-07-21 Thread Daniel P. Berrange

On Tue, Jul 21, 2015 at 11:44:27AM +0200, Martin Kletzander wrote:
 On Tue, Jul 21, 2015 at 09:36:55AM +0200, Christophe Fergeau wrote:
 On Mon, Jul 20, 2015 at 11:25:52AM +0200, Martin Kletzander wrote:
 I spend all morning fixing this to be installed properly in the
 system.  Anyway, I finally managed to make this work and found out the
 guest I used for it is not ready to have multiple monitors.  Anyway,
 looking at everything else this definitely works, so ACK, I'll push it
 in a while.
 
 I'm still a bit worried that all existing guests will have heads='1' in
 their XML as libvirt is adding that by default, but I don't really see a
 way around it :-/ We should make sure libvirt stops adding it from now
 on though ;)
 
 
 Well, how would you then limit the outputs to 1?  Having said that, I
 have no idea why we started adding heads=1 automatically and playing
 with different changes, we have another bug in the parsing/formatting
 code.  You'll also won't be able to migrate from older libvirt with
 heads='1' to newer ones.  I haven't realized that.  I'm thinking about
 reverting the change in libvirt or even using heads  1.  Although
 that won't migrate either.  So the only other thing that we can do is
 to introduce new parameter, like maxHeads.  The sound you just heard
 is me slapping my face because again there will multiple parameters
 meaning similar things...

Introducing a new parameter is not really viable IMHO, because as you
say it'll leave us with two things meaning the same, and will not be
compatible with existing apps that know about the current parameter.

I think we need to figure out a way to drop the 'heads=1' from any
existing configs in /etc/libvirt/qemu when we start up with the new
libvirtd for the first time.

To make this work we probably need to write a magic attribute or
comment into the XML configs to record which version of libvirt
saved it to disk. That way we know whether this is the first time
loading the config with the new libvirt. It will help us with
similar situations in the future. In this case, we'd just look
to see if the libvirt version number was missing from the XML,
and if so, then drop heads=1. If a version number is present, then
we're new enough, so we can keep it.  We'd also ned to pass this
version number in the XML when migrating, or possibly via the
migration cookie.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
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.

2015-07-01 Thread Daniel P. Berrange

On Tue, Jun 30, 2015 at 04:44:10PM -0500, Jeremy White wrote:
 This module uses the usbredir protocol and user space tools,
 which are used by the SPICE project.
 
 Signed-off-by: Jeremy White jwh...@codeweavers.com

[snip]

 diff --git a/drivers/usb/usbredir/Kconfig b/drivers/usb/usbredir/Kconfig
 new file mode 100644
 index 000..284fd02
 --- /dev/null
 +++ b/drivers/usb/usbredir/Kconfig
 @@ -0,0 +1,25 @@
 +config USBREDIR
 + tristate USBREDIR support
 + depends on USB  NET
 + ---help---
 +   This enables connecting a remote USB device over IP using
 +  the USBREDIR protocol.  This module provides a sysfs attach
 +  interface which, if given a socket connected to a remote
 +  usbredirserver, will enable the remote device to behave as
 +  though it were connected to the system running this module.
 +
 +  For more information and user space tools, refer to the
 +  USBREDIR project, which can be found at
 +  http://www.spice-space.org/page/UsbRedir.

[snip]

 new file mode 100644
 index 000..217a2e4
 --- /dev/null
 +++ b/drivers/usb/usbredir/README
 @@ -0,0 +1,20 @@
 +USB Redirection Kernel Module
 +
 +This module allows a Linux system to instatiate USB devices
 +that are located on a remote device.  The USB data is transferred
 +over a socket using the USBREDIR protocol, which is generally
 +used in conjunction with the SPICE project.
 +
 +You will need the USBREDIR user space tools.  They can
 +be found at http://www.spice-space.org/page/UsbRedir.
 +
 +To use, start the usbredirserver on a remote system.
 +For example,
 + ./usbredirserver --port 4000 125f:db8a
 +will export my ADATA thumb drive on the remote system.
 +
 +Next, on the local system, connect a socket and relay that to
 +the kernel module.  The connectkernel utility will do this as follows:
 +  ./connectkernel adata4000 my.remote.device.com 4000
 +
 +The device should attach and be usable on the local system.

What is the security story here ? If I am understanding correctly, you have
a userspace helper which opens a socket, and does a connect() to establish
the connection to the remote system, and then tells the kernel to use the
file descriptor associated with the socket.

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.

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.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] RFC: qxl: allow to specify head limit to qxl driver

2015-06-09 Thread Daniel P. Berrange

On Tue, Jun 09, 2015 at 09:49:44AM +0100, Frediano Ziglio wrote:
 This patch allow to limit number of heads using qxl driver. By default
 qxl driver is not limited on any kind on head use so can decide to use
 as much heads.
 
 libvirt has this as a video card parameter (actually set to 1 but not
 used). This parameter will allow to limit setting a use can do (which
 could be confusing).
 
 This patch rely on some change in spice-protocol which are not still
 accepted. See
 http://lists.freedesktop.org/archives/spice-devel/2015-June/020160.html.
 
 Main question and stop over are parameter name. Consider that this
 parameter is actually more a hint to drivers. I'm looking anyway to
 a way to enforce this in spice-server.

What is the actual benefit of being able to limit the number of
heads in QXL / SPICE ?

I know libvirt has the 'heads' attribute for specifying the number
of video outputs. This is used in hypervisors which only support a
fixed number of outputs and need up-front configuration. Since
QXL/SPICE can dynamically enable/disable heads, there is no need to
support this libvirt configuration attribute - it is better from a
usability POV to have the head count totally dynamic, than to add
a fixed limit.

IOW, unless there's some need to use this limit in order to go
above the 16 head maximum the code currently has, I think adding
this manual limit to QXL/SPICE is a step backwards really. It
feels like a feature in search of a purpose IMHO.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v4] Check too long password

2015-06-03 Thread Daniel P. Berrange

On Wed, Jun 03, 2015 at 04:22:35PM +0200, Cédric Bosdonnat wrote:
 Make sure that the password lenght is under the maximum lenght. If not

Nit-pick if you re-post,  s/lenght/length/g or someone can fixup when
merging

 report it as an authentication failure with an adapted message.
 ---
  Diff to v3:
 
* Removed the checks on the server side and the corresponding code here
* Removed the new error code to reuse 
 SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD.
* spice_channel_send_spice_ticket returns SpiceChannelEvent instead of a 
 gboolean:
  this helps properly propagating the kind of error to the client.
 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Add spec files

2015-04-16 Thread Daniel P. Berrange

On Thu, Apr 16, 2015 at 12:30:27PM +0200, Marc-André Lureau wrote:
 I don't think there are valid reason to put fedora-specific spec file in
 upstream. You can easily build a rpm with fedpkg. nack

We've done this for years with all libvirt related packages and it
has proved pretty userful for both end users and maintainers alike.

For example, when upstream makes a change that would impact the RPM
packaging, it is usually quicker  easier for the person making
that change to do the RPM spec file change upstream at the same
time, than for the downstream maintainer to waste time trying to
figure out what the change was.

By having an upstream RPM spec that is always buildable, it makes
it easier for users to test out new releases that are not yet in
the official distro repos, because again they don't have to figure
out how to fix the RPM spec to work with latest code changes.

Having a single canonical RPM spec that works across all current
versions of RHEL  Fedora has also reduced the workload on the
Fedora maintainers too.

The included RPM spec also serves as a helpful guide for other
distros figuring out how to package libvirt in non-RPM formats

So yes, it might seem odd to include a downstream specific RPM
spec in upstream, but in practice it has proved pretty useful
over the 9+ years libvirt  related apps have been around

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] vncdisplaykeymap: Add support for Xwayland keymap

2015-02-18 Thread Daniel P. Berrange

On Wed, Feb 18, 2015 at 09:47:55AM -0500, Marc-André Lureau wrote:
 
 
 - Original Message -
  From: Daniel P. Berrange berra...@redhat.com
  
  Both Wayland and Xwayland use the evdev + 8 map for keycodes,
  just as regular Xorg with evdev does.
  
  This code is copied from gtk-vnc.
  
  Fixes https://bugs.freedesktop.org/show_bug.cgi?id=89105
  ---
   gtk/vncdisplaykeymap.c | 16 
   1 file changed, 16 insertions(+)
  
  diff --git a/gtk/vncdisplaykeymap.c b/gtk/vncdisplaykeymap.c
  index 340a86f..32fdac5 100644
  --- a/gtk/vncdisplaykeymap.c
  +++ b/gtk/vncdisplaykeymap.c
  @@ -12,6 +12,7 @@
   #include gtk/gtk.h
   #include gdk/gdk.h
   #include gdk/gdkkeysyms.h
  +#include stdlib.h
   #include gtk-compat.h
   #include vncdisplaykeymap.h
   
  @@ -156,6 +157,17 @@ static gboolean check_for_xquartz(GdkDisplay *dpy)
   
  return match;
   }
  +
  +static gboolean check_for_xwayland(GdkDisplay *dpy G_GNUC_UNUSED)
  +{
  +   /* There is no obvious extension name or root window property
  +* that identifies as Xwayland. It also does not report any
  +* XKB info. So this env var check is least-worst option left
  +*/
  +   char *dpystr = getenv(WAYLAND_DISPLAY);
  +
  +   return dpystr != NULL;
  +}
   #endif
   
   const guint16 *vnc_display_keymap_gdk2xtkbd_table(GdkWindow *window,
  @@ -195,6 +207,10 @@ const guint16
  *vnc_display_keymap_gdk2xtkbd_table(GdkWindow *window,
  VNC_DEBUG(Using xquartz keycode mapping);
  *maplen = G_N_ELEMENTS(keymap_xorgxquartz2xtkbd);
  return keymap_xorgxquartz2xtkbd;
  +   } else if (check_for_xwayland(dpy)) {
  +   VNC_DEBUG(Using wayland evdec keycode mapping);
  +   *maplen = G_N_ELEMENTS(keymap_xorgevdev2xtkbd);
  +   return keymap_xorgevdev2xtkbd;
  } else if (keycodes  STRPREFIX(keycodes, evdev_)) {
  VNC_DEBUG(Using evdev keycode mapping);
  *maplen = G_N_ELEMENTS(keymap_xorgevdev2xtkbd);
  --
  2.3.0
 
 We already have wayland input support since:
 
 commit 577263aaf4cffe260e31d19eb4b8bca1c253eff3
 Author: Marc-André Lureau marcandre.lur...@redhat.com
 Date:   Mon May 13 01:11:07 2013 +0200
 
 vncdisplaykeymap: add wayland support
 
 The Wayland keycode are just Linux evdev, but the Gdk backend
 add the +8 offset used by Xorg evdev.
 
 
 Not sure about XWayland, but I would assume this was enough,
 and didn't rely on a not so reliable environment variable.

Your change only works when the GTK application is running as a
native Wayland client.  This fix works when the GTK application
is runing as an X11 client on Xwayland.

The use of the env variable is certainly sucky, but I've not
found any other way to identify that the X server is the
Xwayland one :-( It doesn't report any XKB keyboard info,
the server vendor string is completely generic and there's no
wayland specific extension present.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 2/6] LZ4: Implement ntohl to remove dependency on arpa/inet.h

2015-01-26 Thread Daniel P. Berrange

On Mon, Jan 26, 2015 at 04:53:31PM +0100, Christophe Fergeau wrote:
 On Mon, Jan 26, 2015 at 09:23:58AM +0100, Javier Celaya wrote:
  I saw some comments about MSVC in spice-gtk, and I thought someone could be 
  using it to build the Windows client. Anyway, I don't mind using one 
  solution 
  or another, so you tell me.
 
 spice-common may be referencing MSVC++ as the old spice-client used to
 be built with MSVC++, but this client is gone nowadays. As far as I
 know, spice-gtk for Windows is always built with mingw.

Yep, for the virt-viewer  Windows installer I always build from the
Fedora mingw toolchain + packages. So from that angle we don't care
about MSVC++

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH RFC] Add HACKING file

2014-12-08 Thread Daniel P. Berrange

On Mon, Dec 08, 2014 at 04:25:24PM +0100, Christophe Fergeau wrote:
 On Mon, Dec 08, 2014 at 03:37:32PM +0100, Marc-André Lureau wrote:
  So much of this is quite irrelevant to the discussion at hand about
  unreviewed commit rule.
 
 I know this is about a lot of stuff, libvirt HACKING fine had plenty of
 interesting things, so I thought why not keep them. I don't mind
 dropping everything :)
 
  In general I don't think we need that document, because we follow very
  common participation rules. Having strict rules makes contributions
  more difficult and that's really not what we are after at this point.
 
 It's more about having things documented in a place easy to refer to
 than about having strict rules, but I don't feel too strongly about
 having this in a HACKING file or not.

Note that the title of the libvirt HACKING document says

   Contributor guidelines

not

   Contributor rules

They were not intended to be 100% strict rules that must be followed
on pain of death at all times. They're intended to lay out the common
case so that people have their broad expectations set at the right
level, still leave contributors/reviewers the freedom to deviate if
appropriate in particular patches/scenarios.

FWIW, we've found the HACKING file rules very useful in getting new
contributors onboard  up to speed with the projects way of working.
Long term contributors will of course have mentally interned all the
knowledge from the doc long ago

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 03/11] Add missing (C) to copyright line

2014-10-30 Thread Daniel P. Berrange

On Thu, Oct 30, 2014 at 02:28:25PM +0100, Marc-André Lureau wrote:
 On Thu, Oct 30, 2014 at 2:22 PM, Christophe Fergeau cferg...@redhat.com
 wrote:
 
  On Thu, Oct 30, 2014 at 02:02:27PM +0100, Marc-André Lureau wrote:
   Quoting GNU maintainer manual:
   https://www.gnu.org/prep/maintain/html_node/Copyright-Notices.html
  
   Alternatively, the ‘(C)’ or C-in-a-circle can be omitted entirely; the
   word ‘Copyright’ suffices.
 
  The rule which asks for that to be done is:
  sc_copyright_format:
  @require='Copyright .*Red 'Hat', Inc\.' \
  containing='Copyright .*Red 'Hat\
  halt='Red Hat copyright is missing Inc.'\
$(_sc_search_regexp)
  @prohibit='Copyright [^(].*Red 'Hat \
  halt='consistently use (C) in Red Hat copyright'\
$(_sc_search_regexp)
  @prohibit='\Red''Hat\'\
  halt='spell Red Hat as two words'   \
$(_sc_search_regexp)
 
  I don't know if Red Hat legal department insists on the (C) being present.
  I can reword the commit log to explicit it's only being enforced for Red
  Hat's
  copyright, or I can drop the patch.
 
 
 It might be specified by Red Hat legal somewhere, but then I wonder why
 there wouldn't be checks for that (in RH builds checks), so it's probably
 unneeded.

This syntax-check rule isn't about enforcing a legal requirement
but rather ensuring that we use a consist copyright header across
all source files. 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 03/11] Add missing (C) to copyright line

2014-10-30 Thread Daniel P. Berrange

On Thu, Oct 30, 2014 at 02:55:05PM +0100, Christophe Fergeau wrote:
 On Thu, Oct 30, 2014 at 01:38:26PM +, Daniel P. Berrange wrote:
  On Thu, Oct 30, 2014 at 02:28:25PM +0100, Marc-André Lureau wrote:
   On Thu, Oct 30, 2014 at 2:22 PM, Christophe Fergeau cferg...@redhat.com
   wrote:
   
On Thu, Oct 30, 2014 at 02:02:27PM +0100, Marc-André Lureau wrote:
 Quoting GNU maintainer manual:
 https://www.gnu.org/prep/maintain/html_node/Copyright-Notices.html

 Alternatively, the ‘(C)’ or C-in-a-circle can be omitted entirely; 
 the
 word ‘Copyright’ suffices.
   
The rule which asks for that to be done is:
sc_copyright_format:
@require='Copyright .*Red 'Hat', Inc\.' 
\
containing='Copyright .*Red 'Hat
\
halt='Red Hat copyright is missing Inc.'
\
  $(_sc_search_regexp)
@prohibit='Copyright [^(].*Red 'Hat 
\
halt='consistently use (C) in Red Hat copyright'
\
  $(_sc_search_regexp)
@prohibit='\Red''Hat\'
\
halt='spell Red Hat as two words'   
\
  $(_sc_search_regexp)
   
I don't know if Red Hat legal department insists on the (C) being 
present.
I can reword the commit log to explicit it's only being enforced for Red
Hat's
copyright, or I can drop the patch.
   
   
   It might be specified by Red Hat legal somewhere, but then I wonder why
   there wouldn't be checks for that (in RH builds checks), so it's probably
   unneeded.
  
  This syntax-check rule isn't about enforcing a legal requirement
  but rather ensuring that we use a consist copyright header across
  all source files. 
 
 Do you know why it's not enforced for all copyright lines, but only for
 Red Hat? It's up to other companies/individual to decide how they want
 their copyright line to look with respect to (C)/Copyright/... ?

Yep, the license terms don't allow us change any Copyright line
that other copyright holders have added.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] m4: Update manywarnings from gnulib

2014-10-07 Thread Daniel P. Berrange

On Thu, Oct 02, 2014 at 06:17:53PM +0200, Fabiano Fidêncio wrote:
 From: Cole Robinson crobi...@redhat.com
 
 Fixes these noisy errors on Fedora 21:
 
 gcc: warning: switch '-Wmudflap' is no longer supported

Thanks, I've applied this


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add a SPICE_GTK_CHECK_VERSION macro

2014-02-24 Thread Daniel P. Berrange

On Mon, Feb 24, 2014 at 03:03:56PM +0100, Marc-André Lureau wrote:
 ---
  configure.ac | 10 ++
  doc/reference/spice-gtk-sections.txt |  4 +++
  gtk/Makefile.am  |  4 +++
  gtk/spice-client.h   |  1 +
  gtk/spice-version.h.in   | 70 
 
  5 files changed, 89 insertions(+)
  create mode 100644 gtk/spice-version.h.in
 
 diff --git a/configure.ac b/configure.ac
 index 6e4b29a..192d748 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -75,6 +75,15 @@ AC_CONFIG_SUBDIRS([spice-common])
  COMMON_CFLAGS='-I ${top_srcdir}/spice-common/ -I 
 ${top_srcdir}/spice-common/spice-protocol/'
  AC_SUBST(COMMON_CFLAGS)
  
 +SPICE_GTK_MAJOR_VERSION=`echo $PACKAGE_VERSION | cut -d. -f1`
 +SPICE_GTK_MINOR_VERSION=`echo $PACKAGE_VERSION | cut -d. -f2`
 +SPICE_GTK_MICRO_VERSION=`echo $PACKAGE_VERSION | cut -d. -f3 | cut -d- -f1`
 +[ x$SPICE_GTK_MICRO_VERSION = x ]  SPICE_GTK_MICRO_VERSION = 0
 +
 +AC_SUBST(SPICE_GTK_MAJOR_VERSION)
 +AC_SUBST(SPICE_GTK_MINOR_VERSION)
 +AC_SUBST(SPICE_GTK_MICRO_VERSION)
 +
  dnl =
  dnl Chek optional features
  
 @@ -709,6 +718,7 @@ data/spicy.desktop.in
  data/spicy.nsis
  po/Makefile.in
  gtk/Makefile
 +gtk/spice-version.h
  gtk/controller/Makefile
  doc/Makefile
  doc/reference/Makefile
 diff --git a/doc/reference/spice-gtk-sections.txt 
 b/doc/reference/spice-gtk-sections.txt
 index 08b1b4e..9232a23 100644
 --- a/doc/reference/spice-gtk-sections.txt
 +++ b/doc/reference/spice-gtk-sections.txt
 @@ -402,6 +402,10 @@ SpiceGtkBoxClass
  FILEspice-util/FILE
  spice_util_set_debug
  spice_util_get_version_string
 +SPICE_GTK_CHECK_VERSION
 +SPICE_GTK_MAJOR_VERSION
 +SPICE_GTK_MICRO_VERSION
 +SPICE_GTK_MINOR_VERSION
  SUBSECTION Private
  SPICE_DEBUG
  spice_util_get_debug
 diff --git a/gtk/Makefile.am b/gtk/Makefile.am
 index 61ed88e..8a16120 100644
 --- a/gtk/Makefile.am
 +++ b/gtk/Makefile.am
 @@ -41,8 +41,11 @@ EXTRA_DIST =   \
   spice-client-gtk-manual.defs\
   spice-client-gtk.override   \
   spice-marshal.txt   \
 + spice-version.h.in  \
   $(NULL)
  
 +DISTCLEANFILES = spice-version.h
 +
  bin_PROGRAMS = spicy spicy-stats spicy-screenshot
  if WITH_POLKIT
  acldir = $(ACL_HELPER_DIR)
 @@ -290,6 +293,7 @@ libspice_client_glibinclude_HEADERS = \
   spice-channel.h \
   spice-util.h\
   spice-option.h  \
 + spice-version.h \
   channel-cursor.h\
   channel-display.h   \
   channel-inputs.h\
 diff --git a/gtk/spice-client.h b/gtk/spice-client.h
 index 975259a..98aaffe 100644
 --- a/gtk/spice-client.h
 +++ b/gtk/spice-client.h
 @@ -32,6 +32,7 @@
  #include spice-channel.h
  #include spice-option.h
  #include spice-uri.h
 +#include spice-version.h
  
  #include channel-main.h
  #include channel-display.h
 diff --git a/gtk/spice-version.h.in b/gtk/spice-version.h.in
 new file mode 100644
 index 000..8856914
 --- /dev/null
 +++ b/gtk/spice-version.h.in
 @@ -0,0 +1,70 @@
 +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
 +/*
 +  Copyright (C) 2014 Red Hat, Inc.
 +
 +  This library is free software; you can redistribute it and/or
 +  modify it under the terms of the GNU Lesser General Public
 +  License as published by the Free Software Foundation; either
 +  version 2.1 of the License, or (at your option) any later version.
 +
 +  This library is distributed in the hope that it will be useful,
 +  but WITHOUT ANY WARRANTY; without even the implied warranty of
 +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 +  Lesser General Public License for more details.
 +
 +  You should have received a copy of the GNU Lesser General Public
 +  License along with this library; if not, see 
 http://www.gnu.org/licenses/.
 +*/
 +#ifndef __SPICE_VERSION_H__
 +#define __SPICE_VERSION_H__
 +
 +/**
 + * SECTION:spice-version
 + * @short_description: Spice-Gtk version checking
 + *
 + * Spice-Gtk provides macros to check the version of the library
 + * at compile-time
 + */
 +
 +/**
 + * SPICE_GTK_MAJOR_VERSION:
 + *
 + * Spice-Gtk major version component (e.g. 1 if version is 1.2.3)
 + * Since: 0.24
 + */
 +#define SPICE_GTK_MAJOR_VERSION  (@SPICE_GTK_MAJOR_VERSION@)
 +
 +/**
 + * SPICE_GTK_MINOR_VERSION:
 + *
 + * Spice-Gtk minor version component (e.g. 2 if version is 1.2.3)
 + * Since: 0.24
 + */
 +#define SPICE_GTK_MINOR_VERSION  (@SPICE_GTK_MINOR_VERSION@)
 +
 +/**
 + * SPICE_GTK_MICRO_VERSION:
 + *
 + * Spice-Gtk micro version component (e.g. 3 if version is 1.2.3)
 + * Since: 0.24
 + */
 +#define SPICE_GTK_MICRO_VERSION  (@SPICE_GTK_MICRO_VERSION@)
 +
 +/**
 + * SPICE_GTK_CHECK_VERSION:
 + *

Re: [Spice-devel] Potential New Development for SPICE

2014-02-20 Thread Daniel P. Berrange

On Wed, Feb 19, 2014 at 11:47:43AM -0600, adaptent wrote:
 Dear Sirs,
 
 I have been utilizing SPICE to analyze atoms, which I believe may be
 the first time this has ever been done.

I'd certainly be impressed if our SPICE program had been used
to analyse atoms! Unfortunately I think you might have mixed us
up with a different software program

This mailing list is about the remote desktop display protocol
called SPICE:

   http://en.wikipedia.org/wiki/SPICE_%28protocol%29


It sounds like you're possibly talking about the SPICE electrical
circuit simulator:

   http://en.wikipedia.org/wiki/SPICE

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Win 8 Display Resolution

2014-02-12 Thread Daniel P. Berrange

On Wed, Feb 12, 2014 at 10:13:18PM +1000, Lindsay Mathieson wrote:
 Without a QXL driver isn't it limited to 1024x768 resolution?

You should be able to go higher than that. IIUC even the cirrus driver
supports more than that, but if not then the 'vga' driver for QEMU
should. It is mostly determined by how much video RAM is exposed to
the guest.

QXL gives you other nice features like multi-monitor, dynamic mode
switching via the guest agent, better performance with spice, etc.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] SPICE for text mode serial consoles ?

2014-01-15 Thread Daniel P. Berrange

A long time back OpenStack had a remote text console service, in parallel
with its VNC console console. This was later deleted because it was using
insecure code from an unmaintained 3rd party project. Fairly frequently
though people raise the issue of re-enabling remote text (ie serial)
console access, as a low bandwidth alternative to a fully graphical
console.

I was wondering, given that SPICE is designed with many independant
data channels, would it make sense to provide a way to add new channel
type that could transport a text mode console ? eg SPICE would act as
the backend for QEMU serial ports, as well as the graphical display.
Clients could have a choice of opening the graphics related channels,
the serial port channels, or both.

NB I'm not saying I've time to work on this at all - I was just wondering
if this was conceptually a reasonable feature to propose.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] SPICE for text mode serial consoles ?

2014-01-15 Thread Daniel P. Berrange

On Wed, Jan 15, 2014 at 11:01:16AM -0500, i iordanov wrote:
 Hi Daniel,
 
 Would the spiceterm project from Proxmox be of interest?
 
 https://git.proxmox.com/?p=spiceterm.git;a=blob_plain;f=spiceterm.pod;hb=master

That is running a shell server side and then rendering it
into a graphical display which is then transported over
SPICE as a bitmap. That is a rather inefficient way of
sending a plain text data stream, and the client can't
turn this bitmap back into plain text for display on a
local text terminal. For this to be of value we want
to be transporting plain text end-to-end in both directions.

 On Wed, Jan 15, 2014 at 10:37 AM, Daniel P. Berrange 
 berra...@redhat.comwrote:
 
  A long time back OpenStack had a remote text console service, in parallel
  with its VNC console console. This was later deleted because it was using
  insecure code from an unmaintained 3rd party project. Fairly frequently
  though people raise the issue of re-enabling remote text (ie serial)
  console access, as a low bandwidth alternative to a fully graphical
  console.
 
  I was wondering, given that SPICE is designed with many independant
  data channels, would it make sense to provide a way to add new channel
  type that could transport a text mode console ? eg SPICE would act as
  the backend for QEMU serial ports, as well as the graphical display.
  Clients could have a choice of opening the graphics related channels,
  the serial port channels, or both.
 
  NB I'm not saying I've time to work on this at all - I was just wondering
  if this was conceptually a reasonable feature to propose.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] SPICE for text mode serial consoles ?

2014-01-15 Thread Daniel P. Berrange

On Wed, Jan 15, 2014 at 11:18:42AM -0500, Marc-André Lureau wrote:
 
 
 - Original Message -
  A long time back OpenStack had a remote text console service, in parallel
  with its VNC console console. This was later deleted because it was using
  insecure code from an unmaintained 3rd party project. Fairly frequently
  though people raise the issue of re-enabling remote text (ie serial)
  console access, as a low bandwidth alternative to a fully graphical
  console.
  
  I was wondering, given that SPICE is designed with many independant
  data channels, would it make sense to provide a way to add new channel
  type that could transport a text mode console ? eg SPICE would act as
  the backend for QEMU serial ports, as well as the graphical display.
  Clients could have a choice of opening the graphics related channels,
  the serial port channels, or both.
 
 It's kind of possible today
 
 -chardev spiceport,name=org.spice.spicy,id=charchannel1 -serial 
 chardev:charchannel1
 
 booting kernel with console=tty0 console=ttyS0,115200n8 and connecting with 
 spicy will give you kernel log in client console..
 
 There is even a standard port name for it: org.qemu.console.serial.0

Cool, I love it when my ideas have already been mostly implemented :-)

 But integration with client is left to do, also I don't know what's
 the right way to setup the VM to make this work nicely with login etc..

If you boot with  console=ttyS0 then systemd would automatically start
a login prompt on the serial port. I think you can probably even make it
do both graphical  text logins using 'console=ttyS0 console=tty'

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] Use TLS version 1.0 or better

2013-11-27 Thread Daniel P. Berrange

On Wed, Nov 27, 2013 at 05:23:53PM +0100, David Jaša wrote:
 When creating a TLS socket, both spice-server and spice-gtk currently
 call SSL_CTX_new(TLSv1_method()). The TLSv1_method() function set the
 protocol version to TLS 1.0 exclusively. The correct way to support
 multiple protocol versions is to call SSLv23_method() in spite of its
 scary name. This method will enable all protocol versions deemed secure
 by openssl project. The protocol suite may be further narrowed down by
 setting respective SSL_OP_NO_version_code options of SSL context. This
 possibility is used in this patch in order to block use of SSLv3 that is
 enabled by default in openssl as of now but spice has never used it.
 ---
  server/reds.c |   10 +-
  1 files changed, 9 insertions(+), 1 deletions(-)
 
 diff --git a/server/reds.c b/server/reds.c
 index 2a0002b..263843f 100644
 --- a/server/reds.c
 +++ b/server/reds.c
 @@ -3221,6 +3221,14 @@ static int reds_init_ssl(void)
  SSL_METHOD *ssl_method;
  #endif
  int return_code;
 +/* When some other SSL/TLS version becomes obsolete, add it to this
 + * variable.
 + *
 + * Note that SSLv23_method() even with no SSL_OP_NO_* options uses
 + * just protocol versions deemed secure by openssl project so the
 + * SSL_OP_NO_SSLv2 is already redundant and SSL_OP_NO_SSLv3 option is
 + * present just in order to allow only currently-availabe version or
 + * better. */
  long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
  
  /* Global system initialization*/
 @@ -3228,7 +3236,7 @@ static int reds_init_ssl(void)
  SSL_load_error_strings();
  
  /* Create our context*/
 -ssl_method = TLSv1_method();
 +ssl_method = ssl_method = SSLv23_method();

You're setting the same variable twice.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Use latest warnings.m4 from gnulib

2013-10-03 Thread Daniel P. Berrange

On Fri, Sep 27, 2013 at 09:17:55AM +0200, Christophe Fergeau wrote:
 The one we were using does not work properly with clang, causing
 the build process to try to use -f/-W options that are not
 supported by clang.
 ---
 
 The same patch has already been ACKed and pushed in libvirt-glib/libosinfo/...
 https://www.redhat.com/archives/libvir-list/2013-September/msg01599.html
 
 Christophe
 
 
  m4/warnings.m4 | 82 
 --
  1 file changed, 62 insertions(+), 20 deletions(-)

ACK, nobrainer.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [virt-tools-list] More on virt-viewer for windows

On Tue, Sep 17, 2013 at 12:38:03PM -0600, Eric Blake wrote:
 [adding libvir-list, for some cross-compiling development hints]
 
 On 09/17/2013 11:57 AM, Fernando Lozano wrote:
 
  Yes, the libvirt comes grom 0.10.2. I'm running the latest windows
  binaries provided by spice-space.org:
  
  C:\Program Files\VirtViewer\binvirsh -V
  Virsh command line tool of libvirt 0.10.2
 
 Can someone from the Spice community chime in?  Why is spice-space.org
 shipping a Fedora 18 build of libvirt (0.10.2.x) rather than Fedora 19
 (1.0.5.x)?  Who does the builds, and how often are they updated?

I do builds when releasing new virt-viewer versions, and I use the latest
stable Fedora available at that time. We've not done any virt-viewer
release since F19 came out.

There's no reason why I couldn't do bug fix re-builds of the installer
though at any time if deemed neccessary.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [virt-tools-list] Strange behaviour using qemu+ssh on virt-manager

On Tue, Sep 17, 2013 at 02:38:52PM -0300, Fernando Lozano wrote:
 Hi there,
 
 I am experimenting with different security settings for libvirtd, so
 I can give sysadmins administrative access to the KVM hypervisor
 without giving them root access on the host. I had success using TLS
 (with client-certs) and SASL, but have not managed to make polkit
 and ssh to work so far.
 
 If I change /etc/libvirt/libvirtd.conf auth_tcp or auth_unix_rw  a
 local virsh connection gets this error:
 
 Authorization requires authentication but no agent is available
 
 Thus  I'm using sasl for tcp and none for the unix socket.
 
 When I try a qemu+ssh remote virsh connection evething works fine.
 But then I try the same URL using virt-manager, and then try to open
 a guest console, virt-manager prompts multiple times for a ssh login
 password.
 
 Shoudn't virt-manager resue the same ssh connection for guest
 console access? And even if it needs to open a new ssh connection
 for the spice connection, this should require only one additional
 ssh login.
 
 But I tried many times, carefully typing the password each time, and
 I'm sure they were not typos: virt-manager is actually asking for
 the ssh login password many times!
 
 Maybe people who use ssh keys (passwordless) logins didn't notice,
 but I think virt-manager should't require more than one addtional
 ssh connection per guest console. Is this a bug?

Each console rquires that we setup a new SSH tunnel, since every
console is on a different socket on the remote host and we don't
know them all ahead of time.

If you are using SSH for libvirt, it is expected that you setup
SSH agent + public keys, so that you are not prompted for passwords
at all when logging on.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Use system-wide trust certificate store

On Wed, Sep 18, 2013 at 02:40:52PM +0200, Christophe Fergeau wrote:
 Currently, spice-gtk will look in $HOME/.spicec/spice_truststore.pem
 by default for its trust certificate store (to verify the certificates
 used during SPICE TLS connections). However, these days a system-wide
 trust store can be found in /etc/pki or /etc/ssl.
 This commit checks at compile time where the trust store is located,
 and then loads it before loading the user-specified trust store.
 This can be disabled at compile time using --without-ca-certificates.

I'm curious how useful / desirable this actually is. I can see how
it makes total sense to use the global CA bundle if your application
is making HTTPS connections to public internet services, so you have
all the global CA's known.

For SPICE though, users are pretty unlikely to be purchasing certs
from the commercial CA (protection racket) vendors. They'll almost
certainly be using their own internal CA. 

The question is, would they be likely to append their own private
CA onto the list of the global certs ?  I'm somewhat sceptical.

In addition by making SPICE use the global CA cert bundle by default
we're making it much much easier for $evil people to MITM attack any
SPICE connection by getting a valid cert from any CA in that bundle.

Personally I'm not convinced SPICE should use the global CA list
by default.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Use system-wide trust certificate store

On Wed, Sep 18, 2013 at 03:24:36PM +0200, Christophe Fergeau wrote:
 On Wed, Sep 18, 2013 at 02:11:20PM +0100, Daniel P. Berrange wrote:
  For SPICE though, users are pretty unlikely to be purchasing certs
  from the commercial CA (protection racket) vendors. They'll almost
  certainly be using their own internal CA. 
  
  The question is, would they be likely to append their own private
  CA onto the list of the global certs ?  I'm somewhat sceptical.
 
 I wrote this patch while fixing certificate handling in remote-viewer
 ovirt code. When using oVirt, the same CA is used for the web
 portal/REST API and for the SPICE TLS connections. In such a setup, I don't
 think it's unlikely that the private CA will get added to the global certs
 so that the web portals work without warning screens.
 When this happens, this means that remote-viewer will be able to use
 the oVirt REST API without needing to specify any CA, but the SPICE
 connection will fail because no CA will have been set (--spice-ca-file).
 With this patch, REST and SPICE certificate checks will work/fail for the
 same hosts.
 
  Personally I'm not convinced SPICE should use the global CA list
  by default.
 
 For what it's worth, I'm not entirely convinced either that this patch is a
 good idea ;)

At the very least, if we want to use a global CA list, then  if the
user specifies a custom cacert file for SPICE, this should completely
block any use of the global CA list. That ensures users can setup a
strictly locked down setup where they're not exposed to risks of the
commercial CA vendors.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [virt-tools-list] More on virt-viewer for windows

On Wed, Sep 18, 2013 at 08:24:26AM -0600, Eric Blake wrote:
 On 09/18/2013 08:19 AM, Fernando Lozano wrote:
  Hi,
  Can someone from the Spice community chime in? Why is
  spice-space.org shipping a Fedora 18 build of libvirt (0.10.2.x)
  rather than Fedora 19 (1.0.5.x)? Who does the builds, and how often
  are they updated? 
  I do builds when releasing new virt-viewer versions, and I use the latest
  stable Fedora available at that time. We've not done any virt-viewer
  release since F19 came out.
 
  There's no reason why I couldn't do bug fix re-builds of the installer
  though at any time if deemed neccessary.
  It would be nice to do a bug fix rebuild as soon as 0.10.2.8 is
  released, as that would pick up the patch for mingw refusing to create
  sockets, and hopefully get people a lot further at actually being able
  to use virsh on mingw.
  I got confused by those release numbers: if Fedora has libvirt 1.0.5.x
  why should the next windows build use older 0.10.2.8? I though
  development were being done only on the latest releases.
 
 Since Dan builds from Fedora, he gets whatever version is in the Fedora
 side that he builds from.  Fedora 18 is on the 0.10.2.x series, with
 0.10.2.8 due soon; Fedora 19 is on the 1.0.5.x branch, with 1.0.5.6 due
 soon.  Since the original build was from F18 and Dan didn't build
 anything from F19 yet, it may be faster to do the refresh from Fedora 18
 than to figure out whether Fedora 19 is even buildable for mingw.  Or,
 if he builds from the (still-in-beta) Fedora 20, you'd get a build based
 off of 1.1.2.

We have a new virt-viewer release we want to get out soon. I'll be building
on F19 when i do that.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH] Free coroutine stack when releasing coroutine

2013-09-13 Thread Daniel P. Berrange

From: Daniel P. Berrange berra...@redhat.com

The coroutine_init function mmap's a stack for the
ucontext coroutine, but nothing ever munmaps it.

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 gtk/coroutine_ucontext.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gtk/coroutine_ucontext.c b/gtk/coroutine_ucontext.c
index 79e4afe..f65337e 100644
--- a/gtk/coroutine_ucontext.c
+++ b/gtk/coroutine_ucontext.c
@@ -48,6 +48,8 @@ static int _coroutine_release(struct continuation *cc)
return ret;
}
 
+   munmap(co-cc.stack, co-cc.stack_size);
+
co-caller = NULL;
 
return 0;
-- 
1.8.3.1

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

[Spice-devel] [PATCH] Abort if mmap of coroutine stack fails

2013-09-13 Thread Daniel P. Berrange

From: Daniel P. Berrange berra...@redhat.com

If we fail to mmap the stack, abort the processs rather
than returning an error. This is standard practice in
glib apps, and the caller was not checking the
coroutine_init() return code leading to memory corruption.

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 gtk/coroutine_ucontext.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gtk/coroutine_ucontext.c b/gtk/coroutine_ucontext.c
index af811a7..79e4afe 100644
--- a/gtk/coroutine_ucontext.c
+++ b/gtk/coroutine_ucontext.c
@@ -20,6 +20,7 @@
 
 #include config.h
 
+#include glib.h
 #ifdef HAVE_SYS_TYPES_H
 #include sys/types.h
 #endif
@@ -69,7 +70,8 @@ int coroutine_init(struct coroutine *co)
MAP_PRIVATE | MAP_ANONYMOUS,
-1, 0);
if (co-cc.stack == MAP_FAILED)
-   return -1;
+   g_error(Failed to allocate %u bytes for coroutine stack,
+   (unsigned)co-stack_size);
co-cc.entry = coroutine_trampoline;
co-cc.release = _coroutine_release;
co-exited = 0;
-- 
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] server: bitmap_consistent: replace spice_error with spice_warning

2013-09-03 Thread Daniel P. Berrange

On Sun, Sep 01, 2013 at 09:43:17PM +0300, Uri Lublin wrote:
 bitmap_consistent should return true or false.
 Currently it aborts instead of returning false, due to spice_error.
 Replacing spice_error with spice_warning, provides information and returns
 false, as expected.
 
 This fixes Fedora bz#997932

The issue being fixed here is a security flaw, since it allows an
unprivileged users in the guest OS to crash the entire QEMU process
in the host. It is really bad practice to do security fixes without
the commit message explicitly saying that it is a security fix. People
using spice need to know so that they can apply it to any old branches
they may have.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] SPICE_INTERFACE_KEYBOARD - scancodes vs. utf8

2013-08-06 Thread Daniel P. Berrange

On Tue, Aug 06, 2013 at 03:42:29PM +, Dietmar Maurer wrote:
Having utf8 input was discussed a few weeks ago, it is needed for html
and android clients for ex.

Note that this is a completely different problem.

When running a virtual machine you need the scancodes, so you can feed the
guest with the correct virtual keystrokes. Translating those keystrokes
into
characters is the job of the guest operating system.
There is no meaningful way to pass a utf-8 character stream into a virtual
machine.

So a spice client must somehow figure how to translate the input it gets
into
scancodes. And IMO this should best be done on the client side so the
client can
use all information it has to do it. For gtk this is easy as the key
events carry not
only the character but the keycode too.
For android/html5 clients this is alot trickier ...

AFAIK, neither java nor javascript have access to scancodes. If you solve the
problem
on the server side (spice server library), you need to solve it only once.
Else you need
to implement that clumsy code on each client.

If I remember correctly, qemu already has some code to convert VNC input to
scancodes?

Yes, but that code is broken by design, which is why we invented the raw
scancode extension to VNC, and why SPICE uses scancodes from the start.
It is not possible to round-trip from a character/key symbol, to a hw
cancode back to a character/key symbol without irretrievably loosing
information. eg the key sym you put in from the client will not match
the one Xorg eventually gets in the guest.

It also requires that you manually configure a keycode mapping table
in the QEMU binary itself at startup, to match the keycode mapping in
the guest and the mapping in the client. This obviously means that all
clients must use the same keycode map that was chosen at the time QEMU
started.

I wrote a bit about this awfulness here

https://www.berrange.com/posts/2010/07/04/more-than-you-or-i-ever-wanted-to-know-about-virtual-keyboard-handling/

https://www.berrange.com/posts/2010/07/04/a-summary-of-scan-code-key-codes-sets-used-in-the-pc-virtualization-stack/

Perhaps this brokenness is unavoidable with spice android/html5 clients,
but you should be aware that if you go down that route, you are picking
a solution which is unfixably broken.

The only other option you'd have is to take QEMU's keyboard emulation
out of the picture entirely. Define a spice guest agent extension that
lets you pass key symbols straight to the guest agent, which can give
them to the windowing system without any translation steps involved.
Obviously this wouldn't be much help during intial VM bootup, but maybe
that's an acceptable tradeoff ?

Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] allow config download with https

2013-07-26 Thread Daniel P. Berrange

On Fri, Jul 26, 2013 at 11:26:24AM +, Dietmar Maurer wrote:
  In my opinion, we could support running 'remote-viewer
  https://proxmox.example.com/myvm.vv', 
 
 Well, but we need to handle basic auth.
 
 I guess gio/gvfs should be able to
  fetch files from http[s] (no libcurl please ;). Is it what you (Dietmar) are
  looking for?
 
 Why gvfs? libcurl look much simpler.

I'm not sure I agree with that. Using GVFS/GIO gives us code that
is completely protocol agnostic, it can be used with plain local
files, just as easily as with arbitrary URIs (whether HTPT, FTP,
SSH, or any other GVFS scheme). I'd not want to see libcurl used
unless there is some blocking technical limitation of GVFS that
can't be resolved.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] why getScanCode() will not work for keyboard input

2013-06-25 Thread Daniel P. Berrange

On Tue, Jun 25, 2013 at 01:26:46PM +0200, Marc-André Lureau wrote:
 Hi
 
 On Mon, Jun 24, 2013 at 8:08 AM, i iordanov iiorda...@gmail.com wrote:
  Hi Marc-Andre,
 
  (And anybody else who wants to weigh in on this). After some research,
  reading and experimentation, here is what I've discovered.
 
  getScanCode() works only for physical devices (e.g. USB keyboards, bluetooth
  keyboards, or hardware device keyboards). The vast majority of input devices
  on Android are soft-keyboards which generate KeyEvents with invalid or
  missing scan codes.
 
  Such KeyEvents also don't even have a keycode. In such events, Android sets
  the keycode to KEYCODE_UNKNOWN. In addition, a KeyEvent is quite often a
  sequence of unicode characters rather than a single unicode character.
 
 That makes sense. It would still be nice to handle physical devices
 with their keycode.
 
  What *can* be obtained from every KeyEvent, however, is one or more unicode
  characters with the getUnicodeChar() and getCharacters() functions
  respectively.. For VNC purposes, what I do in bVNC is to convert these
  unicode characters to X11 keysym values, which are then passed to the VNC
  server. These keysyms are not just the ones that correspond to the XT
  keyboard keys, but a much larger set which maps pretty much every unicode
  character to a keysym.
 
  Do you guys see any way in which we can handle this situation where the
  input is not as straight forward as an XT keyboard?
 
  What would work is if mobile clients were able to either send unicode
  characters or X11 keysyms to the server.
 
  Does anybody else have any better ideas?
 
 Probably Spice server and agents should learn to handle arbitrary
 input in unicode (that would also help html5 client support)
 
 However, this needs to be carefully designed, so that hw keycode are
 still issued/interpreted to the guest when the remote doesn't handle
 unicode directly.

The problem with passing around anything that isn't scancodes is
that ultimately when QEMU injects the key events to the virtual
machine, it needs to turn them into hardware scancodes. This is
an inherantly lossy conversion.

Perhaps a way around this problem is to take QEMU  virtual
keyboard hardware out of the loop, and use the spice guest agent
to inject the key event input directly to the windowing system as
unicode characters or key symbols ?

IOW, passing scancodes to QEMU would only be needed during bootup,
once spice agent was running it would take over injecting key events ?
Not sure how this would work if the user had started Xorg and spice
agent, and then switched to a linux virtual console.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] automake: Disable portability warnings

On Mon, Jun 24, 2013 at 11:27:54AM +0200, Christophe Fergeau wrote:
 -Wportability used to be automatically disabled with automake = 1.12 when 
 using
 silent rules, but this is no longer the case with automake 1.13 which is what
 fedora 19 uses: 
 http://www.flameeyes.eu/autotools-mythbuster/automake/silent.html
 As of version 1.13, though, this opt-in is no longer necessary, as all the
 Makefiles are generated to support them. The silent-rules option is now a
 no-op, doing nothing at all, in particular not silencing the portability
 warnings.
 
 This commit disables these warnings in order to avoid autogen.sh breakage
 because of the use of -Werror, they can be reenabled once gtk-doc.make is
 fixed to avoid these portability warnings.
 ---
  configure.ac | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/configure.ac b/configure.ac
 index 8ab5b6b..bd59ccf 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -7,7 +7,7 @@ AC_CONFIG_MACRO_DIR([m4])
  AC_CONFIG_HEADER([config.h])
  AC_CONFIG_AUX_DIR([build-aux])
  
 -AM_INIT_AUTOMAKE([foreign dist-bzip2 -Wall -Werror])
 +AM_INIT_AUTOMAKE([foreign dist-bzip2 -Wall -Werror -Wno-portability])
  m4_ifdef([AM_PROG_AR], [AM_PROG_AR])
  LT_INIT
  AM_MAINTAINER_MODE

ACK

Use of -Wno-portability is pretty standard practice

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)

On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote:
 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
  gtk/channel-main.c | 73 
 +++---
  1 file changed, 69 insertions(+), 4 deletions(-)

IIUC, implicit in your patch here is that the communication protocol
between spice client  guest agent uses guest OS specific line ending
markers, and the spice client must do all conversions to guest OS
format. Is that really what we want ?

Personally I would have said that the line ending markers should be
invariant (always \n) in the comms protocol and that each end point
must handle conversion to/from \r\n if required.

But perhaps there are backwards compat issues forcing your choice
here ?

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)

On Mon, Jun 24, 2013 at 02:45:25PM +0200, Hans de Goede wrote:
 Hi,
 
 On 06/24/2013 02:39 PM, Daniel P. Berrange wrote:
 On Mon, Jun 24, 2013 at 02:31:27PM +0200, Hans de Goede wrote:
 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
   gtk/channel-main.c | 73 
  +++---
   1 file changed, 69 insertions(+), 4 deletions(-)
 
 IIUC, implicit in your patch here is that the communication protocol
 between spice client  guest agent uses guest OS specific line ending
 markers, and the spice client must do all conversions to guest OS
 format. Is that really what we want ?
 
 Yes, this is by design. The reason for this is that there are no real
 conventions as to what line-endings to use inside the clipboard data,
 so ie the clipboard on unix (X) may contain text data with CRLF
 line-endings. And some programs may depend on this.
 
 So to minimize the change of breaking things we don't want to do any
 conversion when doing copy and paste between a unix guest and unix client,
 or a windows guest and windows client.
 
 Personally I would have said that the line ending markers should be
 invariant (always \n) in the comms protocol and that each end point
 must handle conversion to/from \r\n if required.
 
 That was my initial design too, but see above.
 
 But perhaps there are backwards compat issues forcing your choice
 here ?
 
 Well currently we don't do any conversions, so keeping the wire
 format in guest encoding makes things a bit easier wrt backward
 compat handling, but we could have moved to always use \n on the
 wire using guest capabilities bits, so that is not the main reason
 for this.

Ok, that makes sense.

Would be nice to describe this rationale in the commit message for
benefit of people looking back at this change years down the road.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] channel-main: Convert text line-endings if necessary (rhbz#752350)