Re: [Spice-devel] [PATCH] RedChannelClient: use Gobject properties

[Jonathon Jongsma]

On Mon, 2016-11-07 at 12:16 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Fri, Nov 04, 2016 at 11:19:22AM -0500, Jonathon Jongsma wrote:
> > > 
> > > Use g_param_spec_object() instead of g_param_spec_pointer() for
> > > the
> > > 'client' and 'channel' properties now that these types are
> > > GObjects.
> > > This improves refcounting and typesafety slightly.
> > > ---
> > >  server/red-channel-client.c | 32 ---
> > > -
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/server/red-channel-client.c b/server/red-channel-
> > > client.c
> > > index 84dd28f..ad4a545 100644
> > > --- a/server/red-channel-client.c
> > > +++ b/server/red-channel-client.c
> > > @@ -150,10 +150,10 @@ red_channel_client_get_property(GObject
> > > *object,
> > >  g_value_set_pointer(value, self->priv->stream);
> > >  break;
> > >  case PROP_CHANNEL:
> > > -g_value_set_pointer(value, self->priv->channel);
> > > +g_value_set_object(value, self->priv->channel);
> > 
> > If something is using getters, this will introduce a leak, but I
> > could
> > not find anything doing g_object_get(..., "channel", ...)
> > 


good point. I think everything currently uses the convenience function
red_channel_client_get_channel(), so they won't see any change due to
this patch. 

> 
> Probably this property can be write-only + construct so we don't
> need to get it?
> (not a regression by the way, just I noted that one of the last
> patch introduced such a kind of property)

I think it would be a bit weird to make this property write-only when
we provide a convenience function to get the property value
(red_channel_client_get_channel()). Perhaps the bigger question is
whether we should change the _get_channel() function to be have like
g_object_get() and adds a reference to the channel. that would
obviously require more work to change all callers to not leak.

> 
> > 
> > > 
> > >  break;
> > >  case PROP_CLIENT:
> > > -g_value_set_pointer(value, self->priv->client);
> > > +g_value_set_object(value, self->priv->client);
> > >  break;
> > >  case PROP_MONITOR_LATENCY:
> > >  g_value_set_boolean(value, self->priv-
> > > >monitor_latency);
> > > @@ -195,12 +195,10 @@ red_channel_client_set_property(GObject
> > > *object,
> > >  case PROP_CHANNEL:
> > >  if (self->priv->channel)
> > >  g_object_unref(self->priv->channel);
> > > -self->priv->channel = g_value_get_pointer(value);
> > > -if (self->priv->channel)
> > > -g_object_ref(self->priv->channel);
> > > +self->priv->channel = g_value_dup_object(value);
> > >  break;
> > >  case PROP_CLIENT:
> > > -self->priv->client = g_value_get_pointer(value);
> > > +self->priv->client = g_value_get_object(value);
> > 
> > Just to be sure, we don't want to take a ref on the client?
> > 
> 
> Good question. More related to reference counting talk
> than this rationale.
> Currently behavior is if MainChannelClient is closed all clients
> (RedClient and others RedChannelClients) are destroyed.
> But for the other RedChannelClients a strong pointer to RedClient
> make sense. Not sure if RedClient currently has a strong pointer
> to MainChannelClient, surely a RedClient cannot exist without
> a MainChannelClient.

Yeah, it's difficult to say exactly what should happen here, but I
didn't want to change ownership as part of this patch. We probably
should review this as part of our other ownership discussions.

> 
> > 
> > Looks good to me as is.
> > 
> > Acked-by: Christophe Fergeau 
> > 
> > Christophe
> > 
> 
> Agree
> 
> Frediano
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [qxl] spiceqxl: Improve the Xspice and Xorg configuration option descriptions

[Frediano Ziglio]

> 
> On Mon, Oct 31, 2016 at 09:50:54PM +0100, Francois Gouget wrote:
> > Group the options more logically and improve their descriptions.
> > In the Xorg configuration, always show the default in the
> > commented-out sample.
> > 
> > Signed-off-by: Francois Gouget 
> > ---
> >  examples/spiceqxl.xorg.conf.example | 166
> >  +++-
> >  scripts/Xspice  |  57 ++---
> >  2 files changed, 117 insertions(+), 106 deletions(-)
> > 
> > diff --git a/scripts/Xspice b/scripts/Xspice
> > index bf8112f..6d1afaf 100755
> > --- a/scripts/Xspice
> > +++ b/scripts/Xspice
> > @@ -59,37 +59,37 @@ parser.add_argument('--xsession', help='If given, will
> > run after Xorg launch.  S
> >  parser.add_argument('--config', default='spiceqxl.xorg.conf')
> >  # Don't use any options that are already used by Xorg (unless we must)
> >  # specifically, don't use -p and -s.
> > -parser.add_argument('--port', type=int, help='standard spice port')
> > +parser.add_argument('--port', type=int, help="Use the specified port as
> > Spice's regular unencrypted port.")
> > +parser.add_argument('--tls-port', type=int, help='Use the specified port
> > as a TLS (encrypted) port.', default=0)
> > +add_boolean('--disable-ticketing', help="Do not require a client
> > password.")
> > +parser.add_argument('--password', help="Set the password required to
> > connect to the server.")
> > +add_boolean('--sasl', help="Use SASL to authenticate to the server.")
> > +parser.add_argument('--x509-dir', help="Set the directory where the CA
> > certificate, server key and server certificate are searched for TLS, using
> > the same predefined names QEMU uses.")
> > +parser.add_argument('--cacert-file', help="Set the CA certificate file
> > location for TLS.")
> > +parser.add_argument('--x509-key-file', help="Set the server key file
> > location for TLS.")
> > +parser.add_argument('--x509-key-password', help="Set the server key's
> > password for TLS.")
> > +parser.add_argument('--x509-cert-file', help="Set the server certificate
> > file location for TLS.")
> > +parser.add_argument('--dh-file', help="Set the server DH file location for
> > TLS.")
> > +parser.add_argument('--tls-ciphers', help="Set the TLS ciphers preference
> > order.")
> > +add_boolean('--ipv4-only')
> > +add_boolean('--ipv6-only')
> 
> No help for these?
> 
> >  parser.add_argument('--exit-on-disconnect', action='store_true',
> >  help='Exit the X server when any client disconnects')
> >  parser.add_argument('--numheads', type=int, help='Number of virtual heads
> >  to create.')
> > -parser.add_argument('--deferred-fps', type=int, help='If given, render to
> > a buffer and send updates only this many times per second')
> > -parser.add_argument('--tls-port', type=int, help='spice tls port',
> > default=0)
> > -add_boolean('--disable-ticketing', help="do not require a client
> > password")
> > -add_boolean('--sasl', help="enable sasl")
> > -parser.add_argument('--x509-dir', help="x509 directory for tls")
> > -parser.add_argument('--cacert-file', help="ca certificate file for tls")
> > -parser.add_argument('--x509-cert-file', help="server certificate file for
> > tls")
> > -parser.add_argument('--x509-key-file', help="server key file for tls")
> > -parser.add_argument('--x509-key-password', help="key file password for
> > tls")
> > -parser.add_argument('--tls-ciphers')
> > -parser.add_argument('--dh-file')
> > -parser.add_argument('--password', help="set password required to connect
> > to server")
> >  parser.add_argument('--image-compression',
> >  choices = ['off', 'auto_glz', 'auto_lz', 'quic',
> > 'glz', 'lz'],
> > -help='auto_glz by default')
> > +help="Set image compression.")
> >  parser.add_argument('--jpeg-wan-compression',
> >  choices=wan_compression_options,
> > -help='auto by default')
> > +help="Set jpeg wan compression.")
> >  parser.add_argument('--zlib-glz-wan-compression',
> >  choices=wan_compression_options,
> > -help='auto by default')
> > +help="Set zlib glz wan compression.")
> > +parser.add_argument('--deferred-fps', type=int, help='If non zero, the
> > driver will render all operations to the frame buffer, and keep track of a
> > changed rectangle list. The changed rectangles will be transmitted at the
> > rate requested (e.g. 10 frames per second). This can dramatically reduce
> > network bandwidth for some use cases.')
> >  # TODO - sound support
> >  parser.add_argument('--streaming-video', choices=['off', 'all', 'filter'],
> > -help='filter by default')
> > -parser.add_argument('--video-codecs', help="Sets a semicolon-separated
> > list of preferred video codecs to use. Each takes the form encoder:codec,
> > with spice:mjpeg being the default and other 

Re: [Spice-devel] [qxl] spiceqxl: Improve the Xspice and Xorg configuration option descriptions

[Christophe Fergeau]

On Mon, Oct 31, 2016 at 09:50:54PM +0100, Francois Gouget wrote:
> Group the options more logically and improve their descriptions.
> In the Xorg configuration, always show the default in the
> commented-out sample.
> 
> Signed-off-by: Francois Gouget 
> ---
>  examples/spiceqxl.xorg.conf.example | 166 
> +++-
>  scripts/Xspice  |  57 ++---
>  2 files changed, 117 insertions(+), 106 deletions(-)
> 
> diff --git a/scripts/Xspice b/scripts/Xspice
> index bf8112f..6d1afaf 100755
> --- a/scripts/Xspice
> +++ b/scripts/Xspice
> @@ -59,37 +59,37 @@ parser.add_argument('--xsession', help='If given, will 
> run after Xorg launch.  S
>  parser.add_argument('--config', default='spiceqxl.xorg.conf')
>  # Don't use any options that are already used by Xorg (unless we must)
>  # specifically, don't use -p and -s.
> -parser.add_argument('--port', type=int, help='standard spice port')
> +parser.add_argument('--port', type=int, help="Use the specified port as 
> Spice's regular unencrypted port.")
> +parser.add_argument('--tls-port', type=int, help='Use the specified port as 
> a TLS (encrypted) port.', default=0)
> +add_boolean('--disable-ticketing', help="Do not require a client password.")
> +parser.add_argument('--password', help="Set the password required to connect 
> to the server.")
> +add_boolean('--sasl', help="Use SASL to authenticate to the server.")
> +parser.add_argument('--x509-dir', help="Set the directory where the CA 
> certificate, server key and server certificate are searched for TLS, using 
> the same predefined names QEMU uses.")
> +parser.add_argument('--cacert-file', help="Set the CA certificate file 
> location for TLS.")
> +parser.add_argument('--x509-key-file', help="Set the server key file 
> location for TLS.")
> +parser.add_argument('--x509-key-password', help="Set the server key's 
> password for TLS.")
> +parser.add_argument('--x509-cert-file', help="Set the server certificate 
> file location for TLS.")
> +parser.add_argument('--dh-file', help="Set the server DH file location for 
> TLS.")
> +parser.add_argument('--tls-ciphers', help="Set the TLS ciphers preference 
> order.")
> +add_boolean('--ipv4-only')
> +add_boolean('--ipv6-only')

No help for these?

>  parser.add_argument('--exit-on-disconnect', action='store_true', help='Exit 
> the X server when any client disconnects')
>  parser.add_argument('--numheads', type=int, help='Number of virtual heads to 
> create.')
> -parser.add_argument('--deferred-fps', type=int, help='If given, render to a 
> buffer and send updates only this many times per second')
> -parser.add_argument('--tls-port', type=int, help='spice tls port', default=0)
> -add_boolean('--disable-ticketing', help="do not require a client password")
> -add_boolean('--sasl', help="enable sasl")
> -parser.add_argument('--x509-dir', help="x509 directory for tls")
> -parser.add_argument('--cacert-file', help="ca certificate file for tls")
> -parser.add_argument('--x509-cert-file', help="server certificate file for 
> tls")
> -parser.add_argument('--x509-key-file', help="server key file for tls")
> -parser.add_argument('--x509-key-password', help="key file password for tls")
> -parser.add_argument('--tls-ciphers')
> -parser.add_argument('--dh-file')
> -parser.add_argument('--password', help="set password required to connect to 
> server")
>  parser.add_argument('--image-compression',
>  choices = ['off', 'auto_glz', 'auto_lz', 'quic',
> 'glz', 'lz'],
> -help='auto_glz by default')
> +help="Set image compression.")
>  parser.add_argument('--jpeg-wan-compression',
>  choices=wan_compression_options,
> -help='auto by default')
> +help="Set jpeg wan compression.")
>  parser.add_argument('--zlib-glz-wan-compression',
>  choices=wan_compression_options,
> -help='auto by default')
> +help="Set zlib glz wan compression.")
> +parser.add_argument('--deferred-fps', type=int, help='If non zero, the 
> driver will render all operations to the frame buffer, and keep track of a 
> changed rectangle list. The changed rectangles will be transmitted at the 
> rate requested (e.g. 10 frames per second). This can dramatically reduce 
> network bandwidth for some use cases.')
>  # TODO - sound support
>  parser.add_argument('--streaming-video', choices=['off', 'all', 'filter'],
> -help='filter by default')
> -parser.add_argument('--video-codecs', help="Sets a semicolon-separated list 
> of preferred video codecs to use. Each takes the form encoder:codec, with 
> spice:mjpeg being the default and other options being provided by gstreamer 
> for the mjpeg, vp8 and h264 codecs.")
> -add_boolean('--ipv4-only')
> -add_boolean('--ipv6-only')
> +help='Set the streaming video 

Re: [Spice-devel] [vd_agent] vdagentd: Clean up and clarify the usage message

[Christophe Fergeau]

On Wed, Nov 02, 2016 at 04:38:05PM +0100, Francois Gouget wrote:
> udsc is an internal library of sorts that's irrelevant to users.

I've split this in 2 commits (the change which is described in the log +
one with the cosmetic changes) and pushed this.

Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Francois Gouget 
> ---
>  src/vdagentd/vdagentd.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 13a84ce..18e82a7 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -797,16 +797,16 @@ static void usage(FILE *fp)
>  "  -h print this text\n"
>  "  -d log debug messages (use twice for extra 
> info)\n"
>  "  -s   set virtio serial port  [%s]\n"
> -"  -S   set udcs socket [%s]\n"
> +"  -S   set vdagent Unix domain socket [%s]\n"
>  "  -uset uinput device   [%s]\n"
>  "  -f treat uinput device as fake; no ioctls\n"
>  "  -x don't daemonize\n"
> -"  -o Only handle one virtio serial session.\n"
> +"  -o only handle one virtio serial session\n"
>  #ifdef HAVE_CONSOLE_KIT
> -"  -X Disable console kit integration\n"
> +"  -X disable console kit integration\n"
>  #endif
>  #ifdef HAVE_LIBSYSTEMD_LOGIN
> -"  -X Disable systemd-logind integration\n"
> +"  -X disable systemd-logind integration\n"
>  #endif
>  ,VERSION, portdev, vdagentd_socket, uinput_device);
>  }
> -- 
> 2.10.1
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [vd_agent] vdagentd: Make the 'could not create server socket' message more helpful

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Wed, Nov 02, 2016 at 04:36:40PM +0100, Francois Gouget wrote:
> EADDRINUSE usually means the socket file was left behind after a crash.
> 
> Signed-off-by: Francois Gouget 
> ---
>  src/vdagentd/vdagentd.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 8dee414..13a84ce 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -970,8 +970,13 @@ int main(int argc, char *argv[])
>   vdagentd_messages, VDAGENTD_NO_MESSAGES,
>   debug);
>  if (!server) {
> -syslog(LOG_CRIT, "Fatal could not create server socket %s",
> -   vdagentd_socket);
> +if (errno == EADDRINUSE) {
> +syslog(LOG_CRIT, "Fatal the server socket %s exists already. 
> Delete it?",
> +   vdagentd_socket);
> +} else {
> +syslog(LOG_CRIT, "Fatal could not create the server socket %s: 
> %m",
> +   vdagentd_socket);
> +}
>  return 1;
>  }
>  if (chmod(vdagentd_socket, 0666)) {
> -- 
> 2.10.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH] RedChannelClient: use Gobject properties

[Frediano Ziglio]

> 
> On Fri, Nov 04, 2016 at 11:19:22AM -0500, Jonathon Jongsma wrote:
> > Use g_param_spec_object() instead of g_param_spec_pointer() for the
> > 'client' and 'channel' properties now that these types are GObjects.
> > This improves refcounting and typesafety slightly.
> > ---
> >  server/red-channel-client.c | 32 
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 84dd28f..ad4a545 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -150,10 +150,10 @@ red_channel_client_get_property(GObject *object,
> >  g_value_set_pointer(value, self->priv->stream);
> >  break;
> >  case PROP_CHANNEL:
> > -g_value_set_pointer(value, self->priv->channel);
> > +g_value_set_object(value, self->priv->channel);
> 
> If something is using getters, this will introduce a leak, but I could
> not find anything doing g_object_get(..., "channel", ...)
> 

Probably this property can be write-only + construct so we don't
need to get it?
(not a regression by the way, just I noted that one of the last
patch introduced such a kind of property)

> >  break;
> >  case PROP_CLIENT:
> > -g_value_set_pointer(value, self->priv->client);
> > +g_value_set_object(value, self->priv->client);
> >  break;
> >  case PROP_MONITOR_LATENCY:
> >  g_value_set_boolean(value, self->priv->monitor_latency);
> > @@ -195,12 +195,10 @@ red_channel_client_set_property(GObject *object,
> >  case PROP_CHANNEL:
> >  if (self->priv->channel)
> >  g_object_unref(self->priv->channel);
> > -self->priv->channel = g_value_get_pointer(value);
> > -if (self->priv->channel)
> > -g_object_ref(self->priv->channel);
> > +self->priv->channel = g_value_dup_object(value);
> >  break;
> >  case PROP_CLIENT:
> > -self->priv->client = g_value_get_pointer(value);
> > +self->priv->client = g_value_get_object(value);
> 
> Just to be sure, we don't want to take a ref on the client?
> 

Good question. More related to reference counting talk
than this rationale.
Currently behavior is if MainChannelClient is closed all clients
(RedClient and others RedChannelClients) are destroyed.
But for the other RedChannelClients a strong pointer to RedClient
make sense. Not sure if RedClient currently has a strong pointer
to MainChannelClient, surely a RedClient cannot exist without
a MainChannelClient.

> Looks good to me as is.
> 
> Acked-by: Christophe Fergeau 
> 
> Christophe
> 

Agree

Frediano

> >  break;
> >  case PROP_MONITOR_LATENCY:
> >  self->priv->monitor_latency = g_value_get_boolean(value);
> > @@ -312,18 +310,20 @@ static void
> > red_channel_client_class_init(RedChannelClientClass *klass)
> >  | G_PARAM_CONSTRUCT_ONLY);
> >  g_object_class_install_property(object_class, PROP_STREAM, spec);
> >  
> > -spec = g_param_spec_pointer("channel", "channel",
> > -"Associated RedChannel",
> > -G_PARAM_STATIC_STRINGS
> > -| G_PARAM_READWRITE
> > -| G_PARAM_CONSTRUCT_ONLY);
> > +spec = g_param_spec_object("channel", "channel",
> > +   "Associated RedChannel",
> > +   RED_TYPE_CHANNEL,
> > +   G_PARAM_STATIC_STRINGS
> > +   | G_PARAM_READWRITE
> > +   | G_PARAM_CONSTRUCT_ONLY);
> >  g_object_class_install_property(object_class, PROP_CHANNEL, spec);
> >  
> > -spec = g_param_spec_pointer("client", "client",
> > -"Associated RedClient",
> > -G_PARAM_STATIC_STRINGS
> > -| G_PARAM_READWRITE
> > -| G_PARAM_CONSTRUCT_ONLY);
> > +spec = g_param_spec_object("client", "client",
> > +   "Associated RedClient",
> > +   RED_TYPE_CLIENT,
> > +   G_PARAM_STATIC_STRINGS
> > +   | G_PARAM_READWRITE
> > +   | G_PARAM_CONSTRUCT_ONLY);
> >  g_object_class_install_property(object_class, PROP_CLIENT, spec);
> >  
> >  spec = g_param_spec_boolean("monitor-latency", "monitor-latency",
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] RedChannelClient: use Gobject properties

[Christophe Fergeau]

On Fri, Nov 04, 2016 at 11:19:22AM -0500, Jonathon Jongsma wrote:
> Use g_param_spec_object() instead of g_param_spec_pointer() for the
> 'client' and 'channel' properties now that these types are GObjects.
> This improves refcounting and typesafety slightly.
> ---
>  server/red-channel-client.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 84dd28f..ad4a545 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -150,10 +150,10 @@ red_channel_client_get_property(GObject *object,
>  g_value_set_pointer(value, self->priv->stream);
>  break;
>  case PROP_CHANNEL:
> -g_value_set_pointer(value, self->priv->channel);
> +g_value_set_object(value, self->priv->channel);

If something is using getters, this will introduce a leak, but I could
not find anything doing g_object_get(..., "channel", ...)

>  break;
>  case PROP_CLIENT:
> -g_value_set_pointer(value, self->priv->client);
> +g_value_set_object(value, self->priv->client);
>  break;
>  case PROP_MONITOR_LATENCY:
>  g_value_set_boolean(value, self->priv->monitor_latency);
> @@ -195,12 +195,10 @@ red_channel_client_set_property(GObject *object,
>  case PROP_CHANNEL:
>  if (self->priv->channel)
>  g_object_unref(self->priv->channel);
> -self->priv->channel = g_value_get_pointer(value);
> -if (self->priv->channel)
> -g_object_ref(self->priv->channel);
> +self->priv->channel = g_value_dup_object(value);
>  break;
>  case PROP_CLIENT:
> -self->priv->client = g_value_get_pointer(value);
> +self->priv->client = g_value_get_object(value);

Just to be sure, we don't want to take a ref on the client?

Looks good to me as is.

Acked-by: Christophe Fergeau 

Christophe

>  break;
>  case PROP_MONITOR_LATENCY:
>  self->priv->monitor_latency = g_value_get_boolean(value);
> @@ -312,18 +310,20 @@ static void 
> red_channel_client_class_init(RedChannelClientClass *klass)
>  | G_PARAM_CONSTRUCT_ONLY);
>  g_object_class_install_property(object_class, PROP_STREAM, spec);
>  
> -spec = g_param_spec_pointer("channel", "channel",
> -"Associated RedChannel",
> -G_PARAM_STATIC_STRINGS
> -| G_PARAM_READWRITE
> -| G_PARAM_CONSTRUCT_ONLY);
> +spec = g_param_spec_object("channel", "channel",
> +   "Associated RedChannel",
> +   RED_TYPE_CHANNEL,
> +   G_PARAM_STATIC_STRINGS
> +   | G_PARAM_READWRITE
> +   | G_PARAM_CONSTRUCT_ONLY);
>  g_object_class_install_property(object_class, PROP_CHANNEL, spec);
>  
> -spec = g_param_spec_pointer("client", "client",
> -"Associated RedClient",
> -G_PARAM_STATIC_STRINGS
> -| G_PARAM_READWRITE
> -| G_PARAM_CONSTRUCT_ONLY);
> +spec = g_param_spec_object("client", "client",
> +   "Associated RedClient",
> +   RED_TYPE_CLIENT,
> +   G_PARAM_STATIC_STRINGS
> +   | G_PARAM_READWRITE
> +   | G_PARAM_CONSTRUCT_ONLY);
>  g_object_class_install_property(object_class, PROP_CLIENT, spec);
>  
>  spec = g_param_spec_boolean("monitor-latency", "monitor-latency",
> -- 
> 2.7.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice] spice-options-test: Convert to gtest to catch criticals

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Thu, Nov 03, 2016 at 01:30:07PM +0100, Pavel Grunt wrote:
> Fail on glib warnings instead of ignoring them
> 
> Signed-off-by: Pavel Grunt 
> ---
>  server/tests/spice-options-test.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/server/tests/spice-options-test.c 
> b/server/tests/spice-options-test.c
> index e762461..72ec215 100644
> --- a/server/tests/spice-options-test.c
> +++ b/server/tests/spice-options-test.c
> @@ -26,7 +26,7 @@
>  #define g_assert_nonnull g_assert
>  #endif
>  
> -int main(int argc, char *argv[])
> +static void agent_options(void)
>  {
>  SpiceCoreInterface *core ;
>  SpiceServer *server = spice_server_new();
> @@ -49,6 +49,13 @@ int main(int argc, char *argv[])
>  spice_server_set_agent_file_xfer(server, 0);
>  
>  spice_server_destroy(server);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +g_test_init(, , NULL);
> +
> +g_test_add_func("/server/agent options", agent_options);
>  
> -return 0;
> +return g_test_run();
>  }
> -- 
> 2.10.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 3/3] Move InputsChannelClient and InputsChannelClient definition to C file

[Christophe Fergeau]

Duplicate InputsChannelClient in the short log?
This also seems to be removing InputsChannelClientPrivate, it would be
worth a mention in the log.
Also, what is this bringing us in concrete terms? Ensuring internal
users of InputsChannelClient can't try to inherit from it?

Christophe

On Mon, Nov 07, 2016 at 11:13:23AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/inputs-channel-client.c | 28 +++-
>  server/inputs-channel-client.h | 13 -
>  2 files changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/server/inputs-channel-client.c b/server/inputs-channel-client.c
> index 4ab2457..7cb920f 100644
> --- a/server/inputs-channel-client.c
> +++ b/server/inputs-channel-client.c
> @@ -22,26 +22,28 @@
>  #include "migration-protocol.h"
>  #include "red-channel-client.h"
>  
> -G_DEFINE_TYPE(InputsChannelClient, inputs_channel_client, 
> RED_TYPE_CHANNEL_CLIENT)
> +struct InputsChannelClient
> +{
> +RedChannelClient parent;
>  
> -#define INPUTS_CHANNEL_CLIENT_PRIVATE(o) \
> -(G_TYPE_INSTANCE_GET_PRIVATE((o), TYPE_INPUTS_CHANNEL_CLIENT, 
> InputsChannelClientPrivate))
> +uint16_t motion_count;
> +};
>  
> -struct InputsChannelClientPrivate
> +struct InputsChannelClientClass
>  {
> -uint16_t motion_count;
> +RedChannelClientClass parent_class;
>  };
>  
> +G_DEFINE_TYPE(InputsChannelClient, inputs_channel_client, 
> RED_TYPE_CHANNEL_CLIENT)
> +
>  static void
>  inputs_channel_client_class_init(InputsChannelClientClass *klass)
>  {
> -g_type_class_add_private(klass, sizeof(InputsChannelClientPrivate));
>  }
>  
>  static void
>  inputs_channel_client_init(InputsChannelClient *self)
>  {
> -self->priv = INPUTS_CHANNEL_CLIENT_PRIVATE(self);
>  }
>  
>  RedChannelClient* inputs_channel_client_create(RedChannel *channel,
> @@ -93,16 +95,16 @@ void 
> inputs_channel_client_send_migrate_data(RedChannelClient *rcc,
>  
>  spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_INPUTS_MAGIC);
>  spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_INPUTS_VERSION);
> -spice_marshaller_add_uint16(m, icc->priv->motion_count);
> +spice_marshaller_add_uint16(m, icc->motion_count);
>  }
>  
>  void inputs_channel_client_handle_migrate_data(InputsChannelClient *icc,
> uint16_t motion_count)
>  {
> -icc->priv->motion_count = motion_count;
> +icc->motion_count = motion_count;
>  
> -for (; icc->priv->motion_count >= SPICE_INPUT_MOTION_ACK_BUNCH;
> -   icc->priv->motion_count -= SPICE_INPUT_MOTION_ACK_BUNCH) {
> +for (; icc->motion_count >= SPICE_INPUT_MOTION_ACK_BUNCH;
> +   icc->motion_count -= SPICE_INPUT_MOTION_ACK_BUNCH) {
>  red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(icc),
>   RED_PIPE_ITEM_MOUSE_MOTION_ACK);
>  }
> @@ -112,10 +114,10 @@ void 
> inputs_channel_client_on_mouse_motion(InputsChannelClient *icc)
>  {
>  InputsChannel *inputs_channel = 
> INPUTS_CHANNEL(red_channel_client_get_channel(RED_CHANNEL_CLIENT(icc)));
>  
> -if (++icc->priv->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0 &&
> +if (++icc->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0 &&
>  !inputs_channel_is_src_during_migrate(inputs_channel)) {
>  red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(icc),
>   RED_PIPE_ITEM_MOUSE_MOTION_ACK);
> -icc->priv->motion_count = 0;
> +icc->motion_count = 0;
>  }
>  }
> diff --git a/server/inputs-channel-client.h b/server/inputs-channel-client.h
> index 7550b3c..6d4ab98 100644
> --- a/server/inputs-channel-client.h
> +++ b/server/inputs-channel-client.h
> @@ -40,19 +40,6 @@ G_BEGIN_DECLS
>  
>  typedef struct InputsChannelClient InputsChannelClient;
>  typedef struct InputsChannelClientClass InputsChannelClientClass;
> -typedef struct InputsChannelClientPrivate InputsChannelClientPrivate;
> -
> -struct InputsChannelClient
> -{
> -RedChannelClient parent;
> -
> -InputsChannelClientPrivate *priv;
> -};
> -
> -struct InputsChannelClientClass
> -{
> -RedChannelClientClass parent_class;
> -};
>  
>  GType inputs_channel_client_get_type(void) G_GNUC_CONST;
>  
> -- 
> 2.7.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 1/3] Relax conversion to RedChannel

[Christophe Fergeau]

On Mon, Nov 07, 2016 at 11:13:21AM +, Frediano Ziglio wrote:
> Now RED_CHANNEL is a function call so avoid to call multiple
> time for the same conversion in the same functions.



> This speed up and reduce code and also reduce source line
> length.

To be honest, these are not really convincing arguments :)

Would quoting "35 insertions(+), 27 deletions", ie this adds lines of
code, be any useful whether deciding if this patch is good or not ? :)
This also adds a local variable, so more things to keep track of in our
limited brain while processing this code.
And potentially you could get into time of check/time of use races, ie
this check not triggering when it should.

For the record, I'm against doing this kind of changes unless there are
some profiling log showing this is causing a bottleneck.

Christophe


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

Re: [Spice-devel] [spice-server PATCH 8/8] red-record-qxl: child_output_setup: check fcntl return value

[Frediano Ziglio]

> 
> On 10/17/2016 01:08 PM, Frediano Ziglio wrote:
> >>
> >> Also replaced "continue" in while block with an empty
> >> block (added curly braces).
> >>
> >
> > I think you split this in 7/8.
> 
> Right, I'll remove it.
> 
> >
> >> Found by coverity.
> >>
> >> Signed-off-by: Uri Lublin 
> >> ---
> >>  server/red-record-qxl.c | 7 ++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> >> index adc487b..21fc35f 100644
> >> --- a/server/red-record-qxl.c
> >> +++ b/server/red-record-qxl.c
> >> @@ -845,13 +845,18 @@ void red_record_qxl_command(RedRecord *record,
> >> RedMemSlotInfo *slots,
> >>  static void child_output_setup(gpointer user_data)
> >>  {
> >>  int fd = GPOINTER_TO_INT(user_data);
> >> +int r;
> >>
> >>  while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR) {
> >>  }
> >>  close(fd);
> >>
> >>  // make sure file is not closed calling exec()
> >> -fcntl(STDOUT_FILENO, F_SETFD, 0);
> >> +while ((r = fcntl(STDOUT_FILENO, F_SETFD, 0)) < 0 && errno == EINTR)
> >> {
> >> +}
> >> +if (r < 0) {
> >> +spice_error("fcntl F_SETFD failed (%d)\n", errno);
> >> +}
> >>  }
> >>
> >>  RedRecord *red_record_new(const char *filename)
> >
> > I tried to understand better this.
> > First: fcntl(F_SETFD) cannot return EINTR so there's no reason to check
> >  (unless you check for kernel bugs).
> > This would change the code to
> >
> > if (fcntl(STDOUT_FILENO, F_SETFD, 0) < 0) {
> > spice_error("fcntl F_SETFD failed (%d)\n", errno);
> > }
> >
> > Note however that probably this won't do what you are expecting.
> > This will put the message in the log and then spice-server will continue
> > and write the recording into a closed pipe so all fprintf internally will
> > call write which will return EPIPE.
> >
> > Looking at the called function the file descriptor is the file descriptor
> > of "f" which is not created with O_CLOEXEC flag so the easier way to remove
> > this warning is just remove the fcntl line (which is doing nothing).
> 
> I think you are right, but also because of the dup2 that is a few lines
> above the fcntl.
> 
> >
> > About O_CLOEXEC would be worth perhaps setting this flag after setting
> > up file/pipe (before the call to fwrite in red_record_new), something
> > like
> >
> > if (fcntl(fileno(f), F_SETFD, O_CLOEXEC) < 0) {
> > spice_error("fcntl F_SETFD failed (%d)\n", errno);
> > }
> >
> > it's a bit racy but not racy would require using G_SPAWN_CLOEXEC_PIPES
> > in g_spawn_async_with_pipes which is supported since GLib 2.40 or manually
> > build the pipe with pipe2 and O_CLOEXEC and passing it to
> > g_spawn_async_with_pipes
> > (and opening the record file with "w+e" instead of just "w+").
> > But I don't think that risking to leak this file descriptor is a big
> > deal...
> 
> 
> right again.
> 
> I also noticed that if there are 2 (or more) QXL
> devices, the same file is used for all, which is problematic
> 
> Thanks,
>  Uri.
> 
> 

Well, this is a known issue actually :(
Basically we don't support recording for multiple cards.
I proposed some patches to start addressing this issue recording
everything into a single file.

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

Re: [Spice-devel] [spice-server PATCH 4/8] main-channel: getpeername/getsockname return early if no sockfd

[Frediano Ziglio]

> 
> On 10/17/2016 01:38 PM, Frediano Ziglio wrote:
> >>
> >> I'm not sure that needed as it seems getpeername/getsockname
> >> accept int fd and return -1 for fd=-1
> >>
> >> Signed-off-by: Uri Lublin 
> >> ---
> >>  server/main-channel.c | 16 ++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/server/main-channel.c b/server/main-channel.c
> >> index bf84694..9ff4dcd 100644
> >> --- a/server/main-channel.c
> >> +++ b/server/main-channel.c
> >> @@ -281,12 +281,24 @@ MainChannelClient *main_channel_link(MainChannel
> >> *channel, RedClient *client,
> >>
> >>  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa,
> >>  socklen_t *salen)
> >>  {
> >> -return main_chan ?
> >> getsockname(red_channel_get_first_socket(_chan->base), sa, salen) :
> >> -1;
> >> +int socketfd;
> >> +
> >> +if (!main_chan || ((socketfd =
> >> red_channel_get_first_socket(_chan->base)) < 0)) {
> >> +return -1;
> >> +}
> >> +
> >> +return getsockname(socketfd, sa, salen);
> >>  }
> >>
> >>  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa,
> >>  socklen_t *salen)
> >>  {
> >> -return main_chan ?
> >> getpeername(red_channel_get_first_socket(_chan->base), sa, salen) :
> >> -1;
> >> +int socketfd;
> >> +
> >> +if (!main_chan || ((socketfd =
> >> red_channel_get_first_socket(_chan->base)) < 0)) {
> >> +return -1;
> >> +}
> >> +
> >> +return getpeername(socketfd, sa, salen);
> >>  }
> >>
> >>  // TODO: ? shouldn't it disonnect all clients? or shutdown all
> >>  main_channels?
> >
> > Mumble... I don't know why but it does not look that good.
> >
> > These functions assume that there are only one client.
> > They are used only by spice_server_get_sock_info and
> > spice_server_get_peer_info
> > which are no more used by recent Qemu so mainly they are dead code.
> > Perhaps spice_server_get_sock_info and spice_server_get_peer_info should
> > check for the first client and call getpeername/getsockname by themself?
> > Or just adding a main_channel_get_socket instead of 2 obsolete
> > main_channel_*
> > functions?
> 
> Yes, you are correct.
> So, should we deprecate those spice_server_get_*_info calls ?
> 
> Yes, you are correct.
> Currently multi-client is still considered experimental, but we
> do have a lot of code to make it almost work.
> So, should we deprecate those spice_servet_get_*_info calls ?
> 

They are already deprecated in spice-server.h, just retained for
backward compatibility.
Maybe we could just return -1 ??

> >
> > OT: red_channel_get_first_socket is used only for obsolete or wrong code.
> 
> Due to the assumption there is only one client.
> 
> > OT: main_channel_close is causing a dandling file descriptor.
> 
> Yes, this function can iterate over all clients and close their
> socketfd.
> 

Still racing, probably would be better to use shutdown instead
or use dup2 on a /dev/null to atomically replace the file descriptor
or call some other functions.
Currently is called by spice_server_destroy which is never called.
And being spice_server_destroy a final "destructor" call would perhaps
better to just free all resources (memory, sockets and so on).

> Thanks,
>  Uri.
> 
> 

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

Re: [Spice-devel] [qxl 4/5] build: Error out when enabling xspice with X.Org 1.19

[Christophe Fergeau]

Hi,

On Fri, Nov 04, 2016 at 10:42:17AM -0500, Jeremy White wrote:
> 
> On 10/28/2016 05:18 AM, Christophe Fergeau wrote:
> > xspice needs to be updated to cope with some X.Org 1.19 API changes,
> > better to make that explicit at configure time rather than letting
> > people discover the hard way (it builds with warnings but will not work)
> > that it's broken.
> > ---
> >  configure.ac | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 451d42a..2c7bbf7 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -68,7 +68,8 @@ PKG_CHECK_EXISTS(xfont2,
> >  
> >  # Obtain compiler/linker options for the driver dependencies
> >  PKG_CHECK_MODULES(XORG, [xorg-server >= 1.0.99.901] xproto fontsproto 
> > $xfont_pc $REQUIRED_MODULES)
> > -
> > +# Check for xorg 1.19 as XSpice is currently not working with it
> > +PKG_CHECK_EXISTS([XORG119], [xorg-server >= 1.18.99], [has_xorg119=yes], 
> > [:])
> 
> This does not work on my Debian Jessie system with xorg 1.16.
> 
> Shouldn't it be just:
> 
> PKG_CHECK_EXISTS([xorg-server >= 1.18.99], [has_xorg119=yes])

Definitely looks much more correct, though I remember testing this
patch, dunno what happened /o\ I changed it (and retested ;) locally.

Thanks!

Christophe


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

[Spice-devel] [PATCH spice-server 0/3] Minor GObject follow ups

[Frediano Ziglio]

Basically optimizations.

Frediano Ziglio (3):
  Relax conversion to RedChannel
  Relax conversion to RedChannelClient
  Move InputsChannelClient and InputsChannelClient definition to C file

 server/cursor-channel.c| 12 +++
 server/dcc.c   | 40 +++---
 server/display-channel.c   |  7 +++---
 server/inputs-channel-client.c | 28 +---
 server/inputs-channel-client.h | 13 ---
 server/inputs-channel.c|  9 
 server/main-channel-client.c   | 49 --
 server/main-channel.c  | 18 
 server/smartcard.c | 15 +++--
 server/spicevmc.c  | 11 +-
 server/stream.c|  9 +---
 11 files changed, 113 insertions(+), 98 deletions(-)

-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 1/3] Relax conversion to RedChannel

[Frediano Ziglio]

Now RED_CHANNEL is a function call so avoid to call multiple
time for the same conversion in the same functions.
This speed up and reduce code and also reduce source line
length.

Signed-off-by: Frediano Ziglio 
---
 server/cursor-channel.c  | 12 
 server/display-channel.c |  7 ---
 server/inputs-channel.c  |  9 +
 server/main-channel.c| 16 
 server/smartcard.c   |  7 ---
 server/spicevmc.c| 11 ++-
 6 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 202ec89..2732417 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -321,10 +321,12 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd)
 CursorItem *cursor_item;
 int cursor_show = FALSE;
 QXLInstance *qxl;
+RedChannel *channel;
 
 spice_return_if_fail(cursor);
 spice_return_if_fail(cursor_cmd);
 
+channel = RED_CHANNEL(cursor);
 qxl = common_graphics_channel_get_qxl(COMMON_GRAPHICS_CHANNEL(cursor));
 cursor_item = cursor_item_new(qxl, cursor_cmd);
 
@@ -350,11 +352,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd)
 return;
 }
 
-if (red_channel_is_connected(RED_CHANNEL(cursor)) &&
+if (red_channel_is_connected(channel) &&
 (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER
  || cursor_cmd->type != QXL_CURSOR_MOVE
  || cursor_show)) {
-red_channel_pipes_new_add(RED_CHANNEL(cursor),
+red_channel_pipes_new_add(channel,
   new_cursor_pipe_item, cursor_item);
 }
 
@@ -389,7 +391,9 @@ static void cursor_channel_init_client(CursorChannel 
*cursor, CursorChannelClien
 {
 spice_return_if_fail(cursor);
 
-if (!red_channel_is_connected(RED_CHANNEL(cursor))
+RedChannel *channel = RED_CHANNEL(cursor);
+
+if (!red_channel_is_connected(channel)
 || 
common_graphics_channel_get_during_target_migrate(COMMON_GRAPHICS_CHANNEL(cursor)))
 {
 spice_debug("during_target_migrate: skip init");
 return;
@@ -399,7 +403,7 @@ static void cursor_channel_init_client(CursorChannel 
*cursor, CursorChannelClien
 red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(client),
  RED_PIPE_ITEM_TYPE_CURSOR_INIT);
 else
-red_channel_pipes_add_type(RED_CHANNEL(cursor), 
RED_PIPE_ITEM_TYPE_CURSOR_INIT);
+red_channel_pipes_add_type(channel, RED_PIPE_ITEM_TYPE_CURSOR_INIT);
 }
 
 void cursor_channel_do_init(CursorChannel *cursor)
diff --git a/server/display-channel.c b/server/display-channel.c
index bcdde13..0348dce 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1815,6 +1815,7 @@ void display_channel_destroy_surface_wait(DisplayChannel 
*display, uint32_t surf
 void display_channel_destroy_surfaces(DisplayChannel *display)
 {
 int i;
+RedChannel *channel = RED_CHANNEL(display);
 
 spice_debug(NULL);
 //to handle better
@@ -1829,9 +1830,9 @@ void display_channel_destroy_surfaces(DisplayChannel 
*display)
 }
 spice_warn_if_fail(ring_is_empty(>priv->streams));
 
-if (red_channel_is_connected(RED_CHANNEL(display))) {
-red_channel_pipes_add_type(RED_CHANNEL(display), 
RED_PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE);
-red_channel_pipes_add_empty_msg(RED_CHANNEL(display), 
SPICE_MSG_DISPLAY_STREAM_DESTROY_ALL);
+if (red_channel_is_connected(channel)) {
+red_channel_pipes_add_type(channel, 
RED_PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE);
+red_channel_pipes_add_empty_msg(channel, 
SPICE_MSG_DISPLAY_STREAM_DESTROY_ALL);
 }
 
 display_channel_free_glz_drawables(display);
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index 7c397a4..c68a94b 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -602,16 +602,17 @@ inputs_channel_constructed(GObject *object)
 {
 ClientCbs client_cbs = { NULL, };
 InputsChannel *self = INPUTS_CHANNEL(object);
-RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
+RedChannel *channel = RED_CHANNEL(self);
+RedsState *reds = red_channel_get_server(channel);
 
 G_OBJECT_CLASS(inputs_channel_parent_class)->constructed(object);
 
 client_cbs.connect = inputs_connect;
 client_cbs.migrate = inputs_migrate;
-red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, NULL);
+red_channel_register_client_cbs(channel, _cbs, NULL);
 
-red_channel_set_cap(RED_CHANNEL(self), SPICE_INPUTS_CAP_KEY_SCANCODE);
-reds_register_channel(reds, RED_CHANNEL(self));
+red_channel_set_cap(channel, SPICE_INPUTS_CAP_KEY_SCANCODE);
+reds_register_channel(reds, channel);
 
 self->key_modifiers_timer = reds_core_timer_add(reds, 
key_modifiers_sender, self);
 if (!self->key_modifiers_timer) {
diff --git a/server/main-channel.c b/server/main-channel.c
index 

[Spice-devel] [PATCH spice-server 2/3] Relax conversion to RedChannelClient

[Frediano Ziglio]

Now RED_CHANNEL_CLIENT is a function call so avoid to call multiple
time for the same conversion in the same functions.
This speed up and reduce code and also reduce source line
length.

Signed-off-by: Frediano Ziglio 
---
 server/dcc.c | 40 +---
 server/main-channel-client.c | 49 +---
 server/main-channel.c|  2 +-
 server/smartcard.c   |  8 +---
 server/stream.c  |  9 +---
 5 files changed, 63 insertions(+), 45 deletions(-)

diff --git a/server/dcc.c b/server/dcc.c
index 17f9300..f5f797c 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -545,9 +545,10 @@ static int 
display_channel_client_wait_for_init(DisplayChannelClient *dcc)
 {
 dcc->priv->expect_init = TRUE;
 uint64_t end_time = spice_get_monotonic_time_ns() + COMMON_CLIENT_TIMEOUT;
+RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
 for (;;) {
-red_channel_client_receive(RED_CHANNEL_CLIENT(dcc));
-if (!red_channel_client_is_connected(RED_CHANNEL_CLIENT(dcc))) {
+red_channel_client_receive(rcc);
+if (!red_channel_client_is_connected(rcc)) {
 break;
 }
 if (dcc->priv->pixmap_cache && dcc->priv->encoders.glz_dict) {
@@ -561,7 +562,7 @@ static int 
display_channel_client_wait_for_init(DisplayChannelClient *dcc)
 }
 if (spice_get_monotonic_time_ns() > end_time) {
 spice_warning("timeout");
-red_channel_client_disconnect(RED_CHANNEL_CLIENT(dcc));
+red_channel_client_disconnect(rcc);
 break;
 }
 usleep(DISPLAY_CLIENT_RETRY_INTERVAL);
@@ -574,7 +575,7 @@ void dcc_start(DisplayChannelClient *dcc)
 DisplayChannel *display = DCC_TO_DC(dcc);
 RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
 
-red_channel_client_push_set_ack(RED_CHANNEL_CLIENT(dcc));
+red_channel_client_push_set_ack(rcc);
 
 if (red_channel_client_is_waiting_for_migrate_data(rcc))
 return;
@@ -582,7 +583,7 @@ void dcc_start(DisplayChannelClient *dcc)
 if (!display_channel_client_wait_for_init(dcc))
 return;
 
-red_channel_client_ack_zero_messages_window(RED_CHANNEL_CLIENT(dcc));
+red_channel_client_ack_zero_messages_window(rcc);
 if (display->priv->surfaces[0].context.canvas) {
 display_channel_current_flush(display, 0);
 red_channel_client_pipe_add_type(rcc, 
RED_PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE);
@@ -672,21 +673,23 @@ void dcc_push_monitors_config(DisplayChannelClient *dcc)
 DisplayChannel *dc = DCC_TO_DC(dcc);
 MonitorsConfig *monitors_config = dc->priv->monitors_config;
 RedMonitorsConfigItem *mci;
+RedChannelClient *rcc;
 
 if (monitors_config == NULL) {
 spice_warning("monitors_config is NULL");
 return;
 }
 
-if (!red_channel_client_test_remote_cap(RED_CHANNEL_CLIENT(dcc),
+rcc = RED_CHANNEL_CLIENT(dcc);
+if (!red_channel_client_test_remote_cap(rcc,
 
SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
 return;
 }
 
-mci = 
red_monitors_config_item_new(red_channel_client_get_channel(RED_CHANNEL_CLIENT(dcc)),
+mci = red_monitors_config_item_new(red_channel_client_get_channel(rcc),
monitors_config);
-red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), >pipe_item);
-red_channel_client_push(RED_CHANNEL_CLIENT(dcc));
+red_channel_client_pipe_add(rcc, >pipe_item);
+red_channel_client_push(rcc);
 }
 
 static RedSurfaceDestroyItem *red_surface_destroy_item_new(RedChannel *channel,
@@ -960,16 +963,16 @@ int dcc_pixmap_cache_unlocked_add(DisplayChannelClient 
*dcc, uint64_t id,
 NewCacheItem *item;
 uint64_t serial;
 int key;
+RedChannelClient *rcc = RED_CHANNEL_CLIENT(dcc);
 
 spice_assert(size > 0);
 
 item = spice_new(NewCacheItem, 1);
-serial = red_channel_client_get_message_serial(RED_CHANNEL_CLIENT(dcc));
+serial = red_channel_client_get_message_serial(rcc);
 
 if (cache->generation != dcc->priv->pixmap_cache_generation) {
 if (!dcc->priv->pending_pixmaps_sync) {
-red_channel_client_pipe_add_type(
- RED_CHANNEL_CLIENT(dcc), 
RED_PIPE_ITEM_TYPE_PIXMAP_SYNC);
+red_channel_client_pipe_add_type(rcc, 
RED_PIPE_ITEM_TYPE_PIXMAP_SYNC);
 dcc->priv->pending_pixmaps_sync = TRUE;
 }
 free(item);
@@ -1203,6 +1206,7 @@ int dcc_handle_migrate_data(DisplayChannelClient *dcc, 
uint32_t size, void *mess
 SpiceMigrateDataDisplay *migrate_data = (SpiceMigrateDataDisplay *)(header 
+ 1);
 uint8_t *surfaces;
 int i;
+RedChannelClient *rcc;
 
 spice_return_val_if_fail(
 size >= (sizeof(*migrate_data) + sizeof(SpiceMigrateDataHeader)), 
FALSE);
@@ -1210,11 +1214,13 @@ int dcc_handle_migrate_data(DisplayChannelClient *dcc, 
uint32_t 

[Spice-devel] [PATCH spice-server 3/3] Move InputsChannelClient and InputsChannelClient definition to C file

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/inputs-channel-client.c | 28 +++-
 server/inputs-channel-client.h | 13 -
 2 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/server/inputs-channel-client.c b/server/inputs-channel-client.c
index 4ab2457..7cb920f 100644
--- a/server/inputs-channel-client.c
+++ b/server/inputs-channel-client.c
@@ -22,26 +22,28 @@
 #include "migration-protocol.h"
 #include "red-channel-client.h"
 
-G_DEFINE_TYPE(InputsChannelClient, inputs_channel_client, 
RED_TYPE_CHANNEL_CLIENT)
+struct InputsChannelClient
+{
+RedChannelClient parent;
 
-#define INPUTS_CHANNEL_CLIENT_PRIVATE(o) \
-(G_TYPE_INSTANCE_GET_PRIVATE((o), TYPE_INPUTS_CHANNEL_CLIENT, 
InputsChannelClientPrivate))
+uint16_t motion_count;
+};
 
-struct InputsChannelClientPrivate
+struct InputsChannelClientClass
 {
-uint16_t motion_count;
+RedChannelClientClass parent_class;
 };
 
+G_DEFINE_TYPE(InputsChannelClient, inputs_channel_client, 
RED_TYPE_CHANNEL_CLIENT)
+
 static void
 inputs_channel_client_class_init(InputsChannelClientClass *klass)
 {
-g_type_class_add_private(klass, sizeof(InputsChannelClientPrivate));
 }
 
 static void
 inputs_channel_client_init(InputsChannelClient *self)
 {
-self->priv = INPUTS_CHANNEL_CLIENT_PRIVATE(self);
 }
 
 RedChannelClient* inputs_channel_client_create(RedChannel *channel,
@@ -93,16 +95,16 @@ void 
inputs_channel_client_send_migrate_data(RedChannelClient *rcc,
 
 spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_INPUTS_MAGIC);
 spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_INPUTS_VERSION);
-spice_marshaller_add_uint16(m, icc->priv->motion_count);
+spice_marshaller_add_uint16(m, icc->motion_count);
 }
 
 void inputs_channel_client_handle_migrate_data(InputsChannelClient *icc,
uint16_t motion_count)
 {
-icc->priv->motion_count = motion_count;
+icc->motion_count = motion_count;
 
-for (; icc->priv->motion_count >= SPICE_INPUT_MOTION_ACK_BUNCH;
-   icc->priv->motion_count -= SPICE_INPUT_MOTION_ACK_BUNCH) {
+for (; icc->motion_count >= SPICE_INPUT_MOTION_ACK_BUNCH;
+   icc->motion_count -= SPICE_INPUT_MOTION_ACK_BUNCH) {
 red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(icc),
  RED_PIPE_ITEM_MOUSE_MOTION_ACK);
 }
@@ -112,10 +114,10 @@ void 
inputs_channel_client_on_mouse_motion(InputsChannelClient *icc)
 {
 InputsChannel *inputs_channel = 
INPUTS_CHANNEL(red_channel_client_get_channel(RED_CHANNEL_CLIENT(icc)));
 
-if (++icc->priv->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0 &&
+if (++icc->motion_count % SPICE_INPUT_MOTION_ACK_BUNCH == 0 &&
 !inputs_channel_is_src_during_migrate(inputs_channel)) {
 red_channel_client_pipe_add_type(RED_CHANNEL_CLIENT(icc),
  RED_PIPE_ITEM_MOUSE_MOTION_ACK);
-icc->priv->motion_count = 0;
+icc->motion_count = 0;
 }
 }
diff --git a/server/inputs-channel-client.h b/server/inputs-channel-client.h
index 7550b3c..6d4ab98 100644
--- a/server/inputs-channel-client.h
+++ b/server/inputs-channel-client.h
@@ -40,19 +40,6 @@ G_BEGIN_DECLS
 
 typedef struct InputsChannelClient InputsChannelClient;
 typedef struct InputsChannelClientClass InputsChannelClientClass;
-typedef struct InputsChannelClientPrivate InputsChannelClientPrivate;
-
-struct InputsChannelClient
-{
-RedChannelClient parent;
-
-InputsChannelClientPrivate *priv;
-};
-
-struct InputsChannelClientClass
-{
-RedChannelClientClass parent_class;
-};
 
 GType inputs_channel_client_get_type(void) G_GNUC_CONST;
 
-- 
2.7.4

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

Re: [Spice-devel] [PATCH v2] fixup! spicevmc: Change creation of RedCharDeviceSpiceVmc

[Frediano Ziglio]

> 
> ---
> Changes in v2:
>  - pass 'channel' property to char device constructor
>  - assert that 'channel' device is non-null in char device set_property
>  - minor indentation fix
> 
>  server/spicevmc.c | 83
>  ---
>  1 file changed, 61 insertions(+), 22 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index cf743de..532598d 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -89,7 +89,7 @@ struct RedCharDeviceSpiceVmcClass
>  static GType red_char_device_spicevmc_get_type(void) G_GNUC_CONST;
>  static RedCharDevice *red_char_device_spicevmc_new(SpiceCharDeviceInstance
>  *sin,
> RedsState *reds,
> -   uint8_t channel_type);
> +   RedVmcChannel *channel);
>  
>  G_DEFINE_TYPE(RedCharDeviceSpiceVmc, red_char_device_spicevmc,
>  RED_TYPE_CHAR_DEVICE)
>  
> @@ -806,7 +806,19 @@ RedCharDevice *spicevmc_device_connect(RedsState *reds,
> SpiceCharDeviceInstance *sin,
> uint8_t channel_type)
>  {
> -return red_char_device_spicevmc_new(sin, reds, channel_type);
> +RedCharDevice *dev;
> +RedVmcChannel *channel = red_vmc_channel_new(reds, channel_type);
> +if (!channel) {
> +return NULL;
> +}
> +
> +/* char device takes ownership of channel */
> +dev = red_char_device_spicevmc_new(sin, reds, channel);
> +
> +channel->chardev_sin = sin;
> +g_object_unref(channel);
> +
> +return dev;
>  }
>  
>  /* Must be called from RedClient handling thread. */
> @@ -861,18 +873,56 @@ red_char_device_spicevmc_dispose(GObject *object)
>  }
>  }
>  
> +enum {
> +PROP0,
> +PROP_CHANNEL
> +};
> +
> +static void
> +red_char_device_spicevmc_set_property(GObject *object,
> +  guint property_id,
> +  const GValue *value,
> +  GParamSpec *pspec)
> +{
> +RedCharDeviceSpiceVmc *self = RED_CHAR_DEVICE_SPICEVMC(object);
> +
> +switch (property_id)
> +{
> +case PROP_CHANNEL:
> +spice_assert(self->channel == NULL);
> +self->channel = g_value_dup_object(value);
> +spice_assert(self->channel != NULL);
> +self->channel->chardev = RED_CHAR_DEVICE(self);
> +
> +break;
> +default:
> +G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> +}
> +}
> +
>  static void
>  red_char_device_spicevmc_class_init(RedCharDeviceSpiceVmcClass *klass)
>  {
>  GObjectClass *object_class = G_OBJECT_CLASS(klass);
>  RedCharDeviceClass *char_dev_class = RED_CHAR_DEVICE_CLASS(klass);
>  
> +object_class->set_property = red_char_device_spicevmc_set_property;
>  object_class->dispose = red_char_device_spicevmc_dispose;
>  
>  char_dev_class->read_one_msg_from_device =
>  spicevmc_chardev_read_msg_from_dev;
>  char_dev_class->send_msg_to_client =
>  spicevmc_chardev_send_msg_to_client;
>  char_dev_class->send_tokens_to_client =
>  spicevmc_char_dev_send_tokens_to_client;
>  char_dev_class->remove_client = spicevmc_char_dev_remove_client;
> +
> +g_object_class_install_property(object_class,
> +PROP_CHANNEL,
> +g_param_spec_object("channel",
> +"Channel",
> +"Channel associated
> with this device",
> +
> RED_TYPE_VMC_CHANNEL,
> +
> G_PARAM_STATIC_STRINGS
> |
> +G_PARAM_WRITABLE |
> +G_PARAM_CONSTRUCT));
>  }
>  
>  static void
> @@ -883,25 +933,14 @@ red_char_device_spicevmc_init(RedCharDeviceSpiceVmc
> *self)
>  static RedCharDevice *
>  red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
>   RedsState *reds,
> - uint8_t channel_type)
> + RedVmcChannel *channel)
>  {
> -RedCharDeviceSpiceVmc *dev;
> -RedVmcChannel *channel = red_vmc_channel_new(reds, channel_type);
> -if (!channel) {
> -return NULL;
> -}
> -
> -dev = g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> -   "sin", sin,
> -   "spice-server", reds,
> -   "client-tokens-interval", 0ULL,
> -   "self-tokens", ~0ULL,
> -   "opaque", channel,
> -   NULL);
> -
> -channel->chardev = RED_CHAR_DEVICE(dev);
> -channel->chardev_sin = sin;
> -dev->channel = channel;
> -
> -return RED_CHAR_DEVICE(dev);
> +return 

Re: [Spice-devel] spice-vdagent-win 0.8.0 release

[Christophe Fergeau]

On Mon, Nov 07, 2016 at 10:53:38AM +0100, Thorsten Kohfeldt wrote:
> 
> Am 07.11.2016 um 10:21 schrieb Christophe Fergeau:
> > On Mon, Nov 07, 2016 at 10:07:58AM +0100, Thorsten Kohfeldt wrote:
> > > 
> > > BTW,
> > > _all_ binaries' details state 'Version 0.5.1.0, copyright RH 2009'.
> > > Here it would make sense to auto update with the build number, right ?
> > 
> > Now only one build will be made, so no need for that ;)
> 
> Hmm ...
> Shouldn't the Version read 0.8.0.1, copyright RH 2016 ?
> Respectively, what ever you designate the VDA version in digit 0-3
> plus build number in 4th place ?

Ah, updated in that way. Yes. I thought you were saying they should be
different between different fedora builders.

> 
> 
> > > > I've disabled all the various builders now and only kept f25 x86-64. For
> > > > now, I would pick one of the f24 builds.
> > > 
> > > Bad luck,
> > > I tested Win10/x86 with the F23/x86 build and the rawhide/x86 build.
> > > 
> > > Screen resize / auto guest resolution adaption seems to work (depending
> > > on manual choice of F23 libvirt spice viewer menu settings) as well as
> > > copy/paste of text guest->host and host->guest.
> > > 
> > > NO improvement on mouse sluggishness though.
> > 
> > Why "bad luck" ? Seems to be working as expected, I did not expect mouse
> > slugginess improvements from these builds.
> 
> Bad luck I tested F23 and rawhide while you suggested F24.

Ah, this really is not important, I would not recommend rawhide in case
of potential breakage there, otherwise any versions should do (and if
not, there's a bug somewhere ;)

> 
> So, there's no more function other than screen resizing and copy/paste ?
> But it also provides the ability to handle the mouse cursor user-local
> (as far as I understand from the wddm driver related discussion) ?

I believe server-side/client-side mouse is what is currently broken in
the wddm driver and what is causing the slowness. So I would not focus
on this too much right now.

> 
> > > > > Or would we even get those released on the next Fedorapeople ISO 
> > > > > (where
> > > > > I currently cannot find them) ?
> > > > 
> > > > Getting these on virtio-win ISO was discussed a bit, but never moved
> > > > forward.
> > > 
> > > I would really love to see that happen, i.e provision of the 4 binaries
> > > I mentioned above in my rpm2cpio -> zip request (here raw, not zipped).
> > > 
> > > That would allow to install Win drivers, Ballon Service and Spice VDAgent
> > > consistently from one source without having to use the sledge hammer
> > > Spice-Guest-Tools-1.xx.msi (or exe ?) from spice space.
> > > Spice VDAgent seems to be the one missing set of key binaries on that ISO.
> > 
> > Well, I'd push to have the spice-guest-tools installer on that ISO as
> > well ;) Though imo it would be best to move towards single-driver .msi
> > files, plus one installer installing all.
> 
> Eh, do single driver msi files support the Windows device manager driver
> update auto-search-the-disk-for-drivers funtion ? ;^)

Dunno, maybe, maybe not. Note that I did not say this should replace the
unpacked drivers, just that it would be better than the monolithic
spice-guest-tools installer we have now.

> 
> Anyway, the Ballon Service exe is already provided in a certain manner
> on that ISO - not auto installing, but ready to be used by admin scripts.
> Same should be true for Spice Service and VDAgent exes.

Yeah, "provided in a certain manner", if you ask me that's not a good
way at all as obscure manual steps are needed to have something which
works.

Christophe


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

Re: [Spice-devel] [vd_agent] udscs: Clarify the udscs_read_callback() documentation

[Francois Gouget]

On Wed, 2 Nov 2016, Jonathon Jongsma wrote:
[...]
> >  /* Callbacks with this type will be called when a complete message has been
> > - * received. The callback may call udscs_destroy_connection, in which case
> > - * *connp must be made NULL (which udscs_destroy_connection takes care of).
> > + * received. The callback is responsible for free()-ing the data buffer. It
> > + * may call udscs_destroy_connection, in which case *connp must be made 
> > NULL
> > + * (which udscs_destroy_connection takes care of).
> >   */
> >  typedef void (*udscs_read_callback)(struct udscs_connection **connp,
> >  struct udscs_message_header *header, uint8_t *data);
> 
> 
> >From reading the code, it looks like the data buffer should *NOT* be
> freed if udscs_destroy_connection() is called,

Right. This API is a mess. It's also inconsistent with virtio-port.h 
where vdagent_virtio_port_read_callback() must not free the data buffer 
and must not call vdagent_virtio_port_destroy() to destroy the port!


By the way, did I miss something or does udscs_destroy_connection() 
really leak conn->data.buf if conn->read_callback is NULL?
(not that one would want read_callback to be NULL)


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

Re: [Spice-devel] [PATCH qxl-wddm-dod] Update some file copyright statements

[Christophe Fergeau]

On Mon, Nov 07, 2016 at 04:40:11AM -0500, Frediano Ziglio wrote:
> > 
> > 
> > Acked-by: Christophe Fergeau 
> > 
> > On Mon, Nov 07, 2016 at 09:29:44AM +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  qxldod/QxlDod.cpp |  2 +-
> > >  qxldod/compat.cpp | 10 ++
> > >  qxldod/compat.h   | 10 ++
> > >  qxldod/driver.cpp |  2 +-
> > >  4 files changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > > index 69b8db9..85343e4 100755
> > > --- a/qxldod/QxlDod.cpp
> > > +++ b/qxldod/QxlDod.cpp
> > > @@ -1,6 +1,6 @@
> > >  /*
> > >   * Copyright 2013-2016 Red Hat, Inc.
> > > -
> > > + *
> > >   * Licensed under the Apache License, Version 2.0 (the "License");
> > >   * you may not use this file except in compliance with the License.
> > >   *
> > > diff --git a/qxldod/compat.cpp b/qxldod/compat.cpp
> > > index 88f9619..fe223d4 100644
> > > --- a/qxldod/compat.cpp
> > > +++ b/qxldod/compat.cpp
> > > @@ -1,3 +1,13 @@
> > > +/*
> > > + * Copyright 2013-2016 Red Hat, Inc.
> > > + *
> 
> I just noted that perhaps these file should be just
> 
>  * Copyright 2016 Red Hat, Inc.

If they are new ones, this would be better, yes.

Christophe


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

Re: [Spice-devel] [PATCH qxl-wddm-dod] Update some file copyright statements

[Frediano Ziglio]

> 
> 
> Acked-by: Christophe Fergeau 
> 
> On Mon, Nov 07, 2016 at 09:29:44AM +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  qxldod/QxlDod.cpp |  2 +-
> >  qxldod/compat.cpp | 10 ++
> >  qxldod/compat.h   | 10 ++
> >  qxldod/driver.cpp |  2 +-
> >  4 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > index 69b8db9..85343e4 100755
> > --- a/qxldod/QxlDod.cpp
> > +++ b/qxldod/QxlDod.cpp
> > @@ -1,6 +1,6 @@
> >  /*
> >   * Copyright 2013-2016 Red Hat, Inc.
> > -
> > + *
> >   * Licensed under the Apache License, Version 2.0 (the "License");
> >   * you may not use this file except in compliance with the License.
> >   *
> > diff --git a/qxldod/compat.cpp b/qxldod/compat.cpp
> > index 88f9619..fe223d4 100644
> > --- a/qxldod/compat.cpp
> > +++ b/qxldod/compat.cpp
> > @@ -1,3 +1,13 @@
> > +/*
> > + * Copyright 2013-2016 Red Hat, Inc.
> > + *

I just noted that perhaps these file should be just

 * Copyright 2016 Red Hat, Inc.

> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + *
> > + * You may obtain a copy of the License at
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + */
> > +
> >  #include "driver.h"
> >  #include "compat.h"
> >  
> > diff --git a/qxldod/compat.h b/qxldod/compat.h
> > index 3f20b81..b4a8b33 100644
> > --- a/qxldod/compat.h
> > +++ b/qxldod/compat.h
> > @@ -1,3 +1,13 @@
> > +/*
> > + * Copyright 2013-2016 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + *
> > + * You may obtain a copy of the License at
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + */
> > +
> >  #pragma once
> >  #include "BaseObject.h"
> >  
> > diff --git a/qxldod/driver.cpp b/qxldod/driver.cpp
> > index 59dc405..dc84aa8 100755
> > --- a/qxldod/driver.cpp
> > +++ b/qxldod/driver.cpp
> > @@ -1,6 +1,6 @@
> >  /*
> >   * Copyright 2013-2016 Red Hat, Inc.
> > -
> > + *
> >   * Licensed under the Apache License, Version 2.0 (the "License");
> >   * you may not use this file except in compliance with the License.
> >   *

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

Re: [Spice-devel] [PATCH qxl-wddm-dod] Update some file copyright statements

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Mon, Nov 07, 2016 at 09:29:44AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  qxldod/QxlDod.cpp |  2 +-
>  qxldod/compat.cpp | 10 ++
>  qxldod/compat.h   | 10 ++
>  qxldod/driver.cpp |  2 +-
>  4 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 69b8db9..85343e4 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright 2013-2016 Red Hat, Inc.
> -
> + *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
>   *
> diff --git a/qxldod/compat.cpp b/qxldod/compat.cpp
> index 88f9619..fe223d4 100644
> --- a/qxldod/compat.cpp
> +++ b/qxldod/compat.cpp
> @@ -1,3 +1,13 @@
> +/*
> + * Copyright 2013-2016 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + *
> + * You may obtain a copy of the License at
> + * http://www.apache.org/licenses/LICENSE-2.0
> + */
> +
>  #include "driver.h"
>  #include "compat.h"
>  
> diff --git a/qxldod/compat.h b/qxldod/compat.h
> index 3f20b81..b4a8b33 100644
> --- a/qxldod/compat.h
> +++ b/qxldod/compat.h
> @@ -1,3 +1,13 @@
> +/*
> + * Copyright 2013-2016 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + *
> + * You may obtain a copy of the License at
> + * http://www.apache.org/licenses/LICENSE-2.0
> + */
> +
>  #pragma once
>  #include "BaseObject.h"
>  
> diff --git a/qxldod/driver.cpp b/qxldod/driver.cpp
> index 59dc405..dc84aa8 100755
> --- a/qxldod/driver.cpp
> +++ b/qxldod/driver.cpp
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright 2013-2016 Red Hat, Inc.
> -
> + *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
>   *
> -- 
> 2.7.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [PATCH qxl-wddm-dod] Update some file copyright statements

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 qxldod/QxlDod.cpp |  2 +-
 qxldod/compat.cpp | 10 ++
 qxldod/compat.h   | 10 ++
 qxldod/driver.cpp |  2 +-
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index 69b8db9..85343e4 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -1,6 +1,6 @@
 /*
  * Copyright 2013-2016 Red Hat, Inc.
-
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  *
diff --git a/qxldod/compat.cpp b/qxldod/compat.cpp
index 88f9619..fe223d4 100644
--- a/qxldod/compat.cpp
+++ b/qxldod/compat.cpp
@@ -1,3 +1,13 @@
+/*
+ * Copyright 2013-2016 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ */
+
 #include "driver.h"
 #include "compat.h"
 
diff --git a/qxldod/compat.h b/qxldod/compat.h
index 3f20b81..b4a8b33 100644
--- a/qxldod/compat.h
+++ b/qxldod/compat.h
@@ -1,3 +1,13 @@
+/*
+ * Copyright 2013-2016 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ *
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ */
+
 #pragma once
 #include "BaseObject.h"
 
diff --git a/qxldod/driver.cpp b/qxldod/driver.cpp
index 59dc405..dc84aa8 100755
--- a/qxldod/driver.cpp
+++ b/qxldod/driver.cpp
@@ -1,6 +1,6 @@
 /*
  * Copyright 2013-2016 Red Hat, Inc.
-
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  *
-- 
2.7.4

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

Re: [Spice-devel] spice-vdagent-win 0.8.0 release

[Christophe Fergeau]

On Mon, Nov 07, 2016 at 10:07:58AM +0100, Thorsten Kohfeldt wrote:
> 
> Am 07.11.2016 um 09:10 schrieb Christophe Fergeau:
> > On Fri, Nov 04, 2016 at 08:48:48PM +0100, Thorsten Kohfeldt wrote:
> > > > Binary builds can be found at
> > > > https://copr.fedorainfracloud.org/coprs/teuf/spice-mingw/build/473299/
> > > 
> > > Nice offer,
> > > but I'm a little confused by that plethora of choices:
> > > 
> > > 3 target releases (F23, F24, rawhide), 2 architectures (x86, x64),
> > > each of them providing 2 _Windows_ binaries (vdservice.exe, vdagent.exe) 
> > > ...
> > > ... which should be equivalent ? ...
> > > ... usable on Win-(any)-x86 AND Win-(any)-x64 ?
> > 
> > Yeah, that's a fair point, from my side, I only issued one command and
> > only uploaded a single srpm, but the copr is configured to build on 3
> > different distros. The difference in binary content is expected as they
> > were built against different toolchains (respectively the one which is
> > in each of these distros).
> 
> Which is nice if you want to hunt tool chain bugs, but makes support
> an ugly business because of version uncertainties.
> BTW,
> _all_ binaries' details state 'Version 0.5.1.0, copyright RH 2009'.
> Here it would make sense to auto update with the build number, right ?

Now only one build will be made, so no need for that ;)

> 
> 
> > I've disabled all the various builders now and only kept f25 x86-64. For
> > now, I would pick one of the f24 builds.
> 
> Bad luck,
> I tested Win10/x86 with the F23/x86 build and the rawhide/x86 build.
> 
> Screen resize / auto guest resolution adaption seems to work (depending
> on manual choice of F23 libvirt spice viewer menu settings) as well as
> copy/paste of text guest->host and host->guest.
> 
> NO improvement on mouse sluggishness though.

Why "bad luck" ? Seems to be working as expected, I did not expect mouse
slugginess improvements from these builds.

> > > But:
> > > I rpm2cpio-ed them all and those binaries all differ from each other.
> > > 
> > > Which pair of (vdservice.exe, vdagent.exe) would make it into the
> > > Win-installer on spice-space ?
> 
> Would it be possible to add a post processing step to the build chain(s)
> which rpm2cpio-extracts the 4 relevant binaries (service and agent in x86
> and x64) into a single zip file (or maybe even skip the rpm2cpio but pack
> the binaries directly after they were built),
> so that zip file can be downloaded directly by the Windows guest ?

Not really possible on copr, but yeah would be nicer for users. Though
as far as I'm concerned, final product would be the spice-guest-tools
installer, and iirc it's able to consume these directly.

> 
> 
> > > Or would we even get those released on the next Fedorapeople ISO (where
> > > I currently cannot find them) ?
> > 
> > Getting these on virtio-win ISO was discussed a bit, but never moved
> > forward.
> 
> I would really love to see that happen, i.e provision of the 4 binaries
> I mentioned above in my rpm2cpio -> zip request (here raw, not zipped).
> 
> That would allow to install Win drivers, Ballon Service and Spice VDAgent
> consistently from one source without having to use the sledge hammer
> Spice-Guest-Tools-1.xx.msi (or exe ?) from spice space.
> Spice VDAgent seems to be the one missing set of key binaries on that ISO.

Well, I'd push to have the spice-guest-tools installer on that ISO as
well ;) Though imo it would be best to move towards single-driver .msi
files, plus one installer installing all.

Christophe


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

Re: [Spice-devel] [drm/qxl v3 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

On Mon, Nov 07, 2016 at 04:08:48AM -0500, Frediano Ziglio wrote:
> > @@ -124,9 +157,18 @@ void qxl_display_read_client_monitors_config(struct
> > qxl_device *qdev)
> >  {
> >  
> > struct drm_device *dev = qdev->ddev;
> > -   while (qxl_display_copy_rom_client_monitors_config(qdev)) {
> > +   int status;
> > +
> > +   status = qxl_display_copy_rom_client_monitors_config(qdev);
> > +   while (status == MONITORS_CONFIG_BAD_CRC) {
> > qxl_io_log(qdev, "failed crc check for client_monitors_config,"
> >  " retrying\n");
> > +   status = qxl_display_copy_rom_client_monitors_config(qdev);
> > +   }
> > +   if (status == MONITORS_CONFIG_UNCHANGED) {
> > +   qxl_io_log(qdev, "config unchanged\n");
> > +   DRM_DEBUG("ignoring unchanged client monitors config");
> 
> Why log and debug? Looks like a missing debug cleanup.

qxl_io_log is going to be output host-side, DRM_DEBUG is guest-side,
having both was intentional.

Christophe


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

Re: [Spice-devel] [drm/qxl v3 0/7] qxl: Various cleanups/fixes

[Gerd Hoffmann]

On Mo, 2016-11-07 at 09:00 +0100, Christophe Fergeau wrote:
> Hey,
> 
> Same series as v2 except that I removed the use of camel case in patch 6/7.
> Now it's only using an anonymous enum + int.

Can you please not drop the "PATCH" from $subject?

thanks,
  Gerd

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

Re: [Spice-devel] [drm/qxl v3 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Frediano Ziglio]

> 
> When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG
> interrupt,
> we currently always notify userspace that there was some hotplug event.
> 
> However, gnome-shell/mutter is reacting to this event by attempting a
> resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
> and then drmModeSetCrtc. This has the side-effect of causing
> qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
> surface was destroyed and created. After going through QEMU and then the
> remote SPICE client, a new identical monitors config message will be
> sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
> be emitted, and the same scenario occurring again.
> 
> As destroying/creating the primary surface causes a visible screen
> flicker, this makes the guest hard to use (
> https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).
> 
> This commit checks if the screen configuration we received is the same
> one as the current one, and does not notify userspace about it if that's
> the case.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 62
>  ---
>  1 file changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 8cf5177..518333c 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct
> qxl_device *qdev, unsigned c
>   qdev->client_monitors_config->count = count;
>  }
>  
> +enum {
> + MONITORS_CONFIG_MODIFIED,
> + MONITORS_CONFIG_UNCHANGED,
> + MONITORS_CONFIG_BAD_CRC,
> +};
> +
>  static int qxl_display_copy_rom_client_monitors_config(struct qxl_device
>  *qdev)
>  {
>   int i;
>   int num_monitors;
>   uint32_t crc;
> + int status = MONITORS_CONFIG_UNCHANGED;
>  
>   num_monitors = qdev->rom->client_monitors_config.count;
>   crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
> @@ -70,7 +77,7 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
>  sizeof(qdev->rom->client_monitors_config),
>  qdev->rom->client_monitors_config_crc);
> - return 1;
> + return MONITORS_CONFIG_BAD_CRC;
>   }
>   if (num_monitors > qdev->monitors_config->max_allowed) {
>   DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
> %d\n",
> @@ -79,6 +86,10 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   } else {
>   num_monitors = qdev->rom->client_monitors_config.count;
>   }
> + if (qdev->client_monitors_config
> +   && (num_monitors != qdev->client_monitors_config->count)) {
> + status = MONITORS_CONFIG_MODIFIED;
> + }
>   qxl_alloc_client_monitors_config(qdev, num_monitors);
>   /* we copy max from the client but it isn't used */
>   qdev->client_monitors_config->max_allowed =
> @@ -88,17 +99,39 @@ static int
> qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>   >rom->client_monitors_config.heads[i];
>   struct qxl_head *client_head =
>   >client_monitors_config->heads[i];
> - client_head->x = c_rect->left;
> - client_head->y = c_rect->top;
> - client_head->width = c_rect->right - c_rect->left;
> - client_head->height = c_rect->bottom - c_rect->top;
> - client_head->surface_id = 0;
> - client_head->id = i;
> - client_head->flags = 0;
> + if (client_head->x != c_rect->left) {
> + client_head->x = c_rect->left;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->y != c_rect->top) {
> + client_head->y = c_rect->top;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->width != c_rect->right - c_rect->left) {
> + client_head->width = c_rect->right - c_rect->left;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->height != c_rect->bottom - c_rect->top) {
> + client_head->height = c_rect->bottom - c_rect->top;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->surface_id != 0) {
> + client_head->surface_id = 0;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> + if (client_head->id != i) {
> + client_head->id = i;
> + status = MONITORS_CONFIG_MODIFIED;
> + }
> +  

Re: [Spice-devel] [drm/qxl v3 5/7] qxl: Remove qxl_bo_init() return value

[Frediano Ziglio]

> 
> It's always returning 0, and it's always ignored.

Missing the Signed-off-by.

Acked-by: Frediano Ziglio 

You should add the Acked-by message when posting new
versions of the patch series.

Frediano


> ---
>  drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
>  drivers/gpu/drm/qxl/qxl_gem.c | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 5a4720a..feac7e6 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -398,7 +398,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev);
>  int qxl_destroy_monitors_object(struct qxl_device *qdev);
>  
>  /* qxl_gem.c */
> -int qxl_gem_init(struct qxl_device *qdev);
> +void qxl_gem_init(struct qxl_device *qdev);
>  void qxl_gem_fini(struct qxl_device *qdev);
>  int qxl_gem_object_create(struct qxl_device *qdev, int size,
> int alignment, int initial_domain,
> diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
> index d9746e9..3f185c4 100644
> --- a/drivers/gpu/drm/qxl/qxl_gem.c
> +++ b/drivers/gpu/drm/qxl/qxl_gem.c
> @@ -111,10 +111,9 @@ void qxl_gem_object_close(struct drm_gem_object *obj,
>  {
>  }
>  
> -int qxl_gem_init(struct qxl_device *qdev)
> +void qxl_gem_init(struct qxl_device *qdev)
>  {
>   INIT_LIST_HEAD(>gem.objects);
> - return 0;
>  }
>  
>  void qxl_gem_fini(struct qxl_device *qdev)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Gerd Hoffmann]

  Hi,

> I think we should try it an see,

Ok, lets try.  I'll go pick them up and prepare a pull with this and
some virtio-gpu bits,

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

Re: [Spice-devel] spice-vdagent-win 0.8.0 release

[Christophe Fergeau]

On Fri, Nov 04, 2016 at 08:48:48PM +0100, Thorsten Kohfeldt wrote:
> > Binary builds can be found at
> > https://copr.fedorainfracloud.org/coprs/teuf/spice-mingw/build/473299/
> 
> Nice offer,
> but I'm a little confused by that plethora of choices:
> 
> 3 target releases (F23, F24, rawhide), 2 architectures (x86, x64),
> each of them providing 2 _Windows_ binaries (vdservice.exe, vdagent.exe) ...
> ... which should be equivalent ? ...
> ... usable on Win-(any)-x86 AND Win-(any)-x64 ?

Yeah, that's a fair point, from my side, I only issued one command and
only uploaded a single srpm, but the copr is configured to build on 3
different distros. The difference in binary content is expected as they
were built against different toolchains (respectively the one which is
in each of these distros).
I've disabled all the various builders now and only kept f25 x86-64. For
now, I would pick one of the f24 builds.

> 
> But:
> I rpm2cpio-ed them all and those binaries all differ from each other.
> 
> Which pair of (vdservice.exe, vdagent.exe) would make it into the
> Win-installer on spice-space ?
> 
> Or would we even get those released on the next Fedorapeople ISO (where
> I currently cannot find them) ?

Getting these on virtio-win ISO was discussed a bit, but never moved
forward.

Christophe


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

[Spice-devel] [drm/qxl v3 6/7] qxl: Don't notify userspace when monitors config is unchanged

[Christophe Fergeau]

When the QXL driver receives a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt,
we currently always notify userspace that there was some hotplug event.

However, gnome-shell/mutter is reacting to this event by attempting a
resolution change, which it does by issueing drmModeRmFB, drmModeAddFB,
and then drmModeSetCrtc. This has the side-effect of causing
qxl_crtc_mode_set() to tell the QXL virtual hardware that a primary
surface was destroyed and created. After going through QEMU and then the
remote SPICE client, a new identical monitors config message will be
sent, resulting in a QXL_INTERRUPT_CLIENT_MONITORS_CONFIG interrupt to
be emitted, and the same scenario occurring again.

As destroying/creating the primary surface causes a visible screen
flicker, this makes the guest hard to use (
https://bugzilla.redhat.com/show_bug.cgi?id=1266484 ).

This commit checks if the screen configuration we received is the same
one as the current one, and does not notify userspace about it if that's
the case.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 62 ---
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 8cf5177..518333c 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -57,11 +57,18 @@ static void qxl_alloc_client_monitors_config(struct 
qxl_device *qdev, unsigned c
qdev->client_monitors_config->count = count;
 }
 
+enum {
+   MONITORS_CONFIG_MODIFIED,
+   MONITORS_CONFIG_UNCHANGED,
+   MONITORS_CONFIG_BAD_CRC,
+};
+
 static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 {
int i;
int num_monitors;
uint32_t crc;
+   int status = MONITORS_CONFIG_UNCHANGED;
 
num_monitors = qdev->rom->client_monitors_config.count;
crc = crc32(0, (const uint8_t *)>rom->client_monitors_config,
@@ -70,7 +77,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct 
qxl_device *qdev)
qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
   sizeof(qdev->rom->client_monitors_config),
   qdev->rom->client_monitors_config_crc);
-   return 1;
+   return MONITORS_CONFIG_BAD_CRC;
}
if (num_monitors > qdev->monitors_config->max_allowed) {
DRM_DEBUG_KMS("client monitors list will be truncated: %d < 
%d\n",
@@ -79,6 +86,10 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
} else {
num_monitors = qdev->rom->client_monitors_config.count;
}
+   if (qdev->client_monitors_config
+ && (num_monitors != qdev->client_monitors_config->count)) {
+   status = MONITORS_CONFIG_MODIFIED;
+   }
qxl_alloc_client_monitors_config(qdev, num_monitors);
/* we copy max from the client but it isn't used */
qdev->client_monitors_config->max_allowed =
@@ -88,17 +99,39 @@ static int 
qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>rom->client_monitors_config.heads[i];
struct qxl_head *client_head =
>client_monitors_config->heads[i];
-   client_head->x = c_rect->left;
-   client_head->y = c_rect->top;
-   client_head->width = c_rect->right - c_rect->left;
-   client_head->height = c_rect->bottom - c_rect->top;
-   client_head->surface_id = 0;
-   client_head->id = i;
-   client_head->flags = 0;
+   if (client_head->x != c_rect->left) {
+   client_head->x = c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->y != c_rect->top) {
+   client_head->y = c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->width != c_rect->right - c_rect->left) {
+   client_head->width = c_rect->right - c_rect->left;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->height != c_rect->bottom - c_rect->top) {
+   client_head->height = c_rect->bottom - c_rect->top;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->surface_id != 0) {
+   client_head->surface_id = 0;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->id != i) {
+   client_head->id = i;
+   status = MONITORS_CONFIG_MODIFIED;
+   }
+   if (client_head->flags != 0) {
+   client_head->flags = 0;
+ 

[Spice-devel] [drm/qxl v3 3/7] qxl: Add missing '\n' to qxl_io_log() call

[Christophe Fergeau]

The message has to be terminated by a newline as it's not going to get
added automatically.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 28c1423..969c7c1 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -198,7 +198,7 @@ static int qxlfb_framebuffer_dirty(struct drm_framebuffer 
*fb,
/*
 * we are using a shadow draw buffer, at qdev->surface0_shadow
 */
-   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
+   qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]\n", clips->x1, clips->x2,
   clips->y1, clips->y2);
image->dx = clips->x1;
image->dy = clips->y1;
-- 
2.9.3

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

[Spice-devel] [drm/qxl v3 2/7] qxl: Remove unused prototype

[Christophe Fergeau]

qxl_crtc_set_from_monitors_config() is defined in qxl_drv.h but never
implemented.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index a3c1131..5a4720a 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -397,9 +397,6 @@ void qxl_display_read_client_monitors_config(struct 
qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);
 
-/* used by qxl_debugfs only */
-void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-
 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
-- 
2.9.3

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

[Spice-devel] [drm/qxl v3 5/7] qxl: Remove qxl_bo_init() return value

[Christophe Fergeau]

It's always returning 0, and it's always ignored.
---
 drivers/gpu/drm/qxl/qxl_drv.h | 2 +-
 drivers/gpu/drm/qxl/qxl_gem.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 5a4720a..feac7e6 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -398,7 +398,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);
 
 /* qxl_gem.c */
-int qxl_gem_init(struct qxl_device *qdev);
+void qxl_gem_init(struct qxl_device *qdev);
 void qxl_gem_fini(struct qxl_device *qdev);
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
  int alignment, int initial_domain,
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index d9746e9..3f185c4 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -111,10 +111,9 @@ void qxl_gem_object_close(struct drm_gem_object *obj,
 {
 }
 
-int qxl_gem_init(struct qxl_device *qdev)
+void qxl_gem_init(struct qxl_device *qdev)
 {
INIT_LIST_HEAD(>gem.objects);
-   return 0;
 }
 
 void qxl_gem_fini(struct qxl_device *qdev)
-- 
2.9.3

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

[Spice-devel] [drm/qxl v3 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
the resolutions we are going to present to user-space are going to be
rounded down to a multiple of 8. In the QXL arbitrary resolution case,
this is not useful.
This commit forces the actual width/height that was requested by the
client in the drm_display_mode structure rather than keeping the
rounded version.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 518333c..fa99481 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -199,6 +199,9 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
false);
mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = head->width;
+   mode->vdisplay = head->height;
+   drm_mode_set_name(mode);
*pwidth = head->width;
*pheight = head->height;
drm_mode_probed_add(connector, mode);
-- 
2.9.3

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

[Spice-devel] [drm/qxl v3 0/7] qxl: Various cleanups/fixes

[Christophe Fergeau]

Hey,

Same series as v2 except that I removed the use of camel case in patch 6/7.
Now it's only using an anonymous enum + int.

Christophe



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

[Spice-devel] [drm/qxl v3 1/7] qxl: Mark some internal functions as static

[Christophe Fergeau]

They are not used outside of their respective source file

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
 drivers/gpu/drm/qxl/qxl_drv.h | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 04270f5..74fc936 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -578,7 +578,7 @@ int qxl_hw_surface_dealloc(struct qxl_device *qdev,
return 0;
 }
 
-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
+static int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf)
 {
struct qxl_rect rect;
int ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 3aef127..8cf5177 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -36,7 +36,7 @@ static bool qxl_head_enabled(struct qxl_head *head)
return head->width && head->height;
 }
 
-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count)
+static void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned 
count)
 {
if (qdev->client_monitors_config &&
count > qdev->client_monitors_config->count) {
@@ -559,7 +559,7 @@ static bool qxl_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
 }
 
-void
+static void
 qxl_send_monitors_config(struct qxl_device *qdev)
 {
int i;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 8e633ca..a3c1131 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -394,13 +394,11 @@ qxl_framebuffer_init(struct drm_device *dev,
 struct drm_gem_object *obj,
 const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
-void qxl_send_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);
 
 /* used by qxl_debugfs only */
 void qxl_crtc_set_from_monitors_config(struct qxl_device *qdev);
-void qxl_alloc_client_monitors_config(struct qxl_device *qdev, unsigned count);
 
 /* qxl_gem.c */
 int qxl_gem_init(struct qxl_device *qdev);
@@ -573,6 +571,5 @@ int qxl_bo_check_id(struct qxl_device *qdev, struct qxl_bo 
*bo);
 struct qxl_drv_surface *
 qxl_surface_lookup(struct drm_device *dev, int surface_id);
 void qxl_surface_evict(struct qxl_device *qdev, struct qxl_bo *surf, bool 
freeing);
-int qxl_update_surface(struct qxl_device *qdev, struct qxl_bo *surf);
 
 #endif
-- 
2.9.3

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

[Spice-devel] [drm/qxl v3 4/7] qxl: Call qxl_gem_{init,fini}

[Christophe Fergeau]

qdev->gem.objects was initialized directly in qxl_device_init() rather
than going through qxl_gem_init(), and qxl_gem_fini() was never called.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_kms.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index e642242..af685f1 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -131,7 +131,7 @@ static int qxl_device_init(struct qxl_device *qdev,
mutex_init(>update_area_mutex);
mutex_init(>release_mutex);
mutex_init(>surf_evict_mutex);
-   INIT_LIST_HEAD(>gem.objects);
+   qxl_gem_init(qdev);
 
qdev->rom_base = pci_resource_start(pdev, 2);
qdev->rom_size = pci_resource_len(pdev, 2);
@@ -273,6 +273,7 @@ static void qxl_device_fini(struct qxl_device *qdev)
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
qxl_ring_free(qdev->release_ring);
+   qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
io_mapping_free(qdev->surface_mapping);
io_mapping_free(qdev->vram_mapping);
-- 
2.9.3

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