[Spice-devel] [PATCH v2 22/24] Throw exception in case of write failure

[Lukáš Hrázký]

On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> This also introduces the spice::streaming_error::Error class, which we can reuse
> later as a base class for all agent-specific errors. This class provides a
> formatted 'message()' class that returns a string, making it easier to format
> errors without allocating memory at throw-time.

I am not at all convinced this approach has any advantages. How exactly
makes it easier to format messages? This means we would need classes
for all kinds of errors throughout the agent (unless you still want to
use runtime_errors in some places). While, as I said, it makes sense to
me in some places, in others I find it unnecessary work and code bloat
(for lack of better word to express myself).

> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 65 ++++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4aae2cb..a789aad 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -59,15 +59,30 @@ static uint64_t get_time(void)
>  
>  }
>  
> +class Error : public std::runtime_error
> +{
> +public:
> +    Error(const char *msg): runtime_error(msg) {}
> +    virtual std::string message() { return what(); }
> +};
> +

I think introducing this class would warrant a separate commit. Also,
it could go in it's own file straight away.

>  class Stream
>  {
>      typedef std::set<SpiceVideoCodecType> codecs_t;
>  
>  public:
> -    class WriteError : public std::runtime_error
> +    class WriteError final : public Error
>      {
>      public:
> -        WriteError(const char *msg): runtime_error(msg) {}
> +        WriteError(const char *msg, const char *operation, int saved_errno)

The class is called WriteError, but this signature is apparently meant
for an arbitrary operation that uses errno.

> +            : Error(msg), operation(operation), saved_errno(saved_errno) {}
> +        std::string message()
> +        {
> +            return Error::message() + " in " + operation + ": " + strerror(saved_errno);

Ok, but isn't this actually creating the problem you described in an
earlier email? You construct the error string during the exception
handling, thus if this throws an exception (as it can), you get a
terminate?

Lukas

> +        }
> +    private:
> +        const char *operation;
> +        int saved_errno;
>      };
>  
>  public:
> @@ -95,18 +110,11 @@ public:
>      int read_command(bool blocking);
>  
>      template <typename Message, typename ...PayloadArgs>
> -    bool send(PayloadArgs... payload)
> +    void send(PayloadArgs... payload)
>      {
>          Message message(payload...);
>          std::lock_guard<std::mutex> stream_guard(mutex);
> -        size_t expected = message.size(payload...);
> -        size_t written = message.write(*this, payload...);
> -        bool result = written == expected;
> -        if (!result) {
> -            syslog(LOG_ERR, "sent only %zu bytes out of %zu", written, expected);
> -            throw WriteError("Unable to write complete packet");
> -        }
> -        return result;
> +        message.write(*this, payload...);
>      }
>  
>      size_t write_all(const char *what, const void *buf, const size_t len);
> @@ -151,9 +159,9 @@ struct FormatMessage : Message<StreamMsgFormat, FormatMessage>
>      {
>          return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 = {} };
>      }
> -    size_t write(Stream &stream, unsigned w, unsigned h, uint8_t c)
> +    void write(Stream &stream, unsigned w, unsigned h, uint8_t c)
>      {
> -        return stream.write_all("FormatMessage", this, sizeof(message_t));
> +        stream.write_all("FormatMessage", this, sizeof(message_t));
>      }
>  };
>  
> @@ -170,10 +178,10 @@ struct FrameMessage : Message<StreamMsgData, FrameMessage>
>      {
>          return StreamMsgData();
>      }
> -    size_t write(Stream &stream, const void *frame, size_t length)
> +    void write(Stream &stream, const void *frame, size_t length)
>      {
> -        return stream.write_all("FrameMessage header", this, sizeof(message_t))
> -            +  stream.write_all("FrameMessage frame", frame, length);
> +        stream.write_all("FrameMessage header", this, sizeof(message_t));
> +        stream.write_all("FrameMessage frame", frame, length);
>      }
>  };
>  
> @@ -208,11 +216,11 @@ struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>              .data = { }
>          };
>      }
> -    size_t write(Stream &stream, XFixesCursorImage *cursor)
> +    void write(Stream &stream, XFixesCursorImage *cursor)
>      {
>          unsigned pixel_size = pixel_count(cursor) * sizeof(uint32_t);
> -        return stream.write_all("X11CursorMessage header", this, sizeof(message_t))
> -            +  stream.write_all("X11CursorMessage pixels", pixels.get(), pixel_size);
> +        stream.write_all("X11CursorMessage header", this, sizeof(message_t));
> +        stream.write_all("X11CursorMessage pixels", pixels.get(), pixel_size);
>      }
>      void fill_pixels(XFixesCursorImage *cursor)
>      {
> @@ -329,9 +337,7 @@ void X11CursorThread::cursor_changes()
>          }
>  
>          last_serial = cursor->cursor_serial;
> -        if (!stream.send<X11CursorMessage>(cursor)) {
> -            syslog(LOG_WARNING, "FAILED to send cursor\n");
> -        }
> +        stream.send<X11CursorMessage>(cursor);
>      }
>  }
>  
> @@ -462,7 +468,7 @@ size_t Stream::write_all(const char *what, const void *buf, const size_t len)
>                  continue;
>              }
>              syslog(LOG_ERR, "write %s failed - %m", what);
> -            return l;
> +            throw WriteError("streaming agent write failed", what, errno);
>          }
>          written += l;
>      }
> @@ -553,16 +559,13 @@ void ConcreteAgent::CaptureLoop(Stream &stream, FrameLog &frame_log)
>  
>                  syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height, codec);
>  
> -                if (!stream.send<FormatMessage>(width, height, codec))
> -                    throw std::runtime_error("FAILED to send format message");
> +                stream.send<FormatMessage>(width, height, codec);
>              }
>              if (frame_log) {
>                  frame_log.dump(frame.buffer, frame.buffer_size);
>              }
> -            if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
> -                syslog(LOG_ERR, "FAILED to send a frame\n");
> -                break;
> -            }
> +            stream.send<FrameMessage>(frame.buffer, frame.buffer_size);
> +
>              //usleep(1);
>              if (stream.read_command(false) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> @@ -640,6 +643,10 @@ int main(int argc, char* argv[])
>          FrameLog frame_log(log_filename, log_binary);
>          agent.CaptureLoop(streamfd, frame_log);
>      }
> +    catch (Error &err) {
> +        syslog(LOG_ERR, "%s\n", err.message().c_str());
> +        ret = EXIT_FAILURE;
> +    }
>      catch (std::exception &err) {
>          syslog(LOG_ERR, "%s\n", err.what());
>          ret = EXIT_FAILURE;