Re: [Spice-devel] [PATCH spice-common] build: Disable Celt support by default

[Christophe Fergeau]

Hey,
yup fine with me too.

Christophe

On Thu, Jun 13, 2019 at 03:40:17AM -0400, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > Fine with me.
> >
> 
> This is also related to 
> https://gitlab.freedesktop.org/spice/spice/merge_requests/2,
> I talked with Christophe and he agreed would be sensible to disable by 
> default.
>  
> > On 6/12/19 3:06 PM, Frediano Ziglio wrote:
> > > We started disabling Celt support making the option required.
> > > After 2 releases start making it disabled unless explicitly
> > > enabled.
> > >
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >   m4/spice-deps.m4  | 14 ++
> > >   meson_options.txt |  1 +
> > >   2 files changed, 3 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > > index 02230dd..1214341 100644
> > > --- a/m4/spice-deps.m4
> > > +++ b/m4/spice-deps.m4
> > > @@ -101,21 +101,11 @@ AC_DEFUN([SPICE_CHECK_SMARTCARD], [
> > >   AC_DEFUN([SPICE_CHECK_CELT051], [
> > >   AC_ARG_ENABLE([celt051],
> > >   AS_HELP_STRING([--enable-celt051],
> > > -   [Enable celt051 audio codec
> > > @<:@default=auto@:>@]),,
> > > -[enable_celt051="auto"])
> > > +   [Enable celt051 audio codec @<:@default=no@:>@]),,
> > > +[enable_celt051="no"])
> > >   
> > >   if test "x$enable_celt051" != "xno"; then
> > >   PKG_CHECK_MODULES([CELT051], [celt051 >= 0.5.1.1],
> > >   [have_celt051=yes], [have_celt051=no])
> > > -if test "x$enable_celt051" = "xauto"; then
> > > -if test "x$have_celt051" = "xyes"; then
> > > -AC_MSG_ERROR(m4_normalize([
> > > -CELT 0.5.1.x has been detected, \
> > > -but CELT support is no longer
> > > automatically enabled by default. \
> > > -Please explicitly use --enable-celt051 or
> > > --disable-celt051
> > > - ]))
> > > -fi
> > > -# have_celt051 is "no" here, so celt is disabled by default
> > > -fi
> > >   if test "x$enable_celt051" = "xyes" && test "x$have_celt051" !=
> > >   "xyes"; then
> > >   AC_MSG_ERROR(["--enable-celt051 has been specified, but CELT
> > >   0.5.1 is missing"])
> > >   fi
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 7e9e704..c982736 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -12,6 +12,7 @@ option('extra-checks',
> > >   
> > >   option('celt051',
> > >   type : 'feature',
> > > +value : 'disabled',
> > >   yield : true,
> > >   description: 'Enable celt051 audio codec')
> > >   
> 
> Frediano


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

Re: [Spice-devel] [PATCH 1/3] usb: use native libusb procedure for getting error name

[Christophe Fergeau]

On Mon, Apr 15, 2019 at 03:42:05PM +0300, Yuri Benditovich wrote:
> IIUC, what you call 'simpler' is:
> - making unneeded changes in several files (instead of one)
> - in the next patch remove these changes completely
> 
> Did I miss something?

Ah I did not look at the next patches :) Since you intentionally keep
this wrapper because it's going to be shortlived, I'd explain it in the
commit log so that it's clear that it's 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] [PATCH 1/3] usb: use native libusb procedure for getting error name

[Christophe Fergeau]

On Thu, Apr 11, 2019 at 12:37:17PM +, Victor Toso wrote:
> Hi,
> 
> On Thu, Apr 11, 2019 at 02:57:21PM +0300, Yuri Benditovich wrote:
> > On Thu, Apr 11, 2019 at 12:35 PM Victor Toso  wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Apr 10, 2019 at 10:31:37PM +0300, Yuri Benditovich wrote:
> > > > libusb has libusb_error_name procedure that returns name
> > > > for any error that libusb may return, so we do not need
> > > > to analyze error values by ourselves.
> > > >
> > > > Signed-off-by: Yuri Benditovich 
> > >
> > > Before applying the series:
> > >
> > > (master 15e06ead) $ grepi "spice_usbutil_libusb_strerror" src/
> > > src/win-usb-dev.c:116:const char *errstr = 
> > > spice_usbutil_libusb_strerror(rc);
> > > src/win-usb-dev.c:173:const char *errstr = 
> > > spice_usbutil_libusb_strerror(rc);
> > > src/channel-usbredir.c:312: spice_usbutil_libusb_strerror(rc), rc);
> > > src/usbutil.c:62:const char *spice_usbutil_libusb_strerror(enum 
> > > libusb_error error_code)
> > > src/usbutil.h:31:const char *spice_usbutil_libusb_strerror(enum 
> > > libusb_error error_code);
> > > src/usb-device-manager.c:284:const char *desc = 
> > > spice_usbutil_libusb_strerror(rc);
> > > src/usb-device-manager.c:311:const char *desc = 
> > > spice_usbutil_libusb_strerror(rc);
> > > src/usb-device-manager.c:733:errstr = 
> > > spice_usbutil_libusb_strerror(errcode);
> > > src/usb-device-manager.c:1071:const char *desc = 
> > > spice_usbutil_libusb_strerror(rc);
> > >
> > > After applying the series:
> > > (yuri-usb-b-layers-v1 5f87d90d) $ grepi "spice_usbutil_libusb_strerror" 
> > > src/
> > > (yuri-usb-b-layers-v1 5f87d90d) $
> > >
> > > So, I think it makes sense to use this patch to drop this
> > > function and always use libusb_error_name() instead, agree?
> > 
> > Finally, this series drops this functions and uses libusb_error_name.
> 
> Yes,
> 
> > It was possible to drop this function in the first patch, but
> > this would not make too much sense ( as all these new calls to
> > libusb_error_name() would be removed due to isolation of
> > libusb).
> 
> For me it makes sense because I know that this function can be
> dropped now even if later patches would change the code path of
> callers of spice_usbutil_libusb_strerror/libusb_error_name again.
> 
> That is, removing this function as first patch would introduce no
> regression and cleanup the code a bit. One patch less in the
> queue and could be merged before the others ;)

Yep, makes sense to me too to start by making the code simpler, and
merging the preparatory patches early.

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] [spice-common v3] log: Let gcc know about the logging macros which abort

[Christophe Fergeau]

This commit adds a SPICE_UNREACHABLE macro (courtesy of Frediano)
so that gcc does not think that code control can go past
spice_return_{val_,}if_fail(), spice_critical() and spice_error()

This avoids this kind of warnings:

fallthrough.c:

 #include "log.h"

int main(int argc, char **argv)
{
switch(argc) {
case 1:
spice_critical("foo");
   default:
return 0;
}
}

$ gcc  -c$(pkg-config --cflags --libs glib-2.0 spice-protocol)
   -I common   -Wimplicit-fallthrough=5 ./fallthrough.c
In file included from ./fallthrough.c:1:
./fallthrough.c: In function 'main':
common/log.h:73:5: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
   73 | spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" 
format, ## __VA_ARGS__); \
  | 
^~
./fallthrough.c:8:25: note: in expansion of macro 'spice_critical'
8 | spice_critical("foo");
  | ^~
./fallthrough.c:9:17: note: here
9 | default:
  |     ^~~

Signed-off-by: Christophe Fergeau 
---
Maybe a bit too much for a single commit, I can split :)

 common/log.h| 8 ++--
 common/macros.h | 8 
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/common/log.h b/common/log.h
index 7c67e7a..201c87a 100644
--- a/common/log.h
+++ b/common/log.h
@@ -39,16 +39,18 @@ void spice_log(GLogLevelFlags log_level,
const char *format,
...) G_GNUC_PRINTF(4, 5);
 
+/* FIXME: name is misleading, this aborts.. */
 #define spice_return_if_fail(x) G_STMT_START {  \
 if G_LIKELY(x) { } else {   \
-spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition 
`%s' failed", #x); \
+spice_critical("condition `%s' failed", #x);\
 return; \
 }   \
 } G_STMT_END
 
+/* FIXME: name is misleading, this aborts.. */
 #define spice_return_val_if_fail(x, val) G_STMT_START { \
 if G_LIKELY(x) { } else {   \
-spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "condition 
`%s' failed", #x); \
+spice_critical("condition `%s' failed", #x);\
 return (val);   \
 }   \
 } G_STMT_END
@@ -71,10 +73,12 @@ void spice_log(GLogLevelFlags log_level,
 
 #define spice_critical(format, ...) G_STMT_START {  \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+SPICE_UNREACHABLE; 
 \
 } G_STMT_END
 
 #define spice_error(format, ...) G_STMT_START { \
 spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+SPICE_UNREACHABLE; 
  \
 } G_STMT_END
 
 #define spice_warn_if_fail(x) G_STMT_START {\
diff --git a/common/macros.h b/common/macros.h
index 2f24ada..92cd82c 100644
--- a/common/macros.h
+++ b/common/macros.h
@@ -55,4 +55,12 @@
 
 #define SPICE_VERIFY(cond) verify_expr(cond, (void)1)
 
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
+#define SPICE_UNREACHABLE __builtin_unreachable()
+#elif defined(_MSC_VER)
+#define SPICE_UNREACHABLE __assume(0)
+#else
+#define SPICE_UNREACHABLE for(;;) continue
+#endif
+
 #endif // H_SPICE_COMMON_MACROS
-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-protocol] protocol: Generate enums.h again to remove old protocol definitions

[Christophe Fergeau]

On Wed, Mar 27, 2019 at 04:24:02AM -0400, Frediano Ziglio wrote:
> ping

Acked-by: Christophe Fergeau 
though imo this should be reflected in version numbering ;)

> 
> > > 
> > > This is breaking spice-protocol API even if this should not impact
> > > current code. Do we want to advertise this somehow? Switch to
> > > spice-protocol 0.14.x?
> > > 
> > 
> > Fine for me both 0.14.0 or 0.12.16 (the future, current HEAD version)
> > 
> > > On Fri, Feb 22, 2019 at 12:16:52PM +, Frediano Ziglio wrote:
> > > > spice.proto was updated to remove really old definitions,
> > > > update enums.h accordingly.
> > > > 
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  spice/enums.h | 57 ---
> > > >  1 file changed, 57 deletions(-)
> > > > 
> > > > diff --git a/spice/enums.h b/spice/enums.h
> > > > index 34e84c3..172cc4d 100644
> > > > --- a/spice/enums.h
> > > > +++ b/spice/enums.h
> > > > @@ -119,21 +119,6 @@ typedef enum SpiceMouseMode {
> > > >  SPICE_MOUSE_MODE_MASK = 0x3
> > > >  } SpiceMouseMode;
> > > >  
> > > > -typedef enum SpicePubkeyType {
> > > > -SPICE_PUBKEY_TYPE_INVALID,
> > > > -SPICE_PUBKEY_TYPE_RSA,
> > > > -SPICE_PUBKEY_TYPE_RSA2,
> > > > -SPICE_PUBKEY_TYPE_DSA,
> > > > -SPICE_PUBKEY_TYPE_DSA1,
> > > > -SPICE_PUBKEY_TYPE_DSA2,
> > > > -SPICE_PUBKEY_TYPE_DSA3,
> > > > -SPICE_PUBKEY_TYPE_DSA4,
> > > > -SPICE_PUBKEY_TYPE_DH,
> > > > -SPICE_PUBKEY_TYPE_EC,
> > > > -
> > > > -SPICE_PUBKEY_TYPE_ENUM_END
> > > > -} SpicePubkeyType;
> > > > -
> > > >  typedef enum SpiceDataCompressionType {
> > > >  SPICE_DATA_COMPRESSION_TYPE_NONE,
> > > >  SPICE_DATA_COMPRESSION_TYPE_LZ4,
> > > > @@ -397,21 +382,6 @@ typedef enum SpiceAudioFmt {
> > > >  SPICE_AUDIO_FMT_ENUM_END
> > > >  } SpiceAudioFmt;
> > > >  
> > > > -typedef enum SpiceTunnelServiceType {
> > > > -SPICE_TUNNEL_SERVICE_TYPE_INVALID,
> > > > -SPICE_TUNNEL_SERVICE_TYPE_GENERIC,
> > > > -SPICE_TUNNEL_SERVICE_TYPE_IPP,
> > > > -
> > > > -SPICE_TUNNEL_SERVICE_TYPE_ENUM_END
> > > > -} SpiceTunnelServiceType;
> > > > -
> > > > -typedef enum SpiceTunnelIpType {
> > > > -SPICE_TUNNEL_IP_TYPE_INVALID,
> > > > -SPICE_TUNNEL_IP_TYPE_IPv4,
> > > > -
> > > > -SPICE_TUNNEL_IP_TYPE_ENUM_END
> > > > -} SpiceTunnelIpType;
> > > > -
> > > >  typedef enum SpiceVscMessageType {
> > > >  SPICE_VSC_MESSAGE_TYPE_Init = 1,
> > > >  SPICE_VSC_MESSAGE_TYPE_Error,
> > > > @@ -613,33 +583,6 @@ enum {
> > > >  SPICE_MSGC_END_RECORD
> > > >  };
> > > >  
> > > > -enum {
> > > > -SPICE_MSG_TUNNEL_INIT = 101,
> > > > -SPICE_MSG_TUNNEL_SERVICE_IP_MAP,
> > > > -SPICE_MSG_TUNNEL_SOCKET_OPEN,
> > > > -SPICE_MSG_TUNNEL_SOCKET_FIN,
> > > > -SPICE_MSG_TUNNEL_SOCKET_CLOSE,
> > > > -SPICE_MSG_TUNNEL_SOCKET_DATA,
> > > > -SPICE_MSG_TUNNEL_SOCKET_CLOSED_ACK,
> > > > -SPICE_MSG_TUNNEL_SOCKET_TOKEN,
> > > > -
> > > > -SPICE_MSG_END_TUNNEL
> > > > -};
> > > > -
> > > > -enum {
> > > > -SPICE_MSGC_TUNNEL_SERVICE_ADD = 101,
> > > > -SPICE_MSGC_TUNNEL_SERVICE_REMOVE,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_OPEN_ACK,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_OPEN_NACK,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_FIN,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_CLOSED,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_CLOSED_ACK,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_DATA,
> > > > -SPICE_MSGC_TUNNEL_SOCKET_TOKEN,
> > > > -
> > > > -SPICE_MSGC_END_TUNNEL
> > > > -};
> > > > -
> > > >  enum {
> > > >  SPICE_MSG_SMARTCARD_DATA = 101,
> > > >  
> > 
> ___
> 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] char-device: Make RedClient an opaque structure again

[Christophe Fergeau]

On Tue, Mar 12, 2019 at 05:58:40AM -0400, Frediano Ziglio wrote:
> ping

I guess I would reword this as "char-device: Don't use RedClient API"
RedCharDevice only use red_client_get_server() once, we can store a
Reds* in RedCharDeviceClient instead. This will make it possible to turn
the RedClient reference into a generic pointer/handle in a later commit,
which will be useful if we want to split the flow control part of
char-device in a more specialised file

(not sure the last part is accurate regarding your goal ;)

Dunno how that sounds?

Christophe

> 
> > 
> > RedClient was an opaque structure for RedCharDevice.
> > It started to be used when RedsState started to contain all
> > the global state.
> > Make it opaque again.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/char-device.c | 16 +++-
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/server/char-device.c b/server/char-device.c
> > index 040b91147..465c1a125 100644
> > --- a/server/char-device.c
> > +++ b/server/char-device.c
> > @@ -22,8 +22,8 @@
> >  
> >  #include 
> >  #include 
> > +
> >  #include "char-device.h"
> > -#include "red-client.h"
> >  #include "reds.h"
> >  #include "glib-compat.h"
> >  
> > @@ -703,11 +703,10 @@ void red_char_device_destroy(RedCharDevice *char_dev)
> >  g_object_unref(char_dev);
> >  }
> >  
> > -static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
> > -   int do_flow_control,
> > -   uint32_t
> > max_send_queue_size,
> > -   uint32_t
> > num_client_tokens,
> > -   uint32_t
> > num_send_tokens)
> > +static RedCharDeviceClient *
> > +red_char_device_client_new(RedsState *reds, RedClient *client,
> > +   int do_flow_control, uint32_t
> > max_send_queue_size,
> > +   uint32_t num_client_tokens, uint32_t
> > num_send_tokens)
> >  {
> >  RedCharDeviceClient *dev_client;
> >  
> > @@ -717,8 +716,6 @@ static RedCharDeviceClient
> > *red_char_device_client_new(RedClient *client,
> >  dev_client->max_send_queue_size = max_send_queue_size;
> >  dev_client->do_flow_control = do_flow_control;
> >  if (do_flow_control) {
> > -RedsState *reds = red_client_get_server(client);
> > -
> >  dev_client->wait_for_tokens_timer =
> >  reds_core_timer_add(reds, 
> > device_client_wait_for_tokens_timeout,
> >  dev_client);
> > @@ -757,7 +754,8 @@ bool red_char_device_client_add(RedCharDevice *dev,
> >  dev->priv->wait_for_migrate_data = wait_for_migrate_data;
> >  
> >  spice_debug("char device %p, client %p", dev, client);
> > -dev_client = red_char_device_client_new(client, do_flow_control,
> > +dev_client = red_char_device_client_new(dev->priv->reds, client,
> > +do_flow_control,
> >  max_send_queue_size,
> >  num_client_tokens,
> >  num_send_tokens);
> ___
> 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] Two monitors Windows

[Christophe Fergeau]

Hey,

Your VM configuration does not seem to have a spicevmc channel device,
so I assume spice-vdagent is not running in the guest? This is required
for proper mouse behaviour in multiscreen setups.

Christophe

On Thu, Mar 28, 2019 at 10:59:16AM +0100, Mathias Egekvist wrote:
> Hi Spice Developer(s), 
> 
> Not sure this email should go to you, but I am not sure where else to go.
> 
> Recently started using Spice with libvirt and found great performance. 
> I have started a Windows 10 guest vm and it is working fine with 1 monitor. 
> I saw on your website  
> that to add 2 monitors I had to add an extra QXL device and it worked.
> Suddenly the mouse stopped working correctly though and I have been unable to 
> find a solution. When I remove the extra video tag everything works fine 
> again. 
> 
> Is it you I need to ask or someone else?
> If it is a general problem and I can help, please let me know.  
> I'll add all my information below.
> 
> 
> Best regards,
> 
> Mathias Egekvist
> 
> 
> Problem in details:
> The mouse either "glues" it self to the furthest left-side or only move in Y 
> and X-axis as in only one at a time. Sometimes the mouse move normally 
> though, but the problem which persists is every time I click it goes to the 
> top left corner and registers the click there. 
> 
> Setup Details:
> System is Arch Linux and window manager is dwm. 
> VM is Windows 10 running on KVM through libvirt, seen through virt-viewer 
> (have also tried remote-viewer).
> I have the EvTouch USB Graphics Tablet added together with a mouse and 
> keyboard.
> I have two QXL displays added, where I have tried adjusting the different ram 
> options, currently ram & vram 65536 and vgamem 16384. 
> Screen/graphics is Spice server, listen type have tried both none and 
> address. Tried with address 'All Interfaces' with port and tls and opengl on 
> and off. 
> XML you can see here: 
> https://unix.stackexchange.com/questions/507725/cursor-jumps-to-left-corner-windows-10-vm-kvm
>  
> 
>   

> ___
> 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 2/2] docs: Add some notes on event scheduling and threading

[Christophe Fergeau]

On Thu, Mar 28, 2019 at 10:27:46AM -0400, Frediano Ziglio wrote:
> > On Thu, Mar 28, 2019 at 04:25:31AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Mon, Mar 11, 2019 at 02:03:33PM +, Frediano Ziglio wrote:
> > > > > Signed-off-by: Frediano Ziglio 
> > > > > ---
> > > > >  docs/spice_threading_model.txt | 8 
> > > > >  1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/docs/spice_threading_model.txt
> > > > > b/docs/spice_threading_model.txt
> > > > > index 9351141c8..25a3a030c 100644
> > > > > --- a/docs/spice_threading_model.txt
> > > > > +++ b/docs/spice_threading_model.txt
> > > > > @@ -39,6 +39,14 @@ connect, disconnect and migrate. Connect and 
> > > > > migrate
> > > > > are
> > > > > asynchronous (the job
> > > > >  is done while the current thread is doing something else) while
> > > > >  disconnect
> > > > >  is
> > > > >  synchronous (the main thread will wait for termination).
> > > > >  
> > > > > +One aspect to take into consideration is the event scheduling. SPICE
> > > > > uses
> > > > > some
> > > > > +`SpiceCoreInterface` to handle events. As the events will be handled
> > > > > from
> > > > > a
> > > > > +thread based on the core interface you have to use the correct core.
> > > > > Each
> > > > > +channel has an associated core interface which can be retrieved using
> > > > > +`red_channel_get_core_interface`. There's also a main core interface
> > > > > you
> > > > > can get
> > > > > +using `reds_get_core_interface`. `reds_core_timer_*` and
> > > > > `reds_core_watch_*`
> > > > > +functions use the main core interface.
> > > > 
> > > > Do we need a few words as to when to use the main core interface?
> > > > Apart from this, looks good to me.
> > > > 
> > > > Christophe
> > > > 
> > > 
> > > It sounds a nice idea.
> > > 
> > > But honestly I cannot came with an easy rule beside "If code runs on
> > > main thread like Qemu character devices or everything not running in
> > > a channel you can use the main core interface."
> > 
> > Yes, something like your rule would work "Code running in the QEMU
> > thread should use the main core interface. Code running in the cursor or
> > display channel (through RedWorker) should use xxx interface.. Code
> > running in other channels should use yyy. Be aware that a channel's
> 
> IMHO all code running in channels should use channel core, no matter
> if they run on main or not (in the past I adjusted this).
> Note that cursor channel code can run in both main and not main
> for instance, not all cursor channels runs under RedWorker.
> I think it's easier to avoid too much exceptions.
> 
> > ClientCbs run in a different thread context than the rest of the
> > channel" (though the last sentence may no longer be accurate with the
> > work you are doing in that area).
> 
> ClientCbs will be removed, one less thing to know, and new callbacks/vfunc
> will work on the channel thread so not different from other channel code.
> 
> > 
> > Christophe
> > 
> 
> Taking into account that ClientCbs part will be soon (I hope) obsolete
> and that part of "channel core for channel core" part is partially there
> I would update to
> 
> +One aspect to take into consideration is the event scheduling. SPICE uses 
> some
> +`SpiceCoreInterface` to handle events. As the events will be handled from a
> +thread based on the core interface you have to use the correct core. Each
> +channel has an associated core interface which can be retrieved using
> +`red_channel_get_core_interface`. There's also a main core interface you can 
> get
> +using `reds_get_core_interface`. `reds_core_timer_*` and `reds_core_watch_*`
> +functions use the main core interface.


> +Even though multiple channel types run in the main thread and so could use
> +directly the main code interface, for coherence, rule simplicity and to 
> allows
> +the code to be moved in a separate thread easily if needed, it's advisable to
> +use the channel core interface (that will be the main core interface in this
> +case).

I'm not sure if this paragraph makes things clearer or more confusing
(because too many details). Maybe remove it?

> +Currently character devices and not channel code runs in the main thread.

Is this here to mean character device code should be using reds_core_*?

Overall looks good to me.

> OT: I though QEMU moved to GLib to handle events (so the main core interface
> was using GLib) but for performance reasons epoll or other implementations
> are used (that's the reason for SOCKET limitation for Windows).

Ah ok, I assumed epoll was there for historical reasons, not because
of performance. 

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 spice-server] red-channel: Small comment on "core" field

[Christophe Fergeau]

For the series,

Acked-by: Christophe Fergeau 

On Fri, Mar 29, 2019 at 12:29:58PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 121c7e475..4015c12df 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -70,6 +70,9 @@ struct RedChannelPrivate
>  uint32_t type;
>  uint32_t id;
>  
> +/* "core" interface to register events.
> + * Can be thread specific.
> + */
>  SpiceCoreInterfaceInternal *core;
>  gboolean handle_acks;
>  
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

[Spice-devel] [spice-common] build: Update verify.h to latest version

[Christophe Fergeau]

Signed-off-by: Christophe Fergeau 
---
 common/verify.h | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/common/verify.h b/common/verify.h
index 267de29..ecd8cdb 100644
--- a/common/verify.h
+++ b/common/verify.h
@@ -1,6 +1,6 @@
 /* Compile-time assert-like macros.
 
-   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
+   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
 
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU Lesser General Public License as published by
@@ -13,7 +13,7 @@
GNU Lesser General Public License for more details.
 
You should have received a copy of the GNU Lesser General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
 /* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
 
@@ -26,7 +26,7 @@
here generates easier-to-read diagnostics when verify (R) fails.
 
Define _GL_HAVE_STATIC_ASSERT to 1 if static_assert works as per C++11.
-   This will likely be supported by future GCC versions, in C++ mode.
+   This is supported by GCC 6.1.0 and later, in C++ mode.
 
Use this only with GCC.  If we were willing to slow 'configure'
down we could also use it with other compilers, but since this
@@ -36,9 +36,7 @@
  && !defined __cplusplus)
 # define _GL_HAVE__STATIC_ASSERT 1
 #endif
-/* The condition (99 < __GNUC__) is temporary, until we know about the
-   first G++ release that supports static_assert.  */
-#if (99 < __GNUC__) && defined __cplusplus
+#if (6 <= __GNUC__) && defined __cplusplus
 # define _GL_HAVE_STATIC_ASSERT 1
 #endif
 
@@ -248,7 +246,12 @@ template 
 /* Verify requirement R at compile-time, as a declaration without a
trailing ';'.  */
 
-#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#ifdef __GNUC__
+# define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#else
+/* PGI barfs if R is long.  Play it safe.  */
+# define verify(R) _GL_VERIFY (R, "verify (...)")
+#endif
 
 #ifndef __has_builtin
 # define __has_builtin(x) 0
@@ -263,7 +266,7 @@ template 
 # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
-#elif (defined lint \
+#elif ((defined GCC_LINT || defined lint) \
&& (__has_builtin (__builtin_trap) \
|| 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= 
__GNUC_PATCHLEVEL__
   /* Doing it this way helps various packages when configured with
@@ -271,7 +274,8 @@ template 
  when 'assume' silences warnings even with older GCCs.  */
 # define assume(R) ((R) ? (void) 0 : __builtin_trap ())
 #else
-# define assume(R) ((void) (0 && (R)))
+  /* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6.  */
+# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
 #endif
 
 /* @assert.h omit end@  */
-- 
2.21.0

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

Re: [Spice-devel] [spice-common v2 5/8] build: Update verify.h to latest version

[Christophe Fergeau]

On Fri, Mar 29, 2019 at 06:44:49AM -0400, Frediano Ziglio wrote:
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  common/verify.h | 24 ++--
> >  1 file changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/common/verify.h b/common/verify.h
> > index 267de29..3f3dece 100644
> > --- a/common/verify.h
> > +++ b/common/verify.h
> > @@ -1,10 +1,10 @@
> >  /* Compile-time assert-like macros.
> >  
> > -   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
> > +   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
> >  
> > This program is free software: you can redistribute it and/or modify
> > it under the terms of the GNU Lesser General Public License as published
> > by
> > -   the Free Software Foundation; either version 2.1 of the License, or
> > +   the Free Software Foundation; either version 3 of the License, or
> > (at your option) any later version.
> >  
> > This program is distributed in the hope that it will be useful,
> 
> Still not compatible.

*sigh*, sorry about this, gnulib's verify module is lpglv2+
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=modules/verify;h=5216ce890dac58e596a27341e31258e5d6c0d702;hb=HEAD
Using gnulib-tool --import --lgpl=2 results in this s/2.1/3 change being gone
from the diff.

Christophe

> 
> > @@ -13,7 +13,7 @@
> > GNU Lesser General Public License for more details.
> >  
> > You should have received a copy of the GNU Lesser General Public License
> > -   along with this program.  If not, see <http://www.gnu.org/licenses/>.  
> > */
> > +   along with this program.  If not, see <https://www.gnu.org/licenses/>.
> > */
> >  
> >  /* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
> >  
> > @@ -26,7 +26,7 @@
> > here generates easier-to-read diagnostics when verify (R) fails.
> >  
> > Define _GL_HAVE_STATIC_ASSERT to 1 if static_assert works as per C++11.
> > -   This will likely be supported by future GCC versions, in C++ mode.
> > +   This is supported by GCC 6.1.0 and later, in C++ mode.
> >  
> > Use this only with GCC.  If we were willing to slow 'configure'
> > down we could also use it with other compilers, but since this
> > @@ -36,9 +36,7 @@
> >   && !defined __cplusplus)
> >  # define _GL_HAVE__STATIC_ASSERT 1
> >  #endif
> > -/* The condition (99 < __GNUC__) is temporary, until we know about the
> > -   first G++ release that supports static_assert.  */
> > -#if (99 < __GNUC__) && defined __cplusplus
> > +#if (6 <= __GNUC__) && defined __cplusplus
> >  # define _GL_HAVE_STATIC_ASSERT 1
> >  #endif
> >  
> > @@ -248,7 +246,12 @@ template 
> >  /* Verify requirement R at compile-time, as a declaration without a
> > trailing ';'.  */
> >  
> > -#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
> > +#ifdef __GNUC__
> > +# define verify(R) _GL_VERIFY (R, "verify (" #R ")")
> > +#else
> > +/* PGI barfs if R is long.  Play it safe.  */
> > +# define verify(R) _GL_VERIFY (R, "verify (...)")
> > +#endif
> >  
> >  #ifndef __has_builtin
> >  # define __has_builtin(x) 0
> > @@ -263,7 +266,7 @@ template 
> >  # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
> >  #elif 1200 <= _MSC_VER
> >  # define assume(R) __assume (R)
> > -#elif (defined lint \
> > +#elif ((defined GCC_LINT || defined lint) \
> > && (__has_builtin (__builtin_trap) \
> > || 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <=
> > || __GNUC_PATCHLEVEL__
> >/* Doing it this way helps various packages when configured with
> > @@ -271,7 +274,8 @@ template 
> >   when 'assume' silences warnings even with older GCCs.  */
> >  # define assume(R) ((R) ? (void) 0 : __builtin_trap ())
> >  #else
> > -# define assume(R) ((void) (0 && (R)))
> > +  /* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6.  */
> > +# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
> >  #endif
> >  
> >  /* @assert.h omit end@  */
> 
> Frediano


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

Re: [Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort

[Christophe Fergeau]

On Fri, Mar 29, 2019 at 06:51:54AM -0400, Frediano Ziglio wrote:
> > 
> > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > will never return.
> > 
> > Signed-off-by: Christophe Fergeau 
> 
> I prefer the "for" way. But by the way, this is not telling the compiler
> that spice_log (NOT g_log) is not returning but to call abort() after
> spice_log.

Since abort() does not return, overall result is that the compiler won't
think code after spice_critical() can be reached.

> I don't think the compiler will warn that with that flag the function
> it not returning, is not clear why you need these changes.
> Optimization? I don't think that calling an additional function
> and jumping back to original flow would change much.

I'm fairly sure I got warnings during some experiments which led me to
these changes, but I haven't been able to get them now.
One testcase which warns without these changes:

#include "log.h"
#include "stdbool.h"

int main(int argc, char **argv)
{
switch(argc) {
case 1:
spice_critical("foo");
default:
return 0;
}
}


$ LC_ALL=C gcc  -c$(pkg-config --cflags --libs glib-2.0 spice-protocol) -I 
common   -Wimplicit-fallthrough=5 ./fallthrough.c
In file included from ./fallthrough.c:1:
./fallthrough.c: In function 'main':
common/log.h:73:5: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
   73 | spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" 
format, ## __VA_ARGS__); \
  | 
^~
./fallthrough.c:8:25: note: in expansion of macro 'spice_critical'
8 | spice_critical("foo");
  | ^~
./fallthrough.c:9:17: note: here
9 | default:
  | ^~~

(I can add this to the log)

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-common v2 8/8] log: Let gcc know about the logging macros which abort

[Christophe Fergeau]

On Fri, Mar 29, 2019 at 07:18:35AM -0400, Frediano Ziglio wrote:
> > On Fri, Mar 29, 2019 at 06:57:59AM -0400, Frediano Ziglio wrote:
> > > > On Fri, Mar 29, 2019 at 11:30:46AM +0100, Christophe Fergeau wrote:
> > > > > Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
> > > > > will never return.
> > > > > 
> > > > > Signed-off-by: Christophe Fergeau 
> > > > > ---
> > > > >  common/log.h | 5 +
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/common/log.h b/common/log.h
> > > > > index 7c67e7a..1482358 100644
> > > > > --- a/common/log.h
> > > > > +++ b/common/log.h
> > > > > @@ -20,6 +20,7 @@
> > > > >  
> > > > >  #include 
> > > > >  #include 
> > > > > +#include 
> > > > >  #include 
> > > > >  #include 
> > > > >  
> > > > > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> > > > >  #define spice_return_if_fail(x) G_STMT_START {
> > > > >  \
> > > > >  if G_LIKELY(x) { } else {
> > > > >  \
> > > > >  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC,
> > > > >  "condition `%s' failed", #x); \
> > > > > +abort();
> > > > > \
> > > > >  return;
> > > > >  \
> > > > 
> > > > The 'return' statment is now unreachable code & can be removed -
> > > > surprised
> > > > the compiler didn't complain that its unreachable.
> > > > 
> > > 
> > > As OT I would also add that a "spice_return_if_fail" which don't return
> > > is confusing, basically it's a hidden assert.
> > 
> > Yeah it is a rather misleading name.  I see this code is importing glib.h
> > so I wonder why spice_return_if_fail needs to exist at all. Why not just
> > drop it and use g_return_if_fail from glib instead of reinventing the
> > wheel.  Or g_warn_if_fail if you want a log message, or g_assert if
> > you want a fatal error.
> > 
> > https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-if-fail
> > 
> > Regards,
> > Daniel
> 
> Yes, this was discussed. The behaviour is different so it makes sense to keep
> them. For instance for use critical is fatal while in Glib not.
> Another difference is the informations they report.

Renaming spice_return_if_fail() to spice_assert_if_fail() or
something like this would make things much clearer. In the long run,
we should try to move away from these aborts when possible.

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] [spice-common v2 4/8] build: Add missing G_GNUC_PRINTF annotations

[Christophe Fergeau]

They were suggested by gcc when using -Wsuggest-attribute=format

Signed-off-by: Christophe Fergeau 
---
 common/log.c | 1 +
 tests/test-logging.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/common/log.c b/common/log.c
index b73da71..ce162a1 100644
--- a/common/log.c
+++ b/common/log.c
@@ -33,6 +33,7 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init)
 recorder_dump_on_common_signals(0, 0);
 }
 
+G_GNUC_PRINTF(5, 0)
 static void spice_logv(const char *log_domain,
GLogLevelFlags log_level,
const char *strloc,
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6a79ca9..32b0c33 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -27,6 +27,7 @@
 
 #define OTHER_LOG_DOMAIN "Other"
 #define LOG_OTHER_HELPER(suffix, level)
  \
+G_GNUC_PRINTF(1, 2)
  \
 static void G_PASTE(other_, suffix)(const gchar *format, ...)  
  \
 {  
  \
 va_list args;  
  \
-- 
2.21.0

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

[Spice-devel] [spice-common v2 8/8] log: Let gcc know about the logging macros which abort

[Christophe Fergeau]

Without the added abort(), it cannot know g_log(G_LOG_LEVEL_CRITICAL)
will never return.

Signed-off-by: Christophe Fergeau 
---
 common/log.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/common/log.h b/common/log.h
index 7c67e7a..1482358 100644
--- a/common/log.h
+++ b/common/log.h
@@ -20,6 +20,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
 #define spice_return_if_fail(x) G_STMT_START {  \
 if G_LIKELY(x) { } else {   \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition 
`%s' failed", #x); \
+abort();   
\
 return; \
 }   \
 } G_STMT_END
@@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
 #define spice_return_val_if_fail(x, val) G_STMT_START { \
 if G_LIKELY(x) { } else {   \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "condition 
`%s' failed", #x); \
+abort();   
   \
 return (val);   \
 }   \
 } G_STMT_END
@@ -71,10 +74,12 @@ void spice_log(GLogLevelFlags log_level,
 
 #define spice_critical(format, ...) G_STMT_START {  \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+abort();   
 \
 } G_STMT_END
 
 #define spice_error(format, ...) G_STMT_START { \
 spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+abort();   
  \
 } G_STMT_END
 
 #define spice_warn_if_fail(x) G_STMT_START {\
-- 
2.21.0

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

[Spice-devel] [spice-common v2 6/8] test-marshallers: Fix header guard

[Christophe Fergeau]

test-marshallers.h is missing a #define _H_TEST_MARSHALLERS in order to
prevent multiple #include for the same header.

Signed-off-by: Christophe Fergeau 
---
 tests/test-marshallers.h | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h
index 4eab90f..7686067 100644
--- a/tests/test-marshallers.h
+++ b/tests/test-marshallers.h
@@ -1,7 +1,8 @@
+#ifndef H_SPICE_COMMON_TEST_MARSHALLERS
+#define H_SPICE_COMMON_TEST_MARSHALLERS
+
 #include 
 
-#ifndef _H_TEST_MARSHALLERS
-
 typedef struct {
 uint32_t data_size;
 uint8_t dummy_byte;
@@ -26,5 +27,5 @@ typedef struct {
 uint8_t data[0];
 } SpiceMsgMainLenMessage;
 
-#endif /* _H_TEST_MARSHALLERS */
+#endif /* H_SPICE_COMMON_TEST_MARSHALLERS */
 
-- 
2.21.0

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

[Spice-devel] [spice-common v2 3/8] lz: Don't try to print uninitialized variable

[Christophe Fergeau]

encoder->type is only going to be set by lz_set_sizes() after the
error() call. We can use 'type' directly which is what encoder->type is
going to be set to.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 common/lz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/lz.c b/common/lz.c
index 167e118..f92c638 100644
--- a/common/lz.c
+++ b/common/lz.c
@@ -616,7 +616,7 @@ void lz_decode_begin(LzContext *lz, uint8_t *io_ptr, 
unsigned int num_io_bytes,
 
 int type = decode_32(encoder);
 if (type <= LZ_IMAGE_TYPE_INVALID || type > LZ_IMAGE_TYPE_A8) {
-encoder->usr->error(encoder->usr, "invalid lz type %d\n", 
encoder->type);
+encoder->usr->error(encoder->usr, "invalid lz type %d\n", type);
 }
 int width = decode_32(encoder);
 int height = decode_32(encoder);
-- 
2.21.0

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

[Spice-devel] [spice-common v2 2/8] backtrace: Add missing include

[Christophe Fergeau]

This fixes a warning about missing prototype for backtrace()

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 common/backtrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/backtrace.c b/common/backtrace.c
index c4edde1..ff72d1b 100644
--- a/common/backtrace.c
+++ b/common/backtrace.c
@@ -25,6 +25,8 @@
 #include 
 #endif
 
+#include "backtrace.h"
+
 #include 
 #include 
 #include 
-- 
2.21.0

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

[Spice-devel] [spice-common v2 7/8] quic: Fix QUIC_VERSION definition

[Christophe Fergeau]

QUIC_VERSION_MINOR is never used.. Set QUIC_VERSION_MINOR to the same
version as QUIC_VERSION_MAJOR to avoid breaking backwards compatibility,
and fix the QUIC_VERSION macro.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 common/quic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 1760274..f91b23f 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -31,8 +31,8 @@
 /* ASCII "QUIC" */
 #define QUIC_MAGIC 0x43495551
 #define QUIC_VERSION_MAJOR 0U
-#define QUIC_VERSION_MINOR 1U
-#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MAJOR & 
0x))
+#define QUIC_VERSION_MINOR 0U
+#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MINOR & 
0x))
 
 typedef uint8_t BYTE;
 
-- 
2.21.0

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

[Spice-devel] [spice-common v2 1/8] canvas_base: Fix variable shadowing warning

[Christophe Fergeau]

canvas_base.c is #included by spice-common users. They currently don't
enable this warning, but if/when they do, we don't want code from
spice-common to trigger it.

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 common/canvas_base.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/canvas_base.c b/common/canvas_base.c
index 9ffca3e..00a8801 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -1880,11 +1880,11 @@ static void canvas_clip_pixman(CanvasBase *canvas,
 uint32_t n = clip->rects->num_rects;
 SpiceRect *now = clip->rects->rects;
 
-pixman_region32_t clip;
+pixman_region32_t pixman_clip;
 
-if (spice_pixman_region32_init_rects(, now, n)) {
-pixman_region32_intersect(dest_region, dest_region, );
-pixman_region32_fini();
+if (spice_pixman_region32_init_rects(_clip, now, n)) {
+pixman_region32_intersect(dest_region, dest_region, _clip);
+pixman_region32_fini(_clip);
 }
 
 break;
-- 
2.21.0

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

[Spice-devel] [spice-common v2 5/8] build: Update verify.h to latest version

[Christophe Fergeau]

Signed-off-by: Christophe Fergeau 
---
 common/verify.h | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/common/verify.h b/common/verify.h
index 267de29..3f3dece 100644
--- a/common/verify.h
+++ b/common/verify.h
@@ -1,10 +1,10 @@
 /* Compile-time assert-like macros.
 
-   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
+   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
 
This program is free software: you can redistribute it and/or modify
it under the terms of the GNU Lesser General Public License as published by
-   the Free Software Foundation; either version 2.1 of the License, or
+   the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
 
This program is distributed in the hope that it will be useful,
@@ -13,7 +13,7 @@
GNU Lesser General Public License for more details.
 
You should have received a copy of the GNU Lesser General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
 /* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
 
@@ -26,7 +26,7 @@
here generates easier-to-read diagnostics when verify (R) fails.
 
Define _GL_HAVE_STATIC_ASSERT to 1 if static_assert works as per C++11.
-   This will likely be supported by future GCC versions, in C++ mode.
+   This is supported by GCC 6.1.0 and later, in C++ mode.
 
Use this only with GCC.  If we were willing to slow 'configure'
down we could also use it with other compilers, but since this
@@ -36,9 +36,7 @@
  && !defined __cplusplus)
 # define _GL_HAVE__STATIC_ASSERT 1
 #endif
-/* The condition (99 < __GNUC__) is temporary, until we know about the
-   first G++ release that supports static_assert.  */
-#if (99 < __GNUC__) && defined __cplusplus
+#if (6 <= __GNUC__) && defined __cplusplus
 # define _GL_HAVE_STATIC_ASSERT 1
 #endif
 
@@ -248,7 +246,12 @@ template 
 /* Verify requirement R at compile-time, as a declaration without a
trailing ';'.  */
 
-#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#ifdef __GNUC__
+# define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#else
+/* PGI barfs if R is long.  Play it safe.  */
+# define verify(R) _GL_VERIFY (R, "verify (...)")
+#endif
 
 #ifndef __has_builtin
 # define __has_builtin(x) 0
@@ -263,7 +266,7 @@ template 
 # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
-#elif (defined lint \
+#elif ((defined GCC_LINT || defined lint) \
&& (__has_builtin (__builtin_trap) \
|| 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= 
__GNUC_PATCHLEVEL__
   /* Doing it this way helps various packages when configured with
@@ -271,7 +274,8 @@ template 
  when 'assume' silences warnings even with older GCCs.  */
 # define assume(R) ((R) ? (void) 0 : __builtin_trap ())
 #else
-# define assume(R) ((void) (0 && (R)))
+  /* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6.  */
+# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
 #endif
 
 /* @assert.h omit end@  */
-- 
2.21.0

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

Re: [Spice-devel] [spice-common 6/8] test-marshallers: Fix header guard

[Christophe Fergeau]

On Thu, Mar 28, 2019 at 01:54:01PM -0400, Frediano Ziglio wrote:
> > 
> > test-marshallers.h is missing a #define _H_TEST_MARSHALLERS in order to
> > prevent multiple #include for the same header.
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  tests/test-marshallers.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h
> > index 4eab90f..5b113b7 100644
> > --- a/tests/test-marshallers.h
> > +++ b/tests/test-marshallers.h
> > @@ -1,6 +1,7 @@
> >  #include 
> >  
> 
> OT: why this is outside the guard?
> 
> >  #ifndef _H_TEST_MARSHALLERS
> > +#define _H_TEST_MARSHALLERS
> >  
> 
> OT 2: Why not H_SPICE_COMMON_ prefix?

I'll update the patch to fix these as well.

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-common 5/8] build: Update verify.h to latest version

[Christophe Fergeau]

On Thu, Mar 28, 2019 at 01:56:06PM -0400, Frediano Ziglio wrote:
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  common/verify.h | 24 +++-
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/common/verify.h b/common/verify.h
> > index 267de29..b2e5f64 100644
> > --- a/common/verify.h
> > +++ b/common/verify.h
> > @@ -1,19 +1,19 @@
> >  /* Compile-time assert-like macros.
> >  
> > -   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
> > +   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
> >  
> > This program is free software: you can redistribute it and/or modify
> > -   it under the terms of the GNU Lesser General Public License as published
> > by
> > -   the Free Software Foundation; either version 2.1 of the License, or
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > (at your option) any later version.
> >  
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > -   GNU Lesser General Public License for more details.
> > +   GNU General Public License for more details.
> >  
> 
> What ?? Sure about this?

Ah no definitely not, I forgot to run the magic gnulib command to adjust
the license. Will be fixed in v2.

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-common 8/8] log: Let gcc know about the logging macros which abort

[Christophe Fergeau]

On Thu, Mar 28, 2019 at 02:02:58PM -0400, Frediano Ziglio wrote:
> > 
> > The for(;;) hack was taken from glib's logging macros.
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  common/log.h | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/common/log.h b/common/log.h
> > index 7c67e7a..b397306 100644
> > --- a/common/log.h
> > +++ b/common/log.h
> > @@ -20,6 +20,7 @@
> >  
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  
> > @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
> >  #define spice_return_if_fail(x) G_STMT_START {  \
> >  if G_LIKELY(x) { } else {   \
> >  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition
> >  `%s' failed", #x); \
> > +abort();
> > \
> >  return; \
> >  }   \
> >  } G_STMT_END
> > @@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
> >  #define spice_return_val_if_fail(x, val) G_STMT_START { \
> >  if G_LIKELY(x) { } else {   \
> >  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__,
> >  "condition `%s' failed", #x); \
> > +abort();
> > \
> >  return (val);   \
> >  }   \
> >  } G_STMT_END
> > @@ -69,12 +72,17 @@ void spice_log(GLogLevelFlags log_level,
> >  spice_log(G_LOG_LEVEL_WARNING, SPICE_STRLOC, __FUNCTION__, "" format, 
> > ##
> >  __VA_ARGS__); \
> >  } G_STMT_END
> >  
> > +/* for(;;) ; so that GCC knows that control doesn't go past g_error().
> 
> g_error? copy error?
> 
> > + * Put space before ending semicolon to avoid C++ build warnings.
> > + */
> >  #define spice_critical(format, ...) G_STMT_START {
> >  \
> >  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format,
> >  ## __VA_ARGS__); \
> > +for (;;) ;
> 
> I suppose you can use "for(;;) continue;" and remove the comment (that
> "continue" was an old suggestion I had, I agree with C++ warning like
> I agreed at that time with the continue).
> 
> Why some are for and some abort?

Actually, I can't remember, and I haven't been able to reproduce the
warnings I wanted to fix. I changed all to abort(), which should be
enough.

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] [spice-common 2/8] backtrace: Add missing include

[Christophe Fergeau]

This fixes a warning about missing prototype for backtrace()

Signed-off-by: Christophe Fergeau 
---
 common/backtrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/backtrace.c b/common/backtrace.c
index c4edde1..ff72d1b 100644
--- a/common/backtrace.c
+++ b/common/backtrace.c
@@ -25,6 +25,8 @@
 #include 
 #endif
 
+#include "backtrace.h"
+
 #include 
 #include 
 #include 
-- 
2.21.0

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

[Spice-devel] [spice-common 6/8] test-marshallers: Fix header guard

[Christophe Fergeau]

test-marshallers.h is missing a #define _H_TEST_MARSHALLERS in order to
prevent multiple #include for the same header.

Signed-off-by: Christophe Fergeau 
---
 tests/test-marshallers.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h
index 4eab90f..5b113b7 100644
--- a/tests/test-marshallers.h
+++ b/tests/test-marshallers.h
@@ -1,6 +1,7 @@
 #include 
 
 #ifndef _H_TEST_MARSHALLERS
+#define _H_TEST_MARSHALLERS
 
 typedef struct {
 uint32_t data_size;
-- 
2.21.0

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

[Spice-devel] [spice-common 1/8] canvas_base: Fix variable shadowing warning

[Christophe Fergeau]

canvas_base.c is #included by spice-common users. They currently don't
enable this warning, but if/when they do, we don't want code from
spice-common to trigger it.

Signed-off-by: Christophe Fergeau 
---
 common/canvas_base.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/canvas_base.c b/common/canvas_base.c
index 9ffca3e..00a8801 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -1880,11 +1880,11 @@ static void canvas_clip_pixman(CanvasBase *canvas,
 uint32_t n = clip->rects->num_rects;
 SpiceRect *now = clip->rects->rects;
 
-pixman_region32_t clip;
+pixman_region32_t pixman_clip;
 
-if (spice_pixman_region32_init_rects(, now, n)) {
-pixman_region32_intersect(dest_region, dest_region, );
-pixman_region32_fini();
+if (spice_pixman_region32_init_rects(_clip, now, n)) {
+pixman_region32_intersect(dest_region, dest_region, _clip);
+pixman_region32_fini(_clip);
 }
 
 break;
-- 
2.21.0

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

[Spice-devel] [spice-common 8/8] log: Let gcc know about the logging macros which abort

[Christophe Fergeau]

The for(;;) hack was taken from glib's logging macros.

Signed-off-by: Christophe Fergeau 
---
 common/log.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/common/log.h b/common/log.h
index 7c67e7a..b397306 100644
--- a/common/log.h
+++ b/common/log.h
@@ -20,6 +20,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
 #define spice_return_if_fail(x) G_STMT_START {  \
 if G_LIKELY(x) { } else {   \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition 
`%s' failed", #x); \
+abort();   
\
 return; \
 }   \
 } G_STMT_END
@@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
 #define spice_return_val_if_fail(x, val) G_STMT_START { \
 if G_LIKELY(x) { } else {   \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "condition 
`%s' failed", #x); \
+abort();   
   \
 return (val);   \
 }   \
 } G_STMT_END
@@ -69,12 +72,17 @@ void spice_log(GLogLevelFlags log_level,
 spice_log(G_LOG_LEVEL_WARNING, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
 } G_STMT_END
 
+/* for(;;) ; so that GCC knows that control doesn't go past g_error().
+ * Put space before ending semicolon to avoid C++ build warnings.
+ */
 #define spice_critical(format, ...) G_STMT_START {  \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+for (;;) ; 
 \
 } G_STMT_END
 
 #define spice_error(format, ...) G_STMT_START { \
 spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+for (;;) ; 
 \
 } G_STMT_END
 
 #define spice_warn_if_fail(x) G_STMT_START {\
-- 
2.21.0

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

[Spice-devel] [spice-common 0/8] warning fixes

[Christophe Fergeau]

Hey,

This is the first set of hopefully more patches, long story short, I
wanted to add manywarnings.m4 to spice-common, but its compilation is
not fully clean at the moment. Here are patches for some of the warnings
I got when trying to add support for it.

Christophe




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

[Spice-devel] [spice-common 3/8] lz: Don't try to print uninitialized variable

[Christophe Fergeau]

encoder->type is only going to be set by lz_set_sizes() after the
error() call. We can use 'type' directly which is what encoder->type is
going to be set to.

Signed-off-by: Christophe Fergeau 
---
 common/lz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/lz.c b/common/lz.c
index 167e118..f92c638 100644
--- a/common/lz.c
+++ b/common/lz.c
@@ -616,7 +616,7 @@ void lz_decode_begin(LzContext *lz, uint8_t *io_ptr, 
unsigned int num_io_bytes,
 
 int type = decode_32(encoder);
 if (type <= LZ_IMAGE_TYPE_INVALID || type > LZ_IMAGE_TYPE_A8) {
-encoder->usr->error(encoder->usr, "invalid lz type %d\n", 
encoder->type);
+encoder->usr->error(encoder->usr, "invalid lz type %d\n", type);
 }
 int width = decode_32(encoder);
 int height = decode_32(encoder);
-- 
2.21.0

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

[Spice-devel] [spice-common 7/8] quic: Fix QUIC_VERSION definition

[Christophe Fergeau]

QUIC_VERSION_MINOR is never used.. Set QUIC_VERSION_MINOR to the same
version as QUIC_VERSION_MAJOR to avoid breaking backwards compatibility,
and fix the QUIC_VERSION macro.

Signed-off-by: Christophe Fergeau 
---
 common/quic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 1760274..f91b23f 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -31,8 +31,8 @@
 /* ASCII "QUIC" */
 #define QUIC_MAGIC 0x43495551
 #define QUIC_VERSION_MAJOR 0U
-#define QUIC_VERSION_MINOR 1U
-#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MAJOR & 
0x))
+#define QUIC_VERSION_MINOR 0U
+#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MINOR & 
0x))
 
 typedef uint8_t BYTE;
 
