date:20180208

Re: [Spice-devel] [PATCH spice-streaming-agent] Remove using entire std namespace

2018-02-08 Thread Frediano Ziglio

> 
> On Thu, 2018-02-08 at 16:21 +, Frediano Ziglio wrote:
> > As discussed about style the usage or "using namespace" should be
> > avoided.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  src/concrete-agent.cpp| 11 +--
> >  src/mjpeg-fallback.cpp|  3 +--
> >  src/spice-streaming-agent.cpp |  5 ++---
> >  3 files changed, 8 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > index ebeef33..891c09b 100644
> > --- a/src/concrete-agent.cpp
> > +++ b/src/concrete-agent.cpp
> > @@ -13,7 +13,6 @@
> >  #include "concrete-agent.hpp"
> >  #include "static-plugin.hpp"
> >  
> > -using namespace std;
> >  using namespace spice::streaming_agent;
> >  
> >  static inline unsigned MajorVersion(unsigned version)
> > @@ -40,7 +39,7 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned
> > pluginVersion) const
> >  
> >  void ConcreteAgent::Register(Plugin& plugin)
> >  {
> > -plugins.push_back(shared_ptr());
> > +plugins.push_back(std::shared_ptr());
> >  }
> >  
> >  const ConfigureOption* ConcreteAgent::Options() const
> > @@ -56,11 +55,11 @@ void ConcreteAgent::AddOption(const char *name, const
> > char *value)
> >  options.insert(--options.end(), ConcreteConfigureOption(name, value));
> >  }
> >  
> > -void ConcreteAgent::LoadPlugins(const string )
> > +void ConcreteAgent::LoadPlugins(const std::string )
> >  {
> >  StaticPlugin::InitAll(*this);
> >  
> > -string pattern = directory + "/*.so";
> > +std::string pattern = directory + "/*.so";
> >  glob_t globbuf;
> >  
> >  int glob_result = glob(pattern.c_str(), 0, NULL, );
> > @@ -77,7 +76,7 @@ void ConcreteAgent::LoadPlugins(const string )
> >  globfree();
> >  }
> >  
> > -void ConcreteAgent::LoadPlugin(const string _filename)
> > +void ConcreteAgent::LoadPlugin(const std::string _filename)
> >  {
> >  void *dl = dlopen(plugin_filename.c_str(), RTLD_LOCAL|RTLD_NOW);
> >  if (!dl) {
> > @@ -101,7 +100,7 @@ void ConcreteAgent::LoadPlugin(const string
> > _filename)
> >  
> >  FrameCapture *ConcreteAgent::GetBestFrameCapture(const
> >  std::set& codecs)
> >  {
> > -vector> sorted_plugins;
> > +std::vector>
> > sorted_plugins;
> >  
> >  // sort plugins base on ranking, reverse order
> >  for (const auto& plugin: plugins) {
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index 10543ad..74682f3 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -18,7 +18,6 @@
> >  #include "static-plugin.hpp"
> >  #include "jpeg.hpp"
> >  
> > -using namespace std;
> >  using namespace spice::streaming_agent;
> >  
> >  #define ERROR(args) do { \
> > @@ -55,7 +54,7 @@ private:
> >  MjpegSettings settings;
> >  Display *dpy;
> >  
> > -vector frame;
> > +std::vector frame;
> >  
> >  // last frame sizes
> >  uint32_t last_width = ~0u, last_height = ~0u;
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 0e7641e..f4fee2d 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -35,7 +35,6 @@
> >  #include "hexdump.h"
> >  #include "concrete-agent.hpp"
> >  
> > -using namespace std;
> >  using namespace spice::streaming_agent;
> >  
> >  static ConcreteAgent agent;
> > @@ -351,7 +350,7 @@ static void cursor_changes(Display *display, int
> > event_base)
> >  }
> >  
> >  static void
> > -do_capture(const string , FILE *f_log)
> > +do_capture(const std::string , FILE *f_log)
> >  {
> >  streamfd = open(streamport.c_str(), O_RDWR);
> >  if (streamfd < 0)
> > @@ -437,7 +436,7 @@ done:
> >  
> >  int main(int argc, char* argv[])
> >  {
> > -string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> > +std::string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> >  char opt;
> >  const char *log_filename = NULL;
> >  int logmask = LOG_UPTO(LOG_WARNING);
> 
> Acked-by: Lukáš Hrázký 
> 
> Are you planning on doing the namespace spice::streaming_agent in .cpp
> and the renaming of methods too?
> 
> I'm updating the separation patch I just posted and will send an
> update.
> 
> Lukas
> 

I think I finish for today.
I was not planning the method renames, more finishing the namespace
"using" (using either the namespace if implementing it or not including
all namespace like std one).

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

[Spice-devel] [RFC PATCH 0/1] separate the agent business code

2018-02-08 Thread Lukáš Hrázký

Hello,

as previously discussed, I've attempted to separate out the agent code
into a class (temporarily called AgentRunner, see the commit message for
details).

The patch is cleaned up and working, it could only use a better naming
for the class and then to rename the source files accordingly. This
should be enough for Frediano to implement the daemon code comfortably,
as the call to the agent is now ~3 lines. It does probably break
horribly any patches people may have in store for the code in question.

I'm sending this now, and as a RFC, because looking at the code I'd like
to take it a step further. The AgentRunner class does not have a clearly
defined purpose and kind of blurs with the ConcreteAgent class that is
already there. It contains a mix of code all the way down to the network
communication. So I'd like to try to separate the networking code
further and possibly somehow merge the AgentRunner and ConcreteAgent.

I'll be on PTO tomorrow though, so sending this now for comments and
possibly not to delay Frediano, if he likes it and would like to rebase
his daemon code on top of this. Further changes to this code should not
interfere much anymore.

Lukas


Lukáš Hrázký (1):
  separate and encapsulate the agent business code

 src/Makefile.am   |   2 +
 src/main.cpp  | 127 
 src/spice-streaming-agent.cpp | 455 +-
 src/spice-streaming-agent.hpp |  56 ++
 4 files changed, 370 insertions(+), 270 deletions(-)
 create mode 100644 src/main.cpp
 create mode 100644 src/spice-streaming-agent.hpp

-- 
2.16.1

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

[Spice-devel] [RFC PATCH 1/1] separate and encapsulate the agent business code

2018-02-08 Thread Lukáš Hrázký

Create an AgentRunner (TODO: needs a better name) class to encapsulate
the streaming and communication with the server. The basic setup (cmd
arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
functions is moved to the AgentRunner class and modified as little as
possible:
- The cursor updating code is moved into a functor called CursorThread
- Some initialization and cleanup is moved to AgentRunner's constructor
  and destructor
- Some error handling moved over to exceptions, mainly what was in
  main() and do_capture()
- A couple of variables gently renamed.

Signed-off-by: Lukáš Hrázký 
---
 src/Makefile.am   |   2 +
 src/main.cpp  | 127 
 src/spice-streaming-agent.cpp | 455 +-
 src/spice-streaming-agent.hpp |  56 ++
 4 files changed, 370 insertions(+), 270 deletions(-)
 create mode 100644 src/main.cpp
 create mode 100644 src/spice-streaming-agent.hpp

diff --git a/src/Makefile.am b/src/Makefile.am
index 8d5c5bd..3a6fee7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
 
 spice_streaming_agent_SOURCES = \
spice-streaming-agent.cpp \
+   spice-streaming-agent.hpp \
static-plugin.cpp \
static-plugin.hpp \
concrete-agent.cpp \
@@ -56,4 +57,5 @@ spice_streaming_agent_SOURCES = \
mjpeg-fallback.cpp \
jpeg.cpp \
jpeg.hpp \
+   main.cpp \
$(NULL)
diff --git a/src/main.cpp b/src/main.cpp
new file mode 100644
index 000..a309011
--- /dev/null
+++ b/src/main.cpp
@@ -0,0 +1,127 @@
+/* An implementation of a SPICE streaming agent
+ *
+ * \copyright
+ * Copyright 2016-2018 Red Hat Inc. All rights reserved.
+ */
+
+#include 
+#include "spice-streaming-agent.hpp"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using namespace std;
+using namespace SpiceStreamingAgent;
+
+
+static void usage(const char *progname)
+{
+printf("usage: %s \n", progname);
+printf("options are:\n");
+printf("\t-p portname  -- virtio-serial port to use\n");
+printf("\t-i accept commands from stdin\n");
+printf("\t-l file -- log frames to file\n");
+printf("\t--log-binary -- log binary frames (following -l)\n");
+printf("\t-d -- enable debug logs\n");
+printf("\t-c variable=value -- change settings\n");
+printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
+printf("\n");
+printf("\t-h or --help -- print this help message\n");
+
+exit(1);
+}
+
+void handle_interrupt(int intr)
+{
+syslog(LOG_INFO, "Got signal %d, exiting", intr);
+AgentRunner::quit = true;
+}
+
+void register_interrupts(void)
+{
+struct sigaction sa;
+
+memset(, 0, sizeof(sa));
+sa.sa_handler = handle_interrupt;
+if ((sigaction(SIGINT, , NULL) != 0) &&
+(sigaction(SIGTERM, , NULL) != 0)) {
+syslog(LOG_WARNING, "failed to register signal handler %m");
+}
+}
+
+int main(int argc, char* argv[])
+{
+string stream_port = "/dev/virtio-ports/com.redhat.stream.0";
+char opt;
+string log_filename;
+int log_binary = 0;
+bool stdin_ok = false;
+int logmask = LOG_UPTO(LOG_WARNING);
+struct option long_options[] = {
+{ "log-binary", no_argument, _binary, 1},
+{ "help", no_argument, NULL, 'h'},
+{ 0, 0, 0, 0}
+};
+
+if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
+stdin_ok = true;
+}
+
+openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) : LOG_PID, 
LOG_USER);
+setlogmask(logmask);
+
+std::vector> options;
+
+while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, NULL)) != 
-1) {
+switch (opt) {
+case 0:
+/* Handle long options if needed */
+break;
+case 'i':
+stdin_ok = true;
+openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
+break;
+case 'p':
+stream_port = optarg;
+break;
+case 'c': {
+char *p = strchr(optarg, '=');
+if (p == NULL) {
+syslog(LOG_ERR, "wrong 'c' argument %s\n", optarg);
+usage(argv[0]);
+}
+*p++ = '\0';
+options.push_back(std::make_pair(optarg, p));
+break;
+}
+case 'l':
+log_filename = optarg;
+break;
+case 'd':
+logmask = LOG_UPTO(LOG_DEBUG);
+setlogmask(logmask);
+break;
+case 'h':
+usage(argv[0]);
+break;
+}
+}
+
+register_interrupts();
+
+try {
+AgentRunner runner(stream_port, log_filename, log_binary != 0, 
stdin_ok);
+// TODO fix overcomplicated passing of options to the agent
+runner.add_options(options);
+runner.run();
+} catch (const

[Spice-devel] [PATCH spice-streaming-agent] Remove using entire std namespace

2018-02-08 Thread Frediano Ziglio

As discussed about style the usage or "using namespace" should be
avoided.

Signed-off-by: Frediano Ziglio 
---
 src/concrete-agent.cpp| 11 +--
 src/mjpeg-fallback.cpp|  3 +--
 src/spice-streaming-agent.cpp |  5 ++---
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index ebeef33..891c09b 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -13,7 +13,6 @@
 #include "concrete-agent.hpp"
 #include "static-plugin.hpp"
 
-using namespace std;
 using namespace spice::streaming_agent;
 
 static inline unsigned MajorVersion(unsigned version)
@@ -40,7 +39,7 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned 
pluginVersion) const
 
 void ConcreteAgent::Register(Plugin& plugin)
 {
-plugins.push_back(shared_ptr());
+plugins.push_back(std::shared_ptr());
 }
 
 const ConfigureOption* ConcreteAgent::Options() const
@@ -56,11 +55,11 @@ void ConcreteAgent::AddOption(const char *name, const char 
*value)
 options.insert(--options.end(), ConcreteConfigureOption(name, value));
 }
 
-void ConcreteAgent::LoadPlugins(const string )
+void ConcreteAgent::LoadPlugins(const std::string )
 {
 StaticPlugin::InitAll(*this);
 
-string pattern = directory + "/*.so";
+std::string pattern = directory + "/*.so";
 glob_t globbuf;
 
 int glob_result = glob(pattern.c_str(), 0, NULL, );
@@ -77,7 +76,7 @@ void ConcreteAgent::LoadPlugins(const string )
 globfree();
 }
 
