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

[Lukáš Hrázký]

On Mon, 2018-02-12 at 03:53 -0500, Frediano Ziglio wrote:
> > 
> > Create an AgentRunner (TODO: needs a better name) class to encapsulate
> > the streaming and communication with the server. The basic setup (cmd
> 
> Also encapsulate command line interface (quite odd, does it make still
> much sense?)

Well, yes.. if I understand what you mean, that is the other half that
is separated... Want me to mention it explicitly in the commit message?

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

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

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