Re: [Spice-devel] [RFC PATCH spice-streaming-agent 1/2] Move reading stream messages into a function
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, &msg, 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
> > 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, &msg, 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
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, &msg, 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