-void ConcreteAgent::LoadPlugin(const string _filename)
+void ConcreteAgent::LoadPlugin(const std::string _filename)
 {
 void *dl = dlopen(plugin_filename.c_str(), RTLD_LOCAL|RTLD_NOW);
 if (!dl) {
@@ -101,7 +100,7 @@ void ConcreteAgent::LoadPlugin(const string 
_filename)
 
 FrameCapture *ConcreteAgent::GetBestFrameCapture(const 
std::set& codecs)
 {
-vector> sorted_plugins;
+std::vector> sorted_plugins;
 
 // sort plugins base on ranking, reverse order
 for (const auto& plugin: plugins) {
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 10543ad..74682f3 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -18,7 +18,6 @@
 #include "static-plugin.hpp"
 #include "jpeg.hpp"
 
-using namespace std;
 using namespace spice::streaming_agent;
 
 #define ERROR(args) do { \
@@ -55,7 +54,7 @@ private:
 MjpegSettings settings;
 Display *dpy;
 
-vector frame;
+std::vector frame;
 
 // last frame sizes
 uint32_t last_width = ~0u, last_height = ~0u;
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 0e7641e..f4fee2d 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -35,7 +35,6 @@
 #include "hexdump.h"
 #include "concrete-agent.hpp"
 
-using namespace std;
 using namespace spice::streaming_agent;
 
 static ConcreteAgent agent;
@@ -351,7 +350,7 @@ static void cursor_changes(Display *display, int event_base)
 }
 
 static void
-do_capture(const string , FILE *f_log)
+do_capture(const std::string , FILE *f_log)
 {
 streamfd = open(streamport.c_str(), O_RDWR);
 if (streamfd < 0)
@@ -437,7 +436,7 @@ done:
 
 int main(int argc, char* argv[])
 {
-string streamport = "/dev/virtio-ports/com.redhat.stream.0";
+std::string streamport = "/dev/virtio-ports/com.redhat.stream.0";
 char opt;
 const char *log_filename = NULL;
 int logmask = LOG_UPTO(LOG_WARNING);
-- 
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 using entire std namespace

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 16:21 +, Frediano Ziglio wrote:
> As discussed about style the usage or "using namespace" should be
> avoided.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/concrete-agent.cpp| 11 +--
>  src/mjpeg-fallback.cpp|  3 +--
>  src/spice-streaming-agent.cpp |  5 ++---
>  3 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index ebeef33..891c09b 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -13,7 +13,6 @@
>  #include "concrete-agent.hpp"
>  #include "static-plugin.hpp"
>  
> -using namespace std;
>  using namespace spice::streaming_agent;
>  
>  static inline unsigned MajorVersion(unsigned version)
> @@ -40,7 +39,7 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned 
> pluginVersion) const
>  
>  void ConcreteAgent::Register(Plugin& plugin)
>  {
> -plugins.push_back(shared_ptr());
> +plugins.push_back(std::shared_ptr());
>  }
>  
>  const ConfigureOption* ConcreteAgent::Options() const
> @@ -56,11 +55,11 @@ void ConcreteAgent::AddOption(const char *name, const 
> char *value)
>  options.insert(--options.end(), ConcreteConfigureOption(name, value));
>  }
>  
> -void ConcreteAgent::LoadPlugins(const string )
> +void ConcreteAgent::LoadPlugins(const std::string )
>  {
>  StaticPlugin::InitAll(*this);
>  
> -string pattern = directory + "/*.so";
> +std::string pattern = directory + "/*.so";
>  glob_t globbuf;
>  
>  int glob_result = glob(pattern.c_str(), 0, NULL, );
> @@ -77,7 +76,7 @@ void ConcreteAgent::LoadPlugins(const string )
>  globfree();
>  }
>  
> -void ConcreteAgent::LoadPlugin(const string _filename)
> +void ConcreteAgent::LoadPlugin(const std::string _filename)
>  {
>  void *dl = dlopen(plugin_filename.c_str(), RTLD_LOCAL|RTLD_NOW);
>  if (!dl) {
> @@ -101,7 +100,7 @@ void ConcreteAgent::LoadPlugin(const string 
> _filename)
>  
>  FrameCapture *ConcreteAgent::GetBestFrameCapture(const 
> std::set& codecs)
>  {
> -vector> sorted_plugins;
> +std::vector> sorted_plugins;
>  
>  // sort plugins base on ranking, reverse order
>  for (const auto& plugin: plugins) {
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 10543ad..74682f3 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -18,7 +18,6 @@
>  #include "static-plugin.hpp"
>  #include "jpeg.hpp"
>  
> -using namespace std;
>  using namespace spice::streaming_agent;
>  
>  #define ERROR(args) do { \
> @@ -55,7 +54,7 @@ private:
>  MjpegSettings settings;
>  Display *dpy;
>  
> -vector frame;
> +std::vector frame;
>  
>  // last frame sizes
>  uint32_t last_width = ~0u, last_height = ~0u;
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 0e7641e..f4fee2d 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -35,7 +35,6 @@
>  #include "hexdump.h"
>  #include "concrete-agent.hpp"
>  
> -using namespace std;
>  using namespace spice::streaming_agent;
>  
>  static ConcreteAgent agent;
> @@ -351,7 +350,7 @@ static void cursor_changes(Display *display, int 
> event_base)
>  }
>  
>  static void
> -do_capture(const string , FILE *f_log)
> +do_capture(const std::string , FILE *f_log)
>  {
>  streamfd = open(streamport.c_str(), O_RDWR);
>  if (streamfd < 0)
> @@ -437,7 +436,7 @@ done:
>  
>  int main(int argc, char* argv[])
>  {
> -string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> +std::string streamport = "/dev/virtio-ports/com.redhat.stream.0";
>  char opt;
>  const char *log_filename = NULL;
>  int logmask = LOG_UPTO(LOG_WARNING);

Acked-by: Lukáš Hrázký 

Are you planning on doing the namespace spice::streaming_agent in .cpp
and the renaming of methods too?

I'm updating the separation patch I just posted and will send an
update.

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

[Spice-devel] [RFC PATCH spice-streaming-agent v2] separate and encapsulate the agent business code

2018-02-08 Thread Lukáš Hrázký

Create an AgentRunner (TODO: needs a better name) class to encapsulate
the streaming and communication with the server. The basic setup (cmd
arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
functions is moved to the AgentRunner class and modified as little as
possible:
- The cursor updating code is moved into a functor called CursorThread
- Some initialization and cleanup is moved to AgentRunner's constructor
  and destructor
- Some error handling moved over to exceptions, mainly what was in
  main() and do_capture()
- A couple of variables gently renamed.

Signed-off-by: Lukáš Hrázký 
---
Changes since v1:
- update according to Frediano's namespace changes

 src/Makefile.am   |   2 +
 src/main.cpp  | 126 
 src/spice-streaming-agent.cpp | 458 +-
 src/spice-streaming-agent.hpp |  57 ++
 4 files changed, 372 insertions(+), 271 deletions(-)
 create mode 100644 src/main.cpp
 create mode 100644 src/spice-streaming-agent.hpp

diff --git a/src/Makefile.am b/src/Makefile.am
index 6d37274..42bb10f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
 
 spice_streaming_agent_SOURCES = \
spice-streaming-agent.cpp \
+   spice-streaming-agent.hpp \
static-plugin.cpp \
static-plugin.hpp \
concrete-agent.cpp \
@@ -57,4 +58,5 @@ spice_streaming_agent_SOURCES = \
mjpeg-fallback.hpp \
jpeg.cpp \
jpeg.hpp \
+   main.cpp \
$(NULL)
diff --git a/src/main.cpp b/src/main.cpp
new file mode 100644
index 000..46eda85
--- /dev/null
+++ b/src/main.cpp
@@ -0,0 +1,126 @@
+/* An implementation of a SPICE streaming agent
+ *
+ * \copyright
+ * Copyright 2016-2018 Red Hat Inc. All rights reserved.
+ */
+
+#include 
+#include "spice-streaming-agent.hpp"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+namespace ssa = spice::streaming_agent;
+
+static void usage(const char *progname)
+{
+printf("usage: %s \n", progname);
+printf("options are:\n");
+printf("\t-p portname  -- virtio-serial port to use\n");
+printf("\t-i accept commands from stdin\n");
+printf("\t-l file -- log frames to file\n");
+printf("\t--log-binary -- log binary frames (following -l)\n");
+printf("\t-d -- enable debug logs\n");
+printf("\t-c variable=value -- change settings\n");
+printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
+printf("\n");
+printf("\t-h or --help -- print this help message\n");
+
+exit(1);
+}
+
+void handle_interrupt(int intr)
+{
+syslog(LOG_INFO, "Got signal %d, exiting", intr);
+ssa::AgentRunner::quit = true;
+}
+
+void register_interrupts(void)
+{
+struct sigaction sa;
+
+memset(, 0, sizeof(sa));
+sa.sa_handler = handle_interrupt;
+if ((sigaction(SIGINT, , NULL) != 0) &&
+(sigaction(SIGTERM, , NULL) != 0)) {
+syslog(LOG_WARNING, "failed to register signal handler %m");
+}
+}
+
+int main(int argc, char* argv[])
+{
+std::string stream_port = "/dev/virtio-ports/com.redhat.stream.0";
+char opt;
+std::string log_filename;
+int log_binary = 0;
+bool stdin_ok = false;
+int logmask = LOG_UPTO(LOG_WARNING);
+struct option long_options[] = {
+{ "log-binary", no_argument, _binary, 1},
+{ "help", no_argument, NULL, 'h'},
+{ 0, 0, 0, 0}
+};
+
+if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
+stdin_ok = true;
+}
+
+openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) : LOG_PID, 
LOG_USER);
+setlogmask(logmask);
+
+std::vector> options;
+
+while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, NULL)) != 
-1) {
+switch (opt) {
+case 0:
+/* Handle long options if needed */
+break;
+case 'i':
+stdin_ok = true;
+openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
+break;
+case 'p':
+stream_port = optarg;
+break;
+case 'c': {
+char *p = strchr(optarg, '=');
+if (p == NULL) {
+syslog(LOG_ERR, "wrong 'c' argument %s\n", optarg);
+usage(argv[0]);
+}
+*p++ = '\0';
+options.push_back(std::make_pair(optarg, p));
+break;
+}
+case 'l':
+log_filename = optarg;
+break;
+case 'd':
+logmask = LOG_UPTO(LOG_DEBUG);
+setlogmask(logmask);
+break;
+case 'h':
+usage(argv[0]);
+break;
+}
+}
+
+register_interrupts();
+
+try {
+ssa::AgentRunner runner(stream_port, log_filename, log_binary != 0, 
stdin_ok);
+// TODO fix overcomplicated passing of options to the agent
+

Re: [Spice-devel] [PATCH v2 02/13] Update copyright date

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index eb0e30ef..c127f026 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -1,7 +1,7 @@
>  Spice project coding style and coding conventions
>  =
>  
> -Copyright (C) 2009-2016 Red Hat, Inc.
> +Copyright (C) 2009-2018 Red Hat, Inc.
>  Licensed under a Creative Commons Attribution-Share Alike 3.0
>  United States License (see
>  http://creativecommons.org/licenses/by-sa/3.0/us/legalcode).
>  

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [PATCH v2 03/13] Specify file extensions for C++ and Obj-C

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index c127f026..72ed2ef7 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -14,7 +14,16 @@ Names
>  
>  Use lower case and separate words using dashes (e.g., file-name.c,
>  header.h).
>  
> -Use standard file extension for C source and header files.
> +The file extensions used in the SPICE project are:
> +- .c for C source
> +- .cpp for C++ sources
> +- .h for headers that can be included from C code
> +- .hpp for headers that are strictly reserved to C++
> +- .m for Objective-C source files (currently not properly enforced)

I would remove the "(currently not properly enforced)" part, the style
is what we aim, not representing the current state (obviously the code
should try to be coherent to that but there are always exceptions
like "contrib" code just copied or incorporated from other projects").

> +
> +Note that .h headers may contain C++ code as long as the header can
> +sucessfully be included from a C source file.
> +
>  

"sucessfully" typo.

>  Line width
>  ~~

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

Re: [Spice-devel] [PATCH v2 01/13] Add .clang-format with defaults matching what's specified in the style guide

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  .clang-format | 23 +++
>  1 file changed, 23 insertions(+)
>  create mode 100644 .clang-format
> 
> diff --git a/.clang-format b/.clang-format
> new file mode 100644
> index ..339ce846
> --- /dev/null
> +++ b/.clang-format
> @@ -0,0 +1,23 @@
> +Language:Cpp
> +# BasedOnStyle:  LLVM
> +
> +# The following is commented out until widely supported
> +# IncludeBlocks: Regroup
> +SortIncludes: true
> +
> +IncludeCategories:
> +  - Regex:   'config.h'
> +Priority:-1
> +  - Regex:   '^"spice.*"'
> +Priority:1
> +  - Regex:   'glib'
> +Priority:4
> +  - Regex:   '^<.*>'
> +Priority:3
> +  - Regex:   '^".*"'
> +Priority:2
> +
> +ColumnLimit: 100
> +IndentCaseLabels: false
> +IndentWidth: 4
> +BreakBeforeBraces: Allman

In a previous email you posted an example of this file usage that seems to
not follow exactly what the current source files looks like (something
about brackets).

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

Re: [Spice-devel] [PATCH v2 01/13] Add .clang-format with defaults matching what's specified in the style guide

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 10:04, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> .clang-format | 23 +++
>> 1 file changed, 23 insertions(+)
>> create mode 100644 .clang-format
>> 
>> diff --git a/.clang-format b/.clang-format
>> new file mode 100644
>> index ..339ce846
>> --- /dev/null
>> +++ b/.clang-format
>> @@ -0,0 +1,23 @@
>> +Language:Cpp
>> +# BasedOnStyle:  LLVM
>> +
>> +# The following is commented out until widely supported
>> +# IncludeBlocks: Regroup
>> +SortIncludes: true
>> +
>> +IncludeCategories:
>> +  - Regex:   'config.h'
>> +Priority:-1
>> +  - Regex:   '^"spice.*"'
>> +Priority:1
>> +  - Regex:   'glib'
>> +Priority:4
>> +  - Regex:   '^<.*>'
>> +Priority:3
>> +  - Regex:   '^".*"'
>> +Priority:2
>> +
>> +ColumnLimit: 100
>> +IndentCaseLabels: false
>> +IndentWidth: 4
>> +BreakBeforeBraces: Allman
> 
> In a previous email you posted an example of this file usage that seems to
> not follow exactly what the current source files looks like (something
> about brackets).

That is correct. I had added Allman, but that’s the style I use in my own 
projects. I think the style we want is SPICE is called Linux, i.e. it attaches 
braces after conditionals, but puts them on a separate line for classes, 
namespaces and functions.

Fixed in the upcoming iteration.


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

Re: [Spice-devel] [PATCH v2 04/13] Rephrase assertion section

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 72ed2ef7..61cb0701 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -82,7 +82,11 @@ Comments that are prefixed with `FIXME` describe a bug
> that need to be fixed. Ge
>  ASSERT
>  --
>  
> -Use it freely. ASSERT helps testing function arguments and function results
> validity.  ASSERT helps detecting bugs and improve code readability and
> stability.
> +Use assertions liberally. They helps testing function arguments and function
> results validity. Assertions helps detecting bugs and improve code
> readability and stability.
> +
> +Several form of assertion exist, notably:
> +- spice_assert which should be preferred for any assertion related to SPICE
> itself
> +- glib asserts (many forms) which should be preferred for any assertion
> related to the use of glib
>  
>  sizeof

Yes, this section looks... weird.
I think there is a problem here and this entire section is entirely broken
now.
To sum up: we don't use ASSERT! At all. But we use some kind of asserts!
Yes, sounds confusing.
But you have to consider the long story of this project (which unfortunately
I don't know entirely by myself!). Maybe I'm a bit wrong but the project was
in C++ using a lot of Windows knowledge and styles. This is a lot lost in
the current code is is hard to see but I was also a Windows developer for
quite some time (I don't regret it) and I can still note the smell of it!

What's specifically "ASSERT" (not assert, not asserts) ?
ASSERT is a Windows macro that aborts the code if the condition is false.
It helps detecting the bugs but this macro is NEVER compiled in release
code (Windows rules!). The difference seems small but is way different/
Is not used for runtime checks that can happen as this would probably
causes crashes on release. Is used for checks that should never happen
to help developers testing with debugging versions (on Windows you test
with debugg versions). How can I say that? There are some places where
this macro is too aggressive, where should be mathematically impossible
to reach. For instance in common/ring.h they are used to check
internal states at every low level operation (in functions that are
inlined). I just think in the history of code ASSERT were changed to
spice_assert. Other indications (looks like to be a paleontologist) are
in code like common/quic.c (notably encode function) which are basically
testing is a 32 bit integer has more that 32 bits!

In our code we never disable our assert style asserts (either spice_assert
or g_assert) which make them suitable for runtime checks but a bit
overwhelming to check the "obvious".

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Victor Toso

Hi,

On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote:
> > > Depends on many cases. You don't want spurious changes to make harder to
> > > look at the history for instance (that is a point for Nack).
> > > The patch is not fixing anything or adding new feature (another for Nack).
> > > On the other hand applied to code not changed for a long period (where is
> > > unlikely to have to dig the history) or code with small history (where
> > > is faster to skip in any case) changes.
> > > Being style it depends also on personal opinions.
> > 
> > You can’t have it both ways. Here, you are simultaneously saying:
> > 
> > - We don’t want trailing whitespace
> 
> OT: Not only trailing, also tabs for instance.
> 
> > - We nack patches that fix trailing whitespace on their own
> 
> Not saying that, I'm saying is not black and white.

Because the code itself is inconsistent. It would be so much
better to have a few patches that make the code consistent and
then some git hook to check if given patch does not mess around
the coding style instead of discussing this so many times over
years.

toso


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 v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 11:09 +0100, Victor Toso wrote:
> Hi,
> 
> On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote:
> > > > Depends on many cases. You don't want spurious changes to make harder to
> > > > look at the history for instance (that is a point for Nack).
> > > > The patch is not fixing anything or adding new feature (another for 
> > > > Nack).
> > > > On the other hand applied to code not changed for a long period (where 
> > > > is
> > > > unlikely to have to dig the history) or code with small history (where
> > > > is faster to skip in any case) changes.
> > > > Being style it depends also on personal opinions.
> > > 
> > > You can’t have it both ways. Here, you are simultaneously saying:
> > > 
> > > - We don’t want trailing whitespace
> > 
> > OT: Not only trailing, also tabs for instance.
> > 
> > > - We nack patches that fix trailing whitespace on their own
> > 
> > Not saying that, I'm saying is not black and white.
> 
> Because the code itself is inconsistent. It would be so much
> better to have a few patches that make the code consistent and
> then some git hook to check if given patch does not mess around
> the coding style instead of discussing this so many times over
> years.

Agreed!

My (partly philosofical) opinion on this is:

Code quality is more important than history (or diffs). If something is
wrong, just go ahead and fix it. If you want it fixed, it will be
recorded in the history. It's cleaner to have a separate commit for it
in the history. (that is obvious, right?)

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

Re: [Spice-devel] [PATCH v2 11/13] Add mention of header guards

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 780f0615..e2465aa9 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -357,6 +357,21 @@ char *array[] = {
>  "item_3",
>  };
>  
> +Headers
> +---
> +
> +Headers should be protected against multiple inclusion using a macro that
> matches the header file name in uppercase, with all characters that are
> invalid in C replaced with an underscore '_':

Maybe is just me, I'm reading this like: if name is foo.h the guard is
FOO_H. Which actually is not entirely true as should just contain the "FOO_H"
(for instance in public headers is better to include the project in some way).
Maybe is just my definition of "matches".

> +
> +[source,h]
> +---
> +#ifndef MY_MODULE_H
> +#define MY_MODULE_H
> +
> +...
> +
> +#endif /* MY_MODULE_H */
> +---
> +

Are we suggesting C style only comment or is clear the is not important?
(I'm just drinking my first coffee this morning)

>  Header inclusion
>  
>  

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
> > 
> > From: Christophe de Dinechin 
> > 
> > The objective of these guidelines is that:
> > - We avoid introducing new warnings
> > - We know how to fix old ones
> > - We don't have to isolate whitespace changes when submitting patches,
> >   i.e. someone who use tools that automatically strip whitespaces and
> >   therefore "repairs" earlier errors should not be punished for it.
> 
> Sorry, I don't agree with the automatic tool, patches should not
> contain extra changes unless they fix space changes while changing
> these lines for other reasons.
> I'll personally accept single patches fixing the spaces.

I'm with Frediano here. If you want to automatically fix whitespace
errors, you can do it in a separate commit without much effort?

I can also go right now and fix all trailing whitespaces with a bash
oneliner, submit the patch and we have a moot point here? :)

Lukas

> > 
> > Signed-off-by: Christophe de Dinechin 
> > ---
> >  docs/spice_style.txt | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > index e2465aa9..d9644f9a 100644
> > --- a/docs/spice_style.txt
> > +++ b/docs/spice_style.txt
> > @@ -404,3 +404,12 @@ Also in source (no header) files you must include
> > `config.h` at the beginning so
> >  
> >  #include "spice_server.h"
> >  
> > +
> > +
> > +Compilation
> > +---
> > +
> > +The source code should compile without warnings on all variants of GCC and
> > clang available.
> 
> This is quite strong, looks like before sending a patch you should
> use any variant you find.
> 
> I would go with a more open specification not including the compilers:
> 
> "The source code should compile without warnings"
> 
> > +A patch may be rejected if it introduces new warnings.
> > +Warnings that appear over time due to improvements in compilers should be
> > fixed in dedicated patches. A patch should not mix warning fixes and other
> > changes.
> 
> agreed
> 
> > +Any patch may adjust whitespace (e.g. eliminate trailing whitespace).
> > Whitespace adjustments do not require specific patches.
> 
> don't agree
> 
> 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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Frediano Ziglio

> 
> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote:
> > Hey,
> > 
> > On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
> > > On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
> > > > > 
> > > > > From: Christophe de Dinechin 
> > > > > 
> > > > > The objective of these guidelines is that:
> > > > > - We avoid introducing new warnings
> > > > > - We know how to fix old ones
> > > > > - We don't have to isolate whitespace changes when submitting
> > > > > patches,
> > > > >   i.e. someone who use tools that automatically strip whitespaces and
> > > > >   therefore "repairs" earlier errors should not be punished for it.
> > > > 
> > > > Sorry, I don't agree with the automatic tool, patches should not
> > > > contain extra changes unless they fix space changes while changing
> > > > these lines for other reasons.
> > > > I'll personally accept single patches fixing the spaces.
> > > 
> > > I'm with Frediano here. If you want to automatically fix whitespace
> > > errors, you can do it in a separate commit without much effort?
> > > 
> > > I can also go right now and fix all trailing whitespaces with a bash
> > > oneliner, submit the patch and we have a moot point here? :)
> > 
> > Well, it was nacked before :)
> > 
> > https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html
> 
> What is the reasoning behind the nack?
> 
> I'd rather have a style-only commit than to fight the bad indentation
> manually and possibly have unrelated whitespace changes in another
> patch...
> 
> By the way, mine got acked :D
> 
> https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html
> 
> Lukas
> 

Depends on many cases. You don't want spurious changes to make harder to
look at the history for instance (that is a point for Nack).
The patch is not fixing anything or adding new feature (another for Nack).
On the other hand applied to code not changed for a long period (where is
unlikely to have to dig the history) or code with small history (where
is faster to skip in any case) changes.
Being style it depends also on personal opinions.

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 10:52, Frediano Ziglio  wrote:
> 
>> 
>> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote:
>>> Hey,
>>> 
>>> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
 On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
>> 
>> From: Christophe de Dinechin 
>> 
>> The objective of these guidelines is that:
>> - We avoid introducing new warnings
>> - We know how to fix old ones
>> - We don't have to isolate whitespace changes when submitting
>> patches,
>>  i.e. someone who use tools that automatically strip whitespaces and
>>  therefore "repairs" earlier errors should not be punished for it.
> 
> Sorry, I don't agree with the automatic tool, patches should not
> contain extra changes unless they fix space changes while changing
> these lines for other reasons.
> I'll personally accept single patches fixing the spaces.
 
 I'm with Frediano here. If you want to automatically fix whitespace
 errors, you can do it in a separate commit without much effort?
 
 I can also go right now and fix all trailing whitespaces with a bash
 oneliner, submit the patch and we have a moot point here? :)
>>> 
>>> Well, it was nacked before :)
>>> 
>>> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html
>> 
>> What is the reasoning behind the nack?
>> 
>> I'd rather have a style-only commit than to fight the bad indentation
>> manually and possibly have unrelated whitespace changes in another
>> patch...
>> 
>> By the way, mine got acked :D
>> 
>> https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html
>> 
>> Lukas
>> 
> 
> Depends on many cases. You don't want spurious changes to make harder to
> look at the history for instance (that is a point for Nack).
> The patch is not fixing anything or adding new feature (another for Nack).
> On the other hand applied to code not changed for a long period (where is
> unlikely to have to dig the history) or code with small history (where
> is faster to skip in any case) changes.
> Being style it depends also on personal opinions.

You can’t have it both ways. Here, you are simultaneously saying:

- We don’t want trailing whitespace
- We nack patches that fix trailing whitespace on their own
- We nack patches that include incidental whitespace fixes

Sorry, I can’t agree with that, it’s inconsistent.


Christophe


> 
> Frediano

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

Re: [Spice-devel] [PATCH v2 06/13] Rephrase section on short functions for readability

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 3e463d2f..74f4e29d 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -113,7 +113,7 @@ If multiple related constants are to be defined, consider
> the use of enumeration
>  Short functions
>  ---
>  
> -Try to split code to short functions, each having simple functionality, in
> order to improve code readability and re-usability. Prefix with inline short
> functions or functions that were splitted for readability reason.
> +Try to split code to short functions, each having simple functionality, in
> order to improve code readability and re-usability. Prefix with `inline`
> functions that were splitted for readability reason or that are very short.
>  
>  Return on if
>  

I really don't understanding the aiming of both version.
Is mixing the inline concept (optimization suggestion) with readability
and I don't understand the reason.

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote:
> Hey,
> 
> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
> > On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
> > > > 
> > > > From: Christophe de Dinechin 
> > > > 
> > > > The objective of these guidelines is that:
> > > > - We avoid introducing new warnings
> > > > - We know how to fix old ones
> > > > - We don't have to isolate whitespace changes when submitting patches,
> > > >   i.e. someone who use tools that automatically strip whitespaces and
> > > >   therefore "repairs" earlier errors should not be punished for it.
> > > 
> > > Sorry, I don't agree with the automatic tool, patches should not
> > > contain extra changes unless they fix space changes while changing
> > > these lines for other reasons.
> > > I'll personally accept single patches fixing the spaces.
> > 
> > I'm with Frediano here. If you want to automatically fix whitespace
> > errors, you can do it in a separate commit without much effort?
> > 
> > I can also go right now and fix all trailing whitespaces with a bash
> > oneliner, submit the patch and we have a moot point here? :)
> 
> Well, it was nacked before :)
> 
> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html

What is the reasoning behind the nack?

I'd rather have a style-only commit than to fight the bad indentation
manually and possibly have unrelated whitespace changes in another
patch...

By the way, mine got acked :D

https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Frediano Ziglio

> 
> > On 8 Feb 2018, at 10:52, Frediano Ziglio  wrote:
> > 
> >> 
> >> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote:
> >>> Hey,
> >>> 
> >>> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
>  On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> The objective of these guidelines is that:
> >> - We avoid introducing new warnings
> >> - We know how to fix old ones
> >> - We don't have to isolate whitespace changes when submitting
> >> patches,
> >>  i.e. someone who use tools that automatically strip whitespaces and
> >>  therefore "repairs" earlier errors should not be punished for it.
> > 
> > Sorry, I don't agree with the automatic tool, patches should not
> > contain extra changes unless they fix space changes while changing
> > these lines for other reasons.
> > I'll personally accept single patches fixing the spaces.
>  
>  I'm with Frediano here. If you want to automatically fix whitespace
>  errors, you can do it in a separate commit without much effort?
>  
>  I can also go right now and fix all trailing whitespaces with a bash
>  oneliner, submit the patch and we have a moot point here? :)
> >>> 
> >>> Well, it was nacked before :)
> >>> 
> >>> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html
> >> 
> >> What is the reasoning behind the nack?
> >> 
> >> I'd rather have a style-only commit than to fight the bad indentation
> >> manually and possibly have unrelated whitespace changes in another
> >> patch...
> >> 
> >> By the way, mine got acked :D
> >> 
> >> https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html
> >> 
> >> Lukas
> >> 
> > 
> > Depends on many cases. You don't want spurious changes to make harder to
> > look at the history for instance (that is a point for Nack).
> > The patch is not fixing anything or adding new feature (another for Nack).
> > On the other hand applied to code not changed for a long period (where is
> > unlikely to have to dig the history) or code with small history (where
> > is faster to skip in any case) changes.
> > Being style it depends also on personal opinions.
> 
> You can’t have it both ways. Here, you are simultaneously saying:
> 
> - We don’t want trailing whitespace

OT: Not only trailing, also tabs for instance.

> - We nack patches that fix trailing whitespace on their own

Not saying that, I'm saying is not black and white.

> - We nack patches that include incidental whitespace fixes
> 

Not saying that either, I'm saying that if the patch says "fix this"
and is also fixing spaces in unrelated hunks should be split.

> Sorry, I can’t agree with that, it’s inconsistent.
> 
> 
> Christophe
> 

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 11:01, Frediano Ziglio  wrote:
> 
>>> 
>>> On 8 Feb 2018, at 10:52, Frediano Ziglio  wrote:
>>> 
 
 On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote:
> Hey,
> 
> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
>> On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
 
 From: Christophe de Dinechin 
 
 The objective of these guidelines is that:
 - We avoid introducing new warnings
 - We know how to fix old ones
 - We don't have to isolate whitespace changes when submitting
 patches,
 i.e. someone who use tools that automatically strip whitespaces and
 therefore "repairs" earlier errors should not be punished for it.
>>> 
>>> Sorry, I don't agree with the automatic tool, patches should not
>>> contain extra changes unless they fix space changes while changing
>>> these lines for other reasons.
>>> I'll personally accept single patches fixing the spaces.
>> 
>> I'm with Frediano here. If you want to automatically fix whitespace
>> errors, you can do it in a separate commit without much effort?
>> 
>> I can also go right now and fix all trailing whitespaces with a bash
>> oneliner, submit the patch and we have a moot point here? :)
> 
> Well, it was nacked before :)
> 
> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html
 
 What is the reasoning behind the nack?
 
 I'd rather have a style-only commit than to fight the bad indentation
 manually and possibly have unrelated whitespace changes in another
 patch...
 
 By the way, mine got acked :D
 
 https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html
 
 Lukas
 
>>> 
>>> Depends on many cases. You don't want spurious changes to make harder to
>>> look at the history for instance (that is a point for Nack).
>>> The patch is not fixing anything or adding new feature (another for Nack).
>>> On the other hand applied to code not changed for a long period (where is
>>> unlikely to have to dig the history) or code with small history (where
>>> is faster to skip in any case) changes.
>>> Being style it depends also on personal opinions.
>> 
>> You can’t have it both ways. Here, you are simultaneously saying:
>> 
>> - We don’t want trailing whitespace
> 
> OT: Not only trailing, also tabs for instance.
> 
>> - We nack patches that fix trailing whitespace on their own
> 
> Not saying that, I'm saying is not black and white.

Granted, you are not saying it, Marc-André is… But if one nacks patches with 
only whitespace and the other nacks patches where whitespace are incidental, we 
still have a problem.

> 
>> - We nack patches that include incidental whitespace fixes
>> 
> 
> Not saying that either, I'm saying that if the patch says "fix this"
> and is also fixing spaces in unrelated hunks should be split.

Which is exactly what I am saying, isn’t it?

> 
>> Sorry, I can’t agree with that, it’s inconsistent.
>> 
>> 
>> Christophe
>> 
> 
> 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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 11:09, Victor Toso  wrote:
> 
> Hi,
> 
> On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote:
 Depends on many cases. You don't want spurious changes to make harder to
 look at the history for instance (that is a point for Nack).
 The patch is not fixing anything or adding new feature (another for Nack).
 On the other hand applied to code not changed for a long period (where is
 unlikely to have to dig the history) or code with small history (where
 is faster to skip in any case) changes.
 Being style it depends also on personal opinions.
>>> 
>>> You can’t have it both ways. Here, you are simultaneously saying:
>>> 
>>> - We don’t want trailing whitespace
>> 
>> OT: Not only trailing, also tabs for instance.
>> 
>>> - We nack patches that fix trailing whitespace on their own
>> 
>> Not saying that, I'm saying is not black and white.
> 
> Because the code itself is inconsistent. It would be so much
> better to have a few patches that make the code consistent and
> then some git hook to check if given patch does not mess around
> the coding style instead of discussing this so many times over
> years.

I thought I was requesting even less than a big fix. I was only requesting to 
allow small fixes. But if Frediano does not want this, then I would go with you 
and say we fix it once and then enforce it.

Christophe


> 
>toso
> ___
> 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

Re: [Spice-devel] [PATCH v2 05/13] Rephrase section about constants

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> The indent of the rephrasing is that:
> 
> - If you have a single constant, use const, e.g. (visible in debugger)
> const unsigned max_stuff = 42;
> 
> - If you have multiple constants, prefer enums over #define, as
> already suggested later in the guide:
> enum {
>A = 0,
>B = 42,
>C = -1
> }
> 

The const is a bit different from C and C++ and this results
in a bit of confusion. max_stuff above is a compile time
constant in C++, not in C while the enumeration is always
a compile time constant in both languages.
So the choice is not only syntax one.
I think that the "Alternatively, use global `const` variables."
is quite an ancient comment when spice-server was in C++
(much before I join and I think even older than the git repository!)
Unfortunately also C currently can't attach a type to the
enumeration values so if you want to define in C a constant
with a type usually you use a define like

#define MY_CONSTANT ((uint64_t)123)

You are right, this paragraph needs some care. Maybe the
style should be different for C and C++ and here we should
just use enum and #define while having a section for C++
telling about const ?

> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 61cb0701..3e463d2f 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -106,7 +106,9 @@ Using goto is allowed in C code for error handling. In
> any other case it will be
>  Defining Constant values
>  
>  
> -Use defines for constant values for improving readability and ease of
> changes. Alternatively, use global `const` variables.
> +Use defines for constant values for improving readability and ease of
> changes.
> +Alternatively, use global `const` variables for individual values.
> +If multiple related constants are to be defined, consider the use of
> enumerations with initializers.
>  
>  Short functions
>  ---

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

