Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Lukáš Hrázký]

On Tue, 2018-02-20 at 18:37 +0100, Christophe de Dinechin wrote:
> [Forgot to send, sorry for delay]
> 
> > On 19 Feb 2018, at 19:03, Lukáš Hrázký  wrote:
> > 
> > On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
> > > > On 16 Feb 2018, at 17:40, Frediano Ziglio  wrote:
> > > > 
> > > > > 
> > > > > From: Christophe de Dinechin 
> > > > > 
> > > > > Get rid of C-style 'goto done' in do_capture.
> > > > > Get rid of global streamfd, pass it around (cleaned up in later patch)
> > > > > Fixes a race condition, make sure we only use stream after opening
> > > > > 
> > > > > Signed-off-by: Christophe de Dinechin 
> > > > > ---
> > > > > src/spice-streaming-agent.cpp | 79
> > > > > +--
> > > > > 1 file changed, 47 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/src/spice-streaming-agent.cpp 
> > > > > b/src/spice-streaming-agent.cpp
> > > > > index 646d15b..9b8fd45 100644
> > > > > --- a/src/spice-streaming-agent.cpp
> > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
> > > > >StreamMsgData msg;
> > > > > };
> > > > > 
> > > > > +struct SpiceStreamCursorMessage
> > > > > +{
> > > > > +StreamDevHeader hdr;
> > > > > +StreamMsgCursorSet msg;
> > > > > +};
> > > > > +
> > > > 
> > > > This is weird in this patch
> > > 
> > > Yes. I may have merged two patches. Will check.
> > > 
> > > > 
> > > > > +class Stream
> > > > > +{
> > > > > +public:
> > > > > +Stream(const char *name)
> > > > > +{
> > > > > +fd = open(name, O_RDWR);
> > > > > +if (fd < 0)
> > > > > +throw std::runtime_error("failed to open streaming 
> > > > > device");
> > 
> > Braces around if body (according to the coding style). Repeated a few
> > times below.
> > 
> > > > > +}
> > > > > +~Stream()
> > > > > +{
> > > > > +close(fd);
> > > > 
> > > > You probably what to check for -1 to avoid calling close(-1)
> > > 
> > > I think this cannot happen, since in that case you’d have thrown from the 
> > > ctor.
> > > 
> > > > 
> > > > > +}
> > > > > +operator int() { return fd; }
> > > > 
> > > > Sure you want this? I think is safer to avoid implicit cast
> > > > also considering stuff like
> > > > 
> > > > Stream s;
> > > > if (s) …
> > > 
> > > It is removed in a later patch once it’s no longer needed. The objective 
> > > here is to minimize noisy changes at each step.
> > 
> > I'd also rather not see it, but if it's removed later, no big deal.
> 
> So I feel the general agreement is that it’s better to add an explicit getter 
> and use that rather than implicitly convert.
> 
> > 
> > > > 
> > > > > +
> > > > > +private:
> > > > > +int fd = -1;
> > > > 
> > > > So with a default constructor you want a class with
> > > > an invalid state?
> > > > Or just disable default constructor.
> > > 
> > > It’s disabled, there’s a ctor with args.
> > > 
> > > > I would disable also copy.
> > > 
> > > Also implicitly disabled.
> > 
> > It is not, actually, disabled here. It gets disabled later when you add
> > the mutex member, as that is a non-copyable type.
> 
> You are right, of course. But like for the above, the idea is to minimize 
> “noise” in the series.
> 
> Of course, I could explicitly delete constructors and assignment. I feel, but 
> I may be wrong, that having a mutex field “says that”, both in terms of 
> language safety and in terms of “what it means” (i.e. you are really creating 
> a class for the purpose of RAII and thread safety).
> 
> If you think it’s better to add a few lines, I’m OK with it, but I don’t like 
> it much as I read it more as noise than anything else. There are a few 
> choices here, roughly in my personal order of preference:
> 1. Introduce = delete in this patch, remove it when mutex is added
> 2. Introduce = delete in this patch and leave it there even when it becomes 
> “useless"
> 3 Leave a temporary “unsafe” state for a couple of commits
> 4. Merge the two and make sure there’s the mutex at the beginning
> 
> As a side note, this reminds me of a talk I highly recommend, 
> https://www.youtube.com/watch?v=4AfRAVcThyA in case you have not seen it. 
> Among other things, Herb Sutter proposes a $class mechanism that makes it 
> possible to define a class of classes, e.g. “interface”, so that you explain 
> with $class interface { … } what an interface is, and then you can write 
> “interface X { .. }” and have an interface class (e.g. only pure virtual 
> methods, etc).
> 
> Here, I see the mutex as indicative of a “locked resource” class, which I 
> understand as having only explicit ctors for initialization and nothing else 
> to consider, no copy, no assignment, etc. But it could also be made explicit 
> with this mechanism. Post C++ 20, so not right away…
> 
> > 
> > It might be a good idea to explicitly delete the copy ctr/ao anyway...

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

> On 20 Feb 2018, at 22:38, Jonathon Jongsma  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later
>> patch)
>> Fixes a race condition, make sure we only use stream after opening
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/spice-streaming-agent.cpp | 79 +--
>> 
>> 1 file changed, 47 insertions(+), 32 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
>> agent.cpp
>> index 646d15b..9b8fd45 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>> StreamMsgData msg;
>> };
>> 
>> +struct SpiceStreamCursorMessage
>> +{
>> +StreamDevHeader hdr;
>> +StreamMsgCursorSet msg;
>> +};
>> +
>> +class Stream
>> +{
>> +public:
>> +Stream(const char *name)
>> +{
>> +fd = open(name, O_RDWR);
>> +if (fd < 0)
>> +throw std::runtime_error("failed to open streaming
>> device");
>> +}
>> +~Stream()
>> +{
>> +close(fd);
>> +}
>> +operator int() { return fd; }
>> +
>> +private:
>> +int fd = -1;
>> +};
> 
> 
> What about making the 'Stream' constructor take an fd so that the same
> class can be used to encapsulate e.g. socket fds as well?

 I guess you are thinking about the vsock case?

Well, the whole idea of RAII is that one class has both open and close, so I 
don’t think passing an fd as input would work.

If we need to deal with a socket fd, we could have an additional ctor doing a 
‘socket’ call (the dtor closing the socket), and another one looking like 
‘accept’, something like:

Stream(int domain, int type, int protocol); // socket()-like
Stream(int sockfd, struct sockaddr *addr, socklen_t *len); // 
accept-like

If we do that, then each ‘Stream’ would own one fd and only one, and be 
responsible for closing it. But then, we’d probably want to split the Stream 
class between the fd-management aspect (deals with the fd only, offers read and 
write), and the message packaging aspect (holds a mutex, responsible for 
writing or receiving messages, the function I called ‘send’, …)

That being said, my objective was not to create some sort of general purpose 
class, KISS, just what we need. If what we need is for vsocks, that can be done 
in a later step as I outlined above without too much disruption.


Christophe
> 
> 
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> static std::set client_codecs;
>> -static int streamfd = -1;
>> static std::mutex stream_mtx;
>> 
>> -static int have_something_to_read(int timeout)
>> +static int have_something_to_read(int streamfd, int timeout)
>> {
>> struct pollfd pollfd = {streamfd, POLLIN, 0};
>> 
>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>> return 0;
>> }
>> 
>> -static int read_command_from_device(void)
>> +static int read_command_from_device(int streamfd)
>> {
>> StreamDevHeader hdr;
>> uint8_t msg[64];
>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>> return 1;
>> }
>> 
>> -static int read_command(bool blocking)
>> +static int read_command(int streamfd, bool blocking)
>> {
>> int timeout = blocking?-1:0;
>> while (!quit_requested) {
>> -if (!have_something_to_read(timeout)) {
>> +if (!have_something_to_read(streamfd, timeout)) {
>> if (!blocking) {
>> return 0;
>> }
>> sleep(1);
>> continue;
>> }
>> -return read_command_from_device();
>> +return read_command_from_device(streamfd);
>> }
>> 
>> return 1;
>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t
>> len)
>> return written;
>> }
>> 
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned
>> c)
>> +static int spice_stream_send_format(int streamfd, unsigned w,
>> unsigned h, unsigned c)
>> {
>> 
>> SpiceStreamFormatMessage msg;
>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w,
>> unsigned h, unsigned c)
>> return EXIT_SUCCESS;
>> }
>> 
>> -static int spice_stream_send_frame(const void *buf, const unsigned
>> size)
>> +static int spice_stream_send_frame(int streamfd, const void *buf,
>> const unsigned size)
>> {
>> SpiceStreamDataMessage msg;
>> const size_t msgsize = sizeof(msg);
>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>> }
>> 
>> static void
>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int
>> hotspot_y,
>> +send_cursor(int streamfd, unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>> std::function 

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Jonathon Jongsma]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later
> patch)
> Fixes a race condition, make sure we only use stream after opening
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 79 +--
> 
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> agent.cpp
> index 646d15b..9b8fd45 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>  StreamMsgData msg;
>  };
>  
> +struct SpiceStreamCursorMessage
> +{
> +StreamDevHeader hdr;
> +StreamMsgCursorSet msg;
> +};
> +
> +class Stream
> +{
> +public:
> +Stream(const char *name)
> +{
> +fd = open(name, O_RDWR);
> +if (fd < 0)
> +throw std::runtime_error("failed to open streaming
> device");
> +}
> +~Stream()
> +{
> +close(fd);
> +}
> +operator int() { return fd; }
> +
> +private:
> +int fd = -1;
> +};


What about making the 'Stream' constructor take an fd so that the same
class can be used to encapsulate e.g. socket fds as well?


> +
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
>  static std::set client_codecs;
> -static int streamfd = -1;
>  static std::mutex stream_mtx;
>  
> -static int have_something_to_read(int timeout)
> +static int have_something_to_read(int streamfd, int timeout)
>  {
>  struct pollfd pollfd = {streamfd, POLLIN, 0};
>  
> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>  return 0;
>  }
>  
> -static int read_command_from_device(void)
> +static int read_command_from_device(int streamfd)
>  {
>  StreamDevHeader hdr;
>  uint8_t msg[64];
> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>  return 1;
>  }
>  
> -static int read_command(bool blocking)
> +static int read_command(int streamfd, bool blocking)
>  {
>  int timeout = blocking?-1:0;
>  while (!quit_requested) {
> -if (!have_something_to_read(timeout)) {
> +if (!have_something_to_read(streamfd, timeout)) {
>  if (!blocking) {
>  return 0;
>  }
>  sleep(1);
>  continue;
>  }
> -return read_command_from_device();
> +return read_command_from_device(streamfd);
>  }
>  
>  return 1;
> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t
> len)
>  return written;
>  }
>  
> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned
> c)
> +static int spice_stream_send_format(int streamfd, unsigned w,
> unsigned h, unsigned c)
>  {
>  
>  SpiceStreamFormatMessage msg;
> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w,
> unsigned h, unsigned c)
>  return EXIT_SUCCESS;
>  }
>  
> -static int spice_stream_send_frame(const void *buf, const unsigned
> size)
> +static int spice_stream_send_frame(int streamfd, const void *buf,
> const unsigned size)
>  {
>  SpiceStreamDataMessage msg;
>  const size_t msgsize = sizeof(msg);
> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(unsigned width, unsigned height, int hotspot_x, int
> hotspot_y,
> +send_cursor(int streamfd, unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>  std::function fill_cursor)
>  {
>  if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>  write_all(streamfd, msg.get(), cursor_size);
>  }
>  
> -static void cursor_changes(Display *display, int event_base)
> +static void cursor_changes(int streamfd, Display *display, int
> event_base)
>  {
>  unsigned long last_serial = 0;
>  
> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display,
> int event_base)
>  for (unsigned i = 0; i < cursor->width * cursor->height; 
> ++i)
>  pixels[i] = cursor->pixels[i];
>  };
> -send_cursor(cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
> +send_cursor(streamfd,
> +cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
>  }
>  }
>  
>  static void
> -do_capture(const char *streamport, FILE *f_log)
> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>  {
> -streamfd = open(streamport, O_RDWR);
> -if (streamfd < 0)
> -throw std::runtime_error("failed to open the streaming
> device (" +
> - std::string(streamport) + "): "
> - 

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

[Forgot to send, sorry for delay]

> On 19 Feb 2018, at 19:03, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
>>> On 16 Feb 2018, at 17:40, Frediano Ziglio  wrote:
>>> 
 
 From: Christophe de Dinechin 
 
 Get rid of C-style 'goto done' in do_capture.
 Get rid of global streamfd, pass it around (cleaned up in later patch)
 Fixes a race condition, make sure we only use stream after opening
 
 Signed-off-by: Christophe de Dinechin 
 ---
 src/spice-streaming-agent.cpp | 79
 +--
 1 file changed, 47 insertions(+), 32 deletions(-)
 
 diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
 index 646d15b..9b8fd45 100644
 --- a/src/spice-streaming-agent.cpp
 +++ b/src/spice-streaming-agent.cpp
 @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
StreamMsgData msg;
 };
 
 +struct SpiceStreamCursorMessage
 +{
 +StreamDevHeader hdr;
 +StreamMsgCursorSet msg;
 +};
 +
>>> 
>>> This is weird in this patch
>> 
>> Yes. I may have merged two patches. Will check.
>> 
>>> 
 +class Stream
 +{
 +public:
 +Stream(const char *name)
 +{
 +fd = open(name, O_RDWR);
 +if (fd < 0)
 +throw std::runtime_error("failed to open streaming device");
> 
> Braces around if body (according to the coding style). Repeated a few
> times below.
> 
 +}
 +~Stream()
 +{
 +close(fd);
>>> 
>>> You probably what to check for -1 to avoid calling close(-1)
>> 
>> I think this cannot happen, since in that case you’d have thrown from the 
>> ctor.
>> 
>>> 
 +}
 +operator int() { return fd; }
>>> 
>>> Sure you want this? I think is safer to avoid implicit cast
>>> also considering stuff like
>>> 
>>> Stream s;
>>> if (s) …
>> 
>> It is removed in a later patch once it’s no longer needed. The objective 
>> here is to minimize noisy changes at each step.
> 
> I'd also rather not see it, but if it's removed later, no big deal.

So I feel the general agreement is that it’s better to add an explicit getter 
and use that rather than implicitly convert.

> 
>>> 
 +
 +private:
 +int fd = -1;
>>> 
>>> So with a default constructor you want a class with
>>> an invalid state?
>>> Or just disable default constructor.
>> 
>> It’s disabled, there’s a ctor with args.
>> 
>>> I would disable also copy.
>> 
>> Also implicitly disabled.
> 
> It is not, actually, disabled here. It gets disabled later when you add
> the mutex member, as that is a non-copyable type.

You are right, of course. But like for the above, the idea is to minimize 
“noise” in the series.

Of course, I could explicitly delete constructors and assignment. I feel, but I 
may be wrong, that having a mutex field “says that”, both in terms of language 
safety and in terms of “what it means” (i.e. you are really creating a class 
for the purpose of RAII and thread safety).

If you think it’s better to add a few lines, I’m OK with it, but I don’t like 
it much as I read it more as noise than anything else. There are a few choices 
here, roughly in my personal order of preference:
1. Introduce = delete in this patch, remove it when mutex is added
2. Introduce = delete in this patch and leave it there even when it becomes 
“useless"
3 Leave a temporary “unsafe” state for a couple of commits
4. Merge the two and make sure there’s the mutex at the beginning

As a side note, this reminds me of a talk I highly recommend, 
https://www.youtube.com/watch?v=4AfRAVcThyA in case you have not seen it. Among 
other things, Herb Sutter proposes a $class mechanism that makes it possible to 
define a class of classes, e.g. “interface”, so that you explain with $class 
interface { … } what an interface is, and then you can write “interface X { .. 
}” and have an interface class (e.g. only pure virtual methods, etc).

Here, I see the mutex as indicative of a “locked resource” class, which I 
understand as having only explicit ctors for initialization and nothing else to 
consider, no copy, no assignment, etc. But it could also be made explicit with 
this mechanism. Post C++ 20, so not right away…

> 
> It might be a good idea to explicitly delete the copy ctr/ao anyway...
> In case someone ever removes the mutex and doesn't realize this.

That’s my choice #2, it could easily become #1.

> 
>>> 
 +};
 +
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
 static std::set client_codecs;
 -static int streamfd = -1;
 static std::mutex stream_mtx;
 
 -static int have_something_to_read(int timeout)
 +static int have_something_to_read(int streamfd, int timeout)
>>> 
>>> maybe would be better to use the 

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
> > On 16 Feb 2018, at 17:40, Frediano Ziglio  wrote:
> > 
> > > 
> > > From: Christophe de Dinechin 
> > > 
> > > Get rid of C-style 'goto done' in do_capture.
> > > Get rid of global streamfd, pass it around (cleaned up in later patch)
> > > Fixes a race condition, make sure we only use stream after opening
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > > src/spice-streaming-agent.cpp | 79
> > > +--
> > > 1 file changed, 47 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 646d15b..9b8fd45 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
> > > StreamMsgData msg;
> > > };
> > > 
> > > +struct SpiceStreamCursorMessage
> > > +{
> > > +StreamDevHeader hdr;
> > > +StreamMsgCursorSet msg;
> > > +};
> > > +
> > 
> > This is weird in this patch
> 
> Yes. I may have merged two patches. Will check.
> 
> > 
> > > +class Stream
> > > +{
> > > +public:
> > > +Stream(const char *name)
> > > +{
> > > +fd = open(name, O_RDWR);
> > > +if (fd < 0)
> > > +throw std::runtime_error("failed to open streaming device");

Braces around if body (according to the coding style). Repeated a few
times below.

> > > +}
> > > +~Stream()
> > > +{
> > > +close(fd);
> > 
> > You probably what to check for -1 to avoid calling close(-1)
> 
> I think this cannot happen, since in that case you’d have thrown from the 
> ctor.
> 
> > 
> > > +}
> > > +operator int() { return fd; }
> > 
> > Sure you want this? I think is safer to avoid implicit cast
> > also considering stuff like
> > 
> >  Stream s;
> >  if (s) …
> 
> It is removed in a later patch once it’s no longer needed. The objective here 
> is to minimize noisy changes at each step.

I'd also rather not see it, but if it's removed later, no big deal.

> > 
> > > +
> > > +private:
> > > +int fd = -1;
> > 
> > So with a default constructor you want a class with
> > an invalid state?
> > Or just disable default constructor.
> 
> It’s disabled, there’s a ctor with args.
> 
> > I would disable also copy.
> 
> Also implicitly disabled.

It is not, actually, disabled here. It gets disabled later when you add
the mutex member, as that is a non-copyable type.

It might be a good idea to explicitly delete the copy ctr/ao anyway...
In case someone ever removes the mutex and doesn't realize this.

> > 
> > > +};
> > > +
> > > static bool streaming_requested = false;
> > > static bool quit_requested = false;
> > > static bool log_binary = false;
> > > static std::set client_codecs;
> > > -static int streamfd = -1;
> > > static std::mutex stream_mtx;
> > > 
> > > -static int have_something_to_read(int timeout)
> > > +static int have_something_to_read(int streamfd, int timeout)
> > 
> > maybe would be better to use the "fd" name here, is no more
> > bound to stream.
> 
> Kept to minimize changes
> 
> > > {
> > > struct pollfd pollfd = {streamfd, POLLIN, 0};
> > > 
> > > @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
> > > return 0;
> > > }
> > > 
> > > -static int read_command_from_device(void)
> > > +static int read_command_from_device(int streamfd)
> > 
> > Have you considered Stream::read_command?
> 
> For that specific case, I liked the “from_device”. But…
> 
> > Ok, mostly in a following patch
> 
> 
> > 
> > > {
> > > StreamDevHeader hdr;
> > > uint8_t msg[64];
> > > @@ -121,18 +145,18 @@ static int read_command_from_device(void)
> > > return 1;
> > > }
> > > 
> > > -static int read_command(bool blocking)
> > > +static int read_command(int streamfd, bool blocking)
> > > {
> > > int timeout = blocking?-1:0;
> > > while (!quit_requested) {
> > > -if (!have_something_to_read(timeout)) {
> > > +if (!have_something_to_read(streamfd, timeout)) {
> > > if (!blocking) {
> > > return 0;
> > > }
> > > sleep(1);
> > > continue;
> > > }
> > > -return read_command_from_device();
> > > +return read_command_from_device(streamfd);
> > > }
> > > 
> > > return 1;
> > > @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
> > > return written;
> > > }
> > > 
> > > -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> > > +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> > > unsigned c)
> > > {
> > > 
> > > SpiceStreamFormatMessage msg;
> > > @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, 
> > > unsigned
> > > h, unsigned c)
> > > return EXIT_SUCCESS;
> > > }
> > > 
> > > -static int spice_stream_send_frame(const void *buf, 

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

> On 16 Feb 2018, at 17:40, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later patch)
>> Fixes a race condition, make sure we only use stream after opening
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/spice-streaming-agent.cpp | 79
>> +--
>> 1 file changed, 47 insertions(+), 32 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 646d15b..9b8fd45 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>> StreamMsgData msg;
>> };
>> 
>> +struct SpiceStreamCursorMessage
>> +{
>> +StreamDevHeader hdr;
>> +StreamMsgCursorSet msg;
>> +};
>> +
> 
> This is weird in this patch

Yes. I may have merged two patches. Will check.

> 
>> +class Stream
>> +{
>> +public:
>> +Stream(const char *name)
>> +{
>> +fd = open(name, O_RDWR);
>> +if (fd < 0)
>> +throw std::runtime_error("failed to open streaming device");
>> +}
>> +~Stream()
>> +{
>> +close(fd);
> 
> You probably what to check for -1 to avoid calling close(-1)

I think this cannot happen, since in that case you’d have thrown from the ctor.

> 
>> +}
>> +operator int() { return fd; }
> 
> Sure you want this? I think is safer to avoid implicit cast
> also considering stuff like
> 
>  Stream s;
>  if (s) …

It is removed in a later patch once it’s no longer needed. The objective here 
is to minimize noisy changes at each step.

> 
>> +
>> +private:
>> +int fd = -1;
> 
> So with a default constructor you want a class with
> an invalid state?
> Or just disable default constructor.

It’s disabled, there’s a ctor with args.

> I would disable also copy.

Also implicitly disabled.

> 
>> +};
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> static std::set client_codecs;
>> -static int streamfd = -1;
>> static std::mutex stream_mtx;
>> 
>> -static int have_something_to_read(int timeout)
>> +static int have_something_to_read(int streamfd, int timeout)
> 
> maybe would be better to use the "fd" name here, is no more
> bound to stream.

Kept to minimize changes

>> {
>> struct pollfd pollfd = {streamfd, POLLIN, 0};
>> 
>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>> return 0;
>> }
>> 
>> -static int read_command_from_device(void)
>> +static int read_command_from_device(int streamfd)
> 
> Have you considered Stream::read_command?

For that specific case, I liked the “from_device”. But…

> Ok, mostly in a following patch


> 
>> {
>> StreamDevHeader hdr;
>> uint8_t msg[64];
>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>> return 1;
>> }
>> 
>> -static int read_command(bool blocking)
>> +static int read_command(int streamfd, bool blocking)
>> {
>> int timeout = blocking?-1:0;
>> while (!quit_requested) {
>> -if (!have_something_to_read(timeout)) {
>> +if (!have_something_to_read(streamfd, timeout)) {
>> if (!blocking) {
>> return 0;
>> }
>> sleep(1);
>> continue;
>> }
>> -return read_command_from_device();
>> +return read_command_from_device(streamfd);
>> }
>> 
>> return 1;
>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>> return written;
>> }
>> 
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
>> unsigned c)
>> {
>> 
>> SpiceStreamFormatMessage msg;
>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
>> h, unsigned c)
>> return EXIT_SUCCESS;
>> }
>> 
>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>> +static int spice_stream_send_frame(int streamfd, const void *buf, const
>> unsigned size)
>> {
>> SpiceStreamDataMessage msg;
>> const size_t msgsize = sizeof(msg);
>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>> }
>> 
>> static void
>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
>> int hotspot_y,
>> std::function fill_cursor)
>> {
>> if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>> write_all(streamfd, msg.get(), cursor_size);
>> }
>> 
>> -static void cursor_changes(Display *display, int event_base)
>> +static void cursor_changes(int streamfd, Display *display, int event_base)
>> {
>> 

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later patch)
> Fixes a race condition, make sure we only use stream after opening
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 79
>  +--
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 646d15b..9b8fd45 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>  StreamMsgData msg;
>  };
>  
> +struct SpiceStreamCursorMessage
> +{
> +StreamDevHeader hdr;
> +StreamMsgCursorSet msg;
> +};
> +

This is weird in this patch

> +class Stream
> +{
> +public:
> +Stream(const char *name)
> +{
> +fd = open(name, O_RDWR);
> +if (fd < 0)
> +throw std::runtime_error("failed to open streaming device");
> +}
> +~Stream()
> +{
> +close(fd);

You probably what to check for -1 to avoid calling close(-1)

> +}
> +operator int() { return fd; }

Sure you want this? I think is safer to avoid implicit cast
also considering stuff like

  Stream s;
  if (s) ...

> +
> +private:
> +int fd = -1;

So with a default constructor you want a class with
an invalid state?
Or just disable default constructor.
I would disable also copy.

> +};
> +
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
>  static std::set client_codecs;
> -static int streamfd = -1;
>  static std::mutex stream_mtx;
>  
> -static int have_something_to_read(int timeout)
> +static int have_something_to_read(int streamfd, int timeout)

maybe would be better to use the "fd" name here, is no more
bound to stream.

>  {
>  struct pollfd pollfd = {streamfd, POLLIN, 0};
>  
> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>  return 0;
>  }
>  
> -static int read_command_from_device(void)
> +static int read_command_from_device(int streamfd)

Have you considered Stream::read_command?
Ok, mostly in a following patch

>  {
>  StreamDevHeader hdr;
>  uint8_t msg[64];
> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>  return 1;
>  }
>  
> -static int read_command(bool blocking)
> +static int read_command(int streamfd, bool blocking)
>  {
>  int timeout = blocking?-1:0;
>  while (!quit_requested) {
> -if (!have_something_to_read(timeout)) {
> +if (!have_something_to_read(streamfd, timeout)) {
>  if (!blocking) {
>  return 0;
>  }
>  sleep(1);
>  continue;
>  }
> -return read_command_from_device();
> +return read_command_from_device(streamfd);
>  }
>  
>  return 1;
> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>  return written;
>  }
>  
> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> unsigned c)
>  {
>  
>  SpiceStreamFormatMessage msg;
> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
> h, unsigned c)
>  return EXIT_SUCCESS;
>  }
>  
> -static int spice_stream_send_frame(const void *buf, const unsigned size)
> +static int spice_stream_send_frame(int streamfd, const void *buf, const
> unsigned size)
>  {
>  SpiceStreamDataMessage msg;
>  const size_t msgsize = sizeof(msg);
> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
> int hotspot_y,
>  std::function fill_cursor)
>  {
>  if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>  write_all(streamfd, msg.get(), cursor_size);
>  }
>  
> -static void cursor_changes(Display *display, int event_base)
> +static void cursor_changes(int streamfd, Display *display, int event_base)
>  {
>  unsigned long last_serial = 0;
>  
> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
> event_base)
>  for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>  pixels[i] = cursor->pixels[i];
>  };
> -send_cursor(cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
> +send_cursor(streamfd,
> +cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
>  }
>  }
>  
>  static void
> -do_capture(const char *streamport, FILE *f_log)
> +do_capture(int streamfd, const 

[Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

From: Christophe de Dinechin 

Get rid of C-style 'goto done' in do_capture.
Get rid of global streamfd, pass it around (cleaned up in later patch)
Fixes a race condition, make sure we only use stream after opening

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 79 +--
 1 file changed, 47 insertions(+), 32 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 646d15b..9b8fd45 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
 StreamMsgData msg;
 };
 
+struct SpiceStreamCursorMessage
+{
+StreamDevHeader hdr;
+StreamMsgCursorSet msg;
+};
+
+class Stream
+{
+public:
+Stream(const char *name)
+{
+fd = open(name, O_RDWR);
+if (fd < 0)
+throw std::runtime_error("failed to open streaming device");
+}
+~Stream()
+{
+close(fd);
+}
+operator int() { return fd; }
+
+private:
+int fd = -1;
+};
+
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
 static std::set client_codecs;
-static int streamfd = -1;
 static std::mutex stream_mtx;
 
-static int have_something_to_read(int timeout)
+static int have_something_to_read(int streamfd, int timeout)
 {
 struct pollfd pollfd = {streamfd, POLLIN, 0};
 
@@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
-static int read_command_from_device(void)
+static int read_command_from_device(int streamfd)
 {
 StreamDevHeader hdr;
 uint8_t msg[64];
@@ -121,18 +145,18 @@ static int read_command_from_device(void)
 return 1;
 }
 
-static int read_command(bool blocking)
+static int read_command(int streamfd, bool blocking)
 {
 int timeout = blocking?-1:0;
 while (!quit_requested) {
-if (!have_something_to_read(timeout)) {
+if (!have_something_to_read(streamfd, timeout)) {
 if (!blocking) {
 return 0;
 }
 sleep(1);
 continue;
 }
-return read_command_from_device();
+return read_command_from_device(streamfd);
 }
 
 return 1;
@@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
 return written;
 }
 
-static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
+static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
unsigned c)
 {
 
 SpiceStreamFormatMessage msg;
@@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned h, 
unsigned c)
 return EXIT_SUCCESS;
 }
 
-static int spice_stream_send_frame(const void *buf, const unsigned size)
+static int spice_stream_send_frame(int streamfd, const void *buf, const 
unsigned size)
 {
 SpiceStreamDataMessage msg;
 const size_t msgsize = sizeof(msg);
@@ -254,7 +278,7 @@ static void usage(const char *progname)
 }
 
 static void
-send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
+send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int 
hotspot_y,
 std::function fill_cursor)
 {
 if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
@@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int hotspot_x, 
int hotspot_y,
 write_all(streamfd, msg.get(), cursor_size);
 }
 
-static void cursor_changes(Display *display, int event_base)
+static void cursor_changes(int streamfd, Display *display, int event_base)
 {
 unsigned long last_serial = 0;
 
@@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int 
event_base)
 for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
 pixels[i] = cursor->pixels[i];
 };
-send_cursor(cursor->width, cursor->height, cursor->xhot, cursor->yhot, 
fill_cursor);
+send_cursor(streamfd,
+cursor->width, cursor->height, cursor->xhot, cursor->yhot, 
fill_cursor);
 }
 }
 
 static void
-do_capture(const char *streamport, FILE *f_log)
+do_capture(int streamfd, const char *streamport, FILE *f_log)
 {
-streamfd = open(streamport, O_RDWR);
-if (streamfd < 0)
-throw std::runtime_error("failed to open the streaming device (" +
- std::string(streamport) + "): "
- + strerror(errno));
-
 unsigned int frame_count = 0;
 while (!quit_requested) {
 while (!quit_requested && !streaming_requested) {
-if (read_command(true) < 0) {
+if (read_command(streamfd, true) < 0) {
 syslog(LOG_ERR, "FAILED to read command\n");
-goto done;
+return;
 }
 }
 
@@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
 
 syslog(LOG_DEBUG, "wXh %uX%u