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

[Lukáš Hrázký]

On Thu, 2018-02-22 at 18:56 +0100, Christophe de Dinechin wrote:
> > On 22 Feb 2018, at 18:41, Lukáš Hrázký  wrote:
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> > src/spice-streaming-agent.cpp | 31 +--
> > 1 file changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index aeee5d3..20ac1f4 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -79,6 +79,24 @@ static int have_something_to_read(int timeout)
> > return 0;
> > }
> > 
> > +static void read_all(void *msg, size_t len)
> > +{
> > +size_t len_read = 0;
> > +
> > +while (len_read < len) {
> > +ssize_t n = read(streamfd, (uint8_t *) msg + len_read, len - 
> > len_read);
> 
> What about removing read_len, and decrementing len instead of incrementing 
> read_len? (also need to increment the pointer, obviously)

That's a possibility, I just followed the structure of write_all()...

> > +
> > +if (n < 0) {
> > +if (errno == EINTR) {
> > +continue;
> > +}
> > +throw std::runtime_error("Reading message from device failed: 
> > " +
> > + std::string(strerror(errno)));
> 
> If this goes in before my own series, I will use that as an excuse to create 
> the IOError and ReadError classes, and avoid all this duplication of 
> strerror(errno) and string concatenation that bloats the code :-)

No problem...

> > +}
> > +len_read += n;
> > +}
> > +}
> > +
> > static void handle_stream_start_stop(uint32_t len)
> > {
> > uint8_t msg[256];
> > @@ -87,11 +105,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_all(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 +123,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_all(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
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe de Dinechin]

> On 22 Feb 2018, at 18:41, Lukáš Hrázký  wrote:
> 
> Signed-off-by: Lukáš Hrázký 
> ---
> src/spice-streaming-agent.cpp | 31 +--
> 1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index aeee5d3..20ac1f4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -79,6 +79,24 @@ static int have_something_to_read(int timeout)
> return 0;
> }
> 
> +static void read_all(void *msg, size_t len)
> +{
> +size_t len_read = 0;
> +
> +while (len_read < len) {
> +ssize_t n = read(streamfd, (uint8_t *) msg + len_read, len - 
> len_read);

What about removing read_len, and decrementing len instead of incrementing 
read_len? (also need to increment the pointer, obviously)


> +
> +if (n < 0) {
> +if (errno == EINTR) {
> +continue;
> +}
> +throw std::runtime_error("Reading message from device failed: " +
> + std::string(strerror(errno)));

If this goes in before my own series, I will use that as an excuse to create 
the IOError and ReadError classes, and avoid all this duplication of 
strerror(errno) and string concatenation that bloats the code :-)

> +}
> +len_read += n;
> +}
> +}
> +
> static void handle_stream_start_stop(uint32_t len)
> {
> uint8_t msg[256];
> @@ -87,11 +105,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_all(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 +123,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_all(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

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