[Spice-devel] [PATCH 18/22] Convert existing std::runtime_error to the new Error classes
Lukáš Hrázký
lhrazky at redhat.com
Thu Mar 1 15:47:35 UTC 2018
On Thu, 2018-03-01 at 16:23 +0100, Christophe de Dinechin wrote:
> > On 1 Mar 2018, at 15:42, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> >
> > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin at redhat.com>
> > >
> > > 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 at redhat.com>
> > > ---
> > > 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.
Actually, taking into account Frediano's recent patch to remove the
OptionError case (the options are global so plugins should ignore
unknown options, since they probably belong to another plugin), you can
scratch this :) I've read his patch right after finishing with your
series...
> >
> > > }
> > > }
> > > }
> > > 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 :-)
Well... I find it goes somewhat against you wanting to discriminate
exceptions, this line of inheritance and using the MessageDataError in
this "clever" way seems to eliminate the ability to differentiate them.
> 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.
Here I am really confused by that thought. Surely no amount of retrys
is going to help with a wrong protocol version.
> >
> > > }
> > >
> > > 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.
Indeed, but if you eliminate all runtime_errors from the code, it
probably becomes equivalent :)
> 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).
Agreed :)
> >
> > > }
> > >
> > > 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.
I see, I havent realized this aspect. I find it a disadvantage of your
design though. For us, the only place it probably matters are the
tests. I am not sure this could be somehow changed in the framework
without forking it, I would guess not... We could probably add our own
macros or helpers to check for the correct message.
But what about the plugins? If we ever throw some exceptions that can
be caught in the plugins, I find it goes against the standard to throw
exceptions that don't provide the full error message in what().
And all that for the unlikely and (to me) unclear advantage in out of
memory situations? Are you sure you want to do it?
Cheers,
Lukas
> >
> > 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
More information about the Spice-devel
mailing list