Re: [Spice-devel] [RFC PATCH spice-streaming-agent 1/2] Move reading stream messages into a function

2018-02-22 Thread Lukáš Hrázký 
On Thu, 2018-02-22 at 09:34 -0500, Frediano Ziglio wrote:
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> >  src/spice-streaming-agent.cpp | 22 --
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index aeee5d3..b88225f 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -79,6 +79,15 @@ static int have_something_to_read(int timeout)
> >  return 0;
> >  }
> >  
> > +static void read_message(void *msg, size_t len)
> > +{
> > +size_t n = read(streamfd, msg, len);
> > +if (n != len) {
> > +throw std::runtime_error("Reading message from device failed: read 
> > "
> > + std::to_string(n) +
> > + ", expected " + std::to_string(len));
> > +}
> > +}
> > +
> 
> Why not for symmetry a read_all handling EINTR and partial reads?
> I think is also a more or less straight forward copy of the write_all.
> I would pass the "int fd" for reuse as a low level routine.

I'm not that well versed in low-level stuff, so I didn't think of it. I
will surely try and resend the patch.

I am still wondering about the _all in the name, though I think I'm
starting to understand it's supposed to mean those are not partial
reads/writes? I'm not sure why someone would want to do that... :) So
the name still seems weird to me, but if you think it's good...

In both mine and Christophe's ideas of how to move the code to a C++
style, this as well as write_all would become methods of the Stream
class, so not standalone low-level routines. I mean, they could still
be, just that there is no reason for now as they are not needed
elsewhere. I'm not insisting on this though.

> >  static void handle_stream_start_stop(uint32_t len)
> >  {
> >  uint8_t msg[256];
> > @@ -87,11 +96,8 @@ static void handle_stream_start_stop(uint32_t len)
> >  throw std::runtime_error("msg size (" + std::to_string(len) + ") is
> >  too long "
> >   "(longer than " +
> >   std::to_string(sizeof(msg)) + ")");
> >  }
> > -int n = read(streamfd, , len);
> > -if (n != len) {
> > -throw std::runtime_error("read command from device FAILED -- read "
> > + std::to_string(n) +
> > - " expected " + std::to_string(len));
> > -}
> > +
> > +read_message(msg, len);
> >  streaming_requested = (msg[0] != 0); /* num_codecs */
> >  syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
> > streaming_requested ? "START" : "STOP");
> > @@ -108,12 +114,8 @@ static void handle_stream_capabilities(uint32_t len)
> >  if (len > sizeof(caps)) {
> >  throw std::runtime_error("capability message too long");
> >  }
> > -int n = read(streamfd, caps, len);
> > -if (n != len) {
> > -throw std::runtime_error("read command from device FAILED -- read "
> > + std::to_string(n) +
> > - " expected " + std::to_string(len));
> > -}
> >  
> > +read_message(caps, len);
> >  // we currently do not support extensions so just reply so
> >  StreamDevHeader hdr = {
> >  STREAM_DEVICE_PROTOCOL,
> 
> Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [RFC PATCH spice-streaming-agent 1/2] Move reading stream messages into a function

2018-02-22 Thread Frediano Ziglio 
> 
> Signed-off-by: Lukáš Hrázký 
> ---
>  src/spice-streaming-agent.cpp | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index aeee5d3..b88225f 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -79,6 +79,15 @@ static int have_something_to_read(int timeout)
>  return 0;
>  }
>  
> +static void read_message(void *msg, size_t len)
> +{
> +size_t n = read(streamfd, msg, len);
> +if (n != len) {
> +throw std::runtime_error("Reading message from device failed: read "
> + std::to_string(n) +
> + ", expected " + std::to_string(len));
> +}
> +}
> +

Why not for symmetry a read_all handling EINTR and partial reads?
I think is also a more or less straight forward copy of the write_all.
I would pass the "int fd" for reuse as a low level routine.

>  static void handle_stream_start_stop(uint32_t len)
>  {
>  uint8_t msg[256];
> @@ -87,11 +96,8 @@ static void handle_stream_start_stop(uint32_t len)
>  throw std::runtime_error("msg size (" + std::to_string(len) + ") is
>  too long "
>   "(longer than " +
>   std::to_string(sizeof(msg)) + ")");
>  }
> -int n = read(streamfd, , len);
> -if (n != len) {
> -throw std::runtime_error("read command from device FAILED -- read "
> + std::to_string(n) +
> - " expected " + std::to_string(len));
> -}
> +
> +read_message(msg, len);
>  streaming_requested = (msg[0] != 0); /* num_codecs */
>  syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
> streaming_requested ? "START" : "STOP");
> @@ -108,12 +114,8 @@ static void handle_stream_capabilities(uint32_t len)
>  if (len > sizeof(caps)) {
>  throw std::runtime_error("capability message too long");
>  }
> -int n = read(streamfd, caps, len);
> -if (n != len) {
> -throw std::runtime_error("read command from device FAILED -- read "
> + std::to_string(n) +
> - " expected " + std::to_string(len));
> -}
>  
> +read_message(caps, len);
>  // we currently do not support extensions so just reply so
>  StreamDevHeader hdr = {
>  STREAM_DEVICE_PROTOCOL,

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

[Spice-devel] [RFC PATCH spice-streaming-agent 1/2] Move reading stream messages into a function

2018-02-22 Thread Lukáš Hrázký 
Signed-off-by: Lukáš Hrázký 
---
 src/spice-streaming-agent.cpp | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index aeee5d3..b88225f 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -79,6 +79,15 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static void read_message(void *msg, size_t len)
+{
+size_t n = read(streamfd, msg, len);
+if (n != len) {
+throw std::runtime_error("Reading message from device failed: read " + 
std::to_string(n) +
+ ", expected " + std::to_string(len));
+}
+}
+
 static void handle_stream_start_stop(uint32_t len)
 {
 uint8_t msg[256];
@@ -87,11 +96,8 @@ static void handle_stream_start_stop(uint32_t len)
 throw std::runtime_error("msg size (" + std::to_string(len) + ") is 
too long "
  "(longer than " + std::to_string(sizeof(msg)) 
+ ")");
 }
-int n = read(streamfd, , len);
-if (n != len) {
-throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
- " expected " + std::to_string(len));
-}
+
+read_message(msg, len);
 streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
streaming_requested ? "START" : "STOP");
@@ -108,12 +114,8 @@ static void handle_stream_capabilities(uint32_t len)
 if (len > sizeof(caps)) {
 throw std::runtime_error("capability message too long");
 }
-int n = read(streamfd, caps, len);
-if (n != len) {
-throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
- " expected " + std::to_string(len));
-}
 
+read_message(caps, len);
 // we currently do not support extensions so just reply so
 StreamDevHeader hdr = {
 STREAM_DEVICE_PROTOCOL,
-- 
2.16.1

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