Re: [Spice-devel] [PATCH v2 08/13] Remove confusing example

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> The sentence explaining that example makes no real sense, and
> the coding style suggestion is horrendous (not to mention flies in the
> face of all automatic indentation tools)
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 10 +-
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index a9d77afa..ccc60bb3 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -254,15 +254,7 @@ if (condition) {
>  +
>  In case of long condition statement, prefer having additional temporary
>  variable over multiple line condition statement.
>  +
> -In case of new line in condition statement.
> -+
> -[source,c]
> -
> -if (long_name && very_long_name && very_long ||
> -   var_name) {
> -
> -+
> -or indent under the round bracket using spaces
> +Indent long conditionals under the opening parenthesis using spaces
>  +
>  [source,c]
>  

Looks like you are being too kind. Should be "Remove the ugly example" :-)
Good you pointed out.
I think the main reason should be (beside it does not look great) that
this style is not used in the code. Maybe:

"Remove confusing example

The sentence explaining that example makes no real sense, the
mentioned style is not used by current code and the coding style
suggestion is horrendous."

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Frediano Ziglio

> 
> From: Christophe de Dinechin 
> 
> The objective of these guidelines is that:
> - We avoid introducing new warnings
> - We know how to fix old ones
> - We don't have to isolate whitespace changes when submitting patches,
>   i.e. someone who use tools that automatically strip whitespaces and
>   therefore "repairs" earlier errors should not be punished for it.

Sorry, I don't agree with the automatic tool, patches should not
contain extra changes unless they fix space changes while changing
these lines for other reasons.
I'll personally accept single patches fixing the spaces.

> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index e2465aa9..d9644f9a 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -404,3 +404,12 @@ Also in source (no header) files you must include
> `config.h` at the beginning so
>  
>  #include "spice_server.h"
>  
> +
> +
> +Compilation
> +---
> +
> +The source code should compile without warnings on all variants of GCC and
> clang available.

This is quite strong, looks like before sending a patch you should
use any variant you find.

I would go with a more open specification not including the compilers:

"The source code should compile without warnings"

> +A patch may be rejected if it introduces new warnings.
> +Warnings that appear over time due to improvements in compilers should be
> fixed in dedicated patches. A patch should not mix warning fixes and other
> changes.

agreed

> +Any patch may adjust whitespace (e.g. eliminate trailing whitespace).
> Whitespace adjustments do not require specific patches.

don't agree

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

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Victor Toso

Hey,

On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote:
> On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote:
> > > 
> > > From: Christophe de Dinechin 
> > > 
> > > The objective of these guidelines is that:
> > > - We avoid introducing new warnings
> > > - We know how to fix old ones
> > > - We don't have to isolate whitespace changes when submitting patches,
> > >   i.e. someone who use tools that automatically strip whitespaces and
> > >   therefore "repairs" earlier errors should not be punished for it.
> > 
> > Sorry, I don't agree with the automatic tool, patches should not
> > contain extra changes unless they fix space changes while changing
> > these lines for other reasons.
> > I'll personally accept single patches fixing the spaces.
> 
> I'm with Frediano here. If you want to automatically fix whitespace
> errors, you can do it in a separate commit without much effort?
> 
> I can also go right now and fix all trailing whitespaces with a bash
> oneliner, submit the patch and we have a moot point here? :)

Well, it was nacked before :)

https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html



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 v2 12/13] Add guidelines about warnings and whitespaces

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 10:02, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> The objective of these guidelines is that:
>> - We avoid introducing new warnings
>> - We know how to fix old ones
>> - We don't have to isolate whitespace changes when submitting patches,
>>  i.e. someone who use tools that automatically strip whitespaces and
>>  therefore "repairs" earlier errors should not be punished for it.
> 
> Sorry, I don't agree with the automatic tool, patches should not
> contain extra changes unless they fix space changes while changing
> these lines for other reasons.
> I'll personally accept single patches fixing the spaces.

But as Victor pointed out, others don’t.


> 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 9 +
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index e2465aa9..d9644f9a 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -404,3 +404,12 @@ Also in source (no header) files you must include
>> `config.h` at the beginning so
>> 
>> #include "spice_server.h"
>> 
>> +
>> +
>> +Compilation
>> +---
>> +
>> +The source code should compile without warnings on all variants of GCC and
>> clang available.
> 
> This is quite strong, looks like before sending a patch you should
> use any variant you find.

That was not the intent, which is why it says “should” and not “must”.

> 
> I would go with a more open specification not including the compilers:
> 
> "The source code should compile without warnings”

But that’s even stronger. It means on any compiler…

> 
>> +A patch may be rejected if it introduces new warnings.
>> +Warnings that appear over time due to improvements in compilers should be
>> fixed in dedicated patches. A patch should not mix warning fixes and other
>> changes.
> 
> agreed
> 
>> +Any patch may adjust whitespace (e.g. eliminate trailing whitespace).
>> Whitespace adjustments do not require specific patches.
> 
> don't agree

Impasse then, because I care about this one based on observing the history of 
whitespace-only changes being nacked and whitespace-including changes being 
also nacked.

> 
> Frediano

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

Re: [Spice-devel] [PATCH v2 04/13] Rephrase assertion section

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 10:35, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 72ed2ef7..61cb0701 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -82,7 +82,11 @@ Comments that are prefixed with `FIXME` describe a bug
>> that need to be fixed. Ge
>> ASSERT
>> --
>> 
>> -Use it freely. ASSERT helps testing function arguments and function results
>> validity.  ASSERT helps detecting bugs and improve code readability and
>> stability.
>> +Use assertions liberally. They helps testing function arguments and function
>> results validity. Assertions helps detecting bugs and improve code
>> readability and stability.
>> +
>> +Several form of assertion exist, notably:
>> +- spice_assert which should be preferred for any assertion related to SPICE
>> itself
>> +- glib asserts (many forms) which should be preferred for any assertion
>> related to the use of glib
>> 
>> sizeof
> 
> Yes, this section looks... weird.
> I think there is a problem here and this entire section is entirely broken
> now.
> To sum up: we don't use ASSERT! At all. But we use some kind of asserts!
> Yes, sounds confusing.
> But you have to consider the long story of this project (which unfortunately
> I don't know entirely by myself!). Maybe I'm a bit wrong but the project was
> in C++ using a lot of Windows knowledge and styles. This is a lot lost in
> the current code is is hard to see but I was also a Windows developer for
> quite some time (I don't regret it) and I can still note the smell of it!
> 
> What's specifically "ASSERT" (not assert, not asserts) ?
> ASSERT is a Windows macro that aborts the code if the condition is false.
> It helps detecting the bugs but this macro is NEVER compiled in release
> code (Windows rules!). The difference seems small but is way different/
> Is not used for runtime checks that can happen as this would probably
> causes crashes on release. Is used for checks that should never happen
> to help developers testing with debugging versions (on Windows you test
> with debugg versions). How can I say that? There are some places where
> this macro is too aggressive, where should be mathematically impossible
> to reach. For instance in common/ring.h they are used to check
> internal states at every low level operation (in functions that are
> inlined). I just think in the history of code ASSERT were changed to
> spice_assert. Other indications (looks like to be a paleontologist) are
> in code like common/quic.c (notably encode function) which are basically
> testing is a 32 bit integer has more that 32 bits!
> 
> In our code we never disable our assert style asserts (either spice_assert
> or g_assert) which make them suitable for runtime checks but a bit
> overwhelming to check the "obvious”.

OK. So nobody really knows how to phrase this then ;-)

What about this:

Assertions
--

Use assertions liberally. Assertions help testing function arguments and 
function results validity. As a result, they make it easier to detect bugs. 
Also, by expressing the intent of the developer, they make the code easier to 
read.

Several form of assertion exist. SPICE does not use the language `assert` much, 
but instead relies on the following variants:

- `spice_assert`, defined in `common/log.h`, is never disabled at run-time and 
prints an error if the assertion fails.

- `g_assert` and other variants defined in glib such as `g_assert_null`, can be 
diabled by setting `G_DISABLE_ASSERT` (which is never done in practice for 
SPICE code), and emits a message through the g_assertion_message function and 
its variants.

The guidelines for selecting one or the other are not very clear from the 
existing code. The `spice_assert` variant may be preferred for SPICE-only code, 
as it makes it clearer that this assertion is coming from SPICE. The `g_assert` 
and variants may be preferred for assertions related to the use of glib.


I’m handwaving a little too much to my taste here, but I’m afraid than anything 
more precise will lead to another round of debate ;-)

> 
> Frediano

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

Re: [Spice-devel] [PATCH spice-server 1/2] stream-device: handle cursor from device

2018-02-08 Thread Jonathon Jongsma

On Tue, 2018-01-16 at 12:28 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stream-device.c | 154
> ++---
>  1 file changed, 147 insertions(+), 7 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 735f2933..c15742c6 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -23,6 +23,7 @@
>  
>  #include "char-device.h"
>  #include "stream-channel.h"
> +#include "cursor-channel.h"
>  #include "reds.h"
>  
>  #define TYPE_STREAM_DEVICE stream_device_get_type()
> @@ -45,13 +46,16 @@ struct StreamDevice {
>  union {
>  StreamMsgFormat format;
>  StreamMsgCapabilities capabilities;
> +StreamMsgCursorSet cursor_set;
>  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> -} msg;
> +} *msg;
>  uint32_t msg_pos;
> +uint32_t msg_len;
>  bool has_error;
>  bool opened;
>  bool flow_stopped;
>  StreamChannel *stream_channel;
> +CursorChannel *cursor_channel;
>  };
>  
>  struct StreamDeviceClass {
> @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> RED_TYPE_CHAR_DEVICE)
>  typedef bool StreamMsgHandler(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static StreamMsgHandler handle_msg_format, handle_msg_data;
> +static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_cursor_set;
>  
>  static bool handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin,
> const char *error_msg)
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -108,6 +112,10 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>  dev->hdr.type = GUINT16_FROM_LE(dev->hdr.type);
>  dev->hdr.size = GUINT32_FROM_LE(dev->hdr.size);
>  dev->msg_pos = 0;
> +if (dev->msg_len > sizeof(*dev->msg)) {
> +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> +dev->msg_len = sizeof(*dev->msg);
> +}

So if the last message was bigger than the union (in other words: the
last message was a cursor set command), we downsize it to the size of
the union. But if this command is also a cursor set command, we'll just
realloc it larger again when we call handle_msg_cursor_set(). 

Wouldn't it be better to realloc to the size of dev->hdr.size here
instead (with some upper limit on message size)? That would avoid
unnecessary memory churn.

>  }
>  }
>  
> @@ -122,6 +130,9 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>  case STREAM_TYPE_DATA:
>  handled = handle_msg_data(dev, sin);
>  break;
> +case STREAM_TYPE_CURSOR_SET:
> +handled = handle_msg_cursor_set(dev, sin);
> +break;
>  case STREAM_TYPE_CAPABILITIES:
>  /* FIXME */
>  default:
> @@ -194,7 +205,7 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>  }
>  
> -int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> sizeof(StreamMsgFormat) - dev->msg_pos);
> +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);
>  }
> @@ -205,9 +216,9 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  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);
> +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;
>  }
>  
> @@ -238,6 +249,107 @@ handle_msg_data(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  return dev->hdr.size == 0;
>  }
>  
> +/*
> + * Returns number of bits required for a given
> + * cursor type.
> + *
> + * Take into account mask bits.
> + * Returns 0 on error.
> + */
> +static unsigned int
> +get_cursor_type_bits(unsigned int cursor_type)
> +{
> +switch (cursor_type) {
> +case SPICE_CURSOR_TYPE_ALPHA:
> +return 32;
> +case SPICE_CURSOR_TYPE_COLOR24:
> +return 25;
> +case SPICE_CURSOR_TYPE_COLOR32:
> +return 33;
> +default:
> +return 0;
> +}
> +}

Where do these numbers come from?

> +
> +static RedCursorCmd *
> +stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg,
> size_t msg_size)
> +{
> +RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
> +cmd->type = QXL_CURSOR_SET;
> +cmd->u.set.position.x = 0; // TODO
> +cmd->u.set.position.y = 0; // TODO
> +

Re: [Spice-devel] [PATCH spice-server] style: Update style to include some C++ element

2018-02-08 Thread Lukáš Hrázký

On Wed, 2018-02-07 at 10:10 +0100, Christophe de Dinechin wrote:
> Frediano Ziglio writes:
> 
> > These style are used by other SPICE projects like spice-streaming-agent.

"This style is used ..."

> > See discussion "Coding style and naming conventions for C++" at
> > https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.

I'm not sure if referring to ML discussion is a good idea, but if you
think so, not objecting... :)

