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

2017-11-22 Thread Christophe Fergeau 
On Wed, Nov 22, 2017 at 06:19:34PM +0100, Christophe Fergeau wrote:
> On Wed, Nov 22, 2017 at 04:33:23PM +0100, Christophe de Dinechin 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.
> 
> I don't think fixing that issue with Agent::PluginVersionIsCompatible
> requires switching to that libtool ABI versioning, does it? In other
> word, do these 2 changes (a bug fix, and a functional change) have to go
> in the same commit?

Oh, and just realized that the log does not have any explanation of how
you fixed this bug, nor how the suggested versioning works.

Christophe


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

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

2017-11-22 Thread Christophe Fergeau 
On Wed, Nov 22, 2017 at 04:33:23PM +0100, Christophe de Dinechin 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.

I don't think fixing that issue with Agent::PluginVersionIsCompatible
requires switching to that libtool ABI versioning, does it? In other
word, do these 2 changes (a bug fix, and a functional change) have to go
in the same commit?

Christophe

> 
> 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 initialize and does not want to be unloaded,
> + * it may still return true on failure.

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

2017-11-22 Thread Christophe de Dinechin 


> 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 initialize and does not want to be unloaded,
>> + * it may still return true on failure. This is necessary
>>  * if the plugin uses some library which are not safe to be unloaded.
>>  * This public interface is also designed to avoid

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

2017-11-22 Thread Frediano Ziglio 
> 
> 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 initialize and does not want to be unloaded,
> + * it may still return true on failure. This is necessary
>   * if the plugin uses some library which are not safe to be unloaded.
>   * This public interface is also designed to avoid exporting data from
>   * the plugin which could be a problem in some systems.
>   * \return true if plugin should stay loaded, false otherwise
>   */
>  extern "C"

Re: [Spice-devel] [PATCH spice-server v2] RFC: Support abstract Unix sockets

2017-11-22 Thread Christophe Fergeau 
On Wed, Nov 22, 2017 at 11:35:06AM -0500, Frediano Ziglio wrote:
> > 
> > > 
> > > On Wed, Nov 22, 2017 at 12:11:33PM +, Frediano Ziglio wrote:
> > > > Allows to specify abstract Unix sockets addresses.
> > > > These Unix sockets are supported on Linux and allows to not
> > > > have file system names.
> > > 
> > > What would be the use-case? Just cleaner not to have a dummy path in the
> > > FS? Or does it bring more? I'd say why not, though a spice-gtk patch
> > > will be needed too.
> > > 
> > 
> > They have pro and cons. As said they don't have a FS name so for instance
> > programs running on some chroot can access the sockets too. For instance
> > recent Xorg bind to @/tmp/.X11-unix/X also.
> > Also you don't need to unlink the FS entry at the end. This could avoid
> > to do the cleanup from libvirt.
> > On the cons not having a FS prevent easily to change permissions on the
> > socket.
> > 
> 
> On the paranoia level (happily you could not throw rotten tomatoes remotely):
> A pro is that is easy to implement.
> A cons is that you cannot have a FS name starting with "@". But probably
> you want a full name here so would start with "/" in any case.

Yeah, I don't think it's going to be an issue..

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] inputs-channels: Remove leak using tablet device

2017-11-22 Thread Christophe Fergeau 
On Wed, Nov 22, 2017 at 11:17:52AM -0500, Frediano Ziglio wrote:
> > 
> > I'd expect a few lines in the commit log.
> > 
> > Acked-by: Christophe Fergeau 
> > 
> > Christophe
> > 
> 
> What about:
> 
> "Current code does not free allocated tablet resources.
> When a tablet is added some resources are allocated.
> Resources should be released either removing the tablet or
> freeing spice server object.
> Added a test to check these conditions."

Yup, sounds good, thanks!

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 v2] RFC: Support abstract Unix sockets

2017-11-22 Thread Frediano Ziglio 
> 
> > 
> > On Wed, Nov 22, 2017 at 12:11:33PM +, Frediano Ziglio wrote:
> > > Allows to specify abstract Unix sockets addresses.
> > > These Unix sockets are supported on Linux and allows to not
> > > have file system names.
> > 
> > What would be the use-case? Just cleaner not to have a dummy path in the
> > FS? Or does it bring more? I'd say why not, though a spice-gtk patch
> > will be needed too.
> > 
> 
> They have pro and cons. As said they don't have a FS name so for instance
> programs running on some chroot can access the sockets too. For instance
> recent Xorg bind to @/tmp/.X11-unix/X also.
> Also you don't need to unlink the FS entry at the end. This could avoid
> to do the cleanup from libvirt.
> On the cons not having a FS prevent easily to change permissions on the
> socket.
> 

On the paranoia level (happily you could not throw rotten tomatoes remotely):
A pro is that is easy to implement.
A cons is that you cannot have a FS name starting with "@". But probably
you want a full name here so would start with "/" in any case.

> > 
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/reds.c | 6 +-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > Changes since v1:
> > > - do not unlink no file socket.
> > > 
> > > diff --git a/server/reds.c b/server/reds.c
> > > index ebcbe496..ca0bb75a 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -2585,8 +2585,12 @@ static int reds_init_socket(const char *addr, int
> > > portnr, int family)
> > >  
> > >  local.sun_family = AF_UNIX;
> > >  g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
> > > -unlink(local.sun_path);
> > >  len = SUN_LEN();
> > > +if (local.sun_path[0] == '@') {
> > > +local.sun_path[0] = 0;
> > > +} else {
> > > +unlink(local.sun_path);
> > > +}
> > >  if (bind(slisten, (struct sockaddr *), len) == -1) {
> > >  perror("bind");
> > >  return -1;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent] Report initialization errors more precisely

2017-11-22 Thread Frediano Ziglio 
> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/concrete-agent.cpp | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 192054a..ddeac8e 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -89,11 +89,15 @@ void ConcreteAgent::LoadPlugin(const char
> *plugin_filename)
>  PluginInitFunc* init_func =
>  (PluginInitFunc *) dlsym(dl,
>  "spice_streaming_agent_plugin_init");
>  if (!init_func || !init_func(this)) {
> +syslog(LOG_ERR, "error loading plugin %s: %s",
> +   plugin_filename,
> +   init_func ? "module init failed" : "no module init
> function");
>  dlclose(dl);

The init function can return true also if it decided it
cannot work. This can be a normal condition if the specific
plugin require some specific h/w. In this case you should not
have a log.

>  }
>  }
>  catch (std::runtime_error ) {
> -syslog(LOG_ERR, "%s", err.what());
> +syslog(LOG_ERR, "error loading plugin %s: %s",
> +   plugin_filename, err.what());

yes, better and more consistent

>  dlclose(dl);
>  }
>  }

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

Re: [Spice-devel] [PATCH spice-server] inputs-channels: Remove leak using tablet device

2017-11-22 Thread Frediano Ziglio 
> 
> I'd expect a few lines in the commit log.
> 
> Acked-by: Christophe Fergeau 
> 
> Christophe
> 

What about:

"Current code does not free allocated tablet resources.
When a tablet is added some resources are allocated.
Resources should be released either removing the tablet or
freeing spice server object.
Added a test to check these conditions."

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

2017-11-22 Thread Frediano Ziglio 
> 
> 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.
> 

This currently is not true in many ABI unless you add a vfunc before
Agent::PluginVersionIsCompatible which you are not supposed to do.

> 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.
> 

Any ABI came with implementation details, if the compiler decide
to change the registers used there's no way to maintain an ABI.

> 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.
> 

No, to read that variable you need to load the plugin, what is changing
is that you don't need to call a function. Personally having an ABI
relying in exporting variables is not that safe and portable.

> 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.
> 

How are you going to implement that a plugin support from version X
to version Y?

> 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;
> +

Is not a good idea to export variables. In some environment variable
are copied into the executable. But I can do some test about it, I
think using dynamic linking can be safe.
Also you point to libtool versioning but how to get the age with
this interface?
Or the API version itself define the range?

> +/*!
>   * Plugin main entry point.
> - *

Re: [Spice-devel] [PATCH spice-server] inputs-channels: Remove leak using tablet device

2017-11-22 Thread Christophe Fergeau 
I'd expect a few lines in the commit log.

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

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

2017-11-22 Thread Christophe de Dinechin 
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
-- 
2.13.5 (Apple Git-94)

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

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

2017-11-22 Thread Christophe de Dinechin 
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 initialize and does not want to be unloaded,
+ * it may still return true on failure. This is necessary
  * if the plugin uses some library which are not safe to be unloaded.
  * This public interface is also designed to avoid exporting data from
  * the plugin which could be a problem in some systems.
  * \return true if plugin should stay loaded, false otherwise
  */
 extern "C" SpiceStreamingAgent::PluginInitFunc 
spice_streaming_agent_plugin_init;
+
 #endif
 
 #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 192054a..be0ec66 100644
---

[Spice-devel] [PATCH spice-streaming-agent 0/2] Let the agent check plugin versions to avoid ODR violations

2017-11-22 Thread Christophe de Dinechin 
From: Christophe de Dinechin 

This is remotely inspired from the Tao3D module API,
itself inspired by libtool.
(see https://github.com/c3d/tao-3D/blob/master/tao/include/tao/module_api.h#L41
and https://github.com/c3d/tao-3D/blob/master/tao/module_manager.cpp#L1246
for more information)

Christophe de Dinechin (2):
  Implement version checking for plugins without violating ODR
  Add a macro to deal with the boilerplate of writing a streaming agent
plugin

 include/spice-streaming-agent/plugin.hpp | 56 +++-
 src/concrete-agent.cpp   | 35 ++--
 src/concrete-agent.hpp   |  4 ---
 src/mjpeg-fallback.cpp   |  3 --
 4 files changed, 51 insertions(+), 47 deletions(-)

-- 
2.13.5 (Apple Git-94)

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

Re: [Spice-devel] [PATCH spice-streaming-agent] Report reason when there is an error loading the plugin

2017-11-22 Thread Frediano Ziglio 
> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/concrete-agent.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 192054a..9f1295a 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -81,7 +81,8 @@ void ConcreteAgent::LoadPlugin(const char *plugin_filename)
>  {
>  void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW);
>  if (!dl) {
> -syslog(LOG_ERR, "error loading plugin %s", plugin_filename);
> +syslog(LOG_ERR, "error loading plugin %s: %s",
> +   plugin_filename, dlerror());
>  return;
>  }
>  

Acked-by: Frediano Ziglio 

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

[Spice-devel] [PATCH spice-streaming-agent] Report reason when there is an error loading the plugin

2017-11-22 Thread Christophe de Dinechin 
From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/concrete-agent.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 192054a..9f1295a 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -81,7 +81,8 @@ void ConcreteAgent::LoadPlugin(const char *plugin_filename)
 {
 void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW);
 if (!dl) {
-syslog(LOG_ERR, "error loading plugin %s", plugin_filename);
+syslog(LOG_ERR, "error loading plugin %s: %s",
+   plugin_filename, dlerror());
 return;
 }
 
-- 
2.13.5 (Apple Git-94)

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

Re: [Spice-devel] [PATCH spice-server v2] RFC: Support abstract Unix sockets

2017-11-22 Thread Frediano Ziglio 
> 
> On Wed, Nov 22, 2017 at 12:11:33PM +, Frediano Ziglio wrote:
> > Allows to specify abstract Unix sockets addresses.
> > These Unix sockets are supported on Linux and allows to not
> > have file system names.
> 
> What would be the use-case? Just cleaner not to have a dummy path in the
> FS? Or does it bring more? I'd say why not, though a spice-gtk patch
> will be needed too.
> 

They have pro and cons. As said they don't have a FS name so for instance
programs running on some chroot can access the sockets too. For instance
recent Xorg bind to @/tmp/.X11-unix/X also.
Also you don't need to unlink the FS entry at the end. This could avoid
to do the cleanup from libvirt.
On the cons not having a FS prevent easily to change permissions on the
socket.

> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/reds.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > Changes since v1:
> > - do not unlink no file socket.
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index ebcbe496..ca0bb75a 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2585,8 +2585,12 @@ static int reds_init_socket(const char *addr, int
> > portnr, int family)
> >  
> >  local.sun_family = AF_UNIX;
> >  g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
> > -unlink(local.sun_path);
> >  len = SUN_LEN();
> > +if (local.sun_path[0] == '@') {
> > +local.sun_path[0] = 0;
> > +} else {
> > +unlink(local.sun_path);
> > +}
> >  if (bind(slisten, (struct sockaddr *), len) == -1) {
> >  perror("bind");
> >  return -1;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] reds: Do not get time if not needed

2017-11-22 Thread Christophe Fergeau 
On Wed, Nov 22, 2017 at 11:53:55AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 2b43bc0d..8bda85c7 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2036,11 +2036,8 @@ static void reds_handle_ticket(void *opaque)
>  RedLinkInfo *link = (RedLinkInfo *)opaque;
>  RedsState *reds = link->reds;
>  char *password;
> -time_t ltime;
>  int password_size;
>  
> -//todo: use monotonic time
> -time();
>  if (RSA_size(link->tiTicketing.rsa) < SPICE_MAX_PASSWORD_LENGTH) {
>  spice_warning("RSA modulus size is smaller than 
> SPICE_MAX_PASSWORD_LENGTH (%d < %d), "
>"SPICE ticket sent from client may be truncated",
> @@ -2061,7 +2058,8 @@ static void reds_handle_ticket(void *opaque)
>  password[password_size] = '\0';
>  
>  if (reds->config->ticketing_enabled && !link->skip_auth) {
> -int expired =  reds->config->taTicket.expiration_time < ltime;
> +time_t ltime;
> +bool expired;
>  
>  if (strlen(reds->config->taTicket.password) == 0) {
>  spice_warning("Ticketing is enabled, but no password is set. "
> @@ -2069,6 +2067,10 @@ static void reds_handle_ticket(void *opaque)
>  goto error;
>  }
>  
> +//todo: use monotonic time
> +time();
> +expired =  reds->config->taTicket.expiration_time < ltime;
> +

You can drop the extra white-space after the "= ". I'd put the right
hand side member around () to make it more obvious it's a condition, but
we usually disagree on this kind of details ;)

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-server v2] RFC: Support abstract Unix sockets

2017-11-22 Thread Christophe Fergeau 
On Wed, Nov 22, 2017 at 12:11:33PM +, Frediano Ziglio wrote:
> Allows to specify abstract Unix sockets addresses.
> These Unix sockets are supported on Linux and allows to not
> have file system names.

What would be the use-case? Just cleaner not to have a dummy path in the
FS? Or does it bring more? I'd say why not, though a spice-gtk patch
will be needed too.


> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Changes since v1:
> - do not unlink no file socket.
> 
> diff --git a/server/reds.c b/server/reds.c
> index ebcbe496..ca0bb75a 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2585,8 +2585,12 @@ static int reds_init_socket(const char *addr, int 
> portnr, int family)
>  
>  local.sun_family = AF_UNIX;
>  g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
> -unlink(local.sun_path);
>  len = SUN_LEN();
> +if (local.sun_path[0] == '@') {
> +local.sun_path[0] = 0;
> +} else {
> +unlink(local.sun_path);
> +}
>  if (bind(slisten, (struct sockaddr *), len) == -1) {
>  perror("bind");
>  return -1;
> -- 
> 2.14.3
> 
> ___
> 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] fixup! StreamDevice: Handle incomplete reads of StreamMsgFormat

2017-11-22 Thread Uri Lublin 

On 11/22/2017 02:59 PM, Uri Lublin wrote:

On 11/22/2017 12:16 PM, Frediano Ziglio wrote:

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;


Hi Frediano,

There is no msg_pos in StreamDevice.


Okay, I see it fixes a proposed patch that was not yet committed.

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

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

2017-11-22 Thread Uri Lublin 

On 11/22/2017 12:16 PM, Frediano Ziglio wrote:

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;


Hi Frediano,

There is no msg_pos in StreamDevice.

Uri.


  }
  }
  



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

[Spice-devel] [PATCH spice-server v2] RFC: Support abstract Unix sockets

2017-11-22 Thread Frediano Ziglio 
Allows to specify abstract Unix sockets addresses.
These Unix sockets are supported on Linux and allows to not
have file system names.

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

Changes since v1:
- do not unlink no file socket.

diff --git a/server/reds.c b/server/reds.c
index ebcbe496..ca0bb75a 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2585,8 +2585,12 @@ static int reds_init_socket(const char *addr, int 
portnr, int family)
 
 local.sun_family = AF_UNIX;
 g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
-unlink(local.sun_path);
 len = SUN_LEN();
+if (local.sun_path[0] == '@') {
+local.sun_path[0] = 0;
+} else {
+unlink(local.sun_path);
+}
 if (bind(slisten, (struct sockaddr *), len) == -1) {
 perror("bind");
 return -1;
-- 
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-server] RFC: Support abstract Unix sockets

2017-11-22 Thread Daniel P. Berrange 
On Wed, Nov 22, 2017 at 11:54:10AM +, Frediano Ziglio wrote:
> Allows to specify abstract Unix sockets addresses.
> These Unix sockets are supported on Linux and allows to not
> have file system names.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index ebcbe496..2b43bc0d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2587,6 +2587,9 @@ static int reds_init_socket(const char *addr, int 
> portnr, int family)
>  g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
>  unlink(local.sun_path);

You shouldn't be unlinking this path when its an abstract socket.
Likewise if there's any cleanup code elsewhere that unlinks...

>  len = SUN_LEN();
> +if (local.sun_path[0] == '@') {
> +local.sun_path[0] = 0;
> +}


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

[Spice-devel] [PATCH spice-server] RFC: Support abstract Unix sockets

2017-11-22 Thread Frediano Ziglio 
Allows to specify abstract Unix sockets addresses.
These Unix sockets are supported on Linux and allows to not
have file system names.

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index ebcbe496..2b43bc0d 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2587,6 +2587,9 @@ static int reds_init_socket(const char *addr, int portnr, 
int family)
 g_strlcpy(local.sun_path, addr, sizeof(local.sun_path));
 unlink(local.sun_path);
 len = SUN_LEN();
+if (local.sun_path[0] == '@') {
+local.sun_path[0] = 0;
+}
 if (bind(slisten, (struct sockaddr *), len) == -1) {
 perror("bind");
 return -1;
-- 
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] reds: Do not get time if not needed

2017-11-22 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 2b43bc0d..8bda85c7 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2036,11 +2036,8 @@ static void reds_handle_ticket(void *opaque)
 RedLinkInfo *link = (RedLinkInfo *)opaque;
 RedsState *reds = link->reds;
 char *password;
-time_t ltime;
 int password_size;
 
-//todo: use monotonic time
-time();
 if (RSA_size(link->tiTicketing.rsa) < SPICE_MAX_PASSWORD_LENGTH) {
 spice_warning("RSA modulus size is smaller than 
SPICE_MAX_PASSWORD_LENGTH (%d < %d), "
   "SPICE ticket sent from client may be truncated",
@@ -2061,7 +2058,8 @@ static void reds_handle_ticket(void *opaque)
 password[password_size] = '\0';
 
 if (reds->config->ticketing_enabled && !link->skip_auth) {
-int expired =  reds->config->taTicket.expiration_time < ltime;
+time_t ltime;
+bool expired;
 
 if (strlen(reds->config->taTicket.password) == 0) {
 spice_warning("Ticketing is enabled, but no password is set. "
@@ -2069,6 +2067,10 @@ static void reds_handle_ticket(void *opaque)
 goto error;
 }
 
+//todo: use monotonic time
+time();
+expired =  reds->config->taTicket.expiration_time < ltime;
+
 if (expired) {
 spice_warning("Ticket has expired");
 goto error;
-- 
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-server v2] red-channel-client: Avoid weird memory references using MarkerPipeItem

2017-11-22 Thread Frediano Ziglio 
ping

> 
> Instead of having MarkerPipeItem pointing to an external variable with
> the possibility to forget to reset it and have a dangling pointer use
> reference counting to keep the item and mark the item when sent.
> This item is placed into the queue to understand when it was sent. The
> current implementation detect the unqueue when the item is destroyed so
> we use an external variable to use it after the item is released.
> Instead this change update the variable (stored in the item) when the
> item is attempted to be sent. This avoids having to destroy the item.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel-client.c | 30 +++---
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 34202c492..de3ac27cb 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -218,7 +218,7 @@ typedef struct RedEmptyMsgPipeItem {
>  
>  typedef struct MarkerPipeItem {
>  RedPipeItem base;
> -gboolean *item_in_pipe;
> +bool item_in_pipe;
>  } MarkerPipeItem;
>  
>  static void red_channel_client_start_ping_timer(RedChannelClient *rcc,
>  uint32_t timeout)
> @@ -613,6 +613,7 @@ static void red_channel_client_send_item(RedChannelClient
> *rcc, RedPipeItem *ite
>  red_channel_client_send_ping(rcc);
>  break;
>  case RED_PIPE_ITEM_TYPE_MARKER:
> +SPICE_UPCAST(MarkerPipeItem, item)->item_in_pipe = false;
>  break;
>  default:
>  red_channel_send_item(rcc->priv->channel, rcc, item);
> @@ -1757,23 +1758,13 @@ void
> red_channel_client_set_header_sub_list(RedChannelClient *rcc, uint32_t sub_
>  
> rcc->priv->send_data.header.set_msg_sub_list(>priv->send_data.header,
>  sub_list);
>  }
>  
> -static void marker_pipe_item_free(RedPipeItem *base)
> -{
> -MarkerPipeItem *item = SPICE_UPCAST(MarkerPipeItem, base);
> -
> -if (item->item_in_pipe) {
> -*item->item_in_pipe = FALSE;
> -}
> -free(item);
> -}
> -
>  /* TODO: more evil sync stuff. anything with the word wait in it's name. */
>  bool red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  GList *item_pos,
>  int64_t timeout)
>  {
>  uint64_t end_time;
> -gboolean item_in_pipe;
> +bool item_in_pipe;
>  
>  spice_debug("trace");
>  
> @@ -1785,10 +1776,9 @@ bool
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  
>  MarkerPipeItem *mark_item = spice_new0(MarkerPipeItem, 1);
>  
> -red_pipe_item_init_full(_item->base, RED_PIPE_ITEM_TYPE_MARKER,
> -marker_pipe_item_free);
> -item_in_pipe = TRUE;
> -mark_item->item_in_pipe = _in_pipe;
> +red_pipe_item_init(_item->base, RED_PIPE_ITEM_TYPE_MARKER);
> +mark_item->item_in_pipe = true;
> +red_pipe_item_ref(_item->base);
>  red_channel_client_pipe_add_after_pos(rcc, _item->base, item_pos);
>  
>  if (red_channel_client_is_blocked(rcc)) {
> @@ -1797,7 +1787,7 @@ bool
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  }
>  red_channel_client_push(rcc);
>  
> -while (item_in_pipe &&
> +while (mark_item->item_in_pipe &&
> (timeout == -1 || spice_get_monotonic_time_ns() < end_time)) {
>  usleep(CHANNEL_BLOCKED_SLEEP_DURATION);
>  red_channel_client_receive(rcc);
> @@ -1805,9 +1795,11 @@ bool
> red_channel_client_wait_pipe_item_sent(RedChannelClient *rcc,
>  red_channel_client_push(rcc);
>  }
>  
> +item_in_pipe = mark_item->item_in_pipe;
> +red_pipe_item_unref(_item->base);
> +
>  if (item_in_pipe) {
> -// still on the queue, make sure won't overwrite the stack variable
> -mark_item->item_in_pipe = NULL;
> +// still on the queue
>  spice_warning("timeout");
>  return FALSE;
>  } else {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server] inputs-channels: Remove leak using tablet device

2017-11-22 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/inputs-channel.c   |  5 
 server/tests/test-leaks.c | 66 +++
 2 files changed, 71 insertions(+)

diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index f6ad27ec..b4292f0e 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -566,6 +566,7 @@ inputs_channel_finalize(GObject *object)
 InputsChannel *self = INPUTS_CHANNEL(object);
 RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
 
+inputs_channel_detach_tablet(self, self->tablet);
 reds_core_timer_remove(reds, self->key_modifiers_timer);
 
 G_OBJECT_CLASS(inputs_channel_parent_class)->finalize(object);
@@ -652,6 +653,10 @@ int inputs_channel_has_tablet(InputsChannel *inputs)
 void inputs_channel_detach_tablet(InputsChannel *inputs, SpiceTabletInstance 
*tablet)
 {
 spice_printerr("");
+if (tablet != NULL && tablet == inputs->tablet) {
+g_free(tablet->st);
+tablet->st = NULL;
+}
 inputs->tablet = NULL;
 }
 
diff --git a/server/tests/test-leaks.c b/server/tests/test-leaks.c
index 7032000a..41734c57 100644
--- a/server/tests/test-leaks.c
+++ b/server/tests/test-leaks.c
@@ -178,6 +178,71 @@ static void migration_leaks(void)
 basic_event_loop_destroy();
 }
 
+static void tablet_set_logical_size(SpiceTabletInstance* sin, int width, int 
height)
+{
+}
+
+static void tablet_position(SpiceTabletInstance* sin, int x, int y,
+uint32_t buttons_state)
+{
+}
+
+static void tablet_wheel(SpiceTabletInstance* sin, int wheel,
+ uint32_t buttons_state)
+{
+}
+
+static void tablet_buttons(SpiceTabletInstance *sin,
+   uint32_t buttons_state)
+{
+}
+
+static const SpiceTabletInterface tablet_interface = {
+.base = {
+.type  = SPICE_INTERFACE_TABLET,
+.description   = "tablet",
+.major_version = SPICE_INTERFACE_TABLET_MAJOR,
+.minor_version = SPICE_INTERFACE_TABLET_MINOR,
+},
+.set_logical_size   = tablet_set_logical_size,
+.position   = tablet_position,
+.wheel  = tablet_wheel,
+.buttons= tablet_buttons,
+};
+
+static void tablet_leaks(void)
+{
+SpiceCoreInterface *core;
+SpiceServer *server;
+SpiceTabletInstance tablet;
+
+core = basic_event_loop_init();
+g_assert_nonnull(core);
+
+// test if leaks without spice_server_remove_interface
+server = spice_server_new();
+g_assert_nonnull(server);
+g_assert_cmpint(spice_server_init(server, core), ==, 0);
+
+tablet.base.sif = _interface.base;
+spice_server_add_interface(server, );
+
+spice_server_destroy(server);
+
+// test if leaks with spice_server_remove_interface
+server = spice_server_new();
+g_assert_nonnull(server);
+g_assert_cmpint(spice_server_init(server, core), ==, 0);
+
+tablet.base.sif = _interface.base;
+spice_server_add_interface(server, );
+spice_server_remove_interface();
+
+spice_server_destroy(server);
+
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
@@ -185,6 +250,7 @@ int main(int argc, char *argv[])
 g_test_add_func("/server/server leaks", server_leaks);
 g_test_add_func("/server/vmc leaks", vmc_leaks);
 g_test_add_func("/server/migration leaks", migration_leaks);
+g_test_add_func("/server/tablet leaks", tablet_leaks);
 
 return g_test_run();
 }
-- 
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] Reduce dependencies from red-qxl.h

2017-11-22 Thread Frediano Ziglio 
This header is mainly exporting functions to handle public
interface for the QXL devices.
Avoid spreading its inclusion including this header in other
headers.

Signed-off-by: Frediano Ziglio 
---
 server/dcc-send.c| 1 +
 server/display-channel.c | 1 +
 server/display-channel.h | 1 -
 server/red-worker.c  | 1 +
 server/red-worker.h  | 1 -
 5 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/dcc-send.c b/server/dcc-send.c
index 84c10968..59993c10 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -24,6 +24,7 @@
 
 #include "dcc-private.h"
 #include "display-channel-private.h"
+#include "red-qxl.h"
 
 typedef enum {
 FILL_BITS_TYPE_INVALID,
diff --git a/server/display-channel.c b/server/display-channel.c
index ad346c93..345b133d 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -22,6 +22,7 @@
 
 #include "display-channel-private.h"
 #include "glib-compat.h"
+#include "red-qxl.h"
 
 G_DEFINE_TYPE(DisplayChannel, display_channel, TYPE_COMMON_GRAPHICS_CHANNEL)
 
diff --git a/server/display-channel.h b/server/display-channel.h
index 5e3d9eb7..73a06feb 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -32,7 +32,6 @@
 #include "red-record-qxl.h"
 #include "demarshallers.h"
 #include "red-channel.h"
-#include "red-qxl.h"
 #include "dispatcher.h"
 #include "main-channel.h"
 #include "migration-protocol.h"
diff --git a/server/red-worker.c b/server/red-worker.c
index fa57235d..046c723e 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -44,6 +44,7 @@
 
 #include "spice.h"
 #include "red-worker.h"
+#include "red-qxl.h"
 #include "cursor-channel.h"
 #include "tree.h"
 
diff --git a/server/red-worker.h b/server/red-worker.h
index 3c447f12..68c5058d 100644
--- a/server/red-worker.h
+++ b/server/red-worker.h
@@ -24,7 +24,6 @@
 #ifndef RED_WORKER_H_
 #define RED_WORKER_H_
 
-#include "red-qxl.h"
 #include "red-channel.h"
 
 typedef struct AsyncCommand AsyncCommand;
-- 
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] fixup! StreamDevice: Handle incomplete reads of StreamMsgFormat

2017-11-22 Thread 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] Remove X11 dependency from send_cursor

2017-11-22 Thread Frediano Ziglio 
> 
> I know that this was acked by Christophe, but I wanted to comment,
> because that makes me unhappy.
> 
> 
> Frediano Ziglio writes:
> 
> > Allows a better encapsulation in the future
> >
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  src/spice-streaming-agent.cpp | 28 +---
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 23b9768..53ffbf0 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -22,6 +22,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -284,15 +285,17 @@ static void usage(const char *progname)
> >  exit(1);
> >  }
> >
> > -static void send_cursor(const XFixesCursorImage )
> > +static void
> > +send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> > +std::function fill_cursor)
> 
> I find it really strange and error prone to pass the buffer one way
> (as a lambda argument) and its size (implicitly) another way (as a
> computation based on function arguments, i.e. width * height).
> I would have at the very least added a 'pixels' name and a buffer size
> in the lambda interface.
> 
> That suggests something is wrong about the "encapsulation". And indeed,
> if you have to copy pixels to convert from 'long' to 'uint32_t' and use
> that as a justification, it makes me *very* nervous. That alone would
> have been enough for me to nack this way of doing this.
> 
> But what annoys me really is that you are aware that I started a patch
> series where I began working on encapsulation, and trying to do it the
> C++ way, not by happily paper-patching advanced C++ over low-level
> C-style byte-level memory copies.
> 
> One of the comments that came out of the discussion of my patch series
> was precisely X11 encapsulation. So it would be reasonable to at least
> envision that addressing that comment could be part of my effort for the
> next iteration of the series, wouldn't it?
> 

Yes, the idea was to prepare for a better encapsulation having less
dependency.
Yes, I did not mention this in the comment.

> Based on the structure of my patch series, there is already a call
> that looks like
> 
>  stream->send_cursor(*cursor);
> 
> From that, you could correctly infer that this function could be
> an X11-specific feature, in an X11-specific stream class. That provides
> all the encapsulation you need, in a proper C++ way, using inheritance
> to deal with commonalities rather than using std::function and lambdas
> simply to pass a glorified memcpy.
> 

This make the class have dependency on the device itself and the
graphic system while is just called SpiceStream. Or would you prefer
to have part of the class implementation system dependent?

> But now that this patch is acked, I guess that I will have to rebase
> my RAII branch on what I frankly see as not even half of a quarter of some
> brain-damaged idea of encapsulation. For what benefit? What urgent
> problem did this patch solve that was not better addressed by working on
> real encapsulation?
> 

I already rebased your branch on 
https://gitlab.cee.redhat.com/fziglio/spice-streaming-agent-new/tree/c3d
if you want to avoid to rebase. There are other patches on the top, was trying
to change the encapsulation. The branch is surely not in production state.
Yes, the repository is RH internal, I'll have to move externally.
You stated in a previous comment that a different encapsulation was not on
top of your priority so I decided to propose some changes.

> >  {
> > -if (image.width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > -image.height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> > +if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> > +height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> >  return;
> >
> >  size_t cursor_size =
> >  sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> > -image.width * image.height * sizeof(uint32_t);
> > +width * height * sizeof(uint32_t);
> >  std::unique_ptr msg(new uint8_t[cursor_size]);
> >
> >  StreamDevHeader
> >  _hdr(*reinterpret_cast(msg.get()));
> > @@ -305,14 +308,13 @@ static void send_cursor(const XFixesCursorImage
> > )
> >  memset(_msg, 0, sizeof(cursor_msg));
> >
> >  cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> > -cursor_msg.width = image.width;
> > -cursor_msg.height = image.height;
> > -cursor_msg.hot_spot_x = image.xhot;
> > -cursor_msg.hot_spot_y = image.yhot;
> > +cursor_msg.width = width;
> > +cursor_msg.height = height;
> > +cursor_msg.hot_spot_x = hotspot_x;
> > +cursor_msg.hot_spot_y = hotspot_y;
> >
> >  uint32_t *pixels = reinterpret_cast(cursor_msg.data);
> > -for (unsigned i = 0; i < image.width * image.height; ++i)
> > -pixels[i] =

Re: [Spice-devel] [PATCH spice-streaming-agent] Remove X11 dependency from send_cursor

2017-11-22 Thread Frediano Ziglio 

> 
> Frediano Ziglio writes:
> 
> >>
> >> On Mon, Nov 20, 2017 at 03:18:14PM +, Frediano Ziglio wrote:
> >> > Allows a better encapsulation in the future
> >> >
> >> > Signed-off-by: Frediano Ziglio 
> >> > ---
> >> >  src/spice-streaming-agent.cpp | 28 +---
> >> >  1 file changed, 17 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/src/spice-streaming-agent.cpp
> >> > b/src/spice-streaming-agent.cpp
> >> > index 23b9768..53ffbf0 100644
> >> > --- a/src/spice-streaming-agent.cpp
> >> > +++ b/src/spice-streaming-agent.cpp
> >> > @@ -22,6 +22,7 @@
> >> >  #include 
> >> >  #include 
> >> >  #include 
> >> > +#include 
> >> >  #include 
> >> >  #include 
> >> >
> >> > @@ -284,15 +285,17 @@ static void usage(const char *progname)
> >> >  exit(1);
> >> >  }
> >> >
> >> > -static void send_cursor(const XFixesCursorImage )
> >> > +static void
> >> > +send_cursor(unsigned width, unsigned height, int hotspot_x, int
> >> > hotspot_y,
> >> > +std::function fill_cursor)
> >> >  {
> >> > -if (image.width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> >> > -image.height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> >> > +if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> >> > +height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> >> >  return;
> >> >
> >> >  size_t cursor_size =
> >> >  sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> >> > -image.width * image.height * sizeof(uint32_t);
> >> > +width * height * sizeof(uint32_t);
> >> >  std::unique_ptr msg(new uint8_t[cursor_size]);
> >> >
> >> >  StreamDevHeader
> >> >  _hdr(*reinterpret_cast(msg.get()));
> >> > @@ -305,14 +308,13 @@ static void send_cursor(const XFixesCursorImage
> >> > )
> >> >  memset(_msg, 0, sizeof(cursor_msg));
> >> >
> >> >  cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> >> > -cursor_msg.width = image.width;
> >> > -cursor_msg.height = image.height;
> >> > -cursor_msg.hot_spot_x = image.xhot;
> >> > -cursor_msg.hot_spot_y = image.yhot;
> >> > +cursor_msg.width = width;
> >> > +cursor_msg.height = height;
> >> > +cursor_msg.hot_spot_x = hotspot_x;
> >> > +cursor_msg.hot_spot_y = hotspot_y;
> >> >
> >> >  uint32_t *pixels = reinterpret_cast(cursor_msg.data);
> >> > -for (unsigned i = 0; i < image.width * image.height; ++i)
> >> > -pixels[i] = image.pixels[i];
> >> > +fill_cursor(pixels);
> >> >
> >> >  std::lock_guard stream_guard(stream_mtx);
> >> >  write_all(streamfd, msg.get(), cursor_size);
> >> > @@ -336,7 +338,11 @@ static void cursor_changes(Display *display, int
> >> > event_base)
> >> >  continue;
> >> >
> >> >  last_serial = cursor->cursor_serial;
> >> > -send_cursor(*cursor);
> >> > +auto fill_cursor = [&](uint32_t *pixels) {
> >> > +for (unsigned i = 0; i < cursor->width * cursor->height;
> >> > ++i)
> >> > +pixels[i] = cursor->pixels[i];
> >> > +};
> >>
> >> Can't you just pass cursor->pixels to send_cursor() rather than building
> >> a lambda and passing that?
> >>
> >> Christophe
> >>
> >
> > Because the format is X11 format so it won't remove the dependency.
> 
> If send_format is a method in an X11-specific derived class, it does.
> 

In your series send_format is part of a class handling the device
which should be platform independent so no X11.

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] Remove X11 dependency from send_cursor

2017-11-22 Thread Christophe de Dinechin 

Frediano Ziglio writes:

>>
>> On Mon, Nov 20, 2017 at 03:18:14PM +, Frediano Ziglio wrote:
>> > Allows a better encapsulation in the future
>> >
>> > Signed-off-by: Frediano Ziglio 
>> > ---
>> >  src/spice-streaming-agent.cpp | 28 +---
>> >  1 file changed, 17 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> > index 23b9768..53ffbf0 100644
>> > --- a/src/spice-streaming-agent.cpp
>> > +++ b/src/spice-streaming-agent.cpp
>> > @@ -22,6 +22,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >  #include 
>> >  #include 
>> >
>> > @@ -284,15 +285,17 @@ static void usage(const char *progname)
>> >  exit(1);
>> >  }
>> >
>> > -static void send_cursor(const XFixesCursorImage )
>> > +static void
>> > +send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>> > +std::function fill_cursor)
>> >  {
>> > -if (image.width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> > -image.height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
>> > +if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> > +height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
>> >  return;
>> >
>> >  size_t cursor_size =
>> >  sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
>> > -image.width * image.height * sizeof(uint32_t);
>> > +width * height * sizeof(uint32_t);
>> >  std::unique_ptr msg(new uint8_t[cursor_size]);
>> >
>> >  StreamDevHeader
>> >  _hdr(*reinterpret_cast(msg.get()));
>> > @@ -305,14 +308,13 @@ static void send_cursor(const XFixesCursorImage
>> > )
>> >  memset(_msg, 0, sizeof(cursor_msg));
>> >
>> >  cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
>> > -cursor_msg.width = image.width;
>> > -cursor_msg.height = image.height;
>> > -cursor_msg.hot_spot_x = image.xhot;
>> > -cursor_msg.hot_spot_y = image.yhot;
>> > +cursor_msg.width = width;
>> > +cursor_msg.height = height;
>> > +cursor_msg.hot_spot_x = hotspot_x;
>> > +cursor_msg.hot_spot_y = hotspot_y;
>> >
>> >  uint32_t *pixels = reinterpret_cast(cursor_msg.data);
>> > -for (unsigned i = 0; i < image.width * image.height; ++i)
>> > -pixels[i] = image.pixels[i];
>> > +fill_cursor(pixels);
>> >
>> >  std::lock_guard stream_guard(stream_mtx);
>> >  write_all(streamfd, msg.get(), cursor_size);
>> > @@ -336,7 +338,11 @@ static void cursor_changes(Display *display, int
>> > event_base)
>> >  continue;
>> >
>> >  last_serial = cursor->cursor_serial;
>> > -send_cursor(*cursor);
>> > +auto fill_cursor = [&](uint32_t *pixels) {
>> > +for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>> > +pixels[i] = cursor->pixels[i];
>> > +};
>>
>> Can't you just pass cursor->pixels to send_cursor() rather than building
>> a lambda and passing that?
>>
>> Christophe
>>
>
> Because the format is X11 format so it won't remove the dependency.

If send_format is a method in an X11-specific derived class, it does.

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


--
Cheers,
Christophe de Dinechin (IRC c3d)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent] Remove X11 dependency from send_cursor

2017-11-22 Thread Christophe de Dinechin 
I know that this was acked by Christophe, but I wanted to comment,
because that makes me unhappy.


Frediano Ziglio writes:

> Allows a better encapsulation in the future
>
> Signed-off-by: Frediano Ziglio 
> ---
>  src/spice-streaming-agent.cpp | 28 +---
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 23b9768..53ffbf0 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -284,15 +285,17 @@ static void usage(const char *progname)
>  exit(1);
>  }
>
> -static void send_cursor(const XFixesCursorImage )
> +static void
> +send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> +std::function fill_cursor)

I find it really strange and error prone to pass the buffer one way
(as a lambda argument) and its size (implicitly) another way (as a
computation based on function arguments, i.e. width * height).
I would have at the very least added a 'pixels' name and a buffer size
in the lambda interface.

That suggests something is wrong about the "encapsulation". And indeed,
if you have to copy pixels to convert from 'long' to 'uint32_t' and use
that as a justification, it makes me *very* nervous. That alone would
have been enough for me to nack this way of doing this.

But what annoys me really is that you are aware that I started a patch
series where I began working on encapsulation, and trying to do it the
C++ way, not by happily paper-patching advanced C++ over low-level
C-style byte-level memory copies.

One of the comments that came out of the discussion of my patch series
was precisely X11 encapsulation. So it would be reasonable to at least
envision that addressing that comment could be part of my effort for the
next iteration of the series, wouldn't it?

Based on the structure of my patch series, there is already a call
that looks like

 stream->send_cursor(*cursor);

From that, you could correctly infer that this function could be
an X11-specific feature, in an X11-specific stream class. That provides
all the encapsulation you need, in a proper C++ way, using inheritance
to deal with commonalities rather than using std::function and lambdas
simply to pass a glorified memcpy.

But now that this patch is acked, I guess that I will have to rebase
my RAII branch on what I frankly see as not even half of a quarter of some
brain-damaged idea of encapsulation. For what benefit? What urgent
problem did this patch solve that was not better addressed by working on
real encapsulation?

>  {
> -if (image.width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> -image.height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
> +if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> +height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
>  return;
>
>  size_t cursor_size =
>  sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> -image.width * image.height * sizeof(uint32_t);
> +width * height * sizeof(uint32_t);
>  std::unique_ptr msg(new uint8_t[cursor_size]);
>
>  StreamDevHeader _hdr(*reinterpret_cast(msg.get()));
> @@ -305,14 +308,13 @@ static void send_cursor(const XFixesCursorImage )
>  memset(_msg, 0, sizeof(cursor_msg));
>
>  cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> -cursor_msg.width = image.width;
> -cursor_msg.height = image.height;
> -cursor_msg.hot_spot_x = image.xhot;
> -cursor_msg.hot_spot_y = image.yhot;
> +cursor_msg.width = width;
> +cursor_msg.height = height;
> +cursor_msg.hot_spot_x = hotspot_x;
> +cursor_msg.hot_spot_y = hotspot_y;
>
>  uint32_t *pixels = reinterpret_cast(cursor_msg.data);
> -for (unsigned i = 0; i < image.width * image.height; ++i)
> -pixels[i] = image.pixels[i];
> +fill_cursor(pixels);
>
>  std::lock_guard stream_guard(stream_mtx);
>  write_all(streamfd, msg.get(), cursor_size);
> @@ -336,7 +338,11 @@ static void cursor_changes(Display *display, int 
> event_base)
>  continue;
>
>  last_serial = cursor->cursor_serial;
> -send_cursor(*cursor);
> +auto fill_cursor = [&](uint32_t *pixels) {
> +for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
> +pixels[i] = cursor->pixels[i];
> +};
> +send_cursor(cursor->width, cursor->height, cursor->xhot, 
> cursor->yhot, fill_cursor);
>  }
>  }


--
Cheers,
Christophe de Dinechin (IRC c3d)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v3] stream-channel: Tell client we are just streaming data

2017-11-22 Thread Frediano Ziglio 
>
> Frediano Ziglio writes:
> 
> > This give an hint to client which can optimise rendering.
> >
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/stream-channel.c | 7 +++
> >  1 file changed, 7 insertions(+)
> >
> > Changes since v2:
> > - remove conditional code.
> 
> OK, now I'm confused :-)
> 
> Does that mean that:
> 
> a) you simply want this to be independent from your identity macros patch?
> 
> b) you feel it's OK (or maybe even good) to break the build if the wrong
>protocol package is included?
>

This is actually the way we manage updates so it's not a regression,
people trying to compile master spice-server should have master spice-protocol.
To compile in this specific case you don't need spice-common update but
obviously you want the spice-common patch in (as it should produce the updated
file in spice-protocol).

I think identity macros are useful as they allow a bit of independence
between specific commit even if they could introduce some potential
problems (is up to the user to use them or not).

> >
> > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > index c7ca0206..568ceac8 100644
> > --- a/server/stream-channel.c
> > +++ b/server/stream-channel.c
> > @@ -203,6 +203,13 @@ stream_channel_send_item(RedChannelClient *rcc,
> > RedPipeItem *pipe_item)
> >  channel->width, channel->height,
> >  SPICE_SURFACE_FMT_32_xRGB, SPICE_SURFACE_FLAGS_PRIMARY
> >  };
> > +
> > +// give an hint to client that we are sending just streaming
> > +// see spice.proto for capability check here
> > +if (red_channel_client_test_remote_cap(rcc,
> > SPICE_DISPLAY_CAP_MULTI_CODEC)) {
> > +surface_create.flags |= SPICE_SURFACE_FLAGS_STREAMING_MODE;
> > +}
> > +
> >  spice_marshall_msg_display_surface_create(m, _create);
> >  break;
> >  }
> 

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

Re: [Spice-devel] [PATCH spice-server v3] stream-channel: Tell client we are just streaming data

2017-11-22 Thread Christophe de Dinechin 

Frediano Ziglio writes:

> This give an hint to client which can optimise rendering.
>
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stream-channel.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> Changes since v2:
> - remove conditional code.

OK, now I'm confused :-)

Does that mean that:

a) you simply want this to be independent from your identity macros patch?

b) you feel it's OK (or maybe even good) to break the build if the wrong
   protocol package is included?

>
> diff --git a/server/stream-channel.c b/server/stream-channel.c
> index c7ca0206..568ceac8 100644
> --- a/server/stream-channel.c
> +++ b/server/stream-channel.c
> @@ -203,6 +203,13 @@ stream_channel_send_item(RedChannelClient *rcc, 
> RedPipeItem *pipe_item)
>  channel->width, channel->height,
>  SPICE_SURFACE_FMT_32_xRGB, SPICE_SURFACE_FLAGS_PRIMARY
>  };
> +
> +// give an hint to client that we are sending just streaming
> +// see spice.proto for capability check here
> +if (red_channel_client_test_remote_cap(rcc, 
> SPICE_DISPLAY_CAP_MULTI_CODEC)) {
> +surface_create.flags |= SPICE_SURFACE_FLAGS_STREAMING_MODE;
> +}
> +
>  spice_marshall_msg_display_surface_create(m, _create);
>  break;
>  }


--
Cheers,
Christophe de Dinechin (IRC c3d)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel