Re: [Spice-devel] [PATCH spice-streaming-agent 2/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  include/spice-streaming-agent/plugin.hpp | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index 1a58856..c370eab 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -149,6 +149,14 @@ extern "C" unsigned
> spice_streaming_agent_plugin_interface_version;
>   */
>  extern "C" SpiceStreamingAgent::PluginInitFunc
>  spice_streaming_agent_plugin_init;
>  
> +#define SPICE_STREAMING_AGENT_PLUGIN(agent) \
> +__attribute__ ((visibility ("default")))\
> +unsigned spice_streaming_agent_plugin_interface_version =   \
> +SpiceStreamingAgent::PluginInterfaceVersion;\
> +\
> +__attribute__ ((visibility ("default")))\
> +bool spice_streaming_agent_plugin_init(SpiceStreamingAgent::Agent*
> agent)
> +
>  #endif
>  
>  #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP

Surely helps. Some notes:
- spice_streaming_agent_plugin_interface_version is not const so I assume
  you want to allow to change it;
- the attribute is GCC syntax but can be changed if needed;
- I know we use that style of indentation for line continuation but I
  honestly prefer to not have that indentation preferring a " \" at the
  end. This as current style:
  - is hard to maintain. Currently is already broken as last like is
wrong;
  - it make copy harder unless you indent always at a given
standard column;
  - make email patch potentially hard to read.

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

Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Frediano Ziglio]

> 
> > On 22 Nov 2017, at 18:04, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> The C++ One Definition Rule (ODR) states that all translation units
> >> must see the same definitions. In the current code, when we call
> >> Agent::PluginVersionIsCompatible from the plugin, it is an ODR
> >> violation as soon as we have made any change in the Agent class
> >> compared to what the plugin was compiled against.
> >> 
> >> GCC respects the Standard C++ ABI on most platforms, so I believe this
> >> could be made to work despite the violation as long as we never
> >> changed the order of declarations, etc. But the point of the ABI check
> >> is that we should be able to change the agent interface in any way we
> >> want and be safe. And technically, the safe cases would still be an
> >> ODR violation that relies on knowledge of implementation details.
> >> 
> >> Another issue is that we don't want to have to rely on the plugins to
> >> do the version checks. It is more robust to do it in the agent, where
> >> it ensures that it is done in a consistent way and with consistent
> >> error messages. As a matter of fact, the new check is robust against
> >> older plugins and will not load them.
> >> 
> >> The mechanism implemented here is similar to what libtool uses.
> >> It lets any interface update be marked as either compatible with
> >> earlier plugins or incompatible.
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> include/spice-streaming-agent/plugin.hpp | 48
> >> +---
> >> src/concrete-agent.cpp   | 35 ---
> >> src/concrete-agent.hpp   |  4 ---
> >> src/mjpeg-fallback.cpp   |  3 --
> >> 4 files changed, 43 insertions(+), 47 deletions(-)
> >> 
> >> diff --git a/include/spice-streaming-agent/plugin.hpp
> >> b/include/spice-streaming-agent/plugin.hpp
> >> index 727cb3b..1a58856 100644
> >> --- a/include/spice-streaming-agent/plugin.hpp
> >> +++ b/include/spice-streaming-agent/plugin.hpp
> >> @@ -22,11 +22,20 @@ namespace SpiceStreamingAgent {
> >> class FrameCapture;
> >> 
> >> /*!
> >> - * Plugin version, only using few bits, schema is 0xMMmm
> >> - * where MM is major and mm is the minor, can be easily expanded
> >> - * using more bits in the future
> >> + * Plugins use a versioning system similar to that implemented by libtool
> >> + *
> >> http://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> >> + * Update the version information as follows:
> >> + * [ANY CHANGE] If any interfaces have been added, removed, or changed
> >> + * since the last update, increment PluginInterfaceVersion.
> >> + * [COMPATIBLE CHANGE] If interfaces have only been added since the last
> >> + * public release, then increment PluginInterfaceAge.
> >> + * [INCOMPATIBLE CHANGE] If any interfaces have been removed or changed
> >> + * since the last public release, then set PluginInterfaceAge to 0.
> >>  */
> >> -enum Constants : unsigned { PluginVersion = 0x100u };
> >> +enum Constants : unsigned {
> >> +PluginInterfaceVersion = 0,
> >> +PluginInterfaceAge = 0
> >> +};
> >> 
> >> enum Ranks : unsigned {
> >> /// this plugin should not be used
> >> @@ -102,20 +111,6 @@ class Agent
> >> {
> >> public:
> >> /*!
> >> - * Get agent version.
> >> - * Plugin should check the version for compatibility before doing
> >> - * everything.
> >> - * \return version specified like PluginVersion
> >> - */
> >> -virtual unsigned Version() const=0;
> >> -
> >> -/*!
> >> - * Check if a given plugin version is compatible with this agent
> >> - * \return true is compatible
> >> - */
> >> -virtual bool PluginVersionIsCompatible(unsigned pluginVersion)
> >> const=0;
> >> -
> >> -/*!
> >>  * Register a plugin in the system.
> >>  */
> >> virtual void Register(Plugin& plugin)=0;
> >> @@ -135,18 +130,25 @@ typedef bool
> >> PluginInitFunc(SpiceStreamingAgent::Agent*
> >> agent);
> >> 
> >> #ifndef SPICE_STREAMING_AGENT_PROGRAM
> >> /*!
> >> + * Plugin interface version
> >> + * Each plugin should define this variable and set it to
> >> PluginInterfaceVersion
> >> + * That version will be checked by the agent before executing any plugin
> >> code
> >> + */
> >> +extern "C" unsigned spice_streaming_agent_plugin_interface_version;
> >> +
> >> +/*!
> >>  * Plugin main entry point.
> >> - * Plugins should check if the version of the agent is compatible.
> >> - * If is compatible should register itself to the agent and return
> >> - * true.
> >> - * If is not compatible can decide to stay in memory or not returning
> >> - * true (do not unload) or false (safe to unload). This is necessary
> >> + * This entry point is only called if the version check passed.
> >> + * It should return true if it loaded and initialized successfully.
> >> + * If the plugin does not 

Re: [Spice-devel] [PATCH usbredir v2] usbredirserver: show bus:device of the USB device if specifying vid:pid

[Frediano Ziglio]

> 
> From: Chen Hanxiao 
> 
> We use libusb_open_device_with_vid_pid, if multiple devices
> have the same vid:pid, it will only the first one.
> 
> This patch will show the bus:device of the chosen one.
> 
> Signed-off-by: Chen Hanxiao 
> ---
> v2: control output messages by verbose
> 
>  usbredirserver/usbredirserver.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/usbredirserver/usbredirserver.c
> b/usbredirserver/usbredirserver.c
> index 13965dc..4685e0d 100644
> --- a/usbredirserver/usbredirserver.c
> +++ b/usbredirserver/usbredirserver.c
> @@ -321,6 +321,7 @@ int main(int argc, char *argv[])
>  
>  /* Try to find the specified usb device */
>  if (usbvendor != -1) {
> +libusb_device *dev;
>  handle = libusb_open_device_with_vid_pid(ctx, usbvendor,
>   usbproduct);
>  if (!handle) {
> @@ -328,6 +329,14 @@ int main(int argc, char *argv[])
>  "Could not open an usb-device with vid:pid %04x:%04x\n",
>  usbvendor, usbproduct);
>  }
> +if (verbose >= usbredirparser_info) {
> +dev = libusb_get_device(handle);
> +fprintf(stderr, "Open a usb-device with vid:pid %04x:%04x on
> "
> +"bus %03x device %03x\n",
> +usbvendor, usbproduct,
> +libusb_get_bus_number(dev),
> +libusb_get_device_address(dev));
> +}
>  } else {
>  libusb_device **list = NULL;
>  ssize_t i, n;

Patch is good for me.
I would just move the "dev" declaration inside the if so to have
a single hunk change and reduce variable visibility.

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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Christophe de Dinechin]

> On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> include/spice-streaming-agent/plugin.hpp | 8 
>> 1 file changed, 8 insertions(+)
>> 
>> diff --git a/include/spice-streaming-agent/plugin.hpp
>> b/include/spice-streaming-agent/plugin.hpp
>> index 1a58856..c370eab 100644
>> --- a/include/spice-streaming-agent/plugin.hpp
>> +++ b/include/spice-streaming-agent/plugin.hpp
>> @@ -149,6 +149,14 @@ extern "C" unsigned
>> spice_streaming_agent_plugin_interface_version;
>>  */
>> extern "C" SpiceStreamingAgent::PluginInitFunc
>> spice_streaming_agent_plugin_init;
>> 
>> +#define SPICE_STREAMING_AGENT_PLUGIN(agent) \
>> +__attribute__ ((visibility ("default")))\
>> +unsigned spice_streaming_agent_plugin_interface_version =   \
>> +SpiceStreamingAgent::PluginInterfaceVersion;\
>> +\
>> +__attribute__ ((visibility ("default")))\
>> +bool spice_streaming_agent_plugin_init(SpiceStreamingAgent::Agent*
>> agent)
>> +
>> #endif
>> 
>> #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
> 
> Surely helps. Some notes:
> - spice_streaming_agent_plugin_interface_version is not const so I assume
>  you want to allow to change it;

No, it’s that if you make it const, the compiler may optimize it away.
There are two ways I can avoid that:
1. Use its address
2. Check that it won’t eliminate it if the __attribute__ above is used.

> - the attribute is GCC syntax but can be changed if needed;

This was one reason to put it in a macro.


> - I know we use that style of indentation for line continuation but I
>  honestly prefer to not have that indentation preferring a " \" at the
>  end. This as current style:
>  - is hard to maintain. Currently is already broken as last like is
>wrong;

It’s actually the other way round with Emacs, which auto-indents 
trailing \ (also has a C-c C-\ command (c-backslash-region).

The last line is just too long, I will split it.

>  - it make copy harder unless you indent always at a given
>standard column;

Again, the problem goes the opposite way if you use Emacs.

>  - make email patch potentially hard to read.

Why? Are you using a proportional font when reading patch emails?

In any case, that looks like a personal preference, and it goes the
opposite way for me for a variety of reasons:
- Your reasons backwards (when using Emacs to write and read patches)
- Emacs Is Always Right™
- Having \ at a known location helps me “put it out of the way” while reading.


Cheers,
Christophe

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

Re: [Spice-devel] [PATCH usbredir v3] usbredirserver: show bus:device of the USB device if specifying vid:pid

[Frediano Ziglio]

> 
> From: Chen Hanxiao 
> 
> We use libusb_open_device_with_vid_pid, if multiple devices
> have the same vid:pid, it will only the first one.
> 
> This patch will show the bus:device of the chosen one.
> 
> Signed-off-by: Chen Hanxiao 

Acked-by: Frediano Ziglio 

> ---
> v3: move dev declaration inside if
> v2: control output messages by verbose
> 
>  usbredirserver/usbredirserver.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/usbredirserver/usbredirserver.c
> b/usbredirserver/usbredirserver.c
> index 13965dc..d26c4d3 100644
> --- a/usbredirserver/usbredirserver.c
> +++ b/usbredirserver/usbredirserver.c
> @@ -328,6 +328,15 @@ int main(int argc, char *argv[])
>  "Could not open an usb-device with vid:pid %04x:%04x\n",
>  usbvendor, usbproduct);
>  }
> +if (verbose >= usbredirparser_info) {
> +libusb_device *dev;
> +dev = libusb_get_device(handle);
> +fprintf(stderr, "Open a usb-device with vid:pid %04x:%04x on
> "
> +"bus %03x device %03x\n",
> +usbvendor, usbproduct,
> +libusb_get_bus_number(dev),
> +libusb_get_device_address(dev));
> +}
>  } else {
>  libusb_device **list = NULL;
>  ssize_t i, n;

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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Frediano Ziglio]

> 
> > On 23 Nov 2017, at 14:55, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
> >>> 
>  
>  From: Christophe de Dinechin 
>  
>  Signed-off-by: Christophe de Dinechin 
>  ---
>  include/spice-streaming-agent/plugin.hpp | 8 
>  1 file changed, 8 insertions(+)
>  
>  diff --git a/include/spice-streaming-agent/plugin.hpp
>  b/include/spice-streaming-agent/plugin.hpp
>  index 1a58856..c370eab 100644
>  --- a/include/spice-streaming-agent/plugin.hpp
>  +++ b/include/spice-streaming-agent/plugin.hpp
>  @@ -149,6 +149,14 @@ extern "C" unsigned
>  spice_streaming_agent_plugin_interface_version;
>  */
>  extern "C" SpiceStreamingAgent::PluginInitFunc
>  spice_streaming_agent_plugin_init;
>  
>  +#define SPICE_STREAMING_AGENT_PLUGIN(agent)
>  \
>  +__attribute__ ((visibility ("default")))
>  \
>  +unsigned spice_streaming_agent_plugin_interface_version =
>  \
>  +SpiceStreamingAgent::PluginInterfaceVersion;
>  \
>  +
>  \
>  +__attribute__ ((visibility ("default")))
>  \
>  +bool spice_streaming_agent_plugin_init(SpiceStreamingAgent::Agent*
>  agent)
>  +
>  #endif
>  
>  #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
> >>> 
> >>> Surely helps. Some notes:
> >>> - spice_streaming_agent_plugin_interface_version is not const so I assume
> >>> you want to allow to change it;
> >> 
> >> No, it’s that if you make it const, the compiler may optimize it away.
> >> There are two ways I can avoid that:
> >> 1. Use its address
> >> 2. Check that it won’t eliminate it if the __attribute__ above is used.
> >> 
> > 
> > The attribute and the extern "C" should give the compiler quite lot of
> > hints (more the second).
> > 
> >>> - the attribute is GCC syntax but can be changed if needed;
> >> 
> >> This was one reason to put it in a macro.
> >> 
> >> 
> >>> - I know we use that style of indentation for line continuation but I
> >>> honestly prefer to not have that indentation preferring a " \" at the
> >>> end. This as current style:
> >>> - is hard to maintain. Currently is already broken as last like is
> >>>   wrong;
> >> 
> >> It’s actually the other way round with Emacs, which auto-indents
> >> trailing \ (also has a C-c C-\ command (c-backslash-region).
> >> 
> >> The last line is just too long, I will split it.
> >> 
> >>> - it make copy harder unless you indent always at a given
> >>>   standard column;
> >> 
> >> Again, the problem goes the opposite way if you use Emacs.
> >> 
> >>> - make email patch potentially hard to read.
> >> 
> >> Why? Are you using a proportional font when reading patch emails?
> >> 
> > 
> > If lines are long they'll all split in 2 lines.
> > Also if code needs to be indented again there will be a lot of lines
> > of difference just to change indentation forcing people to apply the
> > patch and use some options to ignore spaces which will potentially can
> > hide other unwanted space changes.
> 
> I don’t see how any of this is specific to trailing backslashes.
> You could say the same thing for reindenting code because you
> added an ‘if’.
> 

Consider with indentation

#define XXX  \
something A  \
something BB

adding a longer line:

#define XXX   \
something A   \
something BB  \
something CCC 

Now consider without indentation

#define XXX \
something A \
something BB

adding a longer line:

#define XXX \
something A \
something BB \
something CCC

The diff (and the patch email) is smaller without indentation.
Yes, an "if" would require some reindentation but having indentation
on left and right increase a lot the probability you need it.

> > 
> >> In any case, that looks like a personal preference, and it goes the
> >> opposite way for me for a variety of reasons:
> >> - Your reasons backwards (when using Emacs to write and read patches)
> >> - Emacs Is Always Right™
> >> - Having \ at a known location helps me “put it out of the way” while
> >> reading.
> >> 
> >> 
> >> Cheers,
> >> Christophe
> >> 
> > 
> > Half of your reasoning are based on the editor used, if I remove
> > Emacs from them basically you agree with me :-)
> 
> All of our respective reasonings are based on personal preferences.
> Those, in turn, derive from the tools we use. I am not trying to make
> them anything else than personal preferences. I was only explaining
> that my editor choice happens to turn 100% of your arguments backwards.
> 
> But no, I don’t agree that code with misaligned trailing \ looks better
> or is easier to read than aligned code. It makes it hard to read for me,
> and I see no real reason to write code I find hard to read :-)
> 

Yes, we agree is style. Personally I find not indentation as readable
as the other style.

Frediano

[Spice-devel] [PATCH] red-stream: fix build without SASL

[Uri Lublin]

put red_stream_disable_writev in an #ifdef HAVE_SASL block.
red_stream_disable_writev is only called from functions
that are already in an #ifdef HAVE_SASL block.

Currently when building with SASL disabled, I get:
  CC   red-stream.lo
red-stream.c:441:13: error: 'red_stream_disable_writev'
   defined but not used [-Werror=unused-function]

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

diff --git a/server/red-stream.c b/server/red-stream.c
index af722f2ab..45e92a57b 100644
--- a/server/red-stream.c
+++ b/server/red-stream.c
@@ -438,10 +438,12 @@ bool red_stream_is_ssl(RedStream *stream)
 return (stream->priv->ssl != NULL);
 }
 
+#if HAVE_SASL
 static void red_stream_disable_writev(RedStream *stream)
 {
 stream->priv->writev = NULL;
 }
+#endif
 
 RedStreamSslStatus red_stream_ssl_accept(RedStream *stream)
 {
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 1/3] StreamDevice: Handle incomplete reads of StreamMsgFormat

[Frediano Ziglio]

From: Jonathon Jongsma 

This is currently unlikely to happen since we communicate over a pipe
and the pipe buffer is sufficiently large to avoid splitting the
message. But for completeness, we should handle this scenario.

Signed-off-by: Jonathon Jongsma 
---
 configure.ac   |  2 +-
 server/stream-device.c | 31 ++-
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/configure.ac b/configure.ac
index fb266ad4..3401dba8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -156,7 +156,7 @@ AS_IF([test x"$have_smartcard" = "xyes"], [
 AS_VAR_APPEND([SPICE_REQUIRES], [" libcacard >= 0.1.2"])
 ])
 
-SPICE_PROTOCOL_MIN_VER=0.12.13
+SPICE_PROTOCOL_MIN_VER=0.12.14
 PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 
$SPICE_PROTOCOL_MIN_VER])
 AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 
diff --git a/server/stream-device.c b/server/stream-device.c
index fc5b5065..efa6d8db 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -42,6 +42,12 @@ struct StreamDevice {
 
 StreamDevHeader hdr;
 uint8_t hdr_pos;
+union {
+StreamMsgFormat format;
+StreamMsgCapabilities capabilities;
+uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
+} msg;
+uint32_t msg_pos;
 bool has_error;
 bool opened;
 bool flow_stopped;
@@ -155,19 +161,25 @@ handle_msg_invalid(StreamDevice *dev, 
SpiceCharDeviceInstance *sin, const char *
 static bool
 handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 {
-StreamMsgFormat fmt;
 SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
-int n = sif->read(sin, (uint8_t *) , sizeof(fmt));
-if (n == 0) {
-return false;
-}
-if (n != sizeof(fmt)) {
+
+spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
+spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
+
+int n = sif->read(sin, dev->msg.buf + dev->msg_pos, 
sizeof(StreamMsgFormat) - dev->msg_pos);
+if (n < 0) {
 return handle_msg_invalid(dev, sin, NULL);
 }
-fmt.width = GUINT32_FROM_LE(fmt.width);
-fmt.height = GUINT32_FROM_LE(fmt.height);
-stream_channel_change_format(dev->stream_channel, );
 
+dev->msg_pos += n;
+
+if (dev->msg_pos < sizeof(StreamMsgFormat)) {
+return false;
+}
+
+dev->msg.format.width = GUINT32_FROM_LE(dev->msg.format.width);
+dev->msg.format.height = GUINT32_FROM_LE(dev->msg.format.height);
+stream_channel_change_format(dev->stream_channel, >msg.format);
 return true;
 }
 
@@ -334,6 +346,7 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t 
event)
 allocate_channels(dev);
 }
 dev->hdr_pos = 0;
+dev->msg_pos = 0;
 dev->has_error = false;
 dev->flow_stopped = false;
 red_char_device_reset(char_dev);
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 3/3] test-stream-device: Check incomplete reads of StreamMsgFormat

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/tests/test-stream-device.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index c6f47758..656bf56b 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -139,6 +139,11 @@ static void test_stream_device(void)
 ++message_sizes_end;
 
 p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_VP9);
+
+// this split the second format in half
+*message_sizes_end = p - message - 4;
+++message_sizes_end;
+
 *message_sizes_end = p - message;
 ++message_sizes_end;
 
@@ -163,10 +168,10 @@ static void test_stream_device(void)
 spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
 spice_server_char_device_wakeup(_instance);
 
-// make sure first 2 parts are read completely
-g_assert(message_sizes_curr - message_sizes >= 2);
+// make sure first 3 parts are read completely
+g_assert(message_sizes_curr - message_sizes >= 3);
 // make sure last part is not read at all
-g_assert(message_sizes_curr - message_sizes < 4);
+g_assert(message_sizes_curr - message_sizes < 5);
 
 test_destroy(test);
 basic_event_loop_destroy();
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 2/3] fixup! StreamDevice: Handle incomplete reads of StreamMsgFormat

[Frediano Ziglio]

This fix a regression in message handling.
---
 server/stream-device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/server/stream-device.c b/server/stream-device.c
index efa6d8db..0953a6d0 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -95,6 +95,7 @@ stream_device_read_msg_from_dev(RedCharDevice *self, 
SpiceCharDeviceInstance *si
 if (dev->hdr_pos >= sizeof(dev->hdr)) {
 dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
 dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
+dev->msg_pos = 0;
 }
 }
 
-- 
2.14.3

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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Christophe de Dinechin]

> On 23 Nov 2017, at 14:55, Frediano Ziglio  wrote:
> 
>>> 
>>> On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
>>> 
 
 From: Christophe de Dinechin 
 
 Signed-off-by: Christophe de Dinechin 
 ---
 include/spice-streaming-agent/plugin.hpp | 8 
 1 file changed, 8 insertions(+)
 
 diff --git a/include/spice-streaming-agent/plugin.hpp
 b/include/spice-streaming-agent/plugin.hpp
 index 1a58856..c370eab 100644
 --- a/include/spice-streaming-agent/plugin.hpp
 +++ b/include/spice-streaming-agent/plugin.hpp
 @@ -149,6 +149,14 @@ extern "C" unsigned
 spice_streaming_agent_plugin_interface_version;
 */
 extern "C" SpiceStreamingAgent::PluginInitFunc
 spice_streaming_agent_plugin_init;
 
 +#define SPICE_STREAMING_AGENT_PLUGIN(agent) \
 +__attribute__ ((visibility ("default")))\
 +unsigned spice_streaming_agent_plugin_interface_version =   \
 +SpiceStreamingAgent::PluginInterfaceVersion;\
 +\
 +__attribute__ ((visibility ("default")))\
 +bool spice_streaming_agent_plugin_init(SpiceStreamingAgent::Agent*
 agent)
 +
 #endif
 
 #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
>>> 
>>> Surely helps. Some notes:
>>> - spice_streaming_agent_plugin_interface_version is not const so I assume
>>> you want to allow to change it;
>> 
>> No, it’s that if you make it const, the compiler may optimize it away.
>> There are two ways I can avoid that:
>> 1. Use its address
>> 2. Check that it won’t eliminate it if the __attribute__ above is used.
>> 
> 
> The attribute and the extern "C" should give the compiler quite lot of
> hints (more the second).
> 
>>> - the attribute is GCC syntax but can be changed if needed;
>> 
>> This was one reason to put it in a macro.
>> 
>> 
>>> - I know we use that style of indentation for line continuation but I
>>> honestly prefer to not have that indentation preferring a " \" at the
>>> end. This as current style:
>>> - is hard to maintain. Currently is already broken as last like is
>>>   wrong;
>> 
>> It’s actually the other way round with Emacs, which auto-indents
>> trailing \ (also has a C-c C-\ command (c-backslash-region).
>> 
>> The last line is just too long, I will split it.
>> 
>>> - it make copy harder unless you indent always at a given
>>>   standard column;
>> 
>> Again, the problem goes the opposite way if you use Emacs.
>> 
>>> - make email patch potentially hard to read.
>> 
>> Why? Are you using a proportional font when reading patch emails?
>> 
> 
> If lines are long they'll all split in 2 lines.
> Also if code needs to be indented again there will be a lot of lines
> of difference just to change indentation forcing people to apply the
> patch and use some options to ignore spaces which will potentially can
> hide other unwanted space changes.

I don’t see how any of this is specific to trailing backslashes.
You could say the same thing for reindenting code because you
added an ‘if’.

> 
>> In any case, that looks like a personal preference, and it goes the
>> opposite way for me for a variety of reasons:
>> - Your reasons backwards (when using Emacs to write and read patches)
>> - Emacs Is Always Right™
>> - Having \ at a known location helps me “put it out of the way” while
>> reading.
>> 
>> 
>> Cheers,
>> Christophe
>> 
> 
> Half of your reasoning are based on the editor used, if I remove
> Emacs from them basically you agree with me :-)

All of our respective reasonings are based on personal preferences.
Those, in turn, derive from the tools we use. I am not trying to make
them anything else than personal preferences. I was only explaining
that my editor choice happens to turn 100% of your arguments backwards.

But no, I don’t agree that code with misaligned trailing \ looks better
or is easier to read than aligned code. It makes it hard to read for me,
and I see no real reason to write code I find hard to read :-)

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

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

[Spice-devel] [PATCH usbredir v3] usbredirserver: show bus:device of the USB device if specifying vid:pid

[Chen Hanxiao]

From: Chen Hanxiao 

We use libusb_open_device_with_vid_pid, if multiple devices
have the same vid:pid, it will only the first one.

This patch will show the bus:device of the chosen one.

Signed-off-by: Chen Hanxiao 
---
v3: move dev declaration inside if
v2: control output messages by verbose

 usbredirserver/usbredirserver.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/usbredirserver/usbredirserver.c b/usbredirserver/usbredirserver.c
index 13965dc..d26c4d3 100644
--- a/usbredirserver/usbredirserver.c
+++ b/usbredirserver/usbredirserver.c
@@ -328,6 +328,15 @@ int main(int argc, char *argv[])
 "Could not open an usb-device with vid:pid %04x:%04x\n",
 usbvendor, usbproduct);
 }
+if (verbose >= usbredirparser_info) {
+libusb_device *dev;
+dev = libusb_get_device(handle);
+fprintf(stderr, "Open a usb-device with vid:pid %04x:%04x on "
+"bus %03x device %03x\n",
+usbvendor, usbproduct,
+libusb_get_bus_number(dev),
+libusb_get_device_address(dev));
+}
 } else {
 libusb_device **list = NULL;
 ssize_t i, n;
-- 
2.13.6

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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Frediano Ziglio]

> 
> > On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> include/spice-streaming-agent/plugin.hpp | 8 
> >> 1 file changed, 8 insertions(+)
> >> 
> >> diff --git a/include/spice-streaming-agent/plugin.hpp
> >> b/include/spice-streaming-agent/plugin.hpp
> >> index 1a58856..c370eab 100644
> >> --- a/include/spice-streaming-agent/plugin.hpp
> >> +++ b/include/spice-streaming-agent/plugin.hpp
> >> @@ -149,6 +149,14 @@ extern "C" unsigned
> >> spice_streaming_agent_plugin_interface_version;
> >>  */
> >> extern "C" SpiceStreamingAgent::PluginInitFunc
> >> spice_streaming_agent_plugin_init;
> >> 
> >> +#define SPICE_STREAMING_AGENT_PLUGIN(agent) \
> >> +__attribute__ ((visibility ("default")))\
> >> +unsigned spice_streaming_agent_plugin_interface_version =   \
> >> +SpiceStreamingAgent::PluginInterfaceVersion;\
> >> +\
> >> +__attribute__ ((visibility ("default")))\
> >> +bool spice_streaming_agent_plugin_init(SpiceStreamingAgent::Agent*
> >> agent)
> >> +
> >> #endif
> >> 
> >> #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
> > 
> > Surely helps. Some notes:
> > - spice_streaming_agent_plugin_interface_version is not const so I assume
> >  you want to allow to change it;
> 
> No, it’s that if you make it const, the compiler may optimize it away.
> There are two ways I can avoid that:
> 1. Use its address
> 2. Check that it won’t eliminate it if the __attribute__ above is used.
> 

The attribute and the extern "C" should give the compiler quite lot of
hints (more the second).

> > - the attribute is GCC syntax but can be changed if needed;
> 
> This was one reason to put it in a macro.
> 
> 
> > - I know we use that style of indentation for line continuation but I
> >  honestly prefer to not have that indentation preferring a " \" at the
> >  end. This as current style:
> >  - is hard to maintain. Currently is already broken as last like is
> >wrong;
> 
> It’s actually the other way round with Emacs, which auto-indents
> trailing \ (also has a C-c C-\ command (c-backslash-region).
> 
> The last line is just too long, I will split it.
> 
> >  - it make copy harder unless you indent always at a given
> >standard column;
> 
> Again, the problem goes the opposite way if you use Emacs.
> 
> >  - make email patch potentially hard to read.
> 
> Why? Are you using a proportional font when reading patch emails?
> 

If lines are long they'll all split in 2 lines.
Also if code needs to be indented again there will be a lot of lines
of difference just to change indentation forcing people to apply the
patch and use some options to ignore spaces which will potentially can
hide other unwanted space changes.

> In any case, that looks like a personal preference, and it goes the
> opposite way for me for a variety of reasons:
> - Your reasons backwards (when using Emacs to write and read patches)
> - Emacs Is Always Right™
> - Having \ at a known location helps me “put it out of the way” while
> reading.
> 
> 
> Cheers,
> Christophe
> 

Half of your reasoning are based on the editor used, if I remove
Emacs from them basically you agree with me :-)

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

Re: [Spice-devel] [PATCH usbredir v2] usbredirserver: show bus:device of the USB device if specifying vid:pid

[Chen Hanxiao]

At 2017-11-23 19:28:42, "Frediano Ziglio"  wrote:
>> 
>> From: Chen Hanxiao 
>> 
>> We use libusb_open_device_with_vid_pid, if multiple devices
>> have the same vid:pid, it will only the first one.
>> 
>> This patch will show the bus:device of the chosen one.
>> 
>> Signed-off-by: Chen Hanxiao 
>> ---
>> v2: control output messages by verbose
>> 
>>  usbredirserver/usbredirserver.c | 9 +
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/usbredirserver/usbredirserver.c
>> b/usbredirserver/usbredirserver.c
>> index 13965dc..4685e0d 100644
>> --- a/usbredirserver/usbredirserver.c
>> +++ b/usbredirserver/usbredirserver.c
>> @@ -321,6 +321,7 @@ int main(int argc, char *argv[])
>>  
>>  /* Try to find the specified usb device */
>>  if (usbvendor != -1) {
>> +libusb_device *dev;
>>  handle = libusb_open_device_with_vid_pid(ctx, usbvendor,
>>   usbproduct);
>>  if (!handle) {
>> @@ -328,6 +329,14 @@ int main(int argc, char *argv[])
>>  "Could not open an usb-device with vid:pid %04x:%04x\n",
>>  usbvendor, usbproduct);
>>  }
>> +if (verbose >= usbredirparser_info) {
>> +dev = libusb_get_device(handle);
>> +fprintf(stderr, "Open a usb-device with vid:pid %04x:%04x on
>> "
>> +"bus %03x device %03x\n",
>> +usbvendor, usbproduct,
>> +libusb_get_bus_number(dev),
>> +libusb_get_device_address(dev));
>> +}
>>  } else {
>>  libusb_device **list = NULL;
>>  ssize_t i, n;
>
>Patch is good for me.
>I would just move the "dev" declaration inside the if so to have
>a single hunk change and reduce variable visibility.
>

Sure, I'll fix this in v3

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

[Spice-devel] Cannot install spice-guest-tools in Windows 2016

[Stefan Pietsch]

Dear list,

I tried to install the latest Spice guest tools in Windows 2016, but the
installer says: "Unsupported Windows version".

This is the file I was trying to install:
https://www.spice-space.org/download/binaries/spice-guest-tools/spice-guest-tools-0.132/spice-guest-tools-0.132.exe

Is there a more recent installer available?

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