[Spice-devel] [PATCH spice-streaming-agent 4/9] Introduce a WriteError exception for write_all()

[Frediano Ziglio]

> 
> Update the interface to not return the size written, as it is not needed
> anymore.
> 
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
>  src/error.cpp                 |  3 ++-
>  src/error.hpp                 | 14 +++++++++++++
>  src/spice-streaming-agent.cpp | 48
>  +++++++++++++++----------------------------
>  src/stream-port.cpp           |  7 ++-----
>  src/stream-port.hpp           |  2 +-
>  5 files changed, 35 insertions(+), 39 deletions(-)
> 
> diff --git a/src/error.cpp b/src/error.cpp
> index 1b76ea4..4ef275c 100644
> --- a/src/error.cpp
> +++ b/src/error.cpp
> @@ -7,7 +7,6 @@
>  #include "error.hpp"
>  
>  #include <string.h>
> -#include <syslog.h>
>  
>  
>  namespace spice {
> @@ -26,4 +25,6 @@ IOError::IOError(const std::string &msg, int errno_) :
>  
>  ReadError::ReadError(const std::string &msg, int errno_) : IOError(msg,
>  errno_) {}
>  
> +WriteError::WriteError(const std::string &msg, int errno_) : IOError(msg,
> errno_) {}
> +

Some comment on errno_ of previous patch.

>  }} // namespace spice::streaming_agent
> diff --git a/src/error.hpp b/src/error.hpp
> index de1cb83..d69942c 100644
> --- a/src/error.hpp
> +++ b/src/error.hpp
> @@ -9,6 +9,7 @@
>  
>  #include <exception>
>  #include <string>
> +#include <syslog.h>
>  
>  
>  namespace spice {
> @@ -39,6 +40,19 @@ public:
>      ReadError(const std::string &msg, int errno_);
>  };
>  
> +class WriteError : public IOError
> +{
> +public:
> +    WriteError(const std::string &msg, int errno_);
> +};
> +
> +template<class T>
> +const T &syslog(const T &e) noexcept

Why not accepting a const std::exception &e instead of the template?

> +{
> +    ::syslog(LOG_ERR, "%s\n", e.what());
> +    return e;
> +}
> +
>  }} // namespace spice::streaming_agent
>  
>  #endif // SPICE_STREAMING_AGENT_ERROR_HPP
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 2c0340d..26258d0 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -8,6 +8,7 @@
>  #include "hexdump.h"
>  #include "mjpeg-fallback.hpp"
>  #include "stream-port.hpp"
> +#include "error.hpp"
>  
>  #include <spice/stream-device.h>
>  #include <spice/enums.h>
> @@ -113,9 +114,7 @@ static void handle_stream_capabilities(uint32_t len)
>          STREAM_TYPE_CAPABILITIES,
>          0
>      };
> -    if (write_all(streamfd, &hdr, sizeof(hdr)) != sizeof(hdr)) {
> -        throw std::runtime_error("error writing capabilities");
> -    }
> +    write_all(streamfd, &hdr, sizeof(hdr));
>  }
>  
>  static void handle_stream_error(size_t len)
> @@ -186,7 +185,7 @@ static int read_command(bool blocking)
>      return 1;
>  }
>  
> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> +static void spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>  {
>  
>      SpiceStreamFormatMessage msg;
> @@ -201,40 +200,23 @@ static int spice_stream_send_format(unsigned w,
> unsigned h, unsigned c)
>      msg.msg.codec = c;
>      syslog(LOG_DEBUG, "writing format\n");
>      std::lock_guard<std::mutex> stream_guard(stream_mtx);
> -    if (write_all(streamfd, &msg, msgsize) != msgsize) {
> -        return EXIT_FAILURE;
> -    }
> -    return EXIT_SUCCESS;
> +    write_all(streamfd, &msg, msgsize);
>  }
>  
> -static int spice_stream_send_frame(const void *buf, const unsigned size)
> +static void spice_stream_send_frame(const void *buf, const unsigned size)
>  {
>      SpiceStreamDataMessage msg;
>      const size_t msgsize = sizeof(msg);
> -    ssize_t n;
>  
>      memset(&msg, 0, msgsize);
>      msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>      msg.hdr.type = STREAM_TYPE_DATA;
>      msg.hdr.size = size; /* includes only the body? */
>      std::lock_guard<std::mutex> stream_guard(stream_mtx);
> -    n = write_all(streamfd, &msg, msgsize);
> -    syslog(LOG_DEBUG,
> -           "wrote %ld bytes of header of data msg with frame of size %u
> bytes\n",
> -           n, msg.hdr.size);
> -    if (n != msgsize) {
> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
> -               n, msgsize);
> -        return EXIT_FAILURE;
> -    }
> -    n = write_all(streamfd, buf, size);
> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
> -    if (n != size) {
> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
> -               n, size);
> -        return EXIT_FAILURE;
> -    }
> -    return EXIT_SUCCESS;
> +    write_all(streamfd, &msg, msgsize);
> +    write_all(streamfd, buf, size);
> +
> +    syslog(LOG_DEBUG, "Sent a frame of size %u\n", size);
>  }
>  
>  /* returns current time in micro-seconds */
> @@ -403,9 +385,7 @@ do_capture(const char *streamport, FILE *f_log)
>  
>                  syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>                  codec);
>  
> -                if (spice_stream_send_format(width, height, codec) ==
> EXIT_FAILURE) {
> -                    throw std::runtime_error("FAILED to send format
> message");
> -                }
> +                spice_stream_send_format(width, height, codec);
>              }
>              if (f_log) {
>                  if (log_binary) {
> @@ -416,10 +396,14 @@ do_capture(const char *streamport, FILE *f_log)
>                      hexdump(frame.buffer, frame.buffer_size, f_log);
>                  }
>              }
> -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
> EXIT_FAILURE) {
> -                syslog(LOG_ERR, "FAILED to send a frame\n");
> +
> +            try {
> +                spice_stream_send_frame(frame.buffer, frame.buffer_size);
> +            } catch (const WriteError& e) {

Why not catching IOError, you never know how spice_stream_send_frame can
evolve.

> +                syslog(e);
>                  break;

OT: what is this break suppose to do? Or better what the caller can do with
it ?

>              }
> +
>              //usleep(1);
>              if (read_command(false) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> diff --git a/src/stream-port.cpp b/src/stream-port.cpp
> index f256698..526c564 100644
> --- a/src/stream-port.cpp
> +++ b/src/stream-port.cpp
> @@ -34,7 +34,7 @@ void read_all(int fd, void *msg, size_t len)
>      }
>  }
>  
> -size_t write_all(int fd, const void *buf, const size_t len)
> +void write_all(int fd, const void *buf, const size_t len)
>  {
>      size_t written = 0;
>      while (written < len) {
> @@ -43,13 +43,10 @@ size_t write_all(int fd, const void *buf, const size_t
> len)
>              if (errno == EINTR) {
>                  continue;
>              }
> -            syslog(LOG_ERR, "write failed - %m");
> -            return l;
> +            throw WriteError("Writing message to device failed", errno);
>          }
>          written += l;
>      }
> -    syslog(LOG_DEBUG, "write_all -- %u bytes written\n", (unsigned)written);
> -    return written;
>  }
>  
>  }} // namespace spice::streaming_agent
> diff --git a/src/stream-port.hpp b/src/stream-port.hpp
> index a296a5c..7780a37 100644
> --- a/src/stream-port.hpp
> +++ b/src/stream-port.hpp
> @@ -14,7 +14,7 @@ namespace spice {
>  namespace streaming_agent {
>  
>  void read_all(int fd, void *msg, size_t len);
> -size_t write_all(int fd, const void *buf, const size_t len);
> +void write_all(int fd, const void *buf, const size_t len);
>  
>  }} // namespace spice::streaming_agent
>  

Frediano