-- 
2.21.0

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

[Spice-devel] [spice-common 5/8] build: Update verify.h to latest version

[Christophe Fergeau]

Signed-off-by: Christophe Fergeau 
---
 common/verify.h | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/common/verify.h b/common/verify.h
index 267de29..b2e5f64 100644
--- a/common/verify.h
+++ b/common/verify.h
@@ -1,19 +1,19 @@
 /* Compile-time assert-like macros.
 
-   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
+   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
 
This program is free software: you can redistribute it and/or modify
-   it under the terms of the GNU Lesser General Public License as published by
-   the Free Software Foundation; either version 2.1 of the License, or
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
 
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU Lesser General Public License for more details.
+   GNU General Public License for more details.
 
-   You should have received a copy of the GNU Lesser General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
 /* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
 
@@ -248,7 +248,12 @@ template 
 /* Verify requirement R at compile-time, as a declaration without a
trailing ';'.  */
 
-#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#ifdef __GNUC__
+# define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#else
+/* PGI barfs if R is long.  Play it safe.  */
+# define verify(R) _GL_VERIFY (R, "verify (...)")
+#endif
 
 #ifndef __has_builtin
 # define __has_builtin(x) 0
@@ -263,7 +268,7 @@ template 
 # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
-#elif (defined lint \
+#elif ((defined GCC_LINT || defined lint) \
&& (__has_builtin (__builtin_trap) \
|| 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= 
__GNUC_PATCHLEVEL__
   /* Doing it this way helps various packages when configured with
@@ -271,7 +276,8 @@ template 
  when 'assume' silences warnings even with older GCCs.  */
 # define assume(R) ((R) ? (void) 0 : __builtin_trap ())
 #else
-# define assume(R) ((void) (0 && (R)))
+  /* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6.  */
+# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
 #endif
 
 /* @assert.h omit end@  */
-- 
2.21.0

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

[Spice-devel] [spice-common 4/8] build: Add missing G_GNUC_PRINTF annotations

[Christophe Fergeau]

They were suggested by gcc when using -Wsuggest-attribute=format

Signed-off-by: Christophe Fergeau 
---
 common/log.c | 13 +++--
 tests/test-logging.c |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/common/log.c b/common/log.c
index b73da71..7cb3e36 100644
--- a/common/log.c
+++ b/common/log.c
@@ -33,12 +33,13 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init)
 recorder_dump_on_common_signals(0, 0);
 }
 
-static void spice_logv(const char *log_domain,
-   GLogLevelFlags log_level,
-   const char *strloc,
-   const char *function,
-   const char *format,
-   va_list args)
+static G_GNUC_PRINTF(5, 0)
+void spice_logv(const char *log_domain,
+GLogLevelFlags log_level,
+const char *strloc,
+const char *function,
+const char *format,
+va_list args)
 {
 GString *log_msg;
 
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6a79ca9..32b0c33 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -27,6 +27,7 @@
 
 #define OTHER_LOG_DOMAIN "Other"
 #define LOG_OTHER_HELPER(suffix, level)
  \
+G_GNUC_PRINTF(1, 2)
  \
 static void G_PASTE(other_, suffix)(const gchar *format, ...)  
  \
 {  
  \
 va_list args;  
  \
-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading

[Christophe Fergeau]

On Thu, Mar 28, 2019 at 04:25:31AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Mar 11, 2019 at 02:03:33PM +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  docs/spice_threading_model.txt | 8 
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/docs/spice_threading_model.txt
> > > b/docs/spice_threading_model.txt
> > > index 9351141c8..25a3a030c 100644
> > > --- a/docs/spice_threading_model.txt
> > > +++ b/docs/spice_threading_model.txt
> > > @@ -39,6 +39,14 @@ connect, disconnect and migrate. Connect and migrate 
> > > are
> > > asynchronous (the job
> > >  is done while the current thread is doing something else) while 
> > > disconnect
> > >  is
> > >  synchronous (the main thread will wait for termination).
> > >  
> > > +One aspect to take into consideration is the event scheduling. SPICE uses
> > > some
> > > +`SpiceCoreInterface` to handle events. As the events will be handled from
> > > a
> > > +thread based on the core interface you have to use the correct core. Each
> > > +channel has an associated core interface which can be retrieved using
> > > +`red_channel_get_core_interface`. There's also a main core interface you
> > > can get
> > > +using `reds_get_core_interface`. `reds_core_timer_*` and
> > > `reds_core_watch_*`
> > > +functions use the main core interface.
> > 
> > Do we need a few words as to when to use the main core interface?
> > Apart from this, looks good to me.
> > 
> > Christophe
> > 
> 
> It sounds a nice idea.
> 
> But honestly I cannot came with an easy rule beside "If code runs on
> main thread like Qemu character devices or everything not running in
> a channel you can use the main core interface."

Yes, something like your rule would work "Code running in the QEMU
thread should use the main core interface. Code running in the cursor or
display channel (through RedWorker) should use xxx interface.. Code
running in other channels should use yyy. Be aware that a channel's
ClientCbs run in a different thread context than the rest of the
channel" (though the last sentence may no longer be accurate with the
work you are doing in that area).

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-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list

[Christophe Fergeau]

Hey,

On Wed, Mar 27, 2019 at 10:48:45AM +0200, Yuri Benditovich wrote:
> On Tue, Mar 26, 2019 at 3:11 PM Christophe Fergeau  
> wrote:
> >
> > On Mon, Mar 25, 2019 at 08:03:05PM +0200, Yuri Benditovich wrote:
> > > Yes, it looks like this finally does the same thing.
> >
> > > From my personal point of view, this is less obvious than previous
> > > code, so I would add 2 comments:
> > > that we unreference the device from the new list
> > > that we add reference to the device from the old list, it will be
> > > dereferenced on next line when the old list freed
> >
> > If the part you find less clear is:
> > +libusb_unref_device(dev->data);
> > +dev->data = libusb_ref_device(found->data);
> >
> 
> Yes, it is. It took me some time to understand the intention, so I suppose
> some other (not too smart) readers will also need some thinking and
> the comments would make the reading easier.
> Again, this is my personal opinion, no objections as soon as the code
> does the same thing.

And it took me some time to understand the intention of the swap when
reviewing your code ;) I changed it back to your initial version, and
pushed the whole series!

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-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list

[Christophe Fergeau]

On Mon, Mar 25, 2019 at 08:03:05PM +0200, Yuri Benditovich wrote:
> Yes, it looks like this finally does the same thing.

> From my personal point of view, this is less obvious than previous
> code, so I would add 2 comments:
> that we unreference the device from the new list
> that we add reference to the device from the old list, it will be
> dereferenced on next line when the old list freed

If the part you find less clear is:
+libusb_unref_device(dev->data);
+dev->data = libusb_ref_device(found->data);

I don't really mind changing it back to do the swapping with an
intermediate variable, even if I personally find the unref/ref to be
more readable than the other version ;)

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-gtk v2 10/13] win-usb-dev: maintain list of libusb devices

[Christophe Fergeau]

On Thu, Mar 21, 2019 at 05:16:03PM +0200, Yuri Benditovich wrote:
> > > @@ -412,10 +428,17 @@ static void notify_dev_state_change(GUdevClient 
> > > *self,
> > >  GList *dev;
> > >
> > >  for (dev = g_list_first(old_list); dev != NULL; dev = 
> > > g_list_next(dev)) {
> > > -if (g_list_find_custom(new_list, dev->data, 
> > > gudev_devices_differ) == NULL) {
> > > -/* Found a device that changed its state */
> > > +GList *found = g_list_find_custom(new_list, dev->data, 
> > > compare_libusb_devices);
> > > +if (found == NULL) {
> > >  g_udev_device_print(dev->data, add ? "add" : "remove");
> > > -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, 
> > > add);
> > > +g_udev_notify_device(self, dev->data, add);
> > > +} else if (add) {
> > > +/* keep old existing libusb_device in the new list, when
> > > +   usb-dev-manager will maintain its own list of 
> > > libusb_device,
> > > +   these lists will be completely consistent */
> > > +libusb_device *temp = found->data;
> > > +found->data = dev->data;
> > > +dev->data = temp;
> >
> > I'm still annoyed by this slightly complicated code in a method named 
> > notify_dev_state_change
> > (more on this below)
> >
> > >  }
> > >  }
> > >  }
> > > @@ -446,7 +469,8 @@ static void handle_dev_change(GUdevClient *self)
> > >  /* Unregister devices that are not present anymore */
> > >  notify_dev_state_change(self, priv->udev_list, now_devs, FALSE);
> > >
> > > -/* Register newly inserted devices */
> > > +/* report newly inserted devices and swap pointers to existing 
> > > devices:
> > > +   keep old pointers in now_devs list, keep new pointers in 
> > > udev_list */
> > >  notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
> > >
> > >  /* keep most recent info: free previous list, and keep current list 
> > > */
> > >  g_udev_client_free_device_list(>udev_list);
> > >  priv->udev_list = now_devs;
> >
> > Maybe these 2 lines + the code to replace new libusb_device pointers
> > with the old ones could be moved to a new
> > g_udev_client_update_device_list(self, now_devs);
> > helper?
> >
> 
> From my point of view, this will make the code more complicated,
> as additional procedure should traverse both lists again.

I made an attempt at that, see the 2 patches in answer to this thread.
I did not test them though :-/ Let me know what you think.

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] [spice-gtk 2/2] win-usb-dev: Use 'stable' libusb_device pointers in GUdevClient::udev_list

[Christophe Fergeau]

When getting a new list of USB devices after a WM_DEVICECHANGE event,
libusb_device which were already present before the event may be
represented by a different instance than the one we got in the new list.

Since other layers of spice-gtk may be using that older instance of
libusb_device, this commit makes sure that we always keep the older
instance in GUdevClient::udev_list.

At the moment, this should not be making any difference, but will make
things more consistent later on.

Based on a patch from Yuri Benditovich.

Signed-off-by: Christophe Fergeau 
---

Code is a bit longer this way, but imo it makes more sense to modify the
list when it's being updated, rather than modifying it during
notification.


 src/win-usb-dev.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
index 192ab17f..a16bd3ae 100644
--- a/src/win-usb-dev.c
+++ b/src/win-usb-dev.c
@@ -423,6 +423,25 @@ static gint compare_libusb_devices(gconstpointer a, 
gconstpointer b)
 return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
 }
 
+static void update_device_list(GUdevClient *self, GList *new_device_list)
+{
+GList *dev;
+
+for (dev = g_list_first(new_device_list); dev != NULL; dev = 
g_list_next(dev)) {
+GList *found = g_list_find_custom(self->priv->udev_list, dev->data, 
compare_libusb_devices);
+if (found != NULL) {
+/* keep old existing libusb_device in the new list, when
+   usb-dev-manager will maintain its own list of libusb_device,
+   these lists will be completely consistent */
+libusb_unref_device(dev->data);
+dev->data = libusb_ref_device(found->data);
+}
+}
+
+g_udev_client_free_device_list(>priv->udev_list);
+self->priv->udev_list = new_device_list;
+}
+
 static void notify_dev_state_change(GUdevClient *self,
 GList *old_list,
 GList *new_list,
@@ -469,8 +488,7 @@ static void handle_dev_change(GUdevClient *self)
 notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
 
 /* keep most recent info: free previous list, and keep current list */
-g_udev_client_free_device_list(>udev_list);
-priv->udev_list = now_devs;
+update_device_list(self, now_devs);
 }
 
 static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, 
LPARAM lparam)
-- 
2.21.0

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

[Spice-devel] [spice-gtk 1/2] win-usb-dev: maintain list of libusb devices

[Christophe Fergeau]

From: Yuri Benditovich 

Change internal device list to maintain libusb devices
instead of GUdevDevice objects. Create temporary
GUdevDevice object only for indication to usb-dev-manager.

Signed-off-by: Yuri Benditovich 
Acked-by: Christophe Fergeau 
---
Changes since version on the list:
- notify_dev_state_change no longer modify the device list

 src/win-usb-dev.c | 73 +--
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
index 7f0213d4..192ab17f 100644
--- a/src/win-usb-dev.c
+++ b/src/win-usb-dev.c
@@ -95,7 +95,7 @@ static void g_udev_device_print_list(GList *l, const gchar 
*msg);
 #else
 static void g_udev_device_print_list(GList *l, const gchar *msg) {}
 #endif
-static void g_udev_device_print(GUdevDevice *udev, const gchar *msg);
+static void g_udev_device_print(libusb_device *dev, const gchar *msg);
 
 static gboolean g_udev_skip_search(libusb_device *dev);
 
@@ -129,8 +129,6 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
 gssize rc;
 libusb_device **lusb_list, **dev;
 GUdevClientPrivate *priv;
-GUdevDeviceInfo *udevinfo;
-GUdevDevice *udevice;
 ssize_t n;
 
 g_return_val_if_fail(G_UDEV_IS_CLIENT(self), -1);
@@ -155,10 +153,7 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
 if (g_udev_skip_search(*dev)) {
 continue;
 }
-udevinfo = g_new0(GUdevDeviceInfo, 1);
-get_usb_dev_info(*dev, udevinfo);
-udevice = g_udev_device_new(udevinfo);
-*devs = g_list_prepend(*devs, udevice);
+*devs = g_list_prepend(*devs, libusb_ref_device(*dev));
 n++;
 }
 libusb_free_device_list(lusb_list, 1);
@@ -166,11 +161,20 @@ g_udev_client_list_devices(GUdevClient *self, GList 
**devs,
 return n;
 }
 
+static void unreference_libusb_device(gpointer data)
+{
+libusb_unref_device((libusb_device *)data);
+}
+
 static void g_udev_client_free_device_list(GList **devs)
 {
 g_return_if_fail(devs != NULL);
 if (*devs) {
-g_list_free_full(*devs, g_object_unref);
+/* the unreference_libusb_device method is required as
+ * libusb_unref_device calling convention differs from glib's
+ * see 558c967ec
+ */
+g_list_free_full(*devs, unreference_libusb_device);
 *devs = NULL;
 }
 }
@@ -246,9 +250,22 @@ static void 
g_udev_client_initable_iface_init(GInitableIface *iface)
 iface->init = g_udev_client_initable_init;
 }
 
+static void g_udev_notify_device(GUdevClient *self, libusb_device *dev, 
gboolean add)
+{
+GUdevDeviceInfo *udevinfo;
+GUdevDevice *udevice;
+udevinfo = g_new0(GUdevDeviceInfo, 1);
+if (get_usb_dev_info(dev, udevinfo)) {
+udevice = g_udev_device_new(udevinfo);
+g_signal_emit(self, signals[UEVENT_SIGNAL], 0, udevice, add);
+} else {
+g_free(udevinfo);
+}
+}
+
 static void report_one_device(gpointer data, gpointer self)
 {
-g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);
+g_udev_notify_device(self, data, TRUE);
 }
 
 void g_udev_client_report_devices(GUdevClient *self)
@@ -388,18 +405,20 @@ static gboolean get_usb_dev_info(libusb_device *dev, 
GUdevDeviceInfo *udevinfo)
 }
 
 /* comparing bus:addr and vid:pid */
-static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
+static gint compare_libusb_devices(gconstpointer a, gconstpointer b)
 {
-GUdevDeviceInfo *ai, *bi;
+libusb_device *a_dev = (libusb_device *)a;
+libusb_device *b_dev = (libusb_device *)b;
+struct libusb_device_descriptor a_desc, b_desc;
 gboolean same_bus, same_addr, same_vid, same_pid;
 
-ai = G_UDEV_DEVICE(a)->priv->udevinfo;
-bi = G_UDEV_DEVICE(b)->priv->udevinfo;
+libusb_get_device_descriptor(a_dev, _desc);
+libusb_get_device_descriptor(b_dev, _desc);
 
-same_bus = (ai->bus == bi->bus);
-same_addr = (ai->addr == bi->addr);
-same_vid = (ai->vid == bi->vid);
-same_pid = (ai->pid == bi->pid);
+same_bus = (libusb_get_bus_number(a_dev) == libusb_get_bus_number(b_dev));
+same_addr = (libusb_get_device_address(a_dev) == 
libusb_get_device_address(b_dev));
+same_vid = (a_desc.idVendor == b_desc.idVendor);
+same_pid = (a_desc.idProduct == b_desc.idProduct);
 
 return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
 }
@@ -412,10 +431,10 @@ static void notify_dev_state_change(GUdevClient *self,
 GList *dev;
 
 for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
-if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == 
NULL) {
-/* Found a device that changed its state */
+GList *found = g_list_find_custom(new_list, dev->data, 
compare_libusb_devices);
+if (found == NULL) {
 g_udev_device_print(dev->data, add ? "add" : &qu

Re: [Spice-devel] [spice-gtk v2 00/13] use persistent libusb_device on Windows

[Christophe Fergeau]

On Thu, Mar 21, 2019 at 02:15:41PM +0100, Christophe Fergeau wrote:
> Hey,
> 
> Overall series looks good to me, you seem to have ignored most of the
> suggestions to change the commit logs/comments, I dunno if that's
> intentional?

Actually, while doing the review, I adjusted the commit logs
https://gitlab.freedesktop.org/teuf/spice-gtk/commits/daynix-cd-sharing-v4
But we can do more adjustments, including going back to the initial logs
:)

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-gtk v2 10/13] win-usb-dev: maintain list of libusb devices

[Christophe Fergeau]

On Tue, Mar 19, 2019 at 07:56:05AM +0200, Yuri Benditovich wrote:
> Change internal device list to maintain libusb devices
> instead of GUdevDevice objects. Create temporary
> GUdevDevice object only for indication to usb-dev-manager.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/win-usb-dev.c | 80 ++-
>  1 file changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index b3b2ed8..0b87e75 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -95,7 +95,7 @@ static void g_udev_device_print_list(GList *l, const gchar 
> *msg);
>  #else
>  static void g_udev_device_print_list(GList *l, const gchar *msg) {}
>  #endif
> -static void g_udev_device_print(GUdevDevice *udev, const gchar *msg);
> +static void g_udev_device_print(libusb_device *dev, const gchar *msg);
>  
>  static gboolean g_udev_skip_search(libusb_device *dev);
>  
> @@ -129,8 +129,6 @@ g_udev_client_list_devices(GUdevClient *self, GList 
> **devs,
>  gssize rc;
>  libusb_device **lusb_list, **dev;
>  GUdevClientPrivate *priv;
> -GUdevDeviceInfo *udevinfo;
> -GUdevDevice *udevice;
>  ssize_t n;
>  
>  g_return_val_if_fail(G_UDEV_IS_CLIENT(self), -1);
> @@ -155,10 +153,7 @@ g_udev_client_list_devices(GUdevClient *self, GList 
> **devs,
>  if (g_udev_skip_search(*dev)) {
>  continue;
>  }
> -udevinfo = g_new0(GUdevDeviceInfo, 1);
> -get_usb_dev_info(*dev, udevinfo);
> -udevice = g_udev_device_new(udevinfo);
> -*devs = g_list_prepend(*devs, udevice);
> +*devs = g_list_prepend(*devs, libusb_ref_device(*dev));
>  n++;
>  }
>  libusb_free_device_list(lusb_list, 1);
> @@ -166,11 +161,17 @@ g_udev_client_list_devices(GUdevClient *self, GList 
> **devs,
>  return n;
>  }
>  
> +static void unreference_libusb_device(gpointer data)
> +{
> +libusb_unref_device((libusb_device *)data);
> +}
> +
>  static void g_udev_client_free_device_list(GList **devs)
>  {
>  g_return_if_fail(devs != NULL);
>  if (*devs) {
> -g_list_free_full(*devs, g_object_unref);
> +/* avoiding casting of imported procedure to GDestroyNotify */

Maybe make this very explicit with something like this?

  /* the unreference_libusb_device method is required as
   * libusb_unref_device calling convention differs from glib's
   * see 558c967ec
   */



> +g_list_free_full(*devs, unreference_libusb_device);
>  *devs = NULL;
>  }
>  }
> @@ -246,9 +247,22 @@ static void 
> g_udev_client_initable_iface_init(GInitableIface *iface)
>  iface->init = g_udev_client_initable_init;
>  }
>  
> +static void g_udev_notify_device(GUdevClient *self, libusb_device *dev, 
> gboolean add)
> +{
> +GUdevDeviceInfo *udevinfo;
> +GUdevDevice *udevice;
> +udevinfo = g_new0(GUdevDeviceInfo, 1);
> +if (get_usb_dev_info(dev, udevinfo)) {
> +udevice = g_udev_device_new(udevinfo);
> +g_signal_emit(self, signals[UEVENT_SIGNAL], 0, udevice, add);
> +} else {
> +g_free(udevinfo);
> +}
> +}
> +
>  static void report_one_device(gpointer data, gpointer self)
>  {
> -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);
> +g_udev_notify_device(self, data, TRUE);
>  }
>  
>  void g_udev_client_report_devices(GUdevClient *self)
> @@ -388,18 +402,20 @@ static gboolean get_usb_dev_info(libusb_device *dev, 
> GUdevDeviceInfo *udevinfo)
>  }
>  
>  /* comparing bus:addr and vid:pid */
> -static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
> +static gint compare_libusb_devices(gconstpointer a, gconstpointer b)
>  {
> -GUdevDeviceInfo *ai, *bi;
> +libusb_device *a_dev = (libusb_device *)a;
> +libusb_device *b_dev = (libusb_device *)b;
> +struct libusb_device_descriptor a_desc, b_desc;
>  gboolean same_bus, same_addr, same_vid, same_pid;
>  
> -ai = G_UDEV_DEVICE(a)->priv->udevinfo;
> -bi = G_UDEV_DEVICE(b)->priv->udevinfo;
> +libusb_get_device_descriptor(a_dev, _desc);
> +libusb_get_device_descriptor(b_dev, _desc);
>  
> -same_bus = (ai->bus == bi->bus);
> -same_addr = (ai->addr == bi->addr);
> -same_vid = (ai->vid == bi->vid);
> -same_pid = (ai->pid == bi->pid);
> +same_bus = (libusb_get_bus_number(a_dev) == 
> libusb_get_bus_number(b_dev));
> +same_addr = (libusb_get_device_address(a_dev) == 
> libusb_get_device_address(b_dev));
> +same_vid = (a_desc.idVendor == b_desc.idVendor);
> +same_pid = (a_desc.idProduct == b_desc.idProduct);
>  
>  return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
>  }
> @@ -412,10 +428,17 @@ static void notify_dev_state_change(GUdevClient *self,
>  GList *dev;
>  
>  for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
> -if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == 
> NULL) {
> -/* Found a device that changed 

Re: [Spice-devel] [spice-gtk v2 00/13] use persistent libusb_device on Windows

[Christophe Fergeau]

Hey,

Overall series looks good to me, you seem to have ignored most of the
suggestions to change the commit logs/comments, I dunno if that's
intentional? Still a few comments on "win-usb-dev: maintain list of
libusb devices"

So apart from this patch,

Acked-by: Christophe Fergeau 

Christophe

On Tue, Mar 19, 2019 at 07:55:55AM +0200, Yuri Benditovich wrote:
> This series unifies part of USB redirection code for Windows making
> it the same as for Linux by using persistent libusb_device also on
> Windows.
> 
> Changes from v1:
> minor fixes per v1 review
> rebase to latest master branch
> 
> Yuri Benditovich (13):
>   usb-redir: replace USE_GUDEV with G_OS_WIN32
>   usb-redir: remove unused 'subsystem' parameter
>   usb-redir: reuse libusb context under Windows
>   usb-redir: do not add device if one with the same bus:addr exists
>   win-usb-dev: strict comparison of USB devices
>   win-usb-dev: remove unused procedure
>   usb-redir: discard cold-plug device list under Windows
>   usb-redir: change signal prototype of win-usb-dev
>   win-usb-dev: do not allocate memory for hub devices
>   win-usb-dev: maintain list of libusb devices
>   win-usb-dev: report libusb_device via signal
>   usb-redir: use persistent libusb_device under Windows also
>   win-usb-dev: remove unused code
> 
>  src/spice-marshal.txt|   1 +
>  src/usb-device-manager.c | 267 ++-
>  src/win-usb-dev.c| 254 ++---
>  src/win-usb-dev.h|  35 +
>  4 files changed, 110 insertions(+), 447 deletions(-)
> 
> -- 
> 2.17.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 v2 2/2] worker: Fix potential sprintf overflow

[Christophe Fergeau]

On Wed, Mar 20, 2019 at 03:57:46PM +, Frediano Ziglio wrote:
> From: Christophe Fergeau 
> 
> If worker->qxl->id is bigger than 0x7ff (in other words, it's a
> negative signed int) then
> printf(worker_str, "display[%d]", worker->qxl->id);
> will need:
> 
> "display[]" -> 9 bytes
> %d -> 11 bytes
> 
> The trailing \0 will thus overflow our 20 bytes destination.
> As QXLInstance::id should be an unsigned int, this commit changes the
> format string to use %u. This also switches to snprintf.
> ---
>  server/red-worker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 8051d1e4..50612aca 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>  worker->zlib_glz_state = reds_get_zlib_glz_state(reds);
>  worker->driver_cap_monitors_config = 0;
>  char worker_str[SPICE_STAT_NODE_NAME_MAX];
> -sprintf(worker_str, "display[%d]", worker->qxl->id);
> +snprintf(worker_str, sizeof(worker_str), "display[%u]", (unsigned 
> int)worker->qxl->id);

I'd still add a &0xff at the end to make it explicit that we expect a
uint8_t. It's a patch I wrote, so no further comments ;)

Christophe

>  stat_init_node(>stat, reds, NULL, worker_str, TRUE);
>  stat_init_counter(>wakeup_counter, reds, >stat, 
> "wakeups", TRUE);
>  stat_init_counter(>command_counter, reds, >stat, 
> "commands", TRUE);
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 2/2] red-worker: Avoid possible string formatting warning

[Christophe Fergeau]

On Wed, Mar 20, 2019 at 11:20:20AM -0400, Frediano Ziglio wrote:
> > 
> > On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote:
> > > Although id is not supposed to be big prevent possible
> > > warning/overflow.
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/red-worker.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > This was signaled by Christophe Fergeau
> > > 
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index 8051d1e4..a25a0cd8 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > >  worker->zlib_glz_state = reds_get_zlib_glz_state(reds);
> > >  worker->driver_cap_monitors_config = 0;
> > >  char worker_str[SPICE_STAT_NODE_NAME_MAX];
> > > -sprintf(worker_str, "display[%d]", worker->qxl->id);
> > > +snprintf(worker_str, sizeof(worker_str), "display[%d]",
> > > worker->qxl->id);
> > 
> > You pointed out that in the protocol, the id is 8 bits, so I'd change to
> > worker->qxl->id & 0xff while at it.
> > 
> > Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can
> > still get snprintf to misbehave:
> > "display[]" is 9 bytes
> > %d may need 11 bytes to be printed (if id is less than (unsigned
> > int)-40)
> > so we'd be need 20 bytes in the buffer plus the trailing \0.
> > 
> > Christophe
> > 
> 
> Your patch was better and it has a better explanation.
> 
> The number you brought as example is weird! It does not fit into
> a 32 bit so cannot be worker->qxl->id.

Yep, sorry, unsigned/signed mix up in my head ;) It should have been
-20 

> As we currently support only
> architectures where int is a 32 bit the (unsigned int)whatever will
> fit into 10 characters however formatting with %d will be converted
> to signed (so int) which won't fit in 10 characters (with the
> additional sign it requires 11 characters).

Yep, this is what I meant, with a wrong example.

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 spice-server 2/2] red-worker: Avoid possible string formatting warning

[Christophe Fergeau]

On Wed, Mar 20, 2019 at 02:51:29PM +, Frediano Ziglio wrote:
> Although id is not supposed to be big prevent possible
> warning/overflow.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-worker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This was signaled by Christophe Fergeau
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 8051d1e4..a25a0cd8 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1291,7 +1291,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>  worker->zlib_glz_state = reds_get_zlib_glz_state(reds);
>  worker->driver_cap_monitors_config = 0;
>  char worker_str[SPICE_STAT_NODE_NAME_MAX];
> -sprintf(worker_str, "display[%d]", worker->qxl->id);
> +snprintf(worker_str, sizeof(worker_str), "display[%d]", worker->qxl->id);

You pointed out that in the protocol, the id is 8 bits, so I'd change to
worker->qxl->id & 0xff while at it.

Note that with SPICE_STAT_NODE_NAME_MAX (which is 20), you can
still get snprintf to misbehave:
"display[]" is 9 bytes
%d may need 11 bytes to be printed (if id is less than (unsigned 
int)-40)
so we'd be need 20 bytes in the buffer plus the trailing \0.

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 spice-server 1/2] test-stream-device: Remove interface before next loop

[Christophe Fergeau]

On Tue, Mar 19, 2019 at 01:01:54PM +, Frediano Ziglio wrote:
> We should not reuse the same interface twice as doing so will
> cause dandling pointers.

'dangling'

Acked-by: Christophe Fergeau 


> Unregister it at every iteration.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/tests/test-stream-device.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> This avoids next patch to cause failure during tests.
> 
> diff --git a/server/tests/test-stream-device.c 
> b/server/tests/test-stream-device.c
> index e63952ac..ce37822f 100644
> --- a/server/tests/test-stream-device.c
> +++ b/server/tests/test-stream-device.c
> @@ -342,6 +342,7 @@ static void test_stream_device(TestFixture *fixture, 
> gconstpointer user_data)
>  g_assert(message_sizes_curr - message_sizes == 5 || !device_enabled);
>  
>  check_vmc_error_message();
> +spice_server_remove_interface(_instance.base);
>  }
>  }
>  
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 2/2] reds: Check we don't register a channel twice in reds_register_channel

[Christophe Fergeau]

On Tue, Mar 19, 2019 at 01:01:55PM +, Frediano Ziglio wrote:
> To avoid possibly regression check it only if extra checks are

"To avoid possible/potential regressions, check it .."

> enabled.
> This allows to check previous "Move channel registration to constructed
> vfunc" commit.
> 
> Signed-off-by: Frediano Ziglio 
> Acked-by: Snir Sheriber 
> ---
>  server/reds.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index bc043764..f84ab1b8 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -380,6 +380,11 @@ void stat_remove_counter(SpiceServer *reds, 
> RedStatCounter *counter)
>  void reds_register_channel(RedsState *reds, RedChannel *channel)
>  {
>  spice_assert(reds);
> +if (spice_extra_checks) {
> +uint32_t this_type, this_id;
> +g_object_get(channel, "channel-type", _type, "id", _id, 
> NULL);
> +spice_assert(reds_find_channel(reds, this_type, this_id) == NULL);
> +}

Maybe something like this:

uint32_t this_type, this_id;
g_object_get(channel, "channel-type", _type, "id", _id, NULL);
if (spice_extra_checks) {
g_warn_if_fail(reds_find_channel(reds, this_type, this_id) == NULL);
} else
g_assert(reds_find_channel(reds, this_type, this_id) == NULL);
}

so that we can catch these misuses?

Christophe


>  reds->channels = g_list_prepend(reds->channels, channel);
>  // create new channel in the client if possible
>  main_channel_registered_new_channel(reds->main_channel, channel);
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk v3] smartcard: Warn if multiple readers are detected

[Christophe Fergeau]

Ping? Or should I just go with one of the earlier versions?


On Fri, Feb 22, 2019 at 11:40:32AM +0100, Christophe Fergeau wrote:
> spice-server does not deal properly with multiple smartcard readers,
> only the first one will be working. Add a warning when this happens to
> make it easier to diagnose such issues.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  src/smartcard-manager.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> index ceecfdc7..35bb2757 100644
> --- a/src/smartcard-manager.c
> +++ b/src/smartcard-manager.c
> @@ -389,6 +389,24 @@ typedef struct {
>  GError *err;
>  } SmartcardManagerInitArgs;
>  
> +
> +static void smartcard_reader_free(gpointer data)
> +{
> +g_boxed_free(SPICE_TYPE_SMARTCARD_READER, data);
> +}
> +
> +/* spice-server only supports one smartcard reader being in use */
> +static void smartcard_check_reader_count(void)
> +{
> +GList *readers;
> +
> +readers = 
> spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
> +if (g_list_length(readers) > 1) {
> +g_warning("Multiple smartcard readers are plugged in, only the first 
> one will be shared with the VM");
> +}
> +g_list_free_full(readers, smartcard_reader_free);
> +}
> +
>  static gboolean smartcard_manager_init(SmartcardManagerInitArgs *args)
>  {
>  gchar *emul_args = NULL;
> @@ -442,6 +460,7 @@ init:
>  "Failed to initialize smartcard");
>  goto end;
>  }
> +smartcard_check_reader_count();
>  
>  retval = TRUE;
>  
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk v2 2/2] win-usb-dev: remove ifdef for libusb on 1.0.13

[Christophe Fergeau]

On Fri, Feb 22, 2019 at 05:42:22PM +, Victor Toso wrote:
> From: Victor Toso 
> 
> We already require libusb 1.0.16 or above since 8269a5be62f4ce1
> (build-sys: drop support for libusb < 1.0.16)
> 
> Signed-off-by: Victor Toso 
> ---
>  src/win-usb-dev.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 327976d..b614af0 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -570,9 +570,7 @@ static gboolean g_udev_skip_search(GUdevDevice *udev)
>  g_return_val_if_fail(udevinfo != NULL, FALSE);
>  
>  skip = ((udevinfo->addr == 0xff) ||  /* root hub (HCD) */
> -#if defined(LIBUSBX_API_VERSION) && (LIBUSBX_API_VERSION >= 0x01FF)
> -(udevinfo->addr == 1) || /* root hub addr for libusbx >= 1.0.13 
> */
> -#endif
> +(udevinfo->addr == 1) || /* root hub addr since libusbx 1.0.13 */

I'd drop "since libusbx 1.0.13" from the comment.
Apart from that, series looks good to me.

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 spice-common v3 0/5] Generate C declarations automatically

[Christophe Fergeau]

On Mon, Mar 11, 2019 at 12:42:10PM -0400, Frediano Ziglio wrote:
> > 
> > Series looks good to me,
> > 
> > Reviewed-by: Christophe Fergeau 
> > 
> 
> Why not ack? Not good enough? Not tested? Missing something?

libvirt/qemu use of Reviewed-by/Acked-by confuses me, and to me they are
more or less equivalent.

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 spice-protocol] protocol: Generate enums.h again to remove old protocol definitions

[Christophe Fergeau]

This is breaking spice-protocol API even if this should not impact
current code. Do we want to advertise this somehow? Switch to
spice-protocol 0.14.x?

On Fri, Feb 22, 2019 at 12:16:52PM +, Frediano Ziglio wrote:
> spice.proto was updated to remove really old definitions,
> update enums.h accordingly.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  spice/enums.h | 57 ---
>  1 file changed, 57 deletions(-)
> 
> diff --git a/spice/enums.h b/spice/enums.h
> index 34e84c3..172cc4d 100644
> --- a/spice/enums.h
> +++ b/spice/enums.h
> @@ -119,21 +119,6 @@ typedef enum SpiceMouseMode {
>  SPICE_MOUSE_MODE_MASK = 0x3
>  } SpiceMouseMode;
>  
> -typedef enum SpicePubkeyType {
> -SPICE_PUBKEY_TYPE_INVALID,
> -SPICE_PUBKEY_TYPE_RSA,
> -SPICE_PUBKEY_TYPE_RSA2,
> -SPICE_PUBKEY_TYPE_DSA,
> -SPICE_PUBKEY_TYPE_DSA1,
> -SPICE_PUBKEY_TYPE_DSA2,
> -SPICE_PUBKEY_TYPE_DSA3,
> -SPICE_PUBKEY_TYPE_DSA4,
> -SPICE_PUBKEY_TYPE_DH,
> -SPICE_PUBKEY_TYPE_EC,
> -
> -SPICE_PUBKEY_TYPE_ENUM_END
> -} SpicePubkeyType;
> -
>  typedef enum SpiceDataCompressionType {
>  SPICE_DATA_COMPRESSION_TYPE_NONE,
>  SPICE_DATA_COMPRESSION_TYPE_LZ4,
> @@ -397,21 +382,6 @@ typedef enum SpiceAudioFmt {
>  SPICE_AUDIO_FMT_ENUM_END
>  } SpiceAudioFmt;
>  
> -typedef enum SpiceTunnelServiceType {
> -SPICE_TUNNEL_SERVICE_TYPE_INVALID,
> -SPICE_TUNNEL_SERVICE_TYPE_GENERIC,
> -SPICE_TUNNEL_SERVICE_TYPE_IPP,
> -
> -SPICE_TUNNEL_SERVICE_TYPE_ENUM_END
> -} SpiceTunnelServiceType;
> -
> -typedef enum SpiceTunnelIpType {
> -SPICE_TUNNEL_IP_TYPE_INVALID,
> -SPICE_TUNNEL_IP_TYPE_IPv4,
> -
> -SPICE_TUNNEL_IP_TYPE_ENUM_END
> -} SpiceTunnelIpType;
> -
>  typedef enum SpiceVscMessageType {
>  SPICE_VSC_MESSAGE_TYPE_Init = 1,
>  SPICE_VSC_MESSAGE_TYPE_Error,
> @@ -613,33 +583,6 @@ enum {
>  SPICE_MSGC_END_RECORD
>  };
>  
> -enum {
> -SPICE_MSG_TUNNEL_INIT = 101,
> -SPICE_MSG_TUNNEL_SERVICE_IP_MAP,
> -SPICE_MSG_TUNNEL_SOCKET_OPEN,
> -SPICE_MSG_TUNNEL_SOCKET_FIN,
> -SPICE_MSG_TUNNEL_SOCKET_CLOSE,
> -SPICE_MSG_TUNNEL_SOCKET_DATA,
> -SPICE_MSG_TUNNEL_SOCKET_CLOSED_ACK,
> -SPICE_MSG_TUNNEL_SOCKET_TOKEN,
> -
> -SPICE_MSG_END_TUNNEL
> -};
> -
> -enum {
> -SPICE_MSGC_TUNNEL_SERVICE_ADD = 101,
> -SPICE_MSGC_TUNNEL_SERVICE_REMOVE,
> -SPICE_MSGC_TUNNEL_SOCKET_OPEN_ACK,
> -SPICE_MSGC_TUNNEL_SOCKET_OPEN_NACK,
> -SPICE_MSGC_TUNNEL_SOCKET_FIN,
> -SPICE_MSGC_TUNNEL_SOCKET_CLOSED,
> -SPICE_MSGC_TUNNEL_SOCKET_CLOSED_ACK,
> -SPICE_MSGC_TUNNEL_SOCKET_DATA,
> -SPICE_MSGC_TUNNEL_SOCKET_TOKEN,
> -
> -SPICE_MSGC_END_TUNNEL
> -};
> -
>  enum {
>  SPICE_MSG_SMARTCARD_DATA = 101,
>  
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading

[Christophe Fergeau]

On Mon, Mar 11, 2019 at 02:03:33PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  docs/spice_threading_model.txt | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/docs/spice_threading_model.txt b/docs/spice_threading_model.txt
> index 9351141c8..25a3a030c 100644
> --- a/docs/spice_threading_model.txt
> +++ b/docs/spice_threading_model.txt
> @@ -39,6 +39,14 @@ connect, disconnect and migrate. Connect and migrate are 
> asynchronous (the job
>  is done while the current thread is doing something else) while disconnect is
>  synchronous (the main thread will wait for termination).
>  
> +One aspect to take into consideration is the event scheduling. SPICE uses 
> some
> +`SpiceCoreInterface` to handle events. As the events will be handled from a
> +thread based on the core interface you have to use the correct core. Each
> +channel has an associated core interface which can be retrieved using
> +`red_channel_get_core_interface`. There's also a main core interface you can 
> get
> +using `reds_get_core_interface`. `reds_core_timer_*` and `reds_core_watch_*`
> +functions use the main core interface.

Do we need a few words as to when to use the main core interface?
Apart from this, looks good to me.

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 spice-server 1/2] docs: Fix typo

[Christophe Fergeau]

On Mon, Mar 11, 2019 at 02:03:32PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  docs/spice_threading_model.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/spice_threading_model.txt b/docs/spice_threading_model.txt
> index df4922168..9351141c8 100644
> --- a/docs/spice_threading_model.txt
> +++ b/docs/spice_threading_model.txt
> @@ -1,7 +1,7 @@
>  How does SPICE handle threading
>  ---
>  
> -Lots of code run in a single thread.
> +Lots of code runs in a single thread.

"Lots of" is plural, isn't it? Maybe "A lot of code runs .."

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 spice-server 1/2] Update spice-common submodule

[Christophe Fergeau]

For the series:
Acked-by: Christophe Fergeau 

On Fri, Mar 08, 2019 at 12:25:48PM +, Frediano Ziglio wrote:
> This brings in the following changes:
> 
> Frediano Ziglio (11):
>   codegen: Document ptr_array attribute
>   codegen: Use a better type for pointer converted to integer
>   codegen: Reduce indentation
>   codegen: Fix c_type result for TypeAlias
>   codegen: Check wrong attribute
>   codegen: Add a test for attribute combination
>   mem: Fix compile error if alignment-checks option is used
>   log: Remove useless includes
>   proto: Remove obsolete TunnelChannel
>   protocol: Add a dummy TunnelChannel
>   messages: Remove fields not used by the protocol
> 
> Marc-André Lureau (1):
>   docs: add spice URI scheme
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  subprojects/spice-common | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/subprojects/spice-common b/subprojects/spice-common
> index 02530a80d..92d5dfd4b 16
> --- a/subprojects/spice-common
> +++ b/subprojects/spice-common
> @@ -1 +1 @@
> -Subproject commit 02530a80dfa45c936215c47b8e3aa56720eb46b8
> +Subproject commit 92d5dfd4bfa7ae4857e96504a6f14c336ed85338
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server] syntax-check: Add missing contributors names to AUTHORS

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Mon, Mar 18, 2019 at 10:57:41AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  AUTHORS | 1 +
>  1 file changed, 1 insertion(+)
> 
> This fixed "make syntax-check"
> 
> diff --git a/AUTHORS b/AUTHORS
> index 12158e60..a9b9ea79 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -66,5 +66,6 @@ Patches also contributed by
>  Wang Qiang 
>  Yann E. MORIN 
>  Zeeshan Ali (Khattak) 
> +Douglas Paul 
>  
> send patches to get your name here...
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk 12/13] usb-redir: use persistent libusb_device under Windows also

[Christophe Fergeau]

On Wed, Mar 13, 2019 at 10:38:09AM +0200, Yuri Benditovich wrote:
> On Tue, Mar 12, 2019 at 7:31 PM Christophe Fergeau  
> wrote:
> >
> > On Sun, Mar 10, 2019 at 04:46:11PM +0200, Yuri Benditovich wrote:
> > > Unify SpiceUsbDeviceInfo for Linux and Windows by using
> > > persistent libusb_device.
> >
> > This really needs to be more detailed and explain why this is no longer
> > a valid concern:
> >
> > * On win32 we need to do this the hard and slow way, by asking libusb to
> > * re-enumerate all devices and then finding a matching device.
> > * We cannot cache the libusb_device like we do under Linux since the
> > * driver swap we do under windows invalidates the cached libdev.
> >
> 
> Probably it was written years ago for 'libusb + winusb'; it is just
> incorrect for 'libusb + usbdk'.

Ok, thanks. And usb redirection is not going to work with winusb anyway,
so we should not be concerned about people still using libusb+winusb
instead of libusb+usbdk.
This driver swap is not what can be seen when one inserts an unknown
device in Windows, and then you see it automatically installs a driver?

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 spice-common] test-ssl-verify: Improve subject_to_x509_name coverage

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Wed, Mar 13, 2019 at 12:20:22PM +, Frediano Ziglio wrote:
> Check that attempting to quote an invalid character result in
> a error.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  tests/test-ssl-verify.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/test-ssl-verify.c b/tests/test-ssl-verify.c
> index e307e3f..f5c5881 100644
> --- a/tests/test-ssl-verify.c
> +++ b/tests/test-ssl-verify.c
> @@ -126,6 +126,7 @@ int main(int argc, char *argv[])
>  TEST_SUCCESS(quote2, "=a", "\\:a");
>  TEST_SUCCESS(quote3, "a=\\,b,c=d", "a:,b:c:d");
>  TEST_ERROR(quote4, ",", "");
> +TEST_ERROR(quote5, "a\\w=x", "");
>  
>  TEST_ERROR(no_value1, "a", "");
>  TEST_ERROR(no_value2, "a,b=c", "");
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk 10/13] win-usb-dev: maintain list of libusb devices

[Christophe Fergeau]

On Wed, Mar 13, 2019 at 09:36:06AM +0200, Yuri Benditovich wrote:
> On Tue, Mar 12, 2019 at 7:22 PM Christophe Fergeau  
> wrote:
> >
> > On Sun, Mar 10, 2019 at 04:46:09PM +0200, Yuri Benditovich wrote:
> > > Change internal device list to maintain libusb devices
> > > instead of GUdevDevice objects. Create temporary
> > > GUdevDevice object only for indication to usb-dev-manager.
> > >
> > > Signed-off-by: Yuri Benditovich 
> > > ---
> > >  src/win-usb-dev.c | 80 ++-
> > >  1 file changed, 51 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > > index 1ab704d..5d878ea 100644
> > > --- a/src/win-usb-dev.c
> > > +++ b/src/win-usb-dev.c
> > > @@ -95,7 +95,7 @@ static void g_udev_device_print_list(GList *l, const 
> > > gchar *msg);
> > >  #else
> > >  static void g_udev_device_print_list(GList *l, const gchar *msg) {}
> > >  #endif
> > > -static void g_udev_device_print(GUdevDevice *udev, const gchar *msg);
> > > +static void g_udev_device_print(libusb_device *dev, const gchar *msg);
> > >
> > >  static gboolean g_udev_skip_search(libusb_device *dev);
> > >
> > > @@ -129,8 +129,6 @@ g_udev_client_list_devices(GUdevClient *self, GList 
> > > **devs,
> > >  gssize rc;
> > >  libusb_device **lusb_list, **dev;
> > >  GUdevClientPrivate *priv;
> > > -GUdevDeviceInfo *udevinfo;
> > > -GUdevDevice *udevice;
> > >  ssize_t n;
> > >
> > >  g_return_val_if_fail(G_UDEV_IS_CLIENT(self), -1);
> > > @@ -155,10 +153,7 @@ g_udev_client_list_devices(GUdevClient *self, GList 
> > > **devs,
> > >  if (g_udev_skip_search(*dev)) {
> > >  continue;
> > >  }
> > > -udevinfo = g_new0(GUdevDeviceInfo, 1);
> > > -get_usb_dev_info(*dev, udevinfo);
> > > -udevice = g_udev_device_new(udevinfo);
> > > -*devs = g_list_prepend(*devs, udevice);
> > > +*devs = g_list_prepend(*devs, libusb_ref_device(*dev));
> > >  n++;
> > >  }
> > >  libusb_free_device_list(lusb_list, 1);
> > > @@ -166,11 +161,17 @@ g_udev_client_list_devices(GUdevClient *self, GList 
> > > **devs,
> > >  return n;
> > >  }
> > >
> > > +static void unreference_libusb_device(gpointer data)
> > > +{
> > > +libusb_unref_device((libusb_device *)data);
> > > +}
> > > +
> > >  static void g_udev_client_free_device_list(GList **devs)
> > >  {
> > >  g_return_if_fail(devs != NULL);
> > >  if (*devs) {
> > > -g_list_free_full(*devs, g_object_unref);
> > > +/* avoiding casting of imported procedure to GDestroyNotify */
> > > +g_list_free_full(*devs, unreference_libusb_device);
> >
> > Since all unreference_libusb_device is doing is blindly casting a void *
> > to libusb_device *, I'd just use a cast to GDestroyNotify here rather
> > than introduce an intermediate method. If you prefer to keep that
> 
> unreference_libusb_device not only 'blindly casts' pointers but also
> guarantees that libusb_unref_device is called with proper calling
> conventions. There was commit addressing similar issue
> https://gitlab.freedesktop.org/spice/spice-gtk/commit/558c967ecd230fa6ddde553f6206b1bfd86b40e7

oh, very good point, this is in my opinion important enough to be
mentioned in the commit log, and maybe in the comment.

> 
> > intermediate helper, please remove the commit which I don't think is
> > needed.
> 
> Which exact commit is not needed??

Sorry, I meant 'comment', not 'commit' :)

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-gtk 02/13] usb-redir: remove unused 'subsystem' parameter

[Christophe Fergeau]

On Wed, Mar 13, 2019 at 09:49:19AM +0200, Yuri Benditovich wrote:
> On Mon, Mar 11, 2019 at 7:07 PM Christophe Fergeau  
> wrote:
> >
> > On Sun, Mar 10, 2019 at 04:46:01PM +0200, Yuri Benditovich wrote:
> > > Removing unused parameter for GUdevClient constructor.
> >
> > I'd explicitly mention that it's possible because we no longer use the
> > external libgudev, and at this point g_udev_client_new() is just
> > internal API used by the windows code.
> >
> 
> This was mentioned in previous commit.
> This code is compiled under 'windows only' ifdef.
> As I'm not able to guess how you will want to rephrase commit message please
> feel free to change commit message for any submitted commit as you want.

Sure, I can add a few more lines to the commit message before pushing.
I'll adapt what I said above :)

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-gtk 04/13] usb-redir: do not add device if one with the same bus:addr exists

[Christophe Fergeau]

On Wed, Mar 13, 2019 at 09:40:14AM +0200, Yuri Benditovich wrote:
> On Mon, Mar 11, 2019 at 7:17 PM Christophe Fergeau  
> wrote:
> >
> > On Sun, Mar 10, 2019 at 04:46:03PM +0200, Yuri Benditovich wrote:
> > > In initial device enumeration hotplug notification can be
> > > called twice with the same libusb device. For details, see
> > > http://libusb.sourceforge.net/api-1.0/group__libusb__hotplug.html#ga00e0c69ddf1fb1b6774dc918192e8dc7
> > > Filter out devices that already present in the list.
> > > Remove indentical call in spice_usb_device_manager_add_udev,
> > > which add devices under Windows.
> > >
> > > Signed-off-by: Yuri Benditovich 
> > > ---
> > >  src/usb-device-manager.c | 22 +++---
> > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > index debba4d..5cf7ebb 100644
> > > --- a/src/usb-device-manager.c
> > > +++ b/src/usb-device-manager.c
> > > @@ -962,6 +962,17 @@ static void 
> > > spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
> > >  if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> > >  return;
> > >
> > > +if (spice_usb_device_manager_find_device(self,
> > > +libusb_get_bus_number(libdev),
> > > +libusb_get_device_address(libdev))) {
> >
> > Forgot to mention that indentation is slightly off.
> 
> Please refer exact rule in coding conventions which explains how many
> spaces should be inserted to make you think the indentation is ideal.

I don't know if this is mentioned in a coding style document or not, but
aligning with the opening ( is what is done in most of the file.

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-gtk 13/13] win-usb-dev: remove unused code

[Christophe Fergeau]

Great :) 
Acked-by: Christophe Fergeau 

On Sun, Mar 10, 2019 at 04:46:12PM +0200, Yuri Benditovich wrote:
> Remove unused code related to GUdevDevice.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/win-usb-dev.c | 74 ---
>  src/win-usb-dev.h | 27 -
>  2 files changed, 101 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index f42ab86..f80f377 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -49,31 +49,6 @@ G_DEFINE_TYPE_WITH_CODE(GUdevClient, g_udev_client, 
> G_TYPE_OBJECT,
>  G_ADD_PRIVATE(GUdevClient)
>  G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE, 
> g_udev_client_initable_iface_init))
>  
> -
> -typedef struct _GUdevDeviceInfo GUdevDeviceInfo;
> -
> -struct _GUdevDeviceInfo {
> -guint16 bus;
> -guint16 addr;
> -guint16 vid;
> -guint16 pid;
> -guint16 class;
> -gchar sclass[4];
> -gchar sbus[4];
> -gchar saddr[4];
> -gchar svid[8];
> -gchar spid[8];
> -};
> -
> -struct _GUdevDevicePrivate
> -{
> -/* FixMe: move above fields to this structure and access them directly */
> -GUdevDeviceInfo *udevinfo;
> -};
> -
> -G_DEFINE_TYPE_WITH_PRIVATE(GUdevDevice, g_udev_device, G_TYPE_OBJECT)
> -
> -
>  enum
>  {
>  UEVENT_SIGNAL,
> @@ -452,55 +427,6 @@ static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT 
> message, WPARAM wparam, LPARAM
>  return DefWindowProc(hwnd, message, wparam, lparam);
>  }
>  
> -/*** GUdevDevice ***/
> -
> -static void g_udev_device_finalize(GObject *object)
> -{
> -GUdevDevice *device =  G_UDEV_DEVICE(object);
> -
> -g_free(device->priv->udevinfo);
> -if (G_OBJECT_CLASS(g_udev_device_parent_class)->finalize != NULL)
> -(* G_OBJECT_CLASS(g_udev_device_parent_class)->finalize)(object);
> -}
> -
> -static void g_udev_device_class_init(GUdevDeviceClass *klass)
> -{
> -GObjectClass *gobject_class = (GObjectClass *) klass;
> -
> -gobject_class->finalize = g_udev_device_finalize;
> -}
> -
> -static void g_udev_device_init(GUdevDevice *device)
> -{
> -device->priv = g_udev_device_get_instance_private(device);
> -}
> -
> -const gchar *g_udev_device_get_property(GUdevDevice *udev, const gchar 
> *property)
> -{
> -GUdevDeviceInfo* udevinfo;
> -
> -g_return_val_if_fail(G_UDEV_DEVICE(udev), NULL);
> -g_return_val_if_fail(property != NULL, NULL);
> -
> -udevinfo = udev->priv->udevinfo;
> -g_return_val_if_fail(udevinfo != NULL, NULL);
> -
> -if (g_strcmp0(property, "BUSNUM") == 0) {
> -return udevinfo->sbus;
> -} else if (g_strcmp0(property, "DEVNUM") == 0) {
> -return udevinfo->saddr;
> -} else if (g_strcmp0(property, "DEVTYPE") == 0) {
> -return "usb_device";
> -} else if (g_strcmp0(property, "VID") == 0) {
> -return udevinfo->svid;
> -} else if (g_strcmp0(property, "PID") == 0) {
> -return udevinfo->spid;
> -}
> -
> -g_warn_if_reached();
> -return NULL;
> -}
> -
>  #ifdef DEBUG_GUDEV_DEVICE_LISTS
>  static void g_udev_device_print_list(GList *l, const gchar *msg)
>  {
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index 2f899e0..c04f200 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -26,30 +26,6 @@
>  
>  G_BEGIN_DECLS
>  
> -/* GUdevDevice */
> -
> -#define G_UDEV_TYPE_DEVICE (g_udev_device_get_type())
> -#define G_UDEV_DEVICE(o)   (G_TYPE_CHECK_INSTANCE_CAST((o), 
> G_UDEV_TYPE_DEVICE, GUdevDevice))
> -#define G_UDEV_DEVICE_CLASS(k) (G_TYPE_CHECK_CLASS_CAST((k), 
> G_UDEV_TYPE_DEVICE, GUdevDeviceClass))
> -#define G_UDEV_IS_DEVICE(o)(G_TYPE_CHECK_INSTANCE_TYPE ((o), 
> G_UDEV_TYPE_DEVICE))
> -#define G_UDEV_IS_DEVICE_CLASS(k)  (G_TYPE_CHECK_CLASS_TYPE((k), 
> G_UDEV_TYPE_DEVICE))
> -#define G_UDEV_DEVICE_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS((o), 
> G_UDEV_TYPE_DEVICE, GUdevDeviceClass))
> -
> -typedef struct _GUdevDevice GUdevDevice;
> -typedef struct _GUdevDeviceClass GUdevDeviceClass;
> -typedef struct _GUdevDevicePrivate GUdevDevicePrivate;
> -
> -struct _GUdevDevice
> -{
> -  GObject parent;
> -  GUdevDevicePrivate *priv;
> -};
> -
> -struct _GUdevDeviceClass
> -{
> -  GObjectClass parent_class;
> -};
> -
>  /* GUdevClient */
>  
>  #define G_UDEV_TYPE_CLIENT (g_udev_client_get_type())
> @@ -83,9 +59,6 @@ GUdevClient *g_udev_client_new(void);
>  libusb_context *g_udev_client_get_c

Re: [Spice-devel] [spice-gtk 12/13] usb-redir: use persistent libusb_device under Windows also

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:11PM +0200, Yuri Benditovich wrote:
> Unify SpiceUsbDeviceInfo for Linux and Windows by using
> persistent libusb_device.

This really needs to be more detailed and explain why this is no longer
a valid concern:

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

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-gtk 11/13] win-usb-dev: report libusb_device via signal

[Christophe Fergeau]

 spice_usb_device_manager_uevent_cb(GUdevClient *client,
> -   GUdevDevice *udevice,
> +   libusb_device   *dev,
> int  add,
> gpointer user_data)
>  {
>  SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
>  
>  if (add)
> -spice_usb_device_manager_add_udev(self, udevice);
> +spice_usb_device_manager_add_dev(self, dev);
>  else
> -spice_usb_device_manager_remove_udev(self, udevice);
> +spice_usb_device_manager_remove_dev(self,
> +libusb_get_bus_number(dev),
> +libusb_get_device_address(dev));

indentation

>  }
>  #else
>  struct hotplug_idle_cb_args {
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 5d878ea..f42ab86 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -83,9 +83,7 @@ enum
>  static guint signals[LAST_SIGNAL] = { 0, };
>  static GUdevClient *singleton = NULL;
>  
> -static GUdevDevice *g_udev_device_new(GUdevDeviceInfo *udevinfo);
>  static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, 
> LPARAM lparam);
> -static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo 
> *udevinfo);
>  
>  //uncomment to debug gudev device lists.
>  //#define DEBUG_GUDEV_DEVICE_LISTS
> @@ -249,15 +247,7 @@ static void 
> g_udev_client_initable_iface_init(GInitableIface *iface)
>  
>  static void g_udev_notify_device(GUdevClient *self, libusb_device *dev, int 
> add)
>  {
> -GUdevDeviceInfo *udevinfo;
> -GUdevDevice *udevice;
> -udevinfo = g_new0(GUdevDeviceInfo, 1);
> -if (get_usb_dev_info(dev, udevinfo)) {
> -udevice = g_udev_device_new(udevinfo);
> -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, udevice, add);
> -} else {
> -g_free(udevinfo);
> -}
> +g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev, add);

I'd remove g_udev_notify_device and just put these g_signal_emit() where
needed in the code (as was done before the previous commit).

Apart from these,

Acked-by: Christophe Fergeau 


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

Re: [Spice-devel] [spice-gtk 10/13] win-usb-dev: maintain list of libusb devices

[Christophe Fergeau]

== b_desc.idVendor);
> +same_pid = (a_desc.idProduct == b_desc.idProduct);
>  
>  return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
>  }
> @@ -412,10 +428,17 @@ static void notify_dev_state_change(GUdevClient *self,
>  GList *dev;
>  
>  for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
> -if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == 
> NULL) {
> -/* Found a device that changed its state */
> +GList *found = g_list_find_custom(new_list, dev->data, 
> compare_libusb_devices);
> +if (found == NULL) {
>  g_udev_device_print(dev->data, add ? "add" : "remove");
> -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, add);
> +g_udev_notify_device(self, dev->data, add);
> +} else if (add) {
> +/* keep old existing libusb_device in the new list, when
> +   usb-dev-manager will maintain its own list of libusb_device,
> +   these lists will be completely consistent */
> +libusb_device *temp = found->data;
> +found->data = dev->data;
> +dev->data = temp;

Is this chunk required in this commit for proper behaviour the usb code?
Or will it be required in some follow-up commit? If it's only going to
be required later, I'd squash this bit in the commit where it's needed
so that it's clearer why we want this.

Apart from these,

Acked-by: Christophe Fergeau 


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

Re: [Spice-devel] [spice-gtk 09/13] win-usb-dev: do not allocate memory for hub devices

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:08PM +0200, Yuri Benditovich wrote:
> When processing list of USB devices, avoid allocating memory
> for devices which later will be skipped. Use existing libusb
> struct first to check whether the device shall be excluded.

I probably would reword the short log
"win-usb-dev: do not allocate memory for hub devices"
-> "win-usb-dev: skip hub devices early in g_udev_client_list_devices"

> -static gboolean g_udev_skip_search(GUdevDevice *udev)
> +static gboolean g_udev_skip_search(libusb_device *dev)
>  {
> -GUdevDeviceInfo* udevinfo;
>  gboolean skip;
> +uint8_t addr  = libusb_get_device_address(dev);
> +struct libusb_device_descriptor desc;
>  
> -g_return_val_if_fail(G_UDEV_DEVICE(udev), FALSE);
> -
> -udevinfo = udev->priv->udevinfo;
> -g_return_val_if_fail(udevinfo != NULL, FALSE);
> +libusb_get_device_descriptor(dev, );

Do you know if these
libusb_get_device_descriptor/libusb_get_device_address are going to be
expensive compared to using cached values?

>  
> -skip = ((udevinfo->addr == 0xff) ||  /* root hub (HCD) */
> +skip = ((addr == 0xff) ||  /* root hub (HCD) */
>  #if defined(LIBUSBX_API_VERSION) && (LIBUSBX_API_VERSION >= 0x01FF)
> -(udevinfo->addr == 1) || /* root hub addr for libusbx >= 1.0.13 
> */
> +(addr == 1) || /* root hub addr for libusbx >= 1.0.13 */

Note for later: I'm not sure this still needs to be conditional as we
depend on libusb 1.0.16 now.

Looks good otherwise,

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-gtk 08/13] usb-redir: change signal prototype of win-usb-dev

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:07PM +0200, Yuri Benditovich wrote:
> Changing signal definition from (boxed-boxed) to (pointer,int).
> There is no need for additional referencing of GUdevDevice
> object before signal callback.

I still feel it would be nicer to guarantee the GUdevDevice will stay
alive for the whole duration of the signal emission. For example, 2
callbacks may be attached to this signal, we don't want the first one to
be able to drop the last ref to the GUdevDevice and risking the second
one trying to use freed memory.

> Second parameter (action) is 0 for device removal and 1 for device
> addition.

Imo this should either be a gboolean or an enum.

> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c |  8 
>  src/win-usb-dev.c| 18 +-
>  src/win-usb-dev.h|  2 +-
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index f7b43f0..c1a0c92 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -153,8 +153,8 @@ static void channel_event(SpiceChannel *channel, 
> SpiceChannelEvent event,
>gpointer user_data);
>  #ifdef G_OS_WIN32
>  static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> -   const gchar *action,
> GUdevDevice *udevice,
> +   int  add,
> gpointer user_data);
>  static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager  *self,
>GUdevDevice*udev);
> @@ -1070,15 +1070,15 @@ static void 
> spice_usb_device_manager_remove_udev(SpiceUsbDeviceManager  *self,
>  }
>  
>  static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> -   const gchar *action,
> GUdevDevice *udevice,
> +   int  add,
> gpointer user_data)
>  {
>  SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
>  
> -if (g_str_equal(action, "add"))
> +if (add)
>  spice_usb_device_manager_add_udev(self, udevice);
> -else if (g_str_equal (action, "remove"))
> +else
>  spice_usb_device_manager_remove_udev(self, udevice);
>  }
>  #else
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 2c122cf..c74dd57 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -249,7 +249,7 @@ static void 
> g_udev_client_initable_iface_init(GInitableIface *iface)
>  
>  static void report_one_device(gpointer data, gpointer self)
>  {
> -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "add", data);
> +g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);

Ok, so the signal would use a gboolean

>  }
>  
>  void g_udev_client_report_devices(GUdevClient *self)
> @@ -342,11 +342,11 @@ static void g_udev_client_class_init(GUdevClientClass 
> *klass)
>   G_SIGNAL_RUN_FIRST,
>   G_STRUCT_OFFSET(GUdevClientClass, uevent),
>   NULL, NULL,
> - g_cclosure_user_marshal_VOID__BOXED_BOXED,
> + g_cclosure_user_marshal_VOID__POINTER_INT,

This can be g_cclosure_user_marshal_VOID__POINTER_BOOLEAN once
src/spice-marshal.txt has VOID:POINTER,BOOLEAN

>   G_TYPE_NONE,
>   2,
> - G_TYPE_STRING,
> - G_UDEV_TYPE_DEVICE);
> + G_TYPE_POINTER,
> + G_TYPE_INT);
>  
>  /**
>  * GUdevClient::redirecting:
> @@ -408,15 +408,15 @@ static gint gudev_devices_differ(gconstpointer a, 
> gconstpointer b)
>  static void notify_dev_state_change(GUdevClient *self,
>  GList *old_list,
>  GList *new_list,
> -const gchar *action)
> +int add)
>  {
>  GList *dev;
>  
>  for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
>  if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == 
> NULL) {
>  /* Found a device that changed its state */
> -g_udev_device_print(dev->data, action);
> -g_signal_emit(self, signals[UEVENT_SIGNAL], 0, action, 
> dev->data);
> +g_udev_device_print(dev->data, add ? "add" : "remove");
> +g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, add);
>  }
>  }
>  }
> @@ -445,10 +445,10 @@ static void handle_dev_change(GUdevClient *self)
>  g_udev_device_print_list(priv->udev_list, "handle_dev_change: 

Re: [Spice-devel] [spice-gtk 07/13] usb-redir: discard cold-plug device list under Windows

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:06PM +0200, Yuri Benditovich wrote:

Maybe the shortlog would be clearer as
"usb-redir: remove SpiceUsbDeviceManager::coldplug_list"

> Discard the optimization of initial device enumeration.
> Just after connection to 'udev' signal request to report

"... to the 'uevent' signal, request to report ..."

> all the devices  one by one as if they are inserted.
> Further commits will remove device enumeration in
> usb-dev-manager completely.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c | 19 +++
>  src/win-usb-dev.c| 11 +++
>  src/win-usb-dev.h|  2 +-
>  3 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 5cf7ebb..f7b43f0 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -104,7 +104,6 @@ struct _SpiceUsbDeviceManagerPrivate {
>  int redirect_on_connect_rules_count;
>  #ifdef G_OS_WIN32
>  GUdevClient *udev;
> -libusb_device **coldplug_list; /* Avoid needless reprobing during init */
>  #else
>  gboolean redirecting; /* Handled by GUdevClient in the gudev case */
>  libusb_hotplug_callback_handle hp_handle;
> @@ -307,15 +306,7 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  g_signal_connect(G_OBJECT(priv->udev), "uevent",
>   G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
>  /* Do coldplug (detection of already connected devices) */
> -libusb_get_device_list(priv->context, >coldplug_list);
> -list = g_udev_client_query_by_subsystem(priv->udev, "usb");
> -for (it = g_list_first(list); it; it = g_list_next(it)) {
> -spice_usb_device_manager_add_udev(self, it->data);
> -g_object_unref(it->data);
> -}
> -g_list_free(list);
> -libusb_free_device_list(priv->coldplug_list, 1);
> -priv->coldplug_list = NULL;
> +g_udev_client_report_devices(priv->udev);

I think I would have gone with 'coldplug' or 'signal' instead of
'report" but that really is nitpicking


Acked-by: Christophe Fergeau 



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] char-device: Make RedClient an opaque structure again

[Christophe Fergeau]

On Tue, Mar 12, 2019 at 05:58:40AM -0400, Frediano Ziglio wrote:
> ping
> 
> > 
> > RedClient was an opaque structure for RedCharDevice.
> > It started to be used when RedsState started to contain all
> > the global state.
> > Make it opaque again.

It seems we don't put the same meaning to 'opaque'. For me, RedClient is
already an opaque structure from a char-device.c perspective as it's not
dereferencing RedClient * (ie it does not know/care which fields are
defined in struct RedClient).

This commit goes further than that as it removes any calls to
red_client_* API. char-device.c still cares that it is handling a
pointer to a RedClient * (as opposed to a gpointer client_handle which
could be anything). Do you have some longer term goal with respect to
RedClient and RedCharDevice, or is this just an isolated patch?

Christophe


> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/char-device.c | 16 +++-
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/server/char-device.c b/server/char-device.c
> > index 040b91147..465c1a125 100644
> > --- a/server/char-device.c
> > +++ b/server/char-device.c
> > @@ -22,8 +22,8 @@
> >  
> >  #include 
> >  #include 
> > +
> >  #include "char-device.h"
> > -#include "red-client.h"
> >  #include "reds.h"
> >  #include "glib-compat.h"
> >  
> > @@ -703,11 +703,10 @@ void red_char_device_destroy(RedCharDevice *char_dev)
> >  g_object_unref(char_dev);
> >  }
> >  
> > -static RedCharDeviceClient *red_char_device_client_new(RedClient *client,
> > -   int do_flow_control,
> > -   uint32_t
> > max_send_queue_size,
> > -   uint32_t
> > num_client_tokens,
> > -   uint32_t
> > num_send_tokens)
> > +static RedCharDeviceClient *
> > +red_char_device_client_new(RedsState *reds, RedClient *client,
> > +   int do_flow_control, uint32_t
> > max_send_queue_size,
> > +   uint32_t num_client_tokens, uint32_t
> > num_send_tokens)
> >  {
> >  RedCharDeviceClient *dev_client;
> >  
> > @@ -717,8 +716,6 @@ static RedCharDeviceClient
> > *red_char_device_client_new(RedClient *client,
> >  dev_client->max_send_queue_size = max_send_queue_size;
> >  dev_client->do_flow_control = do_flow_control;
> >  if (do_flow_control) {
> > -RedsState *reds = red_client_get_server(client);
> > -
> >  dev_client->wait_for_tokens_timer =
> >  reds_core_timer_add(reds, 
> > device_client_wait_for_tokens_timeout,
> >  dev_client);
> > @@ -757,7 +754,8 @@ bool red_char_device_client_add(RedCharDevice *dev,
> >  dev->priv->wait_for_migrate_data = wait_for_migrate_data;
> >  
> >  spice_debug("char device %p, client %p", dev, client);
> > -dev_client = red_char_device_client_new(client, do_flow_control,
> > +dev_client = red_char_device_client_new(dev->priv->reds, client,
> > +do_flow_control,
> >  max_send_queue_size,
> >  num_client_tokens,
> >  num_send_tokens);
> ___
> 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] [vdagent-win PATCH] Remove support for very old Fedora versions

[Christophe Fergeau]

Ah sure, 
Acked-by: Christophe Fergeau 

On Wed, Feb 27, 2019 at 10:02:37AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  mingw-spice-vdagent.spec.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mingw-spice-vdagent.spec.in b/mingw-spice-vdagent.spec.in
> index d129a7b..5fa9b7a 100644
> --- a/mingw-spice-vdagent.spec.in
> +++ b/mingw-spice-vdagent.spec.in
> @@ -2,7 +2,7 @@
>  
>  #define _version_suffix -e198
>  
> -%if "%{_build_arch}" == "x86_64" && (0%{?fedora} && 0%{?fedora} >= 24)
> +%if "%{_build_arch}" == "x86_64" && 0%{?fedora}
>  %define can_do_check_x64 1
>  %define can_do_check_x86 1
>  %else
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk 06/13] win-usb-dev: remove unused procedure

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:05PM +0200, Yuri Benditovich wrote:
> Remove unused g_udev_device_get_sysfs_attr.
> 

Indeed, last user was apparently removed in 45cfbe8f86c in July 2013.

Acked-by: Christophe Fergeau 

> Signed-off-by: Yuri Benditovich 
> ---
>  src/win-usb-dev.c | 18 --
>  src/win-usb-dev.h |  1 -
>  2 files changed, 19 deletions(-)
> 
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index d96e52a..85ffd26 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -521,24 +521,6 @@ const gchar *g_udev_device_get_property(GUdevDevice 
> *udev, const gchar *property
>  return NULL;
>  }
>  
> -const gchar *g_udev_device_get_sysfs_attr(GUdevDevice *udev, const gchar 
> *attr)
> -{
> -GUdevDeviceInfo* udevinfo;
> -
> -g_return_val_if_fail(G_UDEV_DEVICE(udev), NULL);
> -g_return_val_if_fail(attr != NULL, NULL);
> -
> -udevinfo = udev->priv->udevinfo;
> -g_return_val_if_fail(udevinfo != NULL, NULL);
> -
> -
> -if (g_strcmp0(attr, "bDeviceClass") == 0) {
> -return udevinfo->sclass;
> -}
> -g_warn_if_reached();
> -return NULL;
> -}
> -
>  #ifdef DEBUG_GUDEV_DEVICE_LISTS
>  static void g_udev_device_print_list(GList *l, const gchar *msg)
>  {
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index f3c7466..b7b7eda 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -85,7 +85,6 @@ GList *g_udev_client_query_by_subsystem(GUdevClient 
> *client, const gchar *subsys
>  
>  GType g_udev_device_get_type(void) G_GNUC_CONST;
>  const gchar *g_udev_device_get_property(GUdevDevice *udev, const gchar 
> *property);
> -const gchar *g_udev_device_get_sysfs_attr(GUdevDevice *udev, const gchar 
> *attr);
>  
>  GQuark g_udev_client_error_quark(void);
>  #define G_UDEV_CLIENT_ERROR g_udev_client_error_quark()
> -- 
> 2.17.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] [spice-gtk 04/13] usb-redir: do not add device if one with the same bus:addr exists

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:03PM +0200, Yuri Benditovich wrote:
> In initial device enumeration hotplug notification can be
> called twice with the same libusb device. For details, see
> http://libusb.sourceforge.net/api-1.0/group__libusb__hotplug.html#ga00e0c69ddf1fb1b6774dc918192e8dc7
> Filter out devices that already present in the list.
> Remove indentical call in spice_usb_device_manager_add_udev,
> which add devices under Windows.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index debba4d..5cf7ebb 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -962,6 +962,17 @@ static void 
> spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>  if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
>  return;
>  
> +if (spice_usb_device_manager_find_device(self,
> +libusb_get_bus_number(libdev),
> +libusb_get_device_address(libdev))) {

Forgot to mention that indentation is slightly off.

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-gtk 04/13] usb-redir: do not add device if one with the same bus:addr exists

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:03PM +0200, Yuri Benditovich wrote:
> In initial device enumeration hotplug notification can be

"In initial device enumeration, hotplug notification..."

> called twice with the same libusb device. For details, see
> http://libusb.sourceforge.net/api-1.0/group__libusb__hotplug.html#ga00e0c69ddf1fb1b6774dc918192e8dc7
> Filter out devices that already present in the list.

"This commit filters out the devices that are already known to
SpiceUsbDeviceManager."

> Remove indentical call in spice_usb_device_manager_add_udev,
> which add devices under Windows.

"It also removes the identical call ..., which adds devices..."

Acked-by: Christophe Fergeau 


> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index debba4d..5cf7ebb 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -962,6 +962,17 @@ static void 
> spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>  if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
>  return;
>  
> +if (spice_usb_device_manager_find_device(self,
> +libusb_get_bus_number(libdev),
> +libusb_get_device_address(libdev))) {
> +SPICE_DEBUG("device not added %d:%d %04x:%04x",
> +libusb_get_bus_number(libdev),
> +libusb_get_device_address(libdev),
> +desc.idVendor,
> +desc.idProduct);
> +return;
> +}
> +
>  device = (SpiceUsbDevice*)spice_usb_device_new(libdev);
>  if (!device)
>  return;
> @@ -1025,7 +1036,6 @@ static void 
> spice_usb_device_manager_add_udev(SpiceUsbDeviceManager  *self,
>  {
>  SpiceUsbDeviceManagerPrivate *priv = self->priv;
>  libusb_device *libdev = NULL, **dev_list = NULL;
> -SpiceUsbDevice *device;
>  const gchar *devtype;
>  int i, bus, address;
>  
> @@ -1039,16 +1049,6 @@ static void 
> spice_usb_device_manager_add_udev(SpiceUsbDeviceManager  *self,
>  return;
>  }
>  
> -device = spice_usb_device_manager_find_device(self, bus, address);
> -if (device) {
> -SPICE_DEBUG("USB device 0x%04x:0x%04x at %d.%d already exists, 
> ignored",
> -spice_usb_device_get_vid(device),
> -spice_usb_device_get_pid(device),
> -spice_usb_device_get_busnum(device),
> -spice_usb_device_get_devaddr(device));
> -return;
> -}
> -
>  if (priv->coldplug_list)
>  dev_list = priv->coldplug_list;
>  else
> -- 
> 2.17.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] [spice-gtk 03/13] usb-redir: reuse libusb context under Windows

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:02PM +0200, Yuri Benditovich wrote:
> Do not create own libusb context in usb-device-manager.
> Reuse existing context created by win-usb-dev instead.

I'd rephrase this slightly
« On Windows, do not create a new libusb context in
usb-device-manager.c, but reuse the existing one created by
win-usb-dev.c » + an explanation why we want to do that.
Iirc, we need a shared libusb_context if we want to share libusb_device
between the 2 files, so I'd add that to the commit log.

Apart from this, 

Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c | 11 +--
>  src/win-usb-dev.c|  4 
>  src/win-usb-dev.h|  1 +
>  3 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 6435be8..debba4d 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -282,8 +282,9 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  SpiceUsbDeviceManagerPrivate *priv = self->priv;
>  GList *list;
>  GList *it;
> -int rc;
>  
> +#ifndef G_OS_WIN32
> +int rc;
>  /* Initialize libusb */
>  rc = libusb_init(>context);
>  if (rc < 0) {
> @@ -293,11 +294,6 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  "Error initializing USB support: %s [%i]", desc, rc);
>  return FALSE;
>  }
> -
> -#ifdef G_OS_WIN32
> -#if LIBUSB_API_VERSION >= 0x01000106
> -libusb_set_option(priv->context, LIBUSB_OPTION_USE_USBDK);
> -#endif
>  #endif
>  
>  /* Start listening for usb devices plug / unplug */
> @@ -307,6 +303,7 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  g_warning("Error initializing GUdevClient");
>  return FALSE;
>  }
> +priv->context = g_udev_client_get_context(priv->udev);
>  g_signal_connect(G_OBJECT(priv->udev), "uevent",
>   G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
>  /* Do coldplug (detection of already connected devices) */
> @@ -402,8 +399,10 @@ static void spice_usb_device_manager_finalize(GObject 
> *gobject)
>  g_clear_object(>udev);
>  #endif
>  g_return_if_fail(priv->event_thread == NULL);
> +#ifndef G_OS_WIN32
>  if (priv->context)
>  libusb_exit(priv->context);
> +#endif
>  free(priv->auto_conn_filter_rules);
>  free(priv->redirect_on_connect_rules);
>  #ifdef G_OS_WIN32
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index d0eae06..a8d922f 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -113,6 +113,10 @@ GUdevClient *g_udev_client_new(void)
>  return singleton;
>  }
>  
> +libusb_context *g_udev_client_get_context(GUdevClient *client)
> +{
> +return client->priv->ctx;
> +}
>  
>  /*
>   * devs [in,out] an empty devs list in, full devs list out
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index 0f34a01..f3c7466 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -80,6 +80,7 @@ struct _GUdevClientClass
>  
>  GType g_udev_client_get_type(void) G_GNUC_CONST;
>  GUdevClient *g_udev_client_new(void);
> +libusb_context *g_udev_client_get_context(GUdevClient *client);
>  GList *g_udev_client_query_by_subsystem(GUdevClient *client, const gchar 
> *subsystem);
>  
>  GType g_udev_device_get_type(void) G_GNUC_CONST;
> -- 
> 2.17.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] [spice-gtk 02/13] usb-redir: remove unused 'subsystem' parameter

[Christophe Fergeau]

On Sun, Mar 10, 2019 at 04:46:01PM +0200, Yuri Benditovich wrote:
> Removing unused parameter for GUdevClient constructor.

I'd explicitly mention that it's possible because we no longer use the
external libgudev, and at this point g_udev_client_new() is just
internal API used by the windows code.

Apart from this minor comment, 
Acked-by: Christophe Fergeau 

Christophe

> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c | 5 +
>  src/win-usb-dev.c| 2 +-
>  src/win-usb-dev.h| 2 +-
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 6a36cfa..6435be8 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -283,9 +283,6 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  GList *list;
>  GList *it;
>  int rc;
> -#ifdef G_OS_WIN32
> -const gchar *const subsystems[] = {"usb", NULL};
> -#endif
>  
>  /* Initialize libusb */
>  rc = libusb_init(>context);
> @@ -305,7 +302,7 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  
>  /* Start listening for usb devices plug / unplug */
>  #ifdef G_OS_WIN32
> -priv->udev = g_udev_client_new(subsystems);
> +priv->udev = g_udev_client_new();
>  if (priv->udev == NULL) {
>  g_warning("Error initializing GUdevClient");
>  return FALSE;
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 327976d..d0eae06 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -104,7 +104,7 @@ GQuark g_udev_client_error_quark(void)
>  return g_quark_from_static_string("win-gudev-client-error-quark");
>  }
>  
> -GUdevClient *g_udev_client_new(const gchar* const *subsystems)
> +GUdevClient *g_udev_client_new(void)
>  {
>  if (singleton != NULL)
>  return g_object_ref(singleton);
> diff --git a/src/win-usb-dev.h b/src/win-usb-dev.h
> index 7f40197..0f34a01 100644
> --- a/src/win-usb-dev.h
> +++ b/src/win-usb-dev.h
> @@ -79,7 +79,7 @@ struct _GUdevClientClass
>  };
>  
>  GType g_udev_client_get_type(void) G_GNUC_CONST;
> -GUdevClient *g_udev_client_new(const gchar* const *subsystems);
> +GUdevClient *g_udev_client_new(void);
>  GList *g_udev_client_query_by_subsystem(GUdevClient *client, const gchar 
> *subsystem);
>  
>  GType g_udev_device_get_type(void) G_GNUC_CONST;
> -- 
> 2.17.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] [spice-gtk 01/13] usb-redir: replace USE_GUDEV with G_OS_WIN32

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Sun, Mar 10, 2019 at 04:46:00PM +0200, Yuri Benditovich wrote:
> Replacing USE_GUDEV with G_OS_WIN32 anywhere. GUDEV simulation
> is used only in Windows build.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  src/usb-device-manager.c | 26 +++---
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 2578350..6a36cfa 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -29,11 +29,7 @@
>  
>  #ifdef G_OS_WIN32
>  #include "usbdk_api.h"
> -#endif
> -
> -#if defined(G_OS_WIN32)
>  #include "win-usb-dev.h"
> -#define USE_GUDEV /* win-usb-dev.h provides a fake gudev interface */
>  #endif
>  
>  #include "channel-usbredir-priv.h"
> @@ -106,7 +102,7 @@ struct _SpiceUsbDeviceManagerPrivate {
>  struct usbredirfilter_rule *redirect_on_connect_rules;
>  int auto_conn_filter_rules_count;
>  int redirect_on_connect_rules_count;
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  GUdevClient *udev;
>  libusb_device **coldplug_list; /* Avoid needless reprobing during init */
>  #else
> @@ -156,7 +152,7 @@ static void channel_destroy(SpiceSession *session, 
> SpiceChannel *channel,
>  gpointer user_data);
>  static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
>gpointer user_data);
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> const gchar *action,
> GUdevDevice *udevice,
> @@ -210,7 +206,7 @@ G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
>  static void
>  _set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
>  {
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
>  #else
>  self->priv->redirecting = is_redirecting;
> @@ -235,7 +231,7 @@ gboolean 
> spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager *self)
>  {
>  #ifdef USE_USBREDIR
>  
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  gboolean redirecting;
>  g_object_get(self->priv->udev, "redirecting", , NULL);
>  return redirecting;
> @@ -287,7 +283,7 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  GList *list;
>  GList *it;
>  int rc;
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  const gchar *const subsystems[] = {"usb", NULL};
>  #endif
>  
> @@ -308,7 +304,7 @@ static gboolean 
> spice_usb_device_manager_initable_init(GInitable  *initable,
>  #endif
>  
>  /* Start listening for usb devices plug / unplug */
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  priv->udev = g_udev_client_new(subsystems);
>  if (priv->udev == NULL) {
>  g_warning("Error initializing GUdevClient");
> @@ -366,7 +362,7 @@ static void spice_usb_device_manager_dispose(GObject 
> *gobject)
>  SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(gobject);
>  SpiceUsbDeviceManagerPrivate *priv = self->priv;
>  
> -#ifndef USE_GUDEV
> +#ifndef G_OS_WIN32
>  if (priv->hp_handle) {
>  spice_usb_device_manager_stop_event_listening(self);
>  if (g_atomic_int_get(>event_thread_run)) {
> @@ -405,7 +401,7 @@ static void spice_usb_device_manager_finalize(GObject 
> *gobject)
>  g_ptr_array_unref(priv->devices);
>  
>  #ifdef USE_USBREDIR
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  g_clear_object(>udev);
>  #endif
>  g_return_if_fail(priv->event_thread == NULL);
> @@ -737,7 +733,7 @@ static void 
> spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>  /* -- */
>  /* gudev / libusb Helper functions*/
>  
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  static gboolean spice_usb_device_manager_get_udev_bus_n_address(
>  SpiceUsbDeviceManager *manager, GUdevDevice *udev,
>  int *bus, int *address)
> @@ -927,7 +923,7 @@ 
> spice_usb_device_manager_device_match(SpiceUsbDeviceManager *self, 
> SpiceUsbDevic
>  spice_usb_device_get_devaddr(device) == address);
>  }
>  
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
>  static gboolean
>  spice_usb_device_manager_libdev_match(SpiceUsbDeviceManager *self, 
> libusb_device *libdev,
>const in

Re: [Spice-devel] [PATCH spice-common v3 0/5] Generate C declarations automatically

[Christophe Fergeau]

Series looks good to me, 

Reviewed-by: Christophe Fergeau 

On Mon, Mar 11, 2019 at 01:59:09PM +, Frediano Ziglio wrote:
> Allows to declare C declarations (that were in common/messages.h)
> automatically.
> I added an attribute to:
> - be compatible;
> - allows to declare structure in different way than automatic
>   ones;
> - allows to use different headers (as draw.h).
> 
> Changes since v2:
> - use a --generate-header option instead of --generated-header not passing
>   file name but infere from output file name;
> - remove some TODOs not much related;
> - commit messages improvements.
> 
> Frediano Ziglio (5):
>   codegen: Factor out a function to write output file
>   codegen: Generate headers while generating code
>   codegen: Allows to generate C declarations automatically
>   Allow to generate C declarations for spice.proto
>   Generate automatically most C message declarations
> 
>  common/Makefile.am  |  22 +-
>  common/client_marshallers.h |   2 +-
>  common/meson.build  |  27 +-
>  common/messages.h   | 493 +---
>  python_modules/ptypes.py|  64 +
>  spice.proto | 192 +++---
>  spice_codegen.py| 108 ++--
>  7 files changed, 284 insertions(+), 624 deletions(-)
> 
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-common 4/9] codegen: Allows to generate C declarations automatically

[Christophe Fergeau]

On Fri, Mar 08, 2019 at 04:47:35PM -0500, Frediano Ziglio wrote:
> > 
> > On Sun, Mar 03, 2019 at 07:10:25PM +, Frediano Ziglio wrote:
> > > Allows to specify a @declare attribute for messages and structure
> > > that can generate the needed C structures.
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  python_modules/ptypes.py | 64 
> > >  spice_codegen.py | 47 +
> > >  2 files changed, 111 insertions(+)
> > > 
> > > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > > index 7dca78d..64c198e 100644
> > > --- a/python_modules/ptypes.py
> > > +++ b/python_modules/ptypes.py
> > > @@ -72,6 +72,8 @@ valid_attributes=set([
> > >  'zero',
> > >  # this attribute does not exist on the network, fill just structure
> > >  with the value
> > >  'virtual',
> > > +# generate C structure declarations from protocol definition
> > > +'declare',
> > >  ])
> > >  
> > >  attributes_with_arguments=set([
> > > @@ -485,6 +487,26 @@ class ArrayType(Type):
> > >  def c_type(self):
> > >  return self.element_type.c_type()
> > >  
> > > +def generate_c_declaration(self, writer, member):
> > > +name = member.name
> > > +if member.has_attr("chunk"):
> > > +return writer.writeln('SpiceChunks *%s;' % name)
> > > +if member.has_attr("as_ptr"):
> > > +len_var = member.attributes["as_ptr"][0]
> > > +writer.writeln('uint32_t %s;' % len_var)
> > > +return writer.writeln('%s *%s;' % (self.c_type(), name))
> > > +if member.has_attr("to_ptr"): # TODO len
> > > +return writer.writeln('%s *%s;' % (self.c_type(), name))
> > > +if member.has_attr("ptr_array"): # TODO where the length is
> > > stored? overflow?
> > > +return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> > > +if member.has_end_attr() or self.is_remaining_length(): # TODO 
> > > len
> > > +return writer.writeln('%s %s[0];' % (self.c_type(), name))
> > 
> > These TODO are worrying, but only the has_end_attr() one seems to be run
> > at the moment, the others seem to be dead code?
> > 
> 
> Indeed. And also is more a "for security we should avoid that in the
> protocol" so it's not much related to code generation.
> The idea is that "if you have an array as output you should also have
> the length in the output otherwise you are probably generating an
> overflow".
> 
> The check should be done parsing the protocol file, not generating
> a specific code from it (surely demarshaller code is affected).
> 
> Maybe the best is to move these TODOs in a separate patch.
> And possibly I'll keep it somewhere when I'll have the time to
> generate the proper checks.

Hmm, ok, might indeed be better to move them to a different commit.

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 spice-common 3/9] codegen: Generate headers while generating code

[Christophe Fergeau]

On Fri, Mar 08, 2019 at 04:42:24PM -0500, Frediano Ziglio wrote:
> > 
> > 
> > Bit unsure about the option name. --header-name maybe? Or just infer it
> > from the .c file name?
> > 
> 
> It's similar to --generate-enums and similars. Yes, on the other end this
> option is an additional generation and has a parameter so it not a big
> match indeed. The infer of the header is a good suggestion.
> Maybe a --generate-also-header ?

I don't like the -also- ;) I don't think it's worth debating the name
for ages, if --generate-header is good for you, let's go for it for
consistency with --generate-enus & friends (which I did not notice
before).

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 spice-common 5/9] Allow to generate C declarations for spice.proto

[Christophe Fergeau]

On Fri, Mar 08, 2019 at 04:51:20PM -0500, Frediano Ziglio wrote:
> > 
> > On Sun, Mar 03, 2019 at 07:10:26PM +, Frediano Ziglio wrote:
> > > Generate and include C declarations.
> > > Next patch will use this facility.
> > 
> > Since none of the spice.proto types are decorated with @declare, adding
> > the #include in messages.h won't have any bad consequences
> > 
> 
> Are you suggesting to add a comment or to remove the last hunk?

I would add this in the commit log.

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 v2] display-channel: monitors_config: add 200ms delay

[Christophe Fergeau]

On Mon, Mar 11, 2019 at 12:12:45PM +, Victor Toso wrote:
> Hi,
> 
> On Mon, Mar 11, 2019 at 12:33:20PM +0100, Christophe Fergeau wrote:
> > Hey,
> > 
> > One comment, even if the commit log mentions a bug #, it does not seem
> > to be describing what the bug is/how it's fixed (maybe this can be
> > inferred from the traces, but I did not read them closely as the log
> > does not hint that a bug can be seen by looking at them).
> 
> Oh, well. There is several ways we can write a commit log, like
> there is several ways on how we tell a story. Most of the times,
> I try to explain what the change is about. I do refer to BZ that
> they fix because it is helpful for maintainers and backporting
> patches but I don't see why I need to duplicate BZ comments in
> the commit log too.
> 
> This patch avoids sending multiple monitors_config message to the
> client during short period of time and I tried to explain that.
> I'm more than happy to understand how I can improve the commit
> log but I did not understand how from your comment.

If this commit is fixing a specific bug, then the commit log should have
some description of it/what the fix is. With git, it's entirely possible
that one will be looking at that commit while offline/with bad internet
connection. The bug report may have moved to a different system with a
different bug # in the mean time, the bug report may be long and
confusing, ...
So having a summary in the commit log fixing the bug is imo helpful.

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 v2] display-channel: monitors_config: add 200ms delay

[Christophe Fergeau]

Hey,

One comment, even if the commit log mentions a bug #, it does not seem
to be describing what the bug is/how it's fixed (maybe this can be
inferred from the traces, but I did not read them closely as the log
does not hint that a bug can be seen by looking at them).

Christophe

On Mon, Mar 11, 2019 at 10:02:27AM +, Victor Toso wrote:
> From: Victor Toso 
> 
> The device driver might take a few interactions to reconfigure all
> displays but in each change it can send it down to spice-server. The
> client is not interested in temporary states, only the final monitor
> config can be helpful.
> 
> This patch adds a 200ms delay to wait for more changes from guest
> device. Tested only with QXL in Fedora 29.
> 
> Simple example below, with 4 displays enabled but removing the display 1
> (starting from 0), spice server receives:
> 
>  | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing 
> monitors config
>  | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors config 
> count:3 max:4
>  | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
>  | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 1024x740
>  | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
> 
> Sends to the client #1
> 
>  | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing 
> monitors config
>  | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors config 
> count:3 max:4
>  | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
>  | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
>  | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
> 
> Sends to the client #2
> 
>  | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
>  | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing 
> monitors config
>  | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors config 
> count:1 max:4
>  | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
>  | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing 
> monitors config
>  | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors config 
> count:3 max:4
>  | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
>  | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
>  | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> 
> Sends to the client #3
> 
>  | 16:50:14.973: 
> display-channel.c:2462:display_channel_update_monitors_config: Will update 
> monitors in 200ms ~~
>  | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing 
> monitors config
>  | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors config 
> count:4 max:4
>  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
>  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
>  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 1024x740
>  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0 1024x740
> 
> Sends to the client #4 and final
> 
> With this patch, only the last one is sent.
> Resolves: rhbz#1673072
> Signed-off-by: Victor Toso 
> ---
> 
> Changes v1->v2
> * Using reds_core_timer_* api, which calls the timeout function from the
>   right context (Frediano);
> * Remove the SpiceTimer on finalize (Frediano)
> 
>  server/display-channel-private.h |  2 ++
>  server/display-channel.c | 27 +--
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/server/display-channel-private.h 
> b/server/display-channel-private.h
> index 58179531..848abe81 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
>  
>  int gl_draw_async_count;
>  
> +SpiceTimer *monitors_config_update_timer;
> +
>  /* TODO: some day unify this, make it more runtime.. */
>  stat_info_t add_stat;
>  stat_info_t exclude_stat;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 9bb7fa44..a2f95956 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
>  display_channel_destroy_surfaces(self);
>  image_cache_reset(>priv->image_cache);
>  
> +if (self->priv->monitors_config_update_timer) {
> +RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> +reds_core_timer_remove(reds, 
> self->priv->monitors_config_update_timer);
> +self->priv->monitors_config_update_timer = NULL;
> +}
> +
>  if (spice_extra_checks) {
>  unsigned int count;
>  _Drawable *drawable;
> @@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
> NULL);
>  if (display) {
>  

Re: [Spice-devel] [PATCH spice-common 9/9] codegen: Rename --prefix parameter to --suffix

[Christophe Fergeau]

On Thu, Mar 07, 2019 at 04:05:27PM -0500, Frediano Ziglio wrote:
> > 
> > On Sun, Mar 03, 2019 at 07:10:30PM +, Frediano Ziglio wrote:
> > > The option is used to add a suffix to public functions, not a
> > > prefix.
> > 
> > Sometimes it's a suffix to a prefix, sometimes it's indeed just a
> > suffix. So your change makes sense.
> > 
> 
> I cannot find any place where is used as a "suffix to a prefix",
> it's always appended to a partial function name.

Agreed, I looked too quickly :)


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-common 7/9] codegen: Remove support for --ptrsize

[Christophe Fergeau]

On Thu, Mar 07, 2019 at 04:01:02PM -0500, Frediano Ziglio wrote:
> > 
> > Acked-by: Christophe Fergeau 
> > 
> 
> Thanks, looking at reply to 9/9 however I think one thing is not clear from
> what I wrote in the commit message.
> The reason is not only that was used in protocol 1 only and now is not used
> but also that it was wrong from the beginning and useless. I would keep it
> if I found it useful.
> 
> > On Sun, Mar 03, 2019 at 07:10:28PM +, Frediano Ziglio wrote:
> > > This option was used in protocol 1 to generate 64 bit pointers.
> > > A pointer in the protocol is an offset in the current message.
> > > This allows the possibility to have messages with pointers with
> > > more than 4GB. This feature was removed and not used in protocol 2.
> > > The reason is that messages more than 4GB would cause:
> 
> Maybe this could be changed in
> 
> "The reason this feature was correctly removed in protocol 2 is that
> having 64 bit pointers in the protocol would require messages larger
> than 4GB which would cause:"

Sure, even though the first version was fine too. I would not make as
big of a difference between "no longer used, and 'harmful'" (this patch)
and "no longer used and harmless", and would not mind removing both,
hence my comment on patch #9.

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 spice-common 9/9] codegen: Rename --prefix parameter to --suffix

[Christophe Fergeau]

On Sun, Mar 03, 2019 at 07:10:30PM +, Frediano Ziglio wrote:
> The option is used to add a suffix to public functions, not a
> prefix.

Sometimes it's a suffix to a prefix, sometimes it's indeed just a
suffix. So your change makes sense.

> Currently the option is not used (it was used to generate protocol
> 1 code).

Why not remove it? It's inconsistent with the commit saying ptr_size is
unused so it can be removed.

Acked-by: Christophe Fergeau 

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 spice-common 7/9] codegen: Remove support for --ptrsize

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Sun, Mar 03, 2019 at 07:10:28PM +, Frediano Ziglio wrote:
> This option was used in protocol 1 to generate 64 bit pointers.
> A pointer in the protocol is an offset in the current message.
> This allows the possibility to have messages with pointers with
> more than 4GB. This feature was removed and not used in protocol 2.
> The reason is that messages more than 4GB would cause:
> - huge latency as a single message would take more than 4 seconds
>   to be send in a 10Gb connection;
> - huge memory requirements.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  common/marshaller.c   | 14 +++---
>  common/marshaller.h   |  2 +-
>  python_modules/marshal.py |  2 +-
>  python_modules/ptypes.py  | 18 +++---
>  spice_codegen.py  |  4 
>  5 files changed, 8 insertions(+), 32 deletions(-)
> 
> diff --git a/common/marshaller.c b/common/marshaller.c
> index 4379aa6..c77129b 100644
> --- a/common/marshaller.c
> +++ b/common/marshaller.c
> @@ -96,7 +96,6 @@ typedef struct SpiceMarshallerData SpiceMarshallerData;
>  typedef struct {
>  SpiceMarshaller *marshaller;
>  int item_nr;
> -int is_64bit;
>  size_t offset;
>  } MarshallerRef;
>  
> @@ -419,13 +418,13 @@ SpiceMarshaller 
> *spice_marshaller_get_submarshaller(SpiceMarshaller *m)
>  return m2;
>  }
>  
> -SpiceMarshaller *spice_marshaller_get_ptr_submarshaller(SpiceMarshaller *m, 
> int is_64bit)
> +SpiceMarshaller *spice_marshaller_get_ptr_submarshaller(SpiceMarshaller *m)
>  {
>  SpiceMarshaller *m2;
>  uint8_t *p;
>  int size;
>  
> -size = is_64bit ? 8 : 4;
> +size = 4;
>  
>  p = spice_marshaller_reserve_space(m, size);
>  memset(p, 0, size);
> @@ -433,7 +432,6 @@ SpiceMarshaller 
> *spice_marshaller_get_ptr_submarshaller(SpiceMarshaller *m, int
>  m2->pointer_ref.marshaller = m;
>  m2->pointer_ref.item_nr = m->n_items - 1;
>  m2->pointer_ref.offset = m->items[m->n_items - 1].len - size;
> -m2->pointer_ref.is_64bit = is_64bit;
>  
>  return m2;
>  }
> @@ -538,13 +536,7 @@ void spice_marshaller_flush(SpiceMarshaller *m)
>  for (m2 = m; m2 != NULL; m2 = m2->next) {
>  if (m2->pointer_ref.marshaller != NULL && m2->total_size > 0) {
>  ptr_pos = lookup_ref(>pointer_ref);
> -if (m2->pointer_ref.is_64bit) {
> -write_uint64(ptr_pos,
> - spice_marshaller_get_offset(m2));
> -} else {
> -write_uint32(ptr_pos,
> - spice_marshaller_get_offset(m2));
> -}
> +write_uint32(ptr_pos, spice_marshaller_get_offset(m2));
>  }
>  }
>  }
> diff --git a/common/marshaller.h b/common/marshaller.h
> index 041d16e..a631c85 100644
> --- a/common/marshaller.h
> +++ b/common/marshaller.h
> @@ -53,7 +53,7 @@ size_t spice_marshaller_get_offset(SpiceMarshaller *m);
>  size_t spice_marshaller_get_size(SpiceMarshaller *m);
>  size_t spice_marshaller_get_total_size(SpiceMarshaller *m);
>  SpiceMarshaller *spice_marshaller_get_submarshaller(SpiceMarshaller *m);
> -SpiceMarshaller *spice_marshaller_get_ptr_submarshaller(SpiceMarshaller *m, 
> int is_64bit);
> +SpiceMarshaller *spice_marshaller_get_ptr_submarshaller(SpiceMarshaller *m);
>  int spice_marshaller_fill_iovec(SpiceMarshaller *m, struct iovec *vec,
>  int n_vec, size_t skip_bytes);
>  void *spice_marshaller_add_uint64(SpiceMarshaller *m, uint64_t v);
> diff --git a/python_modules/marshal.py b/python_modules/marshal.py
> index 4e98993..74f3a54 100644
> --- a/python_modules/marshal.py
> +++ b/python_modules/marshal.py
> @@ -234,7 +234,7 @@ def write_array_marshaller(writer, member, array, 
> container_src, scope):
>  def write_pointer_marshaller(writer, member, src):
>  t = member.member_type
>  ptr_func = write_marshal_ptr_function(writer, t.target_type)
> -submarshaller = "spice_marshaller_get_ptr_submarshaller(m, %d)" % (1 if 
> member.get_fixed_nw_size() == 8 else 0)
> +submarshaller = "spice_marshaller_get_ptr_submarshaller(m)"
>  if member.has_attr("marshall"):
>  rest_args = ""
>  if t.target_type.is_array():
> diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> index 64c198e..bd5f542 100644
> --- a/python_modules/ptypes.py
> +++ b/python_modules/ptypes.py
> @@ -4,8 +4,6 @@ import types
>  _types_by_name = {}
>  _types = []
>  
> -default_pointer_size = 4
> -
>  def type_exists(name):
>  return name in _types_by_name

Re: [Spice-devel] [PATCH spice-common 6/9] Generate automatically most C message declarations

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

only one caveat is that some of the structs are getting renamed to
equivalent ones (SpiceMsgDisplayDrawBlend/SpiceMsgDisplayDrawCopy, 
SpiceMsgcMainAgentStart/SpiceMsgcMainAgentTokens and a few more), which
is fine because they are typedef'ed to the other type anyway
I guess it's worth mentioned in the commit log.

Reviewed-by: Christophe Fergeau 

Christophe

On Sun, Mar 03, 2019 at 07:10:27PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  common/messages.h | 495 +-
>  spice.proto   | 192 +-
>  2 files changed, 102 insertions(+), 585 deletions(-)
> 
> diff --git a/common/messages.h b/common/messages.h
> index 36ee59d..5cda1d1 100644
> --- a/common/messages.h
> +++ b/common/messages.h
> @@ -42,10 +42,6 @@
>  
>  SPICE_BEGIN_DECLS
>  
> -typedef struct SpiceMsgData {
> -uint8_t data[0];
> -} SpiceMsgData;
> -
>  typedef struct SpiceMsgCompressedData {
>  uint8_t type;
>  uint32_t uncompressed_size;
> @@ -53,438 +49,8 @@ typedef struct SpiceMsgCompressedData {
>  uint8_t *compressed_data;
>  } SpiceMsgCompressedData;
>  
> -typedef struct SpiceMsgEmpty {
> -uint8_t padding;
> -} SpiceMsgEmpty;
> -
> -typedef struct SpiceMsgInputsInit {
> -uint32_t keyboard_modifiers;
> -} SpiceMsgInputsInit;
> -
> -typedef struct SpiceMsgInputsKeyModifiers {
> -uint32_t modifiers;
> -} SpiceMsgInputsKeyModifiers;
> -
> -typedef struct SpiceMsgMainMultiMediaTime {
> -uint32_t time;
> -} SpiceMsgMainMultiMediaTime;
> -
> -typedef struct SpiceMigrationDstInfo {
> -uint16_t port;
> -uint16_t sport;
> -uint32_t host_size;
> -uint8_t *host_data;
> -uint32_t cert_subject_size;
> -uint8_t *cert_subject_data;
> -} SpiceMigrationDstInfo;
> -
> -typedef struct SpiceMsgMainMigrationBegin {
> -SpiceMigrationDstInfo dst_info;
> -} SpiceMsgMainMigrationBegin;
> -
> -typedef struct SpiceMsgMainMigrateBeginSeamless {
> -SpiceMigrationDstInfo dst_info;
> -uint32_t src_mig_version;
> -} SpiceMsgMainMigrateBeginSeamless;
> -
> -typedef struct SpiceMsgcMainMigrateDstDoSeamless {
> -uint32_t src_version;
> -} SpiceMsgcMainMigrateDstDoSeamless;
> -
> -typedef struct SpiceMsgMainMigrationSwitchHost {
> -uint16_t port;
> -uint16_t sport;
> -uint32_t host_size;
> -uint8_t *host_data;
> -uint32_t cert_subject_size;
> -uint8_t *cert_subject_data;
> -} SpiceMsgMainMigrationSwitchHost;
> -
> -
> -typedef struct SpiceMsgMigrate {
> -uint32_t flags;
> -} SpiceMsgMigrate;
> -
> -typedef struct SpiceResourceID {
> -uint8_t type;
> -uint64_t id;
> -} SpiceResourceID;
> -
> -typedef struct SpiceResourceList {
> -uint16_t count;
> -SpiceResourceID resources[0];
> -} SpiceResourceList;
> -
> -typedef struct SpiceMsgSetAck {
> -uint32_t generation;
> -uint32_t window;
> -} SpiceMsgSetAck;
> -
> -typedef struct SpiceMsgcAckSync {
> -  uint32_t generation;
> -} SpiceMsgcAckSync;
> -
> -typedef struct SpiceWaitForChannel {
> -uint8_t channel_type;
> -uint8_t channel_id;
> -uint64_t message_serial;
> -} SpiceWaitForChannel;
> -
> -typedef struct SpiceMsgWaitForChannels {
> -uint8_t wait_count;
> -SpiceWaitForChannel wait_list[0];
> -} SpiceMsgWaitForChannels;
> -
> -typedef struct SpiceChannelId {
> -uint8_t type;
> -uint8_t id;
> -} SpiceChannelId;
> -
> -typedef struct SpiceMsgMainInit {
> -uint32_t session_id;
> -uint32_t display_channels_hint;
> -uint32_t supported_mouse_modes;
> -uint32_t current_mouse_mode;
> -uint32_t agent_connected;
> -uint32_t agent_tokens;
> -uint32_t multi_media_time;
> -uint32_t ram_hint;
> -} SpiceMsgMainInit;
> -
> -typedef struct SpiceMsgDisconnect {
> -uint64_t time_stamp;
> -uint32_t reason; // SPICE_ERR_?
> -} SpiceMsgDisconnect;
> -
> -typedef struct SpiceMsgNotify {
> -uint64_t time_stamp;
> -uint32_t severity;
> -uint32_t visibilty;
> -uint32_t what;
> -uint32_t message_len;
> -uint8_t message[0];
> -} SpiceMsgNotify;
> -
> -typedef struct SpiceMsgChannels {
> -uint32_t num_of_channels;
> -SpiceChannelId channels[0];
> -} SpiceMsgChannels;
> -
> -typedef struct SpiceMsgMainName {
> -uint32_t name_len;
> -uint8_t name[0];
> -} SpiceMsgMainName;
> -
> -typedef struct SpiceMsgMainUuid {
> -uint8_t uuid[16];
> -} SpiceMsgMainUuid;
> -
> -typedef struct SpiceMsgMainMouseMode {
> -uint32_t supported_mod

Re: [Spice-devel] [PATCH spice-common 5/9] Allow to generate C declarations for spice.proto

[Christophe Fergeau]

On Sun, Mar 03, 2019 at 07:10:26PM +, Frediano Ziglio wrote:
> Generate and include C declarations.
> Next patch will use this facility.

Since none of the spice.proto types are decorated with @declare, adding
the #include in messages.h won't have any bad consequences

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  common/Makefile.am |  6 --
>  common/meson.build | 10 +-
>  common/messages.h  |  2 ++
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/common/Makefile.am b/common/Makefile.am
> index 3da5bad..411831b 100644
> --- a/common/Makefile.am
> +++ b/common/Makefile.am
> @@ -109,8 +109,9 @@ MARSHALLERS_DEPS =
> \
>  
>  # Note despite being autogenerated these are not part of CLEANFILES, they are
>  # actually a part of EXTRA_DIST, to avoid the need for pyparser by end users
> -generated_client_demarshallers.c: $(top_srcdir)/spice.proto 
> $(MARSHALLERS_DEPS)
> - $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py 
> --generate-demarshallers --client --include common/messages.h $< $@ >/dev/null
> +generated_client_demarshallers.c generated_messages.h: 
> $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
> + $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py 
> --generate-demarshallers --client --include common/messages.h \
> + --generated-declaration-file generated_messages.h $< $@ >/dev/null
>  
>  generated_client_marshallers.c generated_client_marshallers.h: 
> $(top_srcdir)/spice.proto $(MARSHALLERS_DEPS)
>   $(AM_V_GEN)$(PYTHON) $(top_srcdir)/spice_codegen.py 
> --generate-marshallers -P --include client_marshallers.h --client \
> @@ -135,6 +136,7 @@ EXTRA_DIST =  \
>   quic_family_tmpl.c  \
>   quic_tmpl.c \
>   snd_codec.h \
> + generated_messages.h\

Unrelated to this patch, but I wonder why snd_codec.h is listed there.
I'd put generated_messages.h close to the $(CLIENT_MARSHALLERS) and
$(SERVER_MARSHALLERS) variables.

Reviewed-by: Christophe Fergeau 

>   $(NULL)
>  
>  -include $(top_srcdir)/git.mk
> diff --git a/common/meson.build b/common/meson.build
> index 9575568..2d769f2 100644
> --- a/common/meson.build
> +++ b/common/meson.build
> @@ -64,7 +64,15 @@ spice_common_dep = declare_dependency(link_with : 
> spice_common_lib,
>  #
>  if spice_common_generate_client_code
>targets = [
> -['client_demarshallers', spice_proto, 
> 'generated_client_demarshallers.c', ['--generate-demarshallers', '--client', 
> '--include', 'common/messages.h', '@INPUT@', '@OUTPUT@']],
> +['client_demarshallers', spice_proto,
> +  ['generated_client_demarshallers.c', 'generated_messages.h'],
> +  ['--generate-demarshallers',
> +'--client',
> +'--include', 'common/messages.h',
> +'--generated-declaration-file', '@OUTPUT1@',
> +'@INPUT@', '@OUTPUT0@'
> +  ]
> +],
>  ['client_marshallers', spice_proto,
>['generated_client_marshallers.c', 'generated_client_marshallers.h'],
>['--generate-marshallers',
> diff --git a/common/messages.h b/common/messages.h
> index f740a8c..36ee59d 100644
> --- a/common/messages.h
> +++ b/common/messages.h
> @@ -558,6 +558,8 @@ typedef struct SpiceMsgDisplayGlDraw {
>  uint32_t h;
>  } SpiceMsgDisplayGlDraw;
>  
> +#include 
> +
>  SPICE_END_DECLS
>  
>  #endif // H_SPICE_COMMON_MESSAGES
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-common 4/9] codegen: Allows to generate C declarations automatically

[Christophe Fergeau]

On Sun, Mar 03, 2019 at 07:10:25PM +, Frediano Ziglio wrote:
> Allows to specify a @declare attribute for messages and structure
> that can generate the needed C structures.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  python_modules/ptypes.py | 64 
>  spice_codegen.py | 47 +
>  2 files changed, 111 insertions(+)
> 
> diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> index 7dca78d..64c198e 100644
> --- a/python_modules/ptypes.py
> +++ b/python_modules/ptypes.py
> @@ -72,6 +72,8 @@ valid_attributes=set([
>  'zero',
>  # this attribute does not exist on the network, fill just structure with 
> the value
>  'virtual',
> +# generate C structure declarations from protocol definition
> +'declare',
>  ])
>  
>  attributes_with_arguments=set([
> @@ -485,6 +487,26 @@ class ArrayType(Type):
>  def c_type(self):
>  return self.element_type.c_type()
>  
> +def generate_c_declaration(self, writer, member):
> +name = member.name
> +if member.has_attr("chunk"):
> +return writer.writeln('SpiceChunks *%s;' % name)
> +if member.has_attr("as_ptr"):
> +len_var = member.attributes["as_ptr"][0]
> +writer.writeln('uint32_t %s;' % len_var)
> +return writer.writeln('%s *%s;' % (self.c_type(), name))
> +if member.has_attr("to_ptr"): # TODO len
> +return writer.writeln('%s *%s;' % (self.c_type(), name))
> +if member.has_attr("ptr_array"): # TODO where the length is stored? 
> overflow?
> +return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> +if member.has_end_attr() or self.is_remaining_length(): # TODO len
> +return writer.writeln('%s %s[0];' % (self.c_type(), name))

These TODO are worrying, but only the has_end_attr() one seems to be run
at the moment, the others seem to be dead code?

Looks good otherwise,

Acked-by: Christophe Fergeau 

> +if self.is_constant_length():
> +return writer.writeln('%s %s[%s];' % (self.c_type(), name, 
> self.size))
> +if self.is_identifier_length():
> +return writer.writeln('%s *%s;' % (self.c_type(), name))
> +raise NotImplementedError('unknown array %s' % str(self))
> +
>  class PointerType(Type):
>  def __init__(self, target_type):
>  Type.__init__(self)
> @@ -529,6 +551,15 @@ class PointerType(Type):
>  def get_num_pointers(self):
>  return 1
>  
> +def generate_c_declaration(self, writer, member):
> +target_type = self.target_type
> +is_array = target_type.is_array()
> +if not is_array or target_type.is_identifier_length():
> +writer.writeln("%s *%s;" % (target_type.c_type(), member.name))
> +return
> +raise NotImplementedError('Some pointers to array declarations are 
> not implemented %s' %
> +member)
> +
>  class Containee:
>  def __init__(self):
>  self.attributes = {}
> @@ -624,6 +655,14 @@ class Member(Containee):
>  names = [prefix + "_" + name for name in names]
>  return names
>  
> +def generate_c_declaration(self, writer):
> +if self.has_attr("zero"):
> +return
> +if self.is_pointer() or self.is_array():
> +self.member_type.generate_c_declaration(writer, self)
> +return
> +return writer.writeln("%s %s;" % (self.member_type.c_type(), 
> self.name))
> +
>  class SwitchCase:
>  def __init__(self, values, member):
>  self.values = values
> @@ -747,6 +786,17 @@ class Switch(Containee):
>  names = names + c.get_pointer_names(marshalled)
>  return names
>  
> +def generate_c_declaration(self, writer):
> +if self.has_attr("anon") and len(self.cases) == 1:
> +self.cases[0].member.generate_c_declaration(writer)
> +return
> +writer.writeln('union {')
> +writer.indent()
> +for m in self.cases:
> +m.member.generate_c_declaration(writer)
> +writer.unindent()
> +writer.writeln('} %s;' % self.name)
> +
>  class ContainerType(Type):
>  def is_fixed_sizeof(self):
>  for m in self.members:
> @@ -857,6 +907,20 @@ class ContainerType(Type):
>  
>  return member
>  
> +def generate_c_declaration(self, writer):
> +if not self.has_attr('declare'):
> +return
> +name = self.c_type()
> +writer.writeln('

Re: [Spice-devel] [PATCH spice-common 3/9] codegen: Generate headers while generating code

[Christophe Fergeau]

allers.h',
> +'--generated-header', '@OUTPUT1@',
> +'@INPUT@', '@OUTPUT0@'
> +  ]
> +]
>]
>  
>spice_common_client_sources = [
> @@ -116,8 +122,13 @@ if spice_common_generate_server_code
>  
>targets = [
>  ['server_demarshallers', spice_proto, 
> 'generated_server_demarshallers.c', ['--generate-demarshallers', '--server', 
> '--include', 'common/messages.h', '@INPUT@', '@OUTPUT@']],
> -['server_marshallers', spice_proto, 'generated_server_marshallers.c', 
> ['--generate-marshallers', '--server'] + structs_args + ['--include', 
> 'common/messages.h', '@INPUT@', '@OUTPUT@']],
> -['server_marshallers_h', spice_proto, 'generated_server_marshallers.h', 
> ['--generate-marshallers', '--server'] + structs_args + ['--include', 
> 'common/messages.h', '-H', '@INPUT@', '@OUTPUT@']],
> +['server_marshallers', spice_proto,
> +  ['generated_server_marshallers.c', 'generated_server_marshallers.h'],
> +  ['--generate-marshallers',
> +'--generated-header', '@OUTPUT1@',
> +'--server'] + structs_args + ['--include', 'common/messages.h', 
> '@INPUT@', '@OUTPUT0@'
> +  ]
> +],
>]
>  
>spice_common_server_sources = []
> diff --git a/spice_codegen.py b/spice_codegen.py
> index 7e22092..4664740 100755
> --- a/spice_codegen.py
> +++ b/spice_codegen.py
> @@ -175,6 +175,8 @@ parser.add_option("--ptrsize", dest="ptrsize",
>help="set default pointer size", default="4")
>  parser.add_option("--license", dest="license",
>help="license to use for generated file(s) (LGPL/BSD)", 
> default="LGPL")
> +parser.add_option("--generated-header", dest="generated_header", 
> metavar="FILE",
> +  help="Name of the file to generate the header")


Bit unsure about the option name. --header-name maybe? Or just infer it
from the .c file name?

apart from this, 
Reviewed-by: Christophe Fergeau 


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-common 1/9] messages: Remove fields not used by the protocol

[Christophe Fergeau]

On Sun, Mar 03, 2019 at 07:10:22PM +, Frediano Ziglio wrote:
> These fields are not used by the protocol.

Maybe make it explicit that 'not used by the protocol' means they are
not in spice.proto, nor will be used in the generated (de)marshallers.
Apart from this,
Acked-by: Christophe Fergeau 


> Avoid spice-gtk and spice-server to use them by mistake.
> This can cause memory errors (data_size is not used or
> is not set correctly) and useless code (spice-gtk uses
> the pub_key* fields but these fields are not sent to
> the server as the protocol does not have them).
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  common/messages.h | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/common/messages.h b/common/messages.h
> index 43d3602..f740a8c 100644
> --- a/common/messages.h
> +++ b/common/messages.h
> @@ -43,7 +43,6 @@
>  SPICE_BEGIN_DECLS
>  
>  typedef struct SpiceMsgData {
> -uint32_t data_size;
>  uint8_t data[0];
>  } SpiceMsgData;
>  
> @@ -75,9 +74,6 @@ typedef struct SpiceMigrationDstInfo {
>  uint16_t sport;
>  uint32_t host_size;
>  uint8_t *host_data;
> -uint16_t pub_key_type;
> -uint32_t pub_key_size;
> -uint8_t *pub_key_data;
>  uint32_t cert_subject_size;
>  uint8_t *cert_subject_data;
>  } SpiceMigrationDstInfo;
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-common 2/9] codegen: Factor out a function to write output file

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Sun, Mar 03, 2019 at 07:10:23PM +, Frediano Ziglio wrote:
> This will be reused to generate C declaration automatically.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  spice_codegen.py | 46 +-
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/spice_codegen.py b/spice_codegen.py
> index 76d7c5e..7e22092 100755
> --- a/spice_codegen.py
> +++ b/spice_codegen.py
> @@ -105,6 +105,30 @@ def write_enums(writer, describe=False):
>  
>  writer.writeln("#endif /* _H_SPICE_ENUMS */")
>  
> +def write_content(dest_file, content, keep_identical_file):
> +if keep_identical_file:
> +try:
> +f = open(dest_file, 'rb')
> +old_content = f.read()
> +f.close()
> +
> +if content == old_content:
> +six.print_("No changes to %s" % dest_file)
> +return
> +
> +except IOError:
> +pass
> +
> +f = open(dest_file, 'wb')
> +if six.PY3:
> +f.write(bytes(content, 'UTF-8'))
> +else:
> +f.write(content)
> +f.close()
> +
> +six.print_("Wrote %s" % dest_file)
> +
> +
>  parser = OptionParser(usage="usage: %prog [options]  
> ")
>  parser.add_option("-e", "--generate-enums",
>action="store_true", dest="generate_enums", default=False,
> @@ -290,25 +314,5 @@ if options.header:
>  content = writer.header.getvalue()
>  else:
>  content = writer.getvalue()
> -if options.keep_identical_file:
> -try:
> -f = open(dest_file, 'rb')
> -old_content = f.read()
> -f.close()
> -
> -if content == old_content:
> -six.print_("No changes to %s" % dest_file)
> -sys.exit(0)
> -
> -except IOError:
> -pass
> -
> -f = open(dest_file, 'wb')
> -if six.PY3:
> -f.write(bytes(content, 'UTF-8'))
> -else:
> -f.write(content)
> -f.close()
> -
> -six.print_("Wrote %s" % dest_file)
> +write_content(dest_file, content, options.keep_identical_file)
>  sys.exit(0)
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-gtk v2 1/2] Deprecate “color-depth” properties

[Christophe Fergeau]

Hey,

On Wed, Mar 06, 2019 at 02:24:24PM +, Victor Toso wrote:
> Hi,
> 
> On Tue, Jan 08, 2019 at 04:22:55PM +0100, Victor Toso wrote:
> > Hi,
> > 
> > On Tue, Jan 08, 2019 at 04:09:54PM +0100, Christophe Fergeau wrote:
> > > On Fri, Dec 14, 2018 at 04:29:46PM +0100, Victor Toso wrote:
> > > > From: Victor Toso 
> > > > 
> > > > With commit 1a980f3712 we deprecated some command line options. The
> > > > color-depth one is the only one which is not used for testing/debug
> > > > purposes.
> > > > 
> > > > The intention of this option is to set guest's video driver color
> > > > configuration, currently only windows guest agent uses this feature up
> > > > to Windows 7. Windows 8 and up, setting color depth below 32 would
> > > > fail. We should not encourage the usage of this feature.
> > > > 
> > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1543538
> > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1350853
> > > > 
> > > > This patch deprecates both SpiceMainChannel::color-depth and
> > > > SpiceSession::color-depth while ignoring any set/get to it.
> > > > 
> > > > Signed-off-by: Victor Toso 
> > > > Acked-by: Christophe Fergeau 
> > > 
> > > Still fine with me, though it's indeed a good question that Uri raised,
> > > whether we want to keep this alive a bit longer for win7 since it's
> > > still supported by MS.
> > 
> >   | Microsoft ended mainstream support for Windows 7 on January 13,
> >   | 2015, but extended support won't end until January 14, 2020.
> > 
> > Oh well, IMHO we could remove it for 0.36 but we can do it for
> > after 0.36 as well.
> 
> Now that 0.36 is out, I'm wondering if this two patches are fine
> for you? CC: teuf and Uri :)

Fine with me,

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] [vdagent-win PATCH] build: Check we can link libpng statically

[Christophe Fergeau]

On Wed, Feb 27, 2019 at 10:52:05AM +, Frediano Ziglio wrote:
> Catch the problem during configure instead of having to wait the
> build to fail.
> On Fedora try for instance to remove mingw64-zlib-static package,
> the missing dependency won't be detected during configure.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  configure.ac   | 10 --
>  m4/pushvars.m4 | 15 +++
>  2 files changed, 23 insertions(+), 2 deletions(-)
>  create mode 100644 m4/pushvars.m4
> 
> diff --git a/configure.ac b/configure.ac
> index fbcb8da..e5d4b2c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -103,8 +103,14 @@ dnl 
> ---
>  dnl - Check library dependencies
>  dnl 
> ---
>  
> -PKG_CHECK_MODULES(LIBPNG, [libpng])
> -PKG_CHECK_MODULES(ZLIB, [zlib])
> +PKG_CHECK_MODULES_STATIC(LIBPNG, [libpng])
> +PKG_CHECK_MODULES_STATIC(ZLIB, [zlib])
> +ACX_SET_VAR(CFLAGS, "$CFLAGS $LIBPNG_CFLAGS -Wall",
> +ACX_SET_VAR(LDFLAGS, "$LDFLAGS -static",
> +ACX_SET_VAR(LIBS, "$LIBS $LIBPNG_LIBS",
> +AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include ]], [[
> +return !!png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, NULL, NULL)
> +  ]])], [], AC_MSG_ERROR([static libpng not found])

Maybe add some indenting to make it explicit that the ACX_SET_VAR calls
are not setting the variables forever, but only for the lifetime of the
AC_LINK_IFELSE, I had written a mail saying most of it is not needed
before looking closer at ACX_SET_VAR ;)

Apart from this, 
Acked-by: Christophe Fergeau 

Christophe

>  
>  dnl 
> ---
>  dnl - Makefiles, etc.
> diff --git a/m4/pushvars.m4 b/m4/pushvars.m4
> new file mode 100644
> index 000..86a27fc
> --- /dev/null
> +++ b/m4/pushvars.m4
> @@ -0,0 +1,15 @@
> +AC_DEFUN([ACX_PUSH_VAR],
> +[m4_pushdef([SAVE$1],save_$1[_]__line__)dnl
> +SAVE$1="$$1"
> +$1=[$2]])
> +
> +AC_DEFUN([ACX_POP_VAR],
> +[$1="$SAVE$1"
> +unset SAVE$1 dnl
> +m4_popdef([SAVE$1])dnl
> +])
> +
> +AC_DEFUN([ACX_SET_VAR],
> +[ACX_PUSH_VAR($1, [$2])
> +[$3]
> +ACX_POP_VAR($1)])
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server v2 1/3] Refactor agent_adjust_capabilities() function

[Christophe Fergeau]

Series looks good to me,

Acked-by: Christophe Fergeau 

On Mon, Mar 04, 2019 at 01:50:43PM -0600, Jonathon Jongsma wrote:
> Make this a RedsState member function rather than a standalone function.
> This means that we simply pass RedsState* as an argument rather than the
> internal member variables of RedsState. This enables the following
> commit which handles the VD_AGENT_CAP_GRAPHICS_DEVICE_INFO capability to
> avoid sending graphics device info to agents that do not support it.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  server/reds.c | 17 +++--
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 2e5c69e60..63bfadb22 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -793,8 +793,9 @@ static void vdi_port_read_buf_free(RedPipeItem *base)
>  g_free(buf);
>  }
>  
> -static void agent_adjust_capabilities(VDAgentMessage *message,
> -  bool clipboard_enabled, bool 
> xfer_enabled)
> +/* certain agent capabilities can be overridden and disabled in the server. 
> In these cases, unset
> + * these capabilities before sending them on to the client */
> +static void reds_adjust_agent_capabilities(RedsState *reds, VDAgentMessage 
> *message)
>  {
>  VDAgentAnnounceCapabilities *capabilities;
>  
> @@ -803,13 +804,13 @@ static void agent_adjust_capabilities(VDAgentMessage 
> *message,
>  }
>  capabilities = (VDAgentAnnounceCapabilities *) message->data;
>  
> -if (!clipboard_enabled) {
> +if (!reds->config->agent_copypaste) {
>  VD_AGENT_CLEAR_CAPABILITY(capabilities->caps, 
> VD_AGENT_CAP_CLIPBOARD);
>  VD_AGENT_CLEAR_CAPABILITY(capabilities->caps, 
> VD_AGENT_CAP_CLIPBOARD_BY_DEMAND);
>  VD_AGENT_CLEAR_CAPABILITY(capabilities->caps, 
> VD_AGENT_CAP_CLIPBOARD_SELECTION);
>  }
>  
> -if (!xfer_enabled) {
> +if (!reds->config->agent_file_xfer) {
>  VD_AGENT_SET_CAPABILITY(capabilities->caps, 
> VD_AGENT_CAP_FILE_XFER_DISABLED);
>  }
>  }
> @@ -879,9 +880,7 @@ static RedPipeItem 
> *vdi_port_read_one_msg_from_device(RedCharDevice *self,
>  }
>  switch (vdi_port_read_buf_process(dev, dispatch_buf)) {
>  case AGENT_MSG_FILTER_OK:
> -agent_adjust_capabilities((VDAgentMessage *) 
> dispatch_buf->data,
> -  reds->config->agent_copypaste,
> -  reds->config->agent_file_xfer);
> +reds_adjust_agent_capabilities(reds, (VDAgentMessage *) 
> dispatch_buf->data);
>  return _buf->base;
>  case AGENT_MSG_FILTER_PROTO_ERROR:
>  reds_agent_remove(reds);
> @@ -1380,9 +1379,7 @@ void reds_on_main_channel_migrate(RedsState *reds, 
> MainChannelClient *mcc)
>  read_buf->len = read_data_len;
>  switch (vdi_port_read_buf_process(agent_dev, read_buf)) {
>  case AGENT_MSG_FILTER_OK:
> -agent_adjust_capabilities((VDAgentMessage *)read_buf->data,
> -  reds->config->agent_copypaste,
> -  reds->config->agent_file_xfer);
> +reds_adjust_agent_capabilities(reds, (VDAgentMessage 
> *)read_buf->data);
>  main_channel_client_push_agent_data(mcc,
>  read_buf->data,
>  read_buf->len,
> -- 
> 2.17.2
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-gtk] egl: Adjust to window scaling

[Christophe Fergeau]

On Sun, Mar 03, 2019 at 10:11:08AM +0200, Snir Sheriber wrote:
> When GDK_SCALE is != 1 and egl is used, the presented image does not
> fit to the window (scale of 2 is often used with hidpi monitors).
> Usually this is not a problem since all components are adjusted by
> gdk/gtk but with egl, pixel-based data is not being scaled. In this
> case window's scale value can be used in order to determine whether
> to use a pixel resource with higher resolution data.
> ---
> 
> In order to reproduce the problem set spice with virgl/Intel-vGPU
> and run spice-gtk with GDK_SCALE=2

This bit belongs in the commit log imo, anything below --- is going to
be ignored by git am.

> 
> This patch is kind of RFC, it fixes the issue, but it's a bit hacky
> and specific. I didn't come across other scale issues but it is likely
> that more of these exist and better and generic fix is needed.
> 
> ---
>  src/spice-widget-egl.c  | 15 +--
>  src/spice-widget-priv.h |  1 +
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-widget-egl.c b/src/spice-widget-egl.c
> index 43fccd7..814a580 100644
> --- a/src/spice-widget-egl.c
> +++ b/src/spice-widget-egl.c
> @@ -326,6 +326,8 @@ static gboolean spice_widget_init_egl_win(SpiceDisplay 
> *display, GdkWindow *win,
>  if (d->egl.surface)
>  return TRUE;
>  
> +d->egl.scale = gdk_window_get_scale_factor(win);
> +
>  #ifdef GDK_WINDOWING_X11
>  if (GDK_IS_X11_WINDOW(win)) {
>  native = (EGLNativeWindowType)GDK_WINDOW_XID(win);
> @@ -431,15 +433,17 @@ void spice_egl_resize_display(SpiceDisplay *display, 
> int w, int h)
>  {
>  SpiceDisplayPrivate *d = display->priv;
>  int prog;
> +gint ws;

why not 'scale' rather than a cryptic 2 letter name? :)
I'm afraid I'm not familiar enough with this part of gtk+/spice-gtk to
give a review on the approach you took though :-/

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-gtk] usb-redir: use persistent libusb device on Windows

[Christophe Fergeau]

On Mon, Feb 25, 2019 at 04:07:03PM +0200, Yuri Benditovich wrote:
> On Fri, Feb 22, 2019 at 2:06 PM Christophe Fergeau  
> wrote:
> > I don't think we should have a hard limit on the number of lines in a
> > patch, however there should be a maximum of 1 logical change per commit,
> > see 
> > https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/
> > for example for a rationale about this.
> > https://gitlab.freedesktop.org/teuf/spice-gtk/tree/gudev would be a
> > rough attempt at such a split (but authorship needs to be set back to
> > you, commit logs needs to be more verbose, ...).
> >
> 
> IMO, this is excellent example of why this approach is wrong.
> For example, there is definitely wrong commit
> https://gitlab.freedesktop.org/teuf/spice-gtk/commit/0135d831bc8929e45bac065f47daa1b4470ab7b0
> But nobody notice it as the problem in it fixed toward end of chain.

When I said "rough attempt at such a split", I meant it to be read as
"this is an untested proof of concept". So yes, there are bugs ;)

> Which tests are expected after _each_ commit?

Really depends on what the commit is doing. Each commit should build,
and basic functionality should still be working. I'd usually keep
extensive testing for the final patch, if some things are broken, I'd
move the fix to the right place, and possibly do more testing for the
commit which was broken.
In an ideal world, >95% of the testing would be automated, which would
make it trivial to test each commit..

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-gtk] usb-redir: use persistent libusb device on Windows

[Christophe Fergeau]

Hi,

On Thu, Feb 21, 2019 at 09:37:06AM +0200, Yuri Benditovich wrote:
> On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau  
> wrote:
> > >  static SpiceUsbDevice*
> > >  spice_usb_device_manager_find_device(SpiceUsbDeviceManager *self,
> > >   const int bus, const int address)
> > > @@ -970,6 +914,16 @@ static void 
> > > spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
> > >  if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> > >  return;
> > >
> > > +if (spice_usb_device_manager_find_device(self,
> > > +libusb_get_bus_number(libdev),
> > > +libusb_get_device_address(libdev))) {
> > > +SPICE_DEBUG("device not added %04x:%04x (%p)",
> > > +desc.idVendor,
> > > +desc.idProduct,
> > > +libdev);
> > > +return;
> > > +}
> > > +
> >
> > I did not understand why we need this?
> 
> There are several reasons:
> 1. It is possible that the hot plug procedure for Linux might be
> called more than once for the same device (this is mentioned in libusb
> API).

It's 
http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea
and this can happen at coldplug time (ie when using
LIBUSB_HOTPLUG_ENUMERATE), so exactly the race I was worried about
before.

> 2. If second instance of usb-device-manager created (as it happens at
> time of remote-viewer termination),

It sounds weird that we create a new one when exiting, probably
something we can/should get rid of if this starts causing problems.

> existing one receives each device that already present.
> 3. It is much simpler to exclude device duplication than later look
> why they present.


All of these explanations would be very useful to have in the log of a
separate commit.

> > > +#ifdef G_OS_WIN32
> > >  static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> > > -   const gchar *action,
> > > -   GUdevDevice *udevice,
> > > +   libusb_device   *libdev,
> > > +   gboolean add,
> > > gpointer 
> > > user_data)
> > >  {
> > >  SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
> > >
> > > -if (g_str_equal(action, "add"))
> > > -spice_usb_device_manager_add_udev(self, udevice);
> > > -else if (g_str_equal (action, "remove"))
> > > -spice_usb_device_manager_remove_udev(self, udevice);
> > > +if (add)
> > > +spice_usb_device_manager_add_dev(self, libdev);
> > > +else
> > > +spice_usb_device_manager_remove_dev(self,
> > > +libusb_get_bus_number(libdev),
> > > +libusb_get_device_address(libdev));
> >
> > This could almost reuse spice_usb_device_manager_hotplug_cb somehow, but
> > I'm not sure this is worth it.
> 
> What I'm expected to do to address this note?

Just random thoughts, maybe you'll think this could be a useful
improvement, maybe you already considered it and rejected it, maybe it's
not a good idea, I don't know. I don't have any expectations, just
something that could be useful to mention.


> > > @@ -1889,9 +1751,7 @@ static SpiceUsbDeviceInfo 
> > > *spice_usb_device_new(libusb_device *libdev)
> > >  info->pid = pid;
> > >  info->ref = 1;
> > >  info->isochronous = probe_isochronous_endpoint(libdev);
> > > -#ifndef G_OS_WIN32
> > >  info->libdev = libusb_ref_device(libdev);
> > > -#endif
> > >
> > >  return info;
> > >  }
> > > @@ -2027,14 +1887,11 @@ static void spice_usb_device_unref(SpiceUsbDevice 
> > > *device)
> > >
> > >  ref_count_is_0 = g_atomic_int_dec_and_test(>ref);
> > >  if (ref_count_is_0) {
> > > -#ifndef G_OS_WIN32
> > >  libusb_unref_device(info->libdev);
> > > -#endif
> >
> > Most of the 'persistent' libusb_device changes for Windows are in the
> > hunks before this, but they also depend on some rework of the hotplug
> > detection in GUdevClient.
> 
> What I'm expected to do to address this note?

Just

[Spice-devel] [spice-gtk v3] smartcard: Warn if multiple readers are detected

[Christophe Fergeau]

spice-server does not deal properly with multiple smartcard readers,
only the first one will be working. Add a warning when this happens to
make it easier to diagnose such issues.

Signed-off-by: Christophe Fergeau 
---
 src/smartcard-manager.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
index ceecfdc7..35bb2757 100644
--- a/src/smartcard-manager.c
+++ b/src/smartcard-manager.c
@@ -389,6 +389,24 @@ typedef struct {
 GError *err;
 } SmartcardManagerInitArgs;
 
+
+static void smartcard_reader_free(gpointer data)
+{
+g_boxed_free(SPICE_TYPE_SMARTCARD_READER, data);
+}
+
+/* spice-server only supports one smartcard reader being in use */
+static void smartcard_check_reader_count(void)
+{
+GList *readers;
+
+readers = 
spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
+if (g_list_length(readers) > 1) {
+g_warning("Multiple smartcard readers are plugged in, only the first 
one will be shared with the VM");
+}
+g_list_free_full(readers, smartcard_reader_free);
+}
+
 static gboolean smartcard_manager_init(SmartcardManagerInitArgs *args)
 {
 gchar *emul_args = NULL;
@@ -442,6 +460,7 @@ init:
 "Failed to initialize smartcard");
 goto end;
 }
+smartcard_check_reader_count();
 
 retval = TRUE;
 
-- 
2.20.1

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

[Spice-devel] [spice-gtk v2] smartcard: Warn if multiple readers are detected

[Christophe Fergeau]

spice-server does not deal properly with multiple smartcard readers,
only the first one will be working. Add a warning when this happens to
make it easier to diagnose such issues.

Signed-off-by: Christophe Fergeau 
---

Alternate version using g_list_length rather than manually counting the
elements.

 src/smartcard-manager.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
index ceecfdc7..144caf9b 100644
--- a/src/smartcard-manager.c
+++ b/src/smartcard-manager.c
@@ -389,6 +389,24 @@ typedef struct {
 GError *err;
 } SmartcardManagerInitArgs;
 
+
+static void unref_smartcard_reader(gpointer data)
+{
+g_boxed_free(SPICE_TYPE_SMARTCARD_READER, data);
+}
+
+/* spice-server only supports one smartcard reader being in use */
+static void smartcard_check_reader_count(void)
+{
+GList *readers;
+
+readers = 
spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
+if (g_list_length(readers) > 1) {
+g_warning("Multiple smartcard readers are plugged in, only the first 
one will be shared with the VM");
+}
+g_list_free_full(readers, unref_smartcard_reader);
+}
+
 static gboolean smartcard_manager_init(SmartcardManagerInitArgs *args)
 {
 gchar *emul_args = NULL;
@@ -442,6 +460,7 @@ init:
 "Failed to initialize smartcard");
 goto end;
 }
+smartcard_check_reader_count();
 
 retval = TRUE;
 
-- 
2.20.1

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

Re: [Spice-devel] [spice-gtk] smartcard: Warn if multiple readers are detected

[Christophe Fergeau]

On Thu, Feb 21, 2019 at 03:59:00PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Feb 20, 2019 at 5:17 PM Christophe Fergeau  
> wrote:
> >
> > spice-server does not deal properly with multiple smartcard readers,
> > only the first one will be working. Add a warning when this happens to
> > make it easier to diagnose such issues.
> >
> > Signed-off-by: Christophe Fergeau 
> > ---
> >  src/smartcard-manager.c | 20 
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/src/smartcard-manager.c b/src/smartcard-manager.c
> > index ceecfdc7..456c3c2e 100644
> > --- a/src/smartcard-manager.c
> > +++ b/src/smartcard-manager.c
> > @@ -389,6 +389,25 @@ typedef struct {
> >  GError *err;
> >  } SmartcardManagerInitArgs;
> >
> > +/* spice-server only supports one smartcard reader being in use */
> > +static void smartcard_check_reader_count(void)
> > +{
> > +unsigned int reader_count = 0;
> > +GList *readers;
> > +GList *it;
> > +
> > +readers = 
> > spice_smartcard_manager_get_readers(spice_smartcard_manager_get());
> > +
> > +for (it = readers; it != NULL; it = it->next) {
> > +reader_count++;
> > +g_boxed_free(SPICE_TYPE_SMARTCARD_READER, it->data);
> > +}
> > +if (reader_count > 1) {
> > +g_warning("Multiple smartcard readers are plugged in, only the 
> > first one will be shared with the VM");
> > +}
> > +g_list_free(readers);
> 
> 
> looks ok, ack
> 
> it could eventually be simplified with g_list_count() &
> g_list_free_full() (that would require wrapping the boxed_free with a
> helper)

I thought about it, given the need for a wrapper, it felt better to do
it this way.

Christophe


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