> On 1 Mar 2018, at 15:42, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> 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”.
OK, done
>
> 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.
OK.
>
>> }
>> } 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”.
OK.
>
>> }
>> } 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... :)
I chose OptionError and OptionValueError, with two distinct messages.
>
>> }
>> }
>> }
>> 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.
As made plain by the inheritance I chose, I purposefully used “Read Error” to mean any error while reading data, not just errors generated by the “read()” system call.
>From ReadError, I derived ProtocolError for errors detected “in” the data and not “while” reading data.
And MessageDataError for any error where there is a mismatch between what we read and some expected value.
So no, it is not an accident, it was carefully designed that way :-)
I also believe that any future “retry path” read errors (i.e. closing the stream and reopening) would also be appropriate to address a protocol or message error (although you could be finer-grained for those). So the class hierarchy makes sense on that front too.
>
>> }
>>
>> 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.
Same answer.
>
>> }
>>
>> 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.
Not at all. Error is ours, runtime_error is not.
But your comment that you did not want to invent classes if we did not need them was correct. I took it into account here. If we come up with a specific action on this error, we can then create a class for it. (for Read or Write errors, I have at least an idea of what we could do to repair, here I don’t).
>
>> }
>>
>> 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?
Yes (and the tests pass). The test framework uses the default what() method, which returns the base message (I’ve been careful to make these base messages informative). We could modify the framework to catch Error and use format_message() instead. Left as an exercise for a future patch.
>
> If it would be correct, it would be clearly a step back, as the message
> doesn't contain the faulty value.
The message seen by the user does. The message seen by what() does not
>
>> );
>> }
>> }
>> @@ -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.
Same answer.
>
> Lukas
>
>> );
>> }
>> }
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel