On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> Appropriate error classes been created for the existing messages:
> - ProtocolError: handles the various kinds of protocol-related errors
> - MessageDataError: a form of protocol error where we want to report
> what was read and what was expected (e.g. message length error,
> version error, etc)
>
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
> include/spice-streaming-agent/errors.hpp | 28 ++++++++++++++++++++++++++++
> src/errors.cpp | 12 ++++++++++++
> src/mjpeg-fallback.cpp | 10 ++++++----
> src/spice-streaming-agent.cpp | 25 ++++++++++---------------
> src/unittests/test-mjpeg-fallback.cpp | 4 ++--
> 5 files changed, 58 insertions(+), 21 deletions(-)
>
> diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
> index 72e9910..870a0fd 100644
> --- a/include/spice-streaming-agent/errors.hpp
> +++ b/include/spice-streaming-agent/errors.hpp
> @@ -60,6 +60,34 @@ public:
> ReadError(const char *msg, int saved_errno): IOError(msg, saved_errno) {}
> };
>
> +class ProtocolError : public ReadError
> +{
> +public:
> + ProtocolError(const char *msg, int saved_errno = 0): ReadError(msg, saved_errno) {}
> +};
> +
> +class MessageDataError : public ProtocolError
> +{
> +public:
> + MessageDataError(const char *msg, size_t received, size_t expected, int saved_errno = 0)
> + : ProtocolError(msg, saved_errno), received(received), expected(expected) {}
> + int format_message(char *buffer, size_t size) const noexcept override;
> +protected:
> + size_t received;
> + size_t expected;
> +};
> +
> +class OptionError : public Error
> +{
> +public:
> + OptionError(const char *msg, const char *option, const char *value)
> + : Error(msg), option(option), value(value) {}
> + int format_message(char *buffer, size_t size) const noexcept override;
> +protected:
> + const char *option;
> + const char *value;
> +};
> +
> }} // namespace spice::streaming_agent
>
> #endif // SPICE_STREAMING_AGENT_ERRORS_HPP
> diff --git a/src/errors.cpp b/src/errors.cpp
> index ff25142..4dc45e4 100644
> --- a/src/errors.cpp
> +++ b/src/errors.cpp
> @@ -59,4 +59,16 @@ int WriteError::format_message(char *buffer, size_t size) const noexcept
> return append_strerror(buffer, size, written);
> }
>
> +int MessageDataError::format_message(char *buffer, size_t size) const noexcept
> +{
> + int written = snprintf(buffer, size, "%s (received %zu, expected %zu)",
> + what(), received, expected);
> + return append_strerror(buffer, size, written);
> +}
> +
> +int OptionError::format_message(char *buffer, size_t size) const noexcept
> +{
> + return snprintf(buffer, size, "%s ('%s' is not a valid %s)", what(), value, option);
> +}
> +
> }} // namespace spice::streaming_agent
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 5758893..1a98535 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -7,6 +7,8 @@
> #include <config.h>
> #include "mjpeg-fallback.hpp"
>
> +#include <spice-streaming-agent/errors.hpp>
> +
> #include <cstring>
> #include <exception>
> #include <stdexcept>
> @@ -59,7 +61,7 @@ MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& settings):
> {
> dpy = XOpenDisplay(NULL);
> if (!dpy)
> - throw std::runtime_error("Unable to initialize X11");
> + throw Error("unable to initialize X11");
> }
>
> MjpegFrameCapture::~MjpegFrameCapture()
> @@ -157,16 +159,16 @@ void MjpegPlugin::ParseOptions(const ConfigureOption *options)
> try {
> settings.fps = stoi(value);
> } catch (const std::exception &e) {
> - throw std::runtime_error("Invalid value '" + value + "' for option 'framerate'.");
> + throw OptionError("invalid mjpeg framerate", options->value, "mjpeg framerate");
You should put the exact option name here, which is "mjpeg.framerate".
Also, the error message that comes out of this is not that great:
"invalid mjpeg framerate ('XXX' is not a valid mjpeg framerate)"
It should state "'XXX' is not a valid value for plugin option
'mjpeg.framerate'" to be clear.
> }
> } else if (name == "mjpeg.quality") {
> try {
> settings.quality = stoi(value);
> } catch (const std::exception &e) {
> - throw std::runtime_error("Invalid value '" + value + "' for option 'mjpeg.quality'.");
> + throw OptionError("invalid mjpeg quality", options->value, "mjpeg quality");
Ditto, "mjpeg.quality".
> }
> } else {
> - throw std::runtime_error("Invalid option '" + name + "'.");
> + throw OptionError("invalid option name", options->name, "mjpeg option name");
No intention to be stingy, but this should probably be a different
error class :) (i.e. there are two distinct errors,
InvalidOptionValueError and UnknownOptionError) You've managed to make
the message formatting somewhat work, though... :)
> }
> }
> }
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 9048935..35e65bb 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -67,7 +67,7 @@ public:
> {
> streamfd = open(name, O_RDWR);
> if (streamfd < 0) {
> - throw std::runtime_error("failed to open streaming device");
> + throw IOError("failed to open streaming device", errno);
> }
> }
> ~Stream()
> @@ -352,13 +352,11 @@ void Stream::handle_stream_start_stop(uint32_t len)
> uint8_t msg[256];
>
> if (len >= sizeof(msg)) {
> - throw std::runtime_error("msg size (" + std::to_string(len) + ") is too long "
> - "(longer than " + std::to_string(sizeof(msg)) + ")");
> + throw MessageDataError("message is too long", len, sizeof(msg));
> }
> int n = read(streamfd, &msg, len);
> if (n != (int) len) {
> - throw std::runtime_error("read command from device FAILED -- read " + std::to_string(n) +
> - " expected " + std::to_string(len));
> + throw MessageDataError("read start/stop command from device failed", n, len, errno);
> }
> is_streaming = (msg[0] != 0); /* num_codecs */
> syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
> @@ -374,12 +372,11 @@ void Stream::handle_stream_capabilities(uint32_t len)
> uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>
> if (len > sizeof(caps)) {
> - throw std::runtime_error("capability message too long");
> + throw MessageDataError("capability message too long", len, sizeof(caps));
> }
> int n = read(streamfd, caps, len);
> if (n != (int) len) {
> - throw std::runtime_error("read command from device FAILED -- read " + std::to_string(n) +
> - " expected " + std::to_string(len));
> + throw MessageDataError("read capabilities from device failed", n, len, errno);
> }
>
> // we currently do not support extensions so just reply so
> @@ -389,7 +386,7 @@ void Stream::handle_stream_capabilities(uint32_t len)
> void Stream::handle_stream_error(uint32_t len)
> {
> // TODO read message and use it
> - throw std::runtime_error("got an error message from server");
> + throw ProtocolError("got an error message from server");
> }
>
> void Stream::read_command_from_device()
> @@ -400,12 +397,10 @@ void Stream::read_command_from_device()
> std::lock_guard<std::mutex> stream_guard(mutex);
> n = read(streamfd, &hdr, sizeof(hdr));
> if (n != sizeof(hdr)) {
> - throw std::runtime_error("read command from device FAILED -- read " + std::to_string(n) +
> - " expected " + std::to_string(sizeof(hdr)));
> + throw MessageDataError("read command from device failed", n, sizeof(hdr), errno);
> }
> if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
> - throw std::runtime_error("BAD VERSION " + std::to_string(hdr.protocol_version) +
> - " (expected is " + std::to_string(STREAM_DEVICE_PROTOCOL) + ")");
> + throw MessageDataError("bad protocol version", hdr.protocol_version, STREAM_DEVICE_PROTOCOL);
I don't think this is a ReadError (from which MessageDataError
inherits), also the constructor arguments have a completely different
meaning. It just matches the "expected: X received: Y" template by
accident.
> }
>
> switch (hdr.type) {
> @@ -416,7 +411,7 @@ void Stream::read_command_from_device()
> case STREAM_TYPE_START_STOP:
> return handle_stream_start_stop(hdr.size);
> }
> - throw std::runtime_error("UNKNOWN msg of type " + std::to_string(hdr.type));
> + throw MessageDataError("unknown message type", hdr.type, 0);
Same as above.
> }
>
> int Stream::read_command(bool blocking)
> @@ -507,7 +502,7 @@ void ConcreteAgent::CaptureLoop(Stream &stream, FrameLog &frame_log)
>
> std::unique_ptr<FrameCapture> capture(GetBestFrameCapture(stream.client_codecs()));
> if (!capture) {
> - throw std::runtime_error("cannot find a suitable capture system");
> + throw Error("cannot find a suitable capture system");
>From the point of view of the argument "throwing specific error classes
for specific errors", this is basically equivalent to throwing a
runtime_error.
> }
>
> while (!quit_requested && stream.streaming_requested()) {
> diff --git a/src/unittests/test-mjpeg-fallback.cpp b/src/unittests/test-mjpeg-fallback.cpp
> index 4a152fe..704decd 100644
> --- a/src/unittests/test-mjpeg-fallback.cpp
> +++ b/src/unittests/test-mjpeg-fallback.cpp
> @@ -35,7 +35,7 @@ SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
> THEN("ParseOptions throws an exception") {
> REQUIRE_THROWS_WITH(
> plugin.ParseOptions(options.data()),
> - "Invalid option 'wakaka'."
> + "invalid option name"
This does not seem to match the OptionError::format_message above, are
you sure this is correct?
If it would be correct, it would be clearly a step back, as the message
doesn't contain the faulty value.
> );
> }
> }
> @@ -50,7 +50,7 @@ SCENARIO("test parsing mjpeg plugin options", "[mjpeg][options]") {
> THEN("ParseOptions throws an exception") {
> REQUIRE_THROWS_WITH(
> plugin.ParseOptions(options.data()),
> - "Invalid value 'toot' for option 'mjpeg.quality'."
> + "invalid mjpeg quality"
Same here.
Lukas
> );
> }
> }
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel