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

[Christophe de Dinechin]

> On 19 Feb 2018, at 14:05, Christophe Fergeau  wrote:
> 
> On Wed, Feb 14, 2018 at 10:13:41PM +0100, Christophe de Dinechin wrote:
 I’d write “config.h”. No reason to ever look config.h in system headers.
>>> 
>>> The reason for the <> is described in [1], 4th paragraph. I've
>>> mentioned it during the previous discussion and didn't get any comment
>>> on it IIRC. Either is fine by me, I don't plan to introduce another
>>> file named 'config.h' anywhere in the source tree.
>>> 
>>> [1] 
>>> https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html
>> 
>> That rationale is remarkably inconsistent with the generated makefiles, 
>> which build for example with:
>> 
>> -I. -I..  -I.. -I.. -I/usr/include/pixman-1  -I/usr/include/cacard 
>> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
>> -I/usr/include/nspr4   -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
>> -pthread  -I/usr/include/opus   -DG_LOG_DOMAIN=\"Spice\" 
>> -I/usr/local/include/spice-1  
>> 
>> or for the streaming agent:
>> 
>> -I. -I..  -DSPICE_STREAMING_AGENT_PROGRAM -I../include 
>> -DPLUGINSDIR=\"/usr/local/lib/spice-streaming-agent/plugins\" 
>> -I/usr/local/include/spice-1  
>> 
>> So you -I. first anyway, and you would prefer the local config.h in any 
>> case. “config.h” just lets compiler find it without looking up at the 
>> command-line options.
> 
> As far as I can tell, this -I. -I.. corresponds to -I$(builddir) -I$(srcdir)
> 
> #include "config.h" would look into $(srcdir) first regardless of these
> -I flags, #include  will look into -I$(builddir) first, which
> is what is documented in the link given by Lukas.

Uri also pointed out more or less the same thing.

Currently, we have a fair number of both.

> 
> Christophe
> ___
> 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] [RFC PATCH 1/1] separate and encapsulate the agent business code

[Christophe Fergeau]

On Wed, Feb 14, 2018 at 10:13:41PM +0100, Christophe de Dinechin wrote:
> >> I’d write “config.h”. No reason to ever look config.h in system headers.
> > 
> > The reason for the <> is described in [1], 4th paragraph. I've
> > mentioned it during the previous discussion and didn't get any comment
> > on it IIRC. Either is fine by me, I don't plan to introduce another
> > file named 'config.h' anywhere in the source tree.
> > 
> > [1] 
> > https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html
> 
> That rationale is remarkably inconsistent with the generated makefiles, which 
> build for example with:
> 
> -I. -I..  -I.. -I.. -I/usr/include/pixman-1  -I/usr/include/cacard 
> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
> -I/usr/include/nspr4   -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
> -pthread  -I/usr/include/opus   -DG_LOG_DOMAIN=\"Spice\" 
> -I/usr/local/include/spice-1  
> 
> or for the streaming agent:
> 
> -I. -I..  -DSPICE_STREAMING_AGENT_PROGRAM -I../include 
> -DPLUGINSDIR=\"/usr/local/lib/spice-streaming-agent/plugins\" 
> -I/usr/local/include/spice-1  
> 
> So you -I. first anyway, and you would prefer the local config.h in any case. 
> “config.h” just lets compiler find it without looking up at the command-line 
> options.

As far as I can tell, this -I. -I.. corresponds to -I$(builddir) -I$(srcdir)

#include "config.h" would look into $(srcdir) first regardless of these
-I flags, #include  will look into -I$(builddir) first, which
is what is documented in the link given by Lukas.

Christophe


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

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

[Lukáš Hrázký]

On Thu, 2018-02-15 at 13:03 +0100, Christophe de Dinechin wrote:
> > On 15 Feb 2018, at 11:51, Lukáš Hrázký  wrote:
> > 
> > On Wed, 2018-02-14 at 22:13 +0100, Christophe de Dinechin wrote:
> > > I cut quite a bit for clarity. What I cut I agree with ;-)
> > > 
> > > 
> > > > On 14 Feb 2018, at 14:45, Lukáš Hrázký  wrote:
> > > > 
> > > > On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
> > > > > Hi Lukas,
> > > > > 
> > > > 
> > > > For your comments which are unrelated to my changes, I commended
> > > > uniformly with "Was in original code." as I didn't know what else to do
> > > > with them :)
> > > 
> > > I understood that. A lot of what I picked up is review of earlier stuff, 
> > > some of which was written before I ever joined. It’s more to document 
> > > later improvements I’d like to see happen, in case you get there first. 
> > > And then I thought that I’d rather send patches, and I sent a first round.
> > 
> > Ok.. I appreciate the patches and I understand your intent to fix
> > things... but at the moment it adds more work for me to rebase my stuff
> > and as small as your patches are, there's quite a number of them,
> > surely creating conflicts for me I'll have to solve. It's not huge, but
> > since there is no need to send those patches right now, seems to me the
> > work could have been avoided?
> 
> The same is true with your mega-patch and all my current branches ;-) And I 
> assume this is true of others pending branches as well. So it really goes 
> both ways.
> 
> So I’m sorry, while I appreciate the direction you want to go, as written 
> earlier, I think it needs to be broken up in smaller individual refactoring 
> operations, not simply rebased. My patches are an attempt to show what I’d 
> like to see.
> 
> If you find it too cumbersome, we can work together on this.

I've realized that since. The patch was a first crude attempt, I'll try
to do better in the future, I promise ;)

> > 
> > > > This way it may be easier to go through them in the future
> > > > and fix them. I can't really go off fixing all that now, I'd never
> > > > finish this and it would be hard to manage the series while trying to
> > > > do separate patches for them.
> > > 
> > > Agreed (and I knew that sending the comments out)
> > > 
> > > > 
> > > > > The commit log does not explain what an AgentRunner is, why it’s a 
> > > > > necessary abstraction, and what this is supposed to be helping with. 
> > > > > As I see it:
> > > > > - It does not cleanly encapsulate resources nor enforces / provides 
> > > > > RAII for things like streamfd and the like.
> > > > 
> > > > That is correct, and was my concern too when sending it, as mentioned
> > > > in the cover letter :) (in case I could have made something more clear,
> > > > please let me know)
> > > > 
> > > > I will surely try to do better :)
> > > > 
> > > > > - Because we throw hapazardly here and there and because of the 
> > > > > previous comment, it actually makes the situation worse in case of 
> > > > > error (at the very least, by making the flow of control harder to 
> > > > > follow, but I suspect by ending in “terminate()” more often than not).
> > > > 
> > > > I don't think we throw haphazardly and don't think it makes errors
> > > > harder to diagnose, but I may be wrong. If you had a concrete advice
> > > > what to improve, please share it :)
> > > 
> > > Well, at the very least, we have a call to “agent.LoadPlugins” in the 
> > > constructor, which may throw, because in ConcreteAgent::LoadPlugin we 
> > > call the init function, and if it throws anything but runtime_error, we 
> > > pass it along. If memory serves me right, bad_alloc for example does not 
> > > derive from runtime_error but directly from exception. And the driver is 
> > > allowed to throw 42 if it wants to.
> > > 
> > > So now we have a potential exception from a plugin within the AgentRunner 
> > > constructor. That smells funny. Please Don’t Do That™
> > 
> > Why don't you want to throw exceptions in a constructor?
> 
> Please read section 2 and 3.8 of 
> https://www.usenix.org/legacy/events/wiess2000/full_papers/dinechin/dinechin.pdf
>  (this exception handling model is what is now used by all modern C++ 
> compilers, including GCC and clang).

As a side note, I find it slightly irritating that you keep referencing
your own publications :P

I did read the suggested sections and while it was very educational,
the takeaway is that using exceptions complicates compiler
optimizations and therefore has overhead for the code in the try block
even when no exception is thrown. There wasn't anything specific to
using exceptions in constructors.

> One of the issues is that it makes it hard to reason about what is going on, 
> since you find yourself having to think about “half-constructed” objects all 
> the time. And since these are rarely exercised code paths, nobody gets them 
> right. The existing code is simply not in 

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

[Christophe de Dinechin]

> On 15 Feb 2018, at 11:51, Lukáš Hrázký  wrote:
> 
> On Wed, 2018-02-14 at 22:13 +0100, Christophe de Dinechin wrote:
>> I cut quite a bit for clarity. What I cut I agree with ;-)
>> 
>> 
>>> On 14 Feb 2018, at 14:45, Lukáš Hrázký  wrote:
>>> 
>>> On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
 Hi Lukas,
 
>>> 
>>> For your comments which are unrelated to my changes, I commended
>>> uniformly with "Was in original code." as I didn't know what else to do
>>> with them :)
>> 
>> I understood that. A lot of what I picked up is review of earlier stuff, 
>> some of which was written before I ever joined. It’s more to document later 
>> improvements I’d like to see happen, in case you get there first. And then I 
>> thought that I’d rather send patches, and I sent a first round.
> 
> Ok.. I appreciate the patches and I understand your intent to fix
> things... but at the moment it adds more work for me to rebase my stuff
> and as small as your patches are, there's quite a number of them,
> surely creating conflicts for me I'll have to solve. It's not huge, but
> since there is no need to send those patches right now, seems to me the
> work could have been avoided?

The same is true with your mega-patch and all my current branches ;-) And I 
assume this is true of others pending branches as well. So it really goes both 
ways.

So I’m sorry, while I appreciate the direction you want to go, as written 
earlier, I think it needs to be broken up in smaller individual refactoring 
operations, not simply rebased. My patches are an attempt to show what I’d like 
to see.

If you find it too cumbersome, we can work together on this.

> 
>>> This way it may be easier to go through them in the future
>>> and fix them. I can't really go off fixing all that now, I'd never
>>> finish this and it would be hard to manage the series while trying to
>>> do separate patches for them.
>> 
>> Agreed (and I knew that sending the comments out)
>> 
>>> 
 The commit log does not explain what an AgentRunner is, why it’s a 
 necessary abstraction, and what this is supposed to be helping with. As I 
 see it:
 - It does not cleanly encapsulate resources nor enforces / provides RAII 
 for things like streamfd and the like.
>>> 
>>> That is correct, and was my concern too when sending it, as mentioned
>>> in the cover letter :) (in case I could have made something more clear,
>>> please let me know)
>>> 
>>> I will surely try to do better :)
>>> 
 - Because we throw hapazardly here and there and because of the previous 
 comment, it actually makes the situation worse in case of error (at the 
 very least, by making the flow of control harder to follow, but I suspect 
 by ending in “terminate()” more often than not).
>>> 
>>> I don't think we throw haphazardly and don't think it makes errors
>>> harder to diagnose, but I may be wrong. If you had a concrete advice
>>> what to improve, please share it :)
>> 
>> Well, at the very least, we have a call to “agent.LoadPlugins” in the 
>> constructor, which may throw, because in ConcreteAgent::LoadPlugin we call 
>> the init function, and if it throws anything but runtime_error, we pass it 
>> along. If memory serves me right, bad_alloc for example does not derive from 
>> runtime_error but directly from exception. And the driver is allowed to 
>> throw 42 if it wants to.
>> 
>> So now we have a potential exception from a plugin within the AgentRunner 
>> constructor. That smells funny. Please Don’t Do That™
> 
> Why don't you want to throw exceptions in a constructor?

Please read section 2 and 3.8 of 
https://www.usenix.org/legacy/events/wiess2000/full_papers/dinechin/dinechin.pdf
 (this exception handling model is what is now used by all modern C++ 
compilers, including GCC and clang).

One of the issues is that it makes it hard to reason about what is going on, 
since you find yourself having to think about “half-constructed” objects all 
the time. And since these are rarely exercised code paths, nobody gets them 
right. The existing code is simply not in good enough shape at the moment.

(and I’m not against throwing from constructors entirely, my own RAII patch has 
a throw from a constructor)


> I find that
> completely normal. Initialization can fail. You just need to guarantee,
> that for any initialization that you do in a constructor that also
> requires some teardown, the teardown gets called even if you get an
> exception thrown in the middle of the constructor. The language
> guarantees that for any objects that get fully initialized, their
> destructor gets called. That's why we want to use RAII where possible
> and that's why there's the TODO comment to fix that.

We don’t have RAII for the moment, which is why I’d rather stay away of 
throwing in a ctor if that can be avoided. Which is why I quickly rebased one 
of my RAII patches yesterday so that we start with that instead of 

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

[Lukáš Hrázký]

On Wed, 2018-02-14 at 22:13 +0100, Christophe de Dinechin wrote:
> I cut quite a bit for clarity. What I cut I agree with ;-)
> 
> 
> > On 14 Feb 2018, at 14:45, Lukáš Hrázký  wrote:
> > 
> > On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
> > > Hi Lukas,
> > > 
> > 
> > For your comments which are unrelated to my changes, I commended
> > uniformly with "Was in original code." as I didn't know what else to do
> > with them :)
> 
> I understood that. A lot of what I picked up is review of earlier stuff, some 
> of which was written before I ever joined. It’s more to document later 
> improvements I’d like to see happen, in case you get there first. And then I 
> thought that I’d rather send patches, and I sent a first round.

Ok.. I appreciate the patches and I understand your intent to fix
things... but at the moment it adds more work for me to rebase my stuff
and as small as your patches are, there's quite a number of them,
surely creating conflicts for me I'll have to solve. It's not huge, but
since there is no need to send those patches right now, seems to me the
work could have been avoided?

> > This way it may be easier to go through them in the future
> > and fix them. I can't really go off fixing all that now, I'd never
> > finish this and it would be hard to manage the series while trying to
> > do separate patches for them.
> 
> Agreed (and I knew that sending the comments out)
> 
> > 
> > > The commit log does not explain what an AgentRunner is, why it’s a 
> > > necessary abstraction, and what this is supposed to be helping with. As I 
> > > see it:
> > > - It does not cleanly encapsulate resources nor enforces / provides RAII 
> > > for things like streamfd and the like.
> > 
> > That is correct, and was my concern too when sending it, as mentioned
> > in the cover letter :) (in case I could have made something more clear,
> > please let me know)
> > 
> > I will surely try to do better :)
> > 
> > > - Because we throw hapazardly here and there and because of the previous 
> > > comment, it actually makes the situation worse in case of error (at the 
> > > very least, by making the flow of control harder to follow, but I suspect 
> > > by ending in “terminate()” more often than not).
> > 
> > I don't think we throw haphazardly and don't think it makes errors
> > harder to diagnose, but I may be wrong. If you had a concrete advice
> > what to improve, please share it :)
> 
> Well, at the very least, we have a call to “agent.LoadPlugins” in the 
> constructor, which may throw, because in ConcreteAgent::LoadPlugin we call 
> the init function, and if it throws anything but runtime_error, we pass it 
> along. If memory serves me right, bad_alloc for example does not derive from 
> runtime_error but directly from exception. And the driver is allowed to throw 
> 42 if it wants to.
> 
> So now we have a potential exception from a plugin within the AgentRunner 
> constructor. That smells funny. Please Don’t Do That™

Why don't you want to throw exceptions in a constructor? I find that
completely normal. Initialization can fail. You just need to guarantee,
that for any initialization that you do in a constructor that also
requires some teardown, the teardown gets called even if you get an
exception thrown in the middle of the constructor. The language
guarantees that for any objects that get fully initialized, their
destructor gets called. That's why we want to use RAII where possible
and that's why there's the TODO comment to fix that.

For the agent.LoadPlugins(), though, I don't see a problem currently,
as it is the first call in the constructor and no other initialization
(aside from member constructors) was done.

How to handle plugin exceptions is another question, I would think we
want to catch each plugin's constructor exception and log it, then keep
going with the other plugins. So that one bad plugin doesn't stop the
agent from working.

> > 
> > > Also, a number of the changes could have their own individual patch set, 
> > > which would make it easier to review and understand later.
> > 
> > As I said, not really sure I can do that :( I'll try though. Ideas
> > welcome.
> 
> Since I need to rebase several of my patch series, we might work together on 
> getting all this done. But that won’t work with mega patches on either side 
> ;-)

Well, a bunch of small patches is certainly better, but doesn't spare
too much work when rebasing. I would say the best would be not to
create more patches for the same code in parallel. I was reluctant to
do it with there being your outstanding patches, but they are old and
unattended... If you don't have to, please don't create more patches
over mine :)

And I am very glad to work on this together with you... Lets just
serialize our work if we can to prevent the conflicts? :)

> > 
> > > > On 8 Feb 2018, at 17:20, Lukáš Hrázký  wrote:
> > > > 
> > > > Create an AgentRunner (TODO: needs a better name) 

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

[Christophe de Dinechin]

I cut quite a bit for clarity. What I cut I agree with ;-)


> On 14 Feb 2018, at 14:45, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
>> Hi Lukas,
>> 
> 
> For your comments which are unrelated to my changes, I commended
> uniformly with "Was in original code." as I didn't know what else to do
> with them :)

I understood that. A lot of what I picked up is review of earlier stuff, some 
of which was written before I ever joined. It’s more to document later 
improvements I’d like to see happen, in case you get there first. And then I 
thought that I’d rather send patches, and I sent a first round.


> This way it may be easier to go through them in the future
> and fix them. I can't really go off fixing all that now, I'd never
> finish this and it would be hard to manage the series while trying to
> do separate patches for them.

Agreed (and I knew that sending the comments out)

> 
>> The commit log does not explain what an AgentRunner is, why it’s a necessary 
>> abstraction, and what this is supposed to be helping with. As I see it:
>> - It does not cleanly encapsulate resources nor enforces / provides RAII for 
>> things like streamfd and the like.
> 
> That is correct, and was my concern too when sending it, as mentioned
> in the cover letter :) (in case I could have made something more clear,
> please let me know)
> 
> I will surely try to do better :)
> 
>> - Because we throw hapazardly here and there and because of the previous 
>> comment, it actually makes the situation worse in case of error (at the very 
>> least, by making the flow of control harder to follow, but I suspect by 
>> ending in “terminate()” more often than not).
> 
> I don't think we throw haphazardly and don't think it makes errors
> harder to diagnose, but I may be wrong. If you had a concrete advice
> what to improve, please share it :)

Well, at the very least, we have a call to “agent.LoadPlugins” in the 
constructor, which may throw, because in ConcreteAgent::LoadPlugin we call the 
init function, and if it throws anything but runtime_error, we pass it along. 
If memory serves me right, bad_alloc for example does not derive from 
runtime_error but directly from exception. And the driver is allowed to throw 
42 if it wants to.

So now we have a potential exception from a plugin within the AgentRunner 
constructor. That smells funny. Please Don’t Do That™

> 
>> Also, a number of the changes could have their own individual patch set, 
>> which would make it easier to review and understand later.
> 
> As I said, not really sure I can do that :( I'll try though. Ideas
> welcome.

Since I need to rebase several of my patch series, we might work together on 
getting all this done. But that won’t work with mega patches on either side ;-)

> 
>>> On 8 Feb 2018, at 17:20, Lukáš Hrázký  wrote:
>>> 
>>> 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 
>> 
>> I’d write “config.h”. No reason to ever look config.h in system headers.
> 
> The reason for the <> is described in [1], 

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

[Christophe de Dinechin]

Hi Lukas,


You asked for it, so here is my TON (TON stands for TON of nits). Except for 
what’s just below this, nothing major, just sharing ideas.

Top-comment: since your objective is to “change as little as possible”, maybe 
it would be possible to start with a patch doing just a file rename? And then 
change the renamed file? In many tools, that would make it easier to see what 
you changed and what you inherited.

The commit log does not explain what an AgentRunner is, why it’s a necessary 
abstraction, and what this is supposed to be helping with. As I see it:
- It does not cleanly encapsulate resources nor enforces / provides RAII for 
things like streamfd and the like.
- Because we throw hapazardly here and there and because of the previous 
comment, it actually makes the situation worse in case of error (at the very 
least, by making the flow of control harder to follow, but I suspect by ending 
in “terminate()” more often than not).

Also, a number of the changes could have their own individual patch set, which 
would make it easier to review and understand later.


> On 8 Feb 2018, at 17:20, Lukáš Hrázký  wrote:
> 
> 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 

I’d write “config.h”. No reason to ever look config.h in system headers.

> +#include "spice-streaming-agent.hpp"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +using namespace std;
> +using namespace SpiceStreamingAgent;

Inherited? I thought we had decided not to use that.

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

As you know, one of my patches splits that into separate sections. Might be 
worth doing in a subsequent patch.

> +
> +void handle_interrupt(int intr)
> +{
> +syslog(LOG_INFO, "Got signal %d, exiting", intr);
> +AgentRunner::quit = true;

nit: since it’s a state and not an action, I’d call it “quit_requested"

> +}
> +
> +void register_interrupts(void)
> +{
> +struct sigaction sa;
> +
> +memset(, 0, sizeof(sa));

Every time I see that, I tell Frediano to use C++-style init. Compilers are 
good at getting rid of memset calls, but still, it’s much shorter:

struct sigaction sa = { 0 };


> +sa.sa_handler = handle_interrupt;

or better yet, with that stuffed in the init

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

1/ 

[Spice-devel] [RFC PATCH 1/1] 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ý 
---
 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