> > Signed-off-by: Frediano Ziglio 
> > ---
> >  docs/spice_style.txt | 38 +-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > index eb0e30ef..0e0028e9 100644
> > --- a/docs/spice_style.txt
> > +++ b/docs/spice_style.txt
> > @@ -1,7 +1,7 @@
> >  Spice project coding style and coding conventions
> >  =
> > 
> > -Copyright (C) 2009-2016 Red Hat, Inc.
> > +Copyright (C) 2009-2018 Red Hat, Inc.
> >  Licensed under a Creative Commons Attribution-Share Alike 3.0
> >  United States License (see 
> > http://creativecommons.org/licenses/by-sa/3.0/us/legalcode).
> > 
> > @@ -379,3 +379,39 @@ Also in source (no header) files you must include 
> > `config.h` at the beginning so
> > 
> >  #include "spice_server.h"
> 
> (Not in your patch) Re-reading the part about headers, it has everything
> completely backwards! I suggest we re-discuss this.
> 
> >  
> > +
> > +C++
> > +---
> > +C++ follows C style if not specified otherwise.
> 
> I would add
> 
> The C++11 dialect is assumed by default. No attempts will be made to
> restrict the code to older variants of C++. Using constructs allowed in
> later variants of C++ will be allowed either:
> 
> - After the default C++ compiler for building the oldest supported RHEL
>   version fully supports the corresponding variant of C++
> 
> - Under the appropriate preprocessor conditions, if the previous
>   condition is not met and C++11 alternatives would have significant
>   drawbacks
> 
> 
> > +
> > +Method names
> > +
> > +
> > +Method names should use lower case and separate words with
> > +underscores.
> 
> What about classes? I see below that you selected camelcase for classes,
> but that is inconsistent with standard C++ classes such as 'string', or
> with most types in C (e.g. ptrdiff_t, struct stat, etc)
> 
> Since it seems that consistency with C and with the standard C++ library
> was the primary rationale for using snake-case, I would go for
> snake-case for classes as well.
> 
> (Frankly, I really could not care less, I'm just trying to be consistent)
> 
> > +
> > +
> > +Namespaces
> > +~~
> > +
> > +Namespaces should use lower case and separate words with underscores.
> > +Namespace blocks should not increase indentation.
> > +Namespaces can be nested and closure can be collapsed but for
> 
> I would not use the word "closure" here, which has a specific meaning in
> computer science (as in lambda variable capture). What about:
> 
> Namespaces can be nested. Namespace closing brackets for nested
> namespaces can be placed together on the same line, but for readability
> reasons the closure should specify the namespace with a comment.

Agreed.

> > +readability reasons the closure should specify the namespace with a
> > +comment.
> > +
> > +[source,cpp]
> > +
> > +namespace spice {
> > +namespace streaming_agent {
> 
> There is a style inconsistency regarding the placement of the opening
> brace relative to other first-column opening braces (function and
> class). So I would suggest that all top-level opening braces be on a
> line of their own, as for 'class' below and for functions.

I'm not a fan, but if you guys decided you want to waste some lines on
lonely opening braces :), please amend the patch.

> > +
> > +class ClassInsideNamespace
> > +{
> > +...
> > +};
> > +
> > +}} // namespace spice::streaming_agent
> > +
> > +
> > +You should not import an entire namespace but use qualified names
> > +instead.
> 
> The C++ terminology is "using namespace" and not "import". So I would
> rephrase as:
> 
> The "using namespace" construct should never be used in headers. It should
> be used sparingly in source files, and only within the body of short
> functions.
> 
> Preferred alternatives to "using namespace" include:
> - using declarations
> using spice::streaming_agent::some_class;
> - namespace aliases
> namespace ssa = spice::streaming_agent;

Agreed as well.

> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> ___
> 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

Re: [Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

2018-02-08 Thread Frediano Ziglio

> 
> From: Marc-André Lureau 
> 
> For some reason, the URIs test didn't include spice+unix:// checks,
> probably because they came about the same time.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  tests/session.c | 28 +---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/session.c b/tests/session.c
> index 7ed4a41..413d812 100644
> --- a/tests/session.c
> +++ b/tests/session.c
> @@ -9,6 +9,7 @@ typedef struct {
>  const gchar *uri_input;
>  const gchar *uri_output;
>  const gchar *message;
> +const gchar *unix_path;
>  } TestCase;
>  
>  static void test_session_uri_bad(void)
> @@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase *tests,
> const guint cases)
>  
>  /* Set URI and check URI, port and tls_port */
>  for (i = 0; i < cases; i++) {
> -gchar *uri, *port, *tls_port, *host, *username, *password;
> +gchar *uri, *port, *tls_port, *host, *username, *password,
> *unix_path;
>  
>  s = spice_session_new();
>  if (tests[i].message != NULL)
> @@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase
> *tests, const guint cases)
>   "host", ,
>   "username", ,
>   "password", ,
> + "unix-path", _path,
>NULL);
> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==, uri);
>  g_assert_cmpstr(tests[i].port, ==, port);
>  g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
>  g_assert_cmpstr(tests[i].host, ==, host);
>  g_assert_cmpstr(tests[i].username, ==, username);
>  g_assert_cmpstr(tests[i].password, ==, password);
>  g_test_assert_expected_messages();
> +g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
>  g_clear_pointer(, g_free);
>  g_clear_pointer(, g_free);
>  g_clear_pointer(_port, g_free);
>  g_clear_pointer(, g_free);
>  g_clear_pointer(, g_free);
>  g_clear_pointer(, g_free);
> +g_clear_pointer(_path, g_free);
>  g_object_unref(s);
>  }
>  
> @@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase *tests,
> const guint cases)
>   "host", tests[i].host,
>   "username", tests[i].username,
>   "password", tests[i].password,
> + "unix-path", tests[i].unix_path,
>NULL);
>  g_object_get(s, "uri", , NULL);
> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==, uri);
>  g_clear_pointer(, g_free);
>  g_object_unref(s);
>  }
> @@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
>  test_session_uri_good(tests, G_N_ELEMENTS(tests));
>  }
>  
> +static void test_session_uri_unix_good(void)
> +{
> +const TestCase tests[] = {
> +{ .uri_input = "spice+unix:///tmp/foo.sock",
> +  .unix_path = "/tmp/foo.sock" },
> +/* perhaps not very clever, but this doesn't raise an error/warning
> */
> +{ .uri_input = "spice+unix://",
> +  .unix_path = "" },
> +/* unix uri don't support passing password or other kind of options
> */
> +{ .uri_input = "spice+unix:///tmp/foo.sock?password=frobnicate",
> +  .unix_path = "/tmp/foo.sock?password=frobnicate" },
> +};
> +
> +test_session_uri_good(tests, G_N_ELEMENTS(tests));
> +}
> +
>  int main(int argc, char* argv[])
>  {
>  g_test_init(, , NULL);
> @@ -285,6 +306,7 @@ int main(int argc, char* argv[])
>  g_test_add_func("/session/bad-uri", test_session_uri_bad);
>  g_test_add_func("/session/good-ipv4-uri", test_session_uri_ipv4_good);
>  g_test_add_func("/session/good-ipv6-uri", test_session_uri_ipv6_good);
> +g_test_add_func("/session/good-unix", test_session_uri_unix_good);
>  
>  return g_test_run();
>  }

Looks good (still to review better).
Can we consider this patch separate from the rest of the series
(that is merge even separately) ?

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

Re: [Spice-devel] [PATCH spice-gtk 2/4] uri: learn to parse spice+tls:// form

2018-02-08 Thread Frediano Ziglio

> 
> From: Marc-André Lureau 
> 
> spice:// has a weird scheme encoding, where it can accept both plain
> and tls ports with URI query parameters. However, it's not very
> convenient nor very common to use (who really want to mix plain & tls
> channels?).
> 
> Instead, let's introduce the more readable form spice+tls://host:port
> 
> This form will not accept query string, thus mixing plain and tls is
> not possible (it would be confusing to have ?port= for plain), nor
> passing password and such.
> 

OT: is this schema extensible? Now we have "spice", "spice+unix"
and "spice+tls", surely we don't want "spice+unix+tls" but maybe
somebody could this of a "spice+tls+kerberos" ?

> Signed-off-by: Marc-André Lureau 
> ---
>  man/spice-client.pod | 29 +
>  src/spice-session.c  | 23 +++
>  2 files changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/man/spice-client.pod b/man/spice-client.pod
> index 7288b84..4b23c7d 100644
> --- a/man/spice-client.pod
> +++ b/man/spice-client.pod
> @@ -12,23 +12,24 @@ can be used to tweak some SPICE-specific option.
>  
>  =head1 URI
>  
> -The most basic SPICE URI which can be used is in the form
> +To initiate a plain SPICE connection (the connection will be
> +unencrypted) to hostname.example.com and port 5900, use the following
> +URI:
> +
>spice://hostname.example.com:5900
>  
> -This will try to initiate a SPICE connection to hostname.example.com
> -to port 5900. This connection will be unencrypted. This URI is
> -equivalent to
> -  spice://hostname.example.com?port=5900
> +In order to start a TLS connection, one would use:
>  
> -In order to start a TLS connection, one would use
> -  spice://hostname.example.com?tls-port=5900
> +  spice+tls://hostname.example.com:5900
>  
> -Other valid URI parameters are 'username' and 'password'. Be careful that
> -passing a password through a SPICE URI might cause the password to be
> -visible by any local user through 'ps'.
> +Note: this form is available since v0.35, you have to use the spice://
> +query string with the tls-port parameter before that.
> +
> +=head1 spice:// URI query string
> +
> +spice:// URI accepts query string. Several parameters can be specified
> +at once if they are separated by & or ;
>  
> -Several parameters can be specified at once if they are separated
> -by & or ;
>spice://hostname.example.com?port=5900;tls-port=5901
>  
>  When using 'tls-port', it's recommended to not specify any non-TLS port.
> @@ -39,6 +40,10 @@ then try to use the TLS port. This means a
> man-in-the-middle could force
>  the whole SPICE session to go in clear text regardless of the TLS settings
>  of the SPICE server.
>  
> +Other valid URI parameters are 'username' and 'password'. Be careful that
> +passing a password through a SPICE URI might cause the password to be
> +visible by any local user through 'ps'.
> +
>  =head1 OPTIONS
>  
>  The following options are accepted when running a SPICE client which
> diff --git a/src/spice-session.c b/src/spice-session.c
> index 2aabf58..7218449 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -389,6 +389,7 @@ spice_session_finalize(GObject *gobject)
>  
>  #define URI_SCHEME_SPICE "spice://"
>  #define URI_SCHEME_SPICE_UNIX "spice+unix://"
> +#define URI_SCHEME_SPICE_TLS "spice+tls://"
>  #define URI_QUERY_START ";?"
>  #define URI_QUERY_SEP   ";&"
>  
> @@ -425,6 +426,7 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>  gchar *authority = NULL;
>  gchar *query = NULL;
>  gchar *tmp = NULL;
> +bool tls_scheme = false;
>  
>  g_return_val_if_fail(original_uri != NULL, -1);
>  
> @@ -438,12 +440,16 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>  /* Break up the URI into its various parts, scheme, authority,
>   * path (ignored) and query
>   */
> -if (!g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
> +if (g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
> +authority = uri + strlen(URI_SCHEME_SPICE);
> +} else if (g_str_has_prefix(uri, URI_SCHEME_SPICE_TLS)) {
> +authority = uri + strlen(URI_SCHEME_SPICE_TLS);
> +tls_scheme = true;
> +} else {
>  g_warning("Expected a URI scheme of '%s' in URI '%s'",
>URI_SCHEME_SPICE, uri);

I think here we should extent like

 g_warning("Expected a URI scheme of '%s' or '%s' in URI '%s'",
   URI_SCHEME_SPICE, URI_SCHEME_SPICE_TLS, uri);

>  goto fail;
>  }
> -authority = uri + strlen(URI_SCHEME_SPICE);
>  
>  tmp = strchr(authority, '@');
>  if (tmp) {
> @@ -502,6 +508,11 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>  }
>  path = NULL;
>  
> +if (tls_scheme && query[0] != '\0') {
> +g_warning(URI_SCHEME_SPICE_TLS " scheme doesn't support query
> string");
>

[Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 .clang-format | 23 +++
 1 file changed, 23 insertions(+)
 create mode 100644 .clang-format

diff --git a/.clang-format b/.clang-format
new file mode 100644
index ..91203600
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,23 @@
+Language:Cpp
+# BasedOnStyle:  LLVM
+
+# The following is commented out until widely supported
+# IncludeBlocks: Regroup
+SortIncludes: true
+
+IncludeCategories:
+  - Regex:   'config.h'
+Priority:-1
+  - Regex:   '^"spice.*"'
+Priority:1
+  - Regex:   'glib'
+Priority:4
+  - Regex:   '^<.*>'
+Priority:3
+  - Regex:   '^".*"'
+Priority:2
+
+ColumnLimit: 100
+IndentCaseLabels: false
+IndentWidth: 4
+BreakBeforeBraces: Linux
-- 
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] Change name space to spice::streaming_agent

2018-02-08 Thread Frediano Ziglio

Discussed recently on the mailing list.
This patch changes the namespace name only.
See discussion "Coding style and naming conventions for C++" at
https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.

Signed-off-by: Frediano Ziglio 
---
 include/spice-streaming-agent/frame-capture.hpp | 5 +++--
 include/spice-streaming-agent/plugin.hpp| 9 +
 src/concrete-agent.cpp  | 2 +-
 src/concrete-agent.hpp  | 5 +++--
 src/mjpeg-fallback.cpp  | 2 +-
 src/mjpeg-fallback.hpp  | 5 +++--
 src/spice-streaming-agent.cpp   | 2 +-
 src/static-plugin.cpp   | 2 +-
 src/static-plugin.hpp   | 5 +++--
 9 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/include/spice-streaming-agent/frame-capture.hpp 
b/include/spice-streaming-agent/frame-capture.hpp
index 6c41e04..313e07b 100644
--- a/include/spice-streaming-agent/frame-capture.hpp
+++ b/include/spice-streaming-agent/frame-capture.hpp
@@ -10,7 +10,8 @@
 
 #include 
 
-namespace SpiceStreamingAgent {
+namespace spice {
+namespace streaming_agent {
 
 struct FrameSize
 {
@@ -57,6 +58,6 @@ protected:
 void operator=(const FrameCapture&) = delete;
 };
 
-}
+}} // spice::streaming_agent
 
 #endif // SPICE_STREAMING_AGENT_FRAME_CAPTURE_HPP
diff --git a/include/spice-streaming-agent/plugin.hpp 
b/include/spice-streaming-agent/plugin.hpp
index f1d5d60..28096dc 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -17,7 +17,8 @@
  * Plugins and register them.
  */
 
-namespace SpiceStreamingAgent {
+namespace spice {
+namespace streaming_agent {
 
 class FrameCapture;
 
@@ -129,9 +130,9 @@ public:
 virtual const ConfigureOption* Options() const = 0;
 };
 
-typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
+typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
 
-}
+}} // spice::streaming_agent
 
 #ifndef SPICE_STREAMING_AGENT_PROGRAM
 /*!
@@ -146,7 +147,7 @@ typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* 
agent);
  * 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;
+extern "C" spice::streaming_agent::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 873a69e..ebeef33 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -14,7 +14,7 @@
 #include "static-plugin.hpp"
 
 using namespace std;
-using namespace SpiceStreamingAgent;
+using namespace spice::streaming_agent;
 
 static inline unsigned MajorVersion(unsigned version)
 {
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index b5876a5..0a8aeec 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -11,7 +11,8 @@
 #include 
 #include 
 
-namespace SpiceStreamingAgent {
+namespace spice {
+namespace streaming_agent {
 
 struct ConcreteConfigureOption: ConfigureOption
 {
@@ -42,6 +43,6 @@ private:
 std::vector options;
 };
 
-}
+}} // spice::streaming_agent
 
 #endif // SPICE_STREAMING_AGENT_CONCRETE_AGENT_HPP
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 7c918a7..10543ad 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -19,7 +19,7 @@
 #include "jpeg.hpp"
 
 using namespace std;
-using namespace SpiceStreamingAgent;
+using namespace spice::streaming_agent;
 
 #define ERROR(args) do { \
 std::ostringstream _s; \
diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
index 8044244..a5f1037 100644
--- a/src/mjpeg-fallback.hpp
+++ b/src/mjpeg-fallback.hpp
@@ -10,7 +10,8 @@
 #include 
 
 
-namespace SpiceStreamingAgent {
+namespace spice {
+namespace streaming_agent {
 
 struct MjpegSettings
 {
@@ -29,6 +30,6 @@ private:
 MjpegSettings settings = { 10, 80 };
 };
 
-} // namespace SpiceStreamingAgent
+}} // spice::streaming_agent
 
 #endif // SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 94d9d25..0e7641e 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -36,7 +36,7 @@
 #include "concrete-agent.hpp"
 
 using namespace std;
-using namespace SpiceStreamingAgent;
+using namespace spice::streaming_agent;
 
 static ConcreteAgent agent;
 
diff --git a/src/static-plugin.cpp b/src/static-plugin.cpp
index d5feb22..c8d55f0 100644
--- a/src/static-plugin.cpp
+++ b/src/static-plugin.cpp
@@ -11,7 +11,7 @@
 #include 
 #include "static-plugin.hpp"
 
-using namespace SpiceStreamingAgent;
+using namespace spice::streaming_agent;
 
 const StaticPlugin *StaticPlugin::list = nullptr;
 
diff --git a/src/static-plugin.hpp b/src/static-plugin.hpp
index 5436b41..4399a9a

Re: [Spice-devel] [PATCH spice-server v3] style: Update style to include some C++ element

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 14:03 +, Frediano Ziglio wrote:
> This style is used by other SPICE projects like spice-streaming-agent.
> See discussion "Coding style and naming conventions for C++" at
> https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  docs/spice_style.txt | 52 
> 
>  1 file changed, 52 insertions(+)
> 
> Changes in v3:
> - fix commit message grammar.
> 
> Changes in v2:
> - more details for namespace usages;
> - specify C++ dialect;
> - fix class indentation.
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index f5d13642..29c1756f 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -380,3 +380,55 @@ Also in source (no header) files you must include 
> `config.h` at the beginning so
>  
>  #include "spice_server.h"
>  
> +
> +C++
> +---
> +C\++ follows C style if not specified otherwise.

Sorry, one more grammar improvement:
"C\++ style follows the C style"

> +The C+\+11 dialect is assumed by default. No attempts will be made to
> +restrict the code to older variants of C+\+.
> +For compatibility reasons don't use more recent C++ dialects.
> +
> +Method names
> +
> +
> +Method names should use lower case and separate words with
> +underscores.
> +
> +
> +Namespaces
> +~~
> +
> +Namespaces should use lower case and separate words with underscores.
> +Namespace blocks should not increase indentation.
> +Namespaces can be nested. Namespace closing brackets for nested
> +namespaces can be placed together on the same line, but for
> +readability reasons the closure should specify the namespace with a
> +comment.
> +
> +[source,cpp]
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +class ClassInsideNamespace {
> +...
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +
> +The `using namespace` construct should never be used in headers. It should
> +be used sparingly in source files, and only within the body of short
> +functions.
> +
> +Preferred alternatives to `using namespace` include:
> +
> +* using declarations
> ++
> +[source,cpp]
> +using spice::streaming_agent::some_class;
> ++
> +* namespace aliases
> ++
> +[source,cpp]
> +namespace ssa = spice::streaming_agent;

Let's get it in, further improvements welcome.

Acked-by: Lukáš Hrázký 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent] Change name space to spice::streaming_agent

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 14:00 +, Frediano Ziglio wrote:
> Discussed recently on the mailing list.
> This patch changes the namespace name only.
> See discussion "Coding style and naming conventions for C++" at
> https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  include/spice-streaming-agent/frame-capture.hpp | 5 +++--
>  include/spice-streaming-agent/plugin.hpp| 9 +
>  src/concrete-agent.cpp  | 2 +-
>  src/concrete-agent.hpp  | 5 +++--
>  src/mjpeg-fallback.cpp  | 2 +-
>  src/mjpeg-fallback.hpp  | 5 +++--
>  src/spice-streaming-agent.cpp   | 2 +-
>  src/static-plugin.cpp   | 2 +-
>  src/static-plugin.hpp   | 5 +++--
>  9 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/frame-capture.hpp 
> b/include/spice-streaming-agent/frame-capture.hpp
> index 6c41e04..313e07b 100644
> --- a/include/spice-streaming-agent/frame-capture.hpp
> +++ b/include/spice-streaming-agent/frame-capture.hpp
> @@ -10,7 +10,8 @@
>  
>  #include 
>  
> -namespace SpiceStreamingAgent {
> +namespace spice {
> +namespace streaming_agent {
>  
>  struct FrameSize
>  {
> @@ -57,6 +58,6 @@ protected:
>  void operator=(const FrameCapture&) = delete;
>  };
>  
> -}
> +}} // spice::streaming_agent
>  
>  #endif // SPICE_STREAMING_AGENT_FRAME_CAPTURE_HPP
> diff --git a/include/spice-streaming-agent/plugin.hpp 
> b/include/spice-streaming-agent/plugin.hpp
> index f1d5d60..28096dc 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -17,7 +17,8 @@
>   * Plugins and register them.
>   */
>  
> -namespace SpiceStreamingAgent {
> +namespace spice {
> +namespace streaming_agent {
>  
>  class FrameCapture;
>  
> @@ -129,9 +130,9 @@ public:
>  virtual const ConfigureOption* Options() const = 0;
>  };
>  
> -typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
> +typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
>  
> -}
> +}} // spice::streaming_agent
>  
>  #ifndef SPICE_STREAMING_AGENT_PROGRAM
>  /*!
> @@ -146,7 +147,7 @@ typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* 
> agent);
>   * 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;
> +extern "C" spice::streaming_agent::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 873a69e..ebeef33 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -14,7 +14,7 @@
>  #include "static-plugin.hpp"
>  
>  using namespace std;
> -using namespace SpiceStreamingAgent;
> +using namespace spice::streaming_agent;
>  
>  static inline unsigned MajorVersion(unsigned version)
>  {
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index b5876a5..0a8aeec 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -11,7 +11,8 @@
>  #include 
>  #include 
>  
> -namespace SpiceStreamingAgent {
> +namespace spice {
> +namespace streaming_agent {
>  
>  struct ConcreteConfigureOption: ConfigureOption
>  {
> @@ -42,6 +43,6 @@ private:
>  std::vector options;
>  };
>  
> -}
> +}} // spice::streaming_agent
>  
>  #endif // SPICE_STREAMING_AGENT_CONCRETE_AGENT_HPP
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 7c918a7..10543ad 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -19,7 +19,7 @@
>  #include "jpeg.hpp"
>  
>  using namespace std;
> -using namespace SpiceStreamingAgent;
> +using namespace spice::streaming_agent;
>  
>  #define ERROR(args) do { \
>  std::ostringstream _s; \
> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> index 8044244..a5f1037 100644
> --- a/src/mjpeg-fallback.hpp
> +++ b/src/mjpeg-fallback.hpp
> @@ -10,7 +10,8 @@
>  #include 
>  
>  
> -namespace SpiceStreamingAgent {
> +namespace spice {
> +namespace streaming_agent {
>  
>  struct MjpegSettings
>  {
> @@ -29,6 +30,6 @@ private:
>  MjpegSettings settings = { 10, 80 };
>  };
>  
> -} // namespace SpiceStreamingAgent
> +}} // spice::streaming_agent
>  
>  #endif // SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 94d9d25..0e7641e 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -36,7 +36,7 @@
>  #include "concrete-agent.hpp"
>  
>  using namespace std;
> -using namespace SpiceStreamingAgent;
> +using namespace spice::streaming_agent;
>  
>  static ConcreteAgent agent;
>  
> diff --git a/src/static-plugin.cpp

[Spice-devel] [PATCH v3 05/11] Rephrase section about constants

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

The indent of the rephrasing is that:

- If you have a single constant, use const, e.g. (visible in debugger)
const unsigned max_stuff = 42;

- If you have multiple constants, prefer enums over #define, as
already suggested later in the guide:
enum {
   A = 0,
   B = 42,
   C = -1
}

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index eef4880f..b92b5b00 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -131,7 +131,9 @@ Using goto is allowed in C code for error handling. In any 
other case it will be
 Defining Constant values
 
 
-Use defines for constant values for improving readability and ease of changes. 
Alternatively, use global `const` variables.
+Use defines for constant values for improving readability and ease of changes.
+Alternatively, use global `const` variables for individual values.
+If multiple related constants are to be defined, consider the use of 
enumerations with initializers.
 
 Short functions
 ---
-- 
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 v3 06/11] Rephrase section on short functions for readability

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index b92b5b00..486efbc9 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -138,7 +138,7 @@ If multiple related constants are to be defined, consider 
the use of enumeration
 Short functions
 ---
 
-Try to split code to short functions, each having simple functionality, in 
order to improve code readability and re-usability. Prefix with inline short 
functions or functions that were splitted for readability reason.
+Try to split code to short functions, each having simple functionality, in 
order to improve code readability and re-usability. Prefix with `inline` 
functions that were splitted for readability reason or that are very short.
 
 Return on if
 
-- 
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 v2 05/13] Rephrase section about constants

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 09:48, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> The indent of the rephrasing is that:
>> 
>> - If you have a single constant, use const, e.g. (visible in debugger)
>>const unsigned max_stuff = 42;
>> 
>> - If you have multiple constants, prefer enums over #define, as
>>already suggested later in the guide:
>>enum {
>>   A = 0,
>>   B = 42,
>>   C = -1
>>}
>> 
> 
> The const is a bit different from C and C++ and this results
> in a bit of confusion. max_stuff above is a compile time
> constant in C++, not in C while the enumeration is always
> a compile time constant in both languages.
> So the choice is not only syntax one.
> I think that the "Alternatively, use global `const` variables."
> is quite an ancient comment when spice-server was in C++
> (much before I join and I think even older than the git repository!)
> Unfortunately also C currently can't attach a type to the
> enumeration values so if you want to define in C a constant
> with a type usually you use a define like
> 
> #define MY_CONSTANT ((uint64_t)123)
> 
> You are right, this paragraph needs some care. Maybe the
> style should be different for C and C++ and here we should
> just use enum and #define while having a section for C++
> telling about const ?

Well, I was also considering debuggability.

Until recently, both gdb and lldb were easily confused by enums, whereas const 
variables tend to work better.

Christophe

> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 61cb0701..3e463d2f 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -106,7 +106,9 @@ Using goto is allowed in C code for error handling. In
>> any other case it will be
>> Defining Constant values
>> 
>> 
>> -Use defines for constant values for improving readability and ease of
>> changes. Alternatively, use global `const` variables.
>> +Use defines for constant values for improving readability and ease of
>> changes.
>> +Alternatively, use global `const` variables for individual values.
>> +If multiple related constants are to be defined, consider the use of
>> enumerations with initializers.
>> 
>> Short functions
>> ---
> 
> 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

Re: [Spice-devel] [PATCH spice-server 0/2] Implement cursor for streaming device

2018-02-08 Thread Frediano Ziglio

ping

almost 6 months old now

> 
> These patches was already posted (second was also acked) however first
> one was not complete. Updated reusing some new fields more coherently.
> 
> Frediano Ziglio (2):
>   stream-device: handle cursor from device
>   stream-device: Implement mouse movement
> 
>  server/stream-device.c | 190
>  +++--
>  1 file changed, 183 insertions(+), 7 deletions(-)
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 0/3] Flush interface and TCP_CORK

2018-02-08 Thread Frediano Ziglio

ping

more than 2 years old now

> 
> These patches try to add interface to support flush interface.
> This interface could help improving bandwidth usage reducing bytes
> sent through network.
> The TCP_CORK is one possible usage of the library which actually
> decrease bandwidth (not using SSL) by a 11%.
> 
> Frediano Ziglio (3):
>   stream: implement interface for manual flush
>   stream: implements flush using TCP_CORK
>   common-graphics-channel: use manual flushing on stream to decrease
> packet fragmentation
> 
>  server/common-graphics-channel.c | 17 +
>  server/red-channel-client.c  |  1 +
>  server/red-stream.c  | 41
>  
>  server/red-stream.h  | 20 
>  4 files changed, 71 insertions(+), 8 deletions(-)
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 06/13] Rephrase section on short functions for readability

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 09:50, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 3e463d2f..74f4e29d 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -113,7 +113,7 @@ If multiple related constants are to be defined, consider
>> the use of enumeration
>> Short functions
>> ---
>> 
>> -Try to split code to short functions, each having simple functionality, in
>> order to improve code readability and re-usability. Prefix with inline short
>> functions or functions that were splitted for readability reason.
>> +Try to split code to short functions, each having simple functionality, in
>> order to improve code readability and re-usability. Prefix with `inline`
>> functions that were splitted for readability reason or that are very short.
>> 
>> Return on if
>> 
> 
> I really don't understanding the aiming of both version.
> Is mixing the inline concept (optimization suggestion) with readability
> and I don't understand the reason.

OK. Needs rephrasing then. I agree I did not improve much. Will do one more 
pass.

> 
> 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 spice-gtk 4/4] tests: add spice+tls:// tests

2018-02-08 Thread marcandre . lureau

From: Marc-André Lureau 

They couldn't not be introduced before, because the test needs both
parsing and generation.

Signed-off-by: Marc-André Lureau 
---
 tests/session.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/tests/session.c b/tests/session.c
index fc874fc..ae2a22c 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -111,6 +111,17 @@ static void test_session_uri_bad(void)
 "*assertion 's->port != NULL || s->tls_port != NULL' 
failed",
 },
 }
+},{
+"spice+tls://hostname?tls-port=1234=3456",
+{
+{
+G_LOG_LEVEL_WARNING,
+"spice+tls:// scheme doesn't support query string",
+},{
+G_LOG_LEVEL_CRITICAL,
+"*assertion 's->port != NULL || s->tls_port != NULL' 
failed",
+},
+}
 },
 };
 
@@ -233,6 +244,9 @@ static void test_session_uri_ipv4_good(void)
   NULL, NULL,
   "spice://127.0.0.1:42?tls-port=5930",
   "spice://127.0.0.1?port=42=5930" },
+{ .uri_input  = "spice+tls://hostname:39",
+  .host = "hostname",
+  .tls_port = "39" }
 };
 
 test_session_uri_good(tests, G_N_ELEMENTS(tests));
-- 
2.16.1.73.g5832b7e9f2

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

[Spice-devel] [PATCH spice-gtk 0/4] Add spice+tls:// uri form

2018-02-08 Thread marcandre . lureau

From: Marc-André Lureau 

Hi,

Here are a few patches related to spice schemes URI handling. They
introduce a spice+tls:// form (see related man page update and patch
for details).

cheers

Marc-André Lureau (4):
  tests: add spice+unix:// URI checks
  uri: learn to parse spice+tls:// form
  uri: generate spice://host:port or spice+tls://host:port
  tests: add spice+tls:// tests

 man/spice-client.pod | 29 ++--
 src/spice-session.c  | 54 +++--
 tests/session.c  | 62 +---
 3 files changed, 108 insertions(+), 37 deletions(-)

-- 
2.16.1.73.g5832b7e9f2

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

[Spice-devel] [PATCH spice-gtk 2/4] uri: learn to parse spice+tls:// form

2018-02-08 Thread marcandre . lureau

From: Marc-André Lureau 

spice:// has a weird scheme encoding, where it can accept both plain
and tls ports with URI query parameters. However, it's not very
convenient nor very common to use (who really want to mix plain & tls
channels?).

Instead, let's introduce the more readable form spice+tls://host:port

This form will not accept query string, thus mixing plain and tls is
not possible (it would be confusing to have ?port= for plain), nor
passing password and such.

Signed-off-by: Marc-André Lureau 
---
 man/spice-client.pod | 29 +
 src/spice-session.c  | 23 +++
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/man/spice-client.pod b/man/spice-client.pod
index 7288b84..4b23c7d 100644
--- a/man/spice-client.pod
+++ b/man/spice-client.pod
@@ -12,23 +12,24 @@ can be used to tweak some SPICE-specific option.
 
 =head1 URI
 
-The most basic SPICE URI which can be used is in the form
+To initiate a plain SPICE connection (the connection will be
+unencrypted) to hostname.example.com and port 5900, use the following
+URI:
+
   spice://hostname.example.com:5900
 
-This will try to initiate a SPICE connection to hostname.example.com
-to port 5900. This connection will be unencrypted. This URI is
-equivalent to
-  spice://hostname.example.com?port=5900
+In order to start a TLS connection, one would use:
 
-In order to start a TLS connection, one would use
-  spice://hostname.example.com?tls-port=5900
+  spice+tls://hostname.example.com:5900
 
-Other valid URI parameters are 'username' and 'password'. Be careful that
-passing a password through a SPICE URI might cause the password to be
-visible by any local user through 'ps'.
+Note: this form is available since v0.35, you have to use the spice://
+query string with the tls-port parameter before that.
+
+=head1 spice:// URI query string
+
+spice:// URI accepts query string. Several parameters can be specified
+at once if they are separated by & or ;
 
-Several parameters can be specified at once if they are separated
-by & or ;
   spice://hostname.example.com?port=5900;tls-port=5901
 
 When using 'tls-port', it's recommended to not specify any non-TLS port.
@@ -39,6 +40,10 @@ then try to use the TLS port. This means a man-in-the-middle 
could force
 the whole SPICE session to go in clear text regardless of the TLS settings
 of the SPICE server.
 
+Other valid URI parameters are 'username' and 'password'. Be careful that
+passing a password through a SPICE URI might cause the password to be
+visible by any local user through 'ps'.
+
 =head1 OPTIONS
 
 The following options are accepted when running a SPICE client which
diff --git a/src/spice-session.c b/src/spice-session.c
index 2aabf58..7218449 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -389,6 +389,7 @@ spice_session_finalize(GObject *gobject)
 
 #define URI_SCHEME_SPICE "spice://"
 #define URI_SCHEME_SPICE_UNIX "spice+unix://"
+#define URI_SCHEME_SPICE_TLS "spice+tls://"
 #define URI_QUERY_START ";?"
 #define URI_QUERY_SEP   ";&"
 
@@ -425,6 +426,7 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 gchar *authority = NULL;
 gchar *query = NULL;
 gchar *tmp = NULL;
+bool tls_scheme = false;
 
 g_return_val_if_fail(original_uri != NULL, -1);
 
@@ -438,12 +440,16 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 /* Break up the URI into its various parts, scheme, authority,
  * path (ignored) and query
  */
-if (!g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
+if (g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
+authority = uri + strlen(URI_SCHEME_SPICE);
+} else if (g_str_has_prefix(uri, URI_SCHEME_SPICE_TLS)) {
+authority = uri + strlen(URI_SCHEME_SPICE_TLS);
+tls_scheme = true;
+} else {
 g_warning("Expected a URI scheme of '%s' in URI '%s'",
   URI_SCHEME_SPICE, uri);
 goto fail;
 }
-authority = uri + strlen(URI_SCHEME_SPICE);
 
 tmp = strchr(authority, '@');
 if (tmp) {
@@ -502,6 +508,11 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 }
 path = NULL;
 
+if (tls_scheme && query[0] != '\0') {
+g_warning(URI_SCHEME_SPICE_TLS " scheme doesn't support query string");
+goto fail;
+}
+
 while (query && query[0] != '\0') {
 gchar key[32], value[128];
 gchar **target_key;
@@ -568,8 +579,12 @@ end:
 s->unix_path = g_strdup(path);
 g_free(uri);
 s->host = host;
-s->port = port;
-s->tls_port = tls_port;
+if (tls_scheme) {
+s->tls_port = port;
+} else {
+s->port = port;
+s->tls_port = tls_port;
+}
 s->username = username;
 s->password = password;
 return 0;
-- 
2.16.1.73.g5832b7e9f2

___
Spice-devel mailing list

[Spice-devel] [PATCH spice-gtk 3/4] uri: generate spice://host:port or spice+tls://host:port

2018-02-08 Thread marcandre . lureau

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 src/spice-session.c | 31 +++
 tests/session.c | 20 ++--
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/src/spice-session.c b/src/spice-session.c
index 7218449..b2517e6 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -400,17 +400,32 @@ static gchar* spice_uri_create(SpiceSession *session)
 if (s->unix_path != NULL) {
 return g_strdup_printf(URI_SCHEME_SPICE_UNIX "%s", s->unix_path);
 } else if (s->host != NULL) {
+GString *str;
 g_return_val_if_fail(s->port != NULL || s->tls_port != NULL, NULL);
 
-GString *str = g_string_new(URI_SCHEME_SPICE);
+if (!!s->tls_port + !!s->port == 1) {
+/* use spice://foo:4390 or spice+tls://.. form */
+const char *port;
 
-g_string_append(str, s->host);
-g_string_append(str, "?");
-if (s->port != NULL) {
-g_string_append_printf(str, "port=%s&", s->port);
-}
-if (s->tls_port != NULL) {
-g_string_append_printf(str, "tls-port=%s", s->tls_port);
+if (s->tls_port) {
+str = g_string_new(URI_SCHEME_SPICE_TLS);
+port = s->tls_port;
+} else {
+str = g_string_new(URI_SCHEME_SPICE);
+port = s->port;
+}
+g_string_append_printf(str, "%s:%s", s->host, port);
+} else {
+/* use spice://foo?port=4390= form */
+str = g_string_new(URI_SCHEME_SPICE);
+g_string_append(str, s->host);
+g_string_append(str, "?");
+if (s->port != NULL) {
+g_string_append_printf(str, "port=%s&", s->port);
+}
+if (s->tls_port != NULL) {
+g_string_append_printf(str, "tls-port=%s", s->tls_port);
+}
 }
 return g_string_free(str, FALSE);
 }
diff --git a/tests/session.c b/tests/session.c
index 413d812..fc874fc 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -201,28 +201,28 @@ static void test_session_uri_ipv4_good(void)
   "localhost",
   NULL, NULL,
   "spice://localhost?port=5900=",
-  "spice://localhost?port=5900&" },
+  "spice://localhost:5900" },
 { "5910", NULL,
   "localhost",
   "user", NULL,
   "spice://user@localhost?tls-port==5910",
-  "spice://localhost?port=5910&" },
+  "spice://localhost:5910" },
 { NULL, "5920",
   "localhost",
   "user", "password",
   "spice://user@localhost?tls-port=5920==password",
-  "spice://localhost?tls-port=5920",
+  "spice+tls://localhost:5920",
   "password may be visible in process listings"},
 { NULL, "5930",
   "localhost",
   NULL, NULL,
   "spice://localhost?port==5930",
-  "spice://localhost?tls-port=5930" },
+  "spice+tls://localhost:5930" },
 { "42", NULL,
   "localhost",
   NULL, NULL,
   "spice://localhost:42",
-  "spice://localhost?port=42&" },
+  "spice://localhost:42" },
 { "42", "5930",
   "localhost",
   NULL, NULL,
@@ -246,28 +246,28 @@ static void test_session_uri_ipv6_good(void)
   "[2010:836B:4179::836B:4179]",
   NULL, NULL,
   "spice://[2010:836B:4179::836B:4179]?port=5900=",
-  "spice://[2010:836B:4179::836B:4179]?port=5900&" },
+  "spice://[2010:836B:4179::836B:4179]:5900" },
 { "5910", NULL,
   "[::192.9.5.5]",
   "user", NULL,
   "spice://user@[::192.9.5.5]?tls-port==5910",
-  "spice://[::192.9.5.5]?port=5910&" },
+  "spice://[::192.9.5.5]:5910" },
 { NULL, "5920",
   "[3ffe:2a00:100:7031::1]",
   "user", "password",
   
"spice://user@[3ffe:2a00:100:7031::1]?tls-port=5920==password",
-  "spice://[3ffe:2a00:100:7031::1]?tls-port=5920",
+  "spice+tls://[3ffe:2a00:100:7031::1]:5920",
   "password may be visible in process listings"},
 { NULL, "5930",
   "[1080:0:0:0:8:800:200C:417A]",
   NULL, NULL,
   "spice://[1080:0:0:0:8:800:200C:417A]?port==5930",
-  "spice://[1080:0:0:0:8:800:200C:417A]?tls-port=5930" },
+  "spice+tls://[1080:0:0:0:8:800:200C:417A]:5930" },
 { "42", NULL,
   "[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]",
   NULL, NULL,
   "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:42",
-  "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]?port=42&" },
+  "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:42" },
 { "42", "5930",
   "[::192.9.5.5]",
   NULL, NULL,
-- 
2.16.1.73.g5832b7e9f2

[Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

2018-02-08 Thread marcandre . lureau

From: Marc-André Lureau 

For some reason, the URIs test didn't include spice+unix:// checks,
probably because they came about the same time.

Signed-off-by: Marc-André Lureau 
---
 tests/session.c | 28 +---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/tests/session.c b/tests/session.c
index 7ed4a41..413d812 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -9,6 +9,7 @@ typedef struct {
 const gchar *uri_input;
 const gchar *uri_output;
 const gchar *message;
+const gchar *unix_path;
 } TestCase;
 
 static void test_session_uri_bad(void)
@@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase *tests, 
const guint cases)
 
 /* Set URI and check URI, port and tls_port */
 for (i = 0; i < cases; i++) {
-gchar *uri, *port, *tls_port, *host, *username, *password;
+gchar *uri, *port, *tls_port, *host, *username, *password, *unix_path;
 
 s = spice_session_new();
 if (tests[i].message != NULL)
@@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase *tests, 
const guint cases)
  "host", ,
  "username", ,
  "password", ,
+ "unix-path", _path,
   NULL);
-g_assert_cmpstr(tests[i].uri_output, ==, uri);
+g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==, uri);
 g_assert_cmpstr(tests[i].port, ==, port);
 g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
 g_assert_cmpstr(tests[i].host, ==, host);
 g_assert_cmpstr(tests[i].username, ==, username);
 g_assert_cmpstr(tests[i].password, ==, password);
 g_test_assert_expected_messages();
+g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
 g_clear_pointer(, g_free);
 g_clear_pointer(, g_free);
 g_clear_pointer(_port, g_free);
 g_clear_pointer(, g_free);
 g_clear_pointer(, g_free);
 g_clear_pointer(, g_free);
+g_clear_pointer(_path, g_free);
 g_object_unref(s);
 }
 
@@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase *tests, 
const guint cases)
  "host", tests[i].host,
  "username", tests[i].username,
  "password", tests[i].password,
+ "unix-path", tests[i].unix_path,
   NULL);
 g_object_get(s, "uri", , NULL);
-g_assert_cmpstr(tests[i].uri_output, ==, uri);
+g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==, uri);
 g_clear_pointer(, g_free);
 g_object_unref(s);
 }
@@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
 test_session_uri_good(tests, G_N_ELEMENTS(tests));
 }
 
+static void test_session_uri_unix_good(void)
+{
+const TestCase tests[] = {
+{ .uri_input = "spice+unix:///tmp/foo.sock",
+  .unix_path = "/tmp/foo.sock" },
+/* perhaps not very clever, but this doesn't raise an error/warning */
+{ .uri_input = "spice+unix://",
+  .unix_path = "" },
+/* unix uri don't support passing password or other kind of options */
+{ .uri_input = "spice+unix:///tmp/foo.sock?password=frobnicate",
+  .unix_path = "/tmp/foo.sock?password=frobnicate" },
+};
+
+test_session_uri_good(tests, G_N_ELEMENTS(tests));
+}
+
 int main(int argc, char* argv[])
 {
 g_test_init(, , NULL);
@@ -285,6 +306,7 @@ int main(int argc, char* argv[])
 g_test_add_func("/session/bad-uri", test_session_uri_bad);
 g_test_add_func("/session/good-ipv4-uri", test_session_uri_ipv4_good);
 g_test_add_func("/session/good-ipv6-uri", test_session_uri_ipv6_good);
+g_test_add_func("/session/good-unix", test_session_uri_unix_good);
 
 return g_test_run();
 }
-- 
2.16.1.73.g5832b7e9f2

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

[Spice-devel] [PATCH v3 02/11] Added External References section

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 20 
 1 file changed, 20 insertions(+)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index f5d13642..10bfbc9a 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -6,6 +6,26 @@ Licensed under a Creative Commons Attribution-Share Alike 3.0
 United States License (see 
http://creativecommons.org/licenses/by-sa/3.0/us/legalcode).
 
 
+External references
+---
+
+In general, unless otherwise noted here (e.g. the use of tabs),
+
+- For C code, SPICE follows the Linux kernel coding conventions as documented 
here: https://www.kernel.org/doc/html/v4.10/process/coding-style.html.
+  Notable deviations from the Linux coding style include:
+  + The use of 4 spaces for indentation instead of tabs
+  + The use of typedefs for structs not considered as a mistake
+  + The use of CamelCase for struct and class names
+
+- For C++ code, SPICE follows the LLVM coding standard 
(https://llvm.org/docs/CodingStandards.html).
+  Notable deviations from the LLVM coding style include:
+  + The format of header comments
+  + The placement of braces after functions and classes (follows the Linux 
style)
+
+In addition, for C++, developers should be aware of the C++ Core Guidelines 
and consider them as best practice unless otherwise agreed on by the team
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md
+
+
 Source Files
 
 
-- 
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 v3 09/11] Add mention of header guards

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index eb2ee252..ae91f987 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -385,6 +385,25 @@ char *array[] = {
 "item_3",
 };
 
+Headers
+---
+
+Headers should be protected against multiple inclusion using a macro that 
contains the header file name in uppercase, with all characters that are 
invalid in C replaced with an underscore '_':
+
+[source,c]
+---
+#ifndef MY_MODULE_H
+#define MY_MODULE_H
+
+...
+
+#endif // MY_MODULE_H
+---
+
+The macro may include additional information, e.g. a component. For example a 
file generally referenced as foo/bar.h could use a FOO_BAR_H macro.
+
+Historically, some headers added underscores liberally, e.g. MY_MODULE_H_. 
This is neither necessary nor discouraged, although as a reminder, a leading 
underscore followed by a capital letter is reserved for the implementation and 
should not be used, so _MY_MODULE_H is, technically speaking, invalid C.
+
 Header inclusion
 
 
-- 
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 v3 00/11] Updates to the style guide

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Changes in v3:

- Integrated clarification regarding headers
- Added external references as a starting point
- Changed [source,h] annotations (unsure if that's accepted)
- Changed brace placement from Allman to Linux in .clang-format

Changes in v2:

- This splits the earlier patch about the style guide into its individual
  components. This also clarifies the intent of some of the changes in the
  commit messages.

- Fixed typos

Christophe de Dinechin (11):
  Add .clang-format with defaults matching what's specified in the style
guide
  Added External References section
  Specify file extensions for C++ and Obj-C
  Rephrase assertion section
  Rephrase section about constants
  Rephrase section on short functions for readability
  Remove confusing example
  Add mention of fall-through comments
  Add mention of header guards
  Add guidelines about warnings and whitespaces
  Rewrite the style guide for headers

 .clang-format|  23 +
 docs/spice_style.txt | 139 ---
 2 files changed, 132 insertions(+), 30 deletions(-)
 create mode 100644 .clang-format

-- 
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 v3 04/11] Rephrase assertion section

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 13032df6..eef4880f 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -99,10 +99,19 @@ FIXME and TODO
 
 Comments that are prefixed with `FIXME` describe a bug that need to be fixed. 
Generally, it is not allowed to commit new code having `FIXME` comment. 
Committing  `FIXME` is allowed only for existing bugs. Comments that are 
prefixed with `TODO` describe further features, optimization or code 
improvements, and they are allowed to be committed along with the relevant code.
 
-ASSERT
---
+Assertions
+--
+
+Use assertions liberally. Assertions help testing function arguments and 
function results validity. As a result, they make it easier to detect bugs. 
Also, by expressing the intent of the developer, they make the code easier to 
read.
+
+Several form of assertion exist. SPICE does not use the language `assert` 
much, but instead relies on the following variants:
+
+- `spice_assert`, defined in `common/log.h`, is never disabled at run-time and 
prints an error if the assertion fails.
+
+- `g_assert` and other variants defined in glib such as `g_assert_null`, can 
be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice for 
SPICE code), and emits a message through the g_assertion_message function and 
its variants.
+
+The guidelines for selecting one or the other are not very clear from the 
existing code. The `spice_assert` variant may be preferred for SPICE-only code, 
as it makes it clearer that this assertion is coming from SPICE. The `g_assert` 
and variants may be preferred for assertions related to the use of glib.
 
-Use it freely. ASSERT helps testing function arguments and function results 
validity.  ASSERT helps detecting bugs and improve code readability and 
stability.
 
 sizeof
 --
-- 
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 v3 10/11] Add guidelines about warnings and whitespaces

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

The objective of these guidelines is that:
- We avoid introducing new warnings
- We know how to fix old ones
- We don't have to isolate whitespace changes when submitting patches,
  i.e. someone who use tools that automatically strip whitespaces and
  therefore "repairs" earlier errors should not be punished for it.

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index ae91f987..108a57a5 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -436,3 +436,12 @@ Also in source (no header) files you must include 
`config.h` at the beginning so
 
 #include "spice_server.h"
 
+
+
+Compilation
+---
+
+The source code should compile without warnings on all variants of GCC and 
clang available.
+A patch may be rejected if it introduces new warnings.
+Warnings that appear over time due to improvements in compilers should be 
fixed in dedicated patches. A patch should not mix warning fixes and other 
changes.
+Any patch may adjust whitespace (e.g. eliminate trailing whitespace). 
Whitespace adjustments do not require specific patches.
-- 
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 v3 11/11] Rewrite the style guide for headers

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

As written, the headers style guide looks quite wrong. In particular,
it places headers in an order that makes it hard to detect hidden
dependencies in SPICE headers.

These rules can be enforced by the .clang-format proposed in earlier patch,
locally if you use the Emacs clang-format.el integration supplied with LLVM.

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 44 +---
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 108a57a5..ff505b2a 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -407,34 +407,48 @@ Historically, some headers added underscores liberally, 
e.g. MY_MODULE_H_. This
 Header inclusion
 
 
-Headers should be included in this order
+Headers should be included in this order:
+- config.h, which should only be included from C source files
+- [module].h, where [module].c is the corresponding implementation file
+- [module]-xyz.h, which are support headers for [module]
+- Other application headers, using #include "file.h"
+- System headers, using #include 
+- If necessary, C++ system headers, using #include 
+
+This order is designed to maximize chances of catching missing headers in 
headers (i.e. headers that are not self-contained).
+
+In summary, Headers should be included in this order
 
 [source,c]
 
-#include 
-#include 
+#include "config.h"
+#include "source.h"
+#include "source-support.h"
+#include "some-other-source.h"
+
 #include 
 #include 
-
-#include "spice_server.h"
+#include 
+#include 
+#include 
+#include 
 
 
-(note the empty line between no spice-server and spice-server headers)
+(note the empty line between application headers included with "" and system 
headers included with <>
 
-Also in source (no header) files you must include `config.h` at the beginning 
so should start (beside comments and copyright) with
+Headers should include only the headers required to process the header itself, 
and otherwise include as little as possible.
 
 [source,c]
 
-#ifdef HAVE_CONFIG_H
-#include 
-#endif
+#ifndef SOURCE_H
+#define SOURCE_H
+#include "application-header-required-for-header.h"
 
-#include 
-#include 
-#include 
-#include 
+#include 
+
+...
 
-#include "spice_server.h"
+#endif /* SOURCE_H */
 
 
 
-- 
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 v3 07/11] Remove confusing example

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

The sentence explaining that example makes no real sense, and
the coding style suggestion is horrendous (not to mention flies in the
face of all automatic indentation tools)

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 486efbc9..68dbeeef 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -279,15 +279,7 @@ if (condition) {
 +
 In case of long condition statement, prefer having additional temporary 
variable over multiple line condition statement.
 +
-In case of new line in condition statement.
-+
-[source,c]
-
-if (long_name && very_long_name && very_long ||
-   var_name) {
-
-+
-or indent under the round bracket using spaces
+Indent long conditionals under the opening parenthesis using spaces
 +
 [source,c]
 
-- 
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 v3 03/11] Specify file extensions for C++ and Obj-C

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 10bfbc9a..13032df6 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -34,7 +34,16 @@ Names
 
 Use lower case and separate words using dashes (e.g., file-name.c, header.h).
 
-Use standard file extension for C source and header files.
+The file extensions used in the SPICE project are:
+- .c for C source
+- .cpp for C++ sources
+- .h for headers that can be included from C code
+- .hpp for headers that are strictly reserved to C++
+- .m for Objective-C source files (currently not properly enforced)
+
+Note that .h headers may contain C++ code as long as the header can
+sucessfully be included from a C source file.
+
 
 Line width
 ~~
-- 
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 v3 08/11] Add mention of fall-through comments

2018-02-08 Thread Christophe de Dinechin

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 docs/spice_style.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 68dbeeef..eb2ee252 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -128,6 +128,11 @@ goto
 
 Using goto is allowed in C code for error handling. In any other case it will 
be used only as a special exception.
 
+switch fall-through
+---
+
+If a switch case falls through (i.e. does not end with a `break`), annotate it 
with a /* Fall through */ comment. Some compilers will emit a warning otherwise.
+
 Defining Constant values
 
 
-- 
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] style: Update style to include some C++ element

2018-02-08 Thread Christophe de Dinechin



> On 8 Feb 2018, at 14:53, Lukáš Hrázký  wrote:
> 
> On Wed, 2018-02-07 at 10:10 +0100, Christophe de Dinechin wrote:
>> Frediano Ziglio writes:
>> 
>>> These style are used by other SPICE projects like spice-streaming-agent.
> 
> "This style is used ..."
> 
>>> See discussion "Coding style and naming conventions for C++" at
>>> https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.
> 
> I'm not sure if referring to ML discussion is a good idea, but if you
> think so, not objecting... :)
> 
>>> Signed-off-by: Frediano Ziglio 
>>> ---
>>> docs/spice_style.txt | 38 +-
>>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>>> index eb0e30ef..0e0028e9 100644
>>> --- a/docs/spice_style.txt
>>> +++ b/docs/spice_style.txt
>>> @@ -1,7 +1,7 @@
>>> Spice project coding style and coding conventions
>>> =
>>> 
>>> -Copyright (C) 2009-2016 Red Hat, Inc.
>>> +Copyright (C) 2009-2018 Red Hat, Inc.
>>> Licensed under a Creative Commons Attribution-Share Alike 3.0
>>> United States License (see 
>>> http://creativecommons.org/licenses/by-sa/3.0/us/legalcode).
>>> 
>>> @@ -379,3 +379,39 @@ Also in source (no header) files you must include 
>>> `config.h` at the beginning so
>>> 
>>> #include "spice_server.h"
>> 
>> (Not in your patch) Re-reading the part about headers, it has everything
>> completely backwards! I suggest we re-discuss this.
>> 
>>> 
>>> +
>>> +C++
>>> +---
>>> +C++ follows C style if not specified otherwise.
>> 
>> I would add
>> 
>> The C++11 dialect is assumed by default. No attempts will be made to
>> restrict the code to older variants of C++. Using constructs allowed in
>> later variants of C++ will be allowed either:
>> 
>> - After the default C++ compiler for building the oldest supported RHEL
>>  version fully supports the corresponding variant of C++
>> 
>> - Under the appropriate preprocessor conditions, if the previous
>>  condition is not met and C++11 alternatives would have significant
>>  drawbacks
>> 
>> 
>>> +
>>> +Method names
>>> +
>>> +
>>> +Method names should use lower case and separate words with
>>> +underscores.
>> 
>> What about classes? I see below that you selected camelcase for classes,
>> but that is inconsistent with standard C++ classes such as 'string', or
>> with most types in C (e.g. ptrdiff_t, struct stat, etc)
>> 
>> Since it seems that consistency with C and with the standard C++ library
>> was the primary rationale for using snake-case, I would go for
>> snake-case for classes as well.
>> 
>> (Frankly, I really could not care less, I'm just trying to be consistent)
>> 
>>> +
>>> +
>>> +Namespaces
>>> +~~
>>> +
>>> +Namespaces should use lower case and separate words with underscores.
>>> +Namespace blocks should not increase indentation.
>>> +Namespaces can be nested and closure can be collapsed but for
>> 
>> I would not use the word "closure" here, which has a specific meaning in
>> computer science (as in lambda variable capture). What about:
>> 
>> Namespaces can be nested. Namespace closing brackets for nested
>> namespaces can be placed together on the same line, but for readability
>> reasons the closure should specify the namespace with a comment.
> 
> Agreed.
> 
>>> +readability reasons the closure should specify the namespace with a
>>> +comment.
>>> +
>>> +[source,cpp]
>>> +
>>> +namespace spice {
>>> +namespace streaming_agent {
>> 
>> There is a style inconsistency regarding the placement of the opening
>> brace relative to other first-column opening braces (function and
>> class). So I would suggest that all top-level opening braces be on a
>> line of their own, as for 'class' below and for functions.
> 
> I'm not a fan, but if you guys decided you want to waste some lines on
> lonely opening braces :), please amend the patch.

I’m more concerned about being able to clang-format a region than about 
discussing brace placement.
The “Linux” placement style on this:

namespace a {
namespace b {

class foo {
class bar {
foo() {
if (a) { b; c; }
if (b) {
c();
d();
e();
}
}
};
};

}}

will give that:

namespace a
{
namespace b
{

class foo
{
class bar
{
foo()
{
if (a) {
b;
c;
}
if (b) {
c();
d();
e();
}
}
};
};
}
}

Don’t break automated tools gratuitously. If you know how to get clang-format 
to do what you want, I don’t care.


> 
>>> +
>>> +class ClassInsideNamespace
>>> +{
>>> +...
>>> +};
>>> +
>>> +}} // namespace spice::streaming_agent
>>> +
>>> +
>>> +You should not import an entire namespace but use qualified names
>>> +instead.
>> 
>> The C++

Re: [Spice-devel] [PATCH spice-server v3] style: Update style to include some C++ element

2018-02-08 Thread Frediano Ziglio

> 
> > On 8 Feb 2018, at 15:16, Lukáš Hrázký  wrote:
> > 
> > On Thu, 2018-02-08 at 14:03 +, Frediano Ziglio wrote:
> >> This style is used by other SPICE projects like spice-streaming-agent.
> >> See discussion "Coding style and naming conventions for C++" at
> >> https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.
> >> 
> >> Signed-off-by: Frediano Ziglio 
> >> ---
> >> docs/spice_style.txt | 52
> >> 
> >> 1 file changed, 52 insertions(+)
> >> 
> >> Changes in v3:
> >> - fix commit message grammar.
> >> 
> >> Changes in v2:
> >> - more details for namespace usages;
> >> - specify C++ dialect;
> >> - fix class indentation.
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index f5d13642..29c1756f 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -380,3 +380,55 @@ Also in source (no header) files you must include
> >> `config.h` at the beginning so
> >> 
> >> #include "spice_server.h"
> >> 
> >> +
> >> +C++
> >> +---
> >> +C\++ follows C style if not specified otherwise.
> > 
> > Sorry, one more grammar improvement:
> > "C\++ style follows the C style"
> > 
> >> +The C+\+11 dialect is assumed by default. No attempts will be made to
> >> +restrict the code to older variants of C+\+.
> >> +For compatibility reasons don't use more recent C++ dialects.
> 
> I notice you have C++, C\++ and C+\+ within a few lines from one another. Not
> sure why. I thought you needed to escape + only if at the beginning of a
> line.
> 

I though too... but for some reasons code kept doing weird stuff, looks
like the ++ is also some kind of code block quotation so something like

test ++code whatever++ end

is formatting the "code whatever" using monospace fonts.

But if you quote any of the + in the single "C++" (header) for
some reasons the backslash is taken verbatim!

> 
> >> +
> >> +Method names
> >> +
> >> +
> >> +Method names should use lower case and separate words with
> >> +underscores.
> >> +
> >> +
> >> +Namespaces
> >> +~~
> >> +
> >> +Namespaces should use lower case and separate words with underscores.
> >> +Namespace blocks should not increase indentation.
> >> +Namespaces can be nested. Namespace closing brackets for nested
> >> +namespaces can be placed together on the same line, but for
> >> +readability reasons the closure should specify the namespace with a
> >> +comment.
> >> +
> >> +[source,cpp]
> >> +
> >> +namespace spice {
> >> +namespace streaming_agent {
> >> +
> >> +class ClassInsideNamespace {
> >> +...
> >> +};
> >> +
> >> +}} // namespace spice::streaming_agent
> >> +
> >> +
> >> +The `using namespace` construct should never be used in headers. It
> >> should
> >> +be used sparingly in source files, and only within the body of short
> >> +functions.
> >> +
> >> +Preferred alternatives to `using namespace` include:
> >> +
> >> +* using declarations
> >> ++
> >> +[source,cpp]
> >> +using spice::streaming_agent::some_class;
> >> ++
> >> +* namespace aliases
> >> ++
> >> +[source,cpp]
> >> +namespace ssa = spice::streaming_agent;
> > 
> > Let's get it in, further improvements welcome.
> > 
> > Acked-by: Lukáš Hrázký 

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

Re: [Spice-devel] [PATCH spice-server] style: Update style to include some C++ element

2018-02-08 Thread Lukáš Hrázký

On Thu, 2018-02-08 at 15:33 +0100, Christophe de Dinechin wrote:
> > On 8 Feb 2018, at 14:53, Lukáš Hrázký  wrote:
> > 
> > On Wed, 2018-02-07 at 10:10 +0100, Christophe de Dinechin wrote:
> > > Frediano Ziglio writes:
> > > 
> > > > These style are used by other SPICE projects like spice-streaming-agent.
> > 
> > "This style is used ..."
> > 
> > > > See discussion "Coding style and naming conventions for C++" at
> > > > https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.
> > 
> > I'm not sure if referring to ML discussion is a good idea, but if you
> > think so, not objecting... :)
> > 
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > > docs/spice_style.txt | 38 +-
> > > > 1 file changed, 37 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > > > index eb0e30ef..0e0028e9 100644
> > > > --- a/docs/spice_style.txt
> > > > +++ b/docs/spice_style.txt
> > > > @@ -1,7 +1,7 @@
> > > > Spice project coding style and coding conventions
> > > > =
> > > > 
> > > > -Copyright (C) 2009-2016 Red Hat, Inc.
> > > > +Copyright (C) 2009-2018 Red Hat, Inc.
> > > > Licensed under a Creative Commons Attribution-Share Alike 3.0
> > > > United States License (see 
> > > > http://creativecommons.org/licenses/by-sa/3.0/us/legalcode).
> > > > 
> > > > @@ -379,3 +379,39 @@ Also in source (no header) files you must include 
> > > > `config.h` at the beginning so
> > > > 
> > > > #include "spice_server.h"
> > > 
> > > (Not in your patch) Re-reading the part about headers, it has everything
> > > completely backwards! I suggest we re-discuss this.
> > > 
> > > > 
> > > > +
> > > > +C++
> > > > +---
> > > > +C++ follows C style if not specified otherwise.
> > > 
> > > I would add
> > > 
> > > The C++11 dialect is assumed by default. No attempts will be made to
> > > restrict the code to older variants of C++. Using constructs allowed in
> > > later variants of C++ will be allowed either:
> > > 
> > > - After the default C++ compiler for building the oldest supported RHEL
> > >  version fully supports the corresponding variant of C++
> > > 
> > > - Under the appropriate preprocessor conditions, if the previous
> > >  condition is not met and C++11 alternatives would have significant
> > >  drawbacks
> > > 
> > > 
> > > > +
> > > > +Method names
> > > > +
> > > > +
> > > > +Method names should use lower case and separate words with
> > > > +underscores.
> > > 
> > > What about classes? I see below that you selected camelcase for classes,
> > > but that is inconsistent with standard C++ classes such as 'string', or
> > > with most types in C (e.g. ptrdiff_t, struct stat, etc)
> > > 
> > > Since it seems that consistency with C and with the standard C++ library
> > > was the primary rationale for using snake-case, I would go for
> > > snake-case for classes as well.
> > > 
> > > (Frankly, I really could not care less, I'm just trying to be consistent)
> > > 
> > > > +
> > > > +
> > > > +Namespaces
> > > > +~~
> > > > +
> > > > +Namespaces should use lower case and separate words with underscores.
> > > > +Namespace blocks should not increase indentation.
> > > > +Namespaces can be nested and closure can be collapsed but for
> > > 
> > > I would not use the word "closure" here, which has a specific meaning in
> > > computer science (as in lambda variable capture). What about:
> > > 
> > > Namespaces can be nested. Namespace closing brackets for nested
> > > namespaces can be placed together on the same line, but for readability
> > > reasons the closure should specify the namespace with a comment.
> > 
> > Agreed.
> > 
> > > > +readability reasons the closure should specify the namespace with a
> > > > +comment.
> > > > +
> > > > +[source,cpp]
> > > > +
> > > > +namespace spice {
> > > > +namespace streaming_agent {
> > > 
> > > There is a style inconsistency regarding the placement of the opening
> > > brace relative to other first-column opening braces (function and
> > > class). So I would suggest that all top-level opening braces be on a
> > > line of their own, as for 'class' below and for functions.
> > 
> > I'm not a fan, but if you guys decided you want to waste some lines on
> > lonely opening braces :), please amend the patch.
> 
> I’m more concerned about being able to clang-format a region than about 
> discussing brace placement.
> The “Linux” placement style on this:
> 
> namespace a {
> namespace b {
> 
> class foo {
> class bar {
> foo() {
> if (a) { b; c; }
> if (b) {
> c();
> d();
> e();
> }
> }
> };
> };
> 
> }}
> 
> will give that:
> 
> namespace a
> {
> namespace b
> {
> 
> class foo
> {
> class bar
> {
> foo()
> {
> if (a) {
>

59 matches

Mail list logo