On Thu, 2018-03-01 at 17:52 +0100, Christophe de Dinechin wrote:
> > On 1 Mar 2018, at 16:47, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > On Thu, 2018-03-01 at 16:23 +0100, Christophe de Dinechin wrote:
> > > > 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.
> >
> > 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…
>
> Well, I have a rather large-ish series on dynamic options that changes this part quite a bit too. And for what it’s worth, with the new handling, we can detect when no plugin handles the option. IMO, it’s OK for a plugin to throw if it can’t deal with an option, and it’s up to the caller to retry. Of course, that means you have a specific exception class to start with ;-)
>
>
> >
> > > >
> > > > > }
> > > > > }
> > > > > }
> > > > > 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 do not arbitrarily want to discriminate exceptions. I’m balancing that against the KISS-based desire to not create exception classes that are not needed. There is nothing “clever” in saying that it’s a similar issue if we have a bad protocol version or a bad incoming message. Both belong to the same class of “what is this input data? I expected A and I got B instead”.
>
> Again, there is nothing really strong or magical here. If we need a new error class, we can add it in a sensible way. The framework is in place (after that series, that is).
Ok, agreed.
> >
> > > 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.
>
> Well, it depends if you really got a wrong protocol version, or if you interpreted as a “version” some other set of bytes because you were out of sync. This is also true for other MessageData errors. In all cases, you can retry with the idea that closing/reopening can put you back in sync. But if the retry fails, then you give up.
I see. Just a point that once we start incrementing the version, it
will become much more probable the exception will mean just that,
incompatible version, instead of bad data... But the possibility is
still there, yeah.
> But again, if I’m wrong on this prediction, it will be easy enough to add one class and change one throw point. The goal is not to predict the future with 100% accuracy, just to head in the right direction and provide a sensible basis for error handling.
>
> >
> > > >
> > > > > }
> > > > >
> > > > > 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 :)
>
> This comment ignores the explanations in the commit logs, the explanations on this mailing list, the existence of format_message and syslog member functions, and half a dozen other key points. There is no equivalence whatsoever.
:D The equivalence was meant in the single aspect of discriminating
exceptions. It was also meant as a minor remark, so lets not dwell on
this :)
>
> >
> > > 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.
>
> The framework already has provision for that. Use REQUIRE_THROWS_AS.
That does not allow you to check the message text.
>
> > 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().
>
> We own the error policy for agent plugins. If the error policy is that:
>
> - Either plugins pass along the exception to the agent, or
> - They have to report errors using Error::syslog()
>
> then the problem goes away. And frankly, it’s better than ad-hoc formatting on a plugin basis.
>
> > And all that for the unlikely and (to me) unclear advantage in out of
> > memory situations?
>
> I’m annoyed you (once again) singled out one of the many advantages I already exposed, in order to incorrectly present the two approaches as roughly equivalent when they really are not. There are many reasons why the C++ Core Guidelines discourage the kind of uses of std::runtime_error that is currently done by the agent. The out-of-memory situation is only one of the many benefits I already documented.
And I'm annoyed you (once again) take my specific remark as a general
push-back against your whole patch :) (not annoyed, actually, just
joking! ;)) See below.
>
> > Are you sure you want to do it?
>
> I find this question interesting. Do you seriously assume that I sent this patch series without “wanting” to do this?
>
> Here is my question back to you. Why the push back? It’s not as if this was giving you extra work or making things more complicated. It opens a number of possibilities that are next to impossible with the current code, including, but not limited to, being able to intelligently deal with different categories of errors, localizing error messages, reusing message formatting, having a single place where we send everything to syslog (which is a good breakpoint location, BTW), etc.
I think there must be some critical difference in the way we think,
that we seem to keep misunderstanding each other even though I think we
both try to make ourselves very clear. Maybe it's my fault, I'm new to
this mail-only discussions.
Anyway, in this specific email thread it happened several times that I
was discussing some concrete aspect of your error handling design and
you generalized my remarks as criticism of the whole. It was not meant
that way. Please let me know if I can amend that in the future and make
myself more clear. I suppose it's mostly when I try to express (often
minor) disadvantages, wanting to make sure you take them into account,
which you interpret as major/blocker points and address them as such.
Specifically for this part of the discussion and my "Are you sure you
want to do it?" question, I would think it was pretty clear I was
asking purely about the delayed formatting function and therefore not
having a complete message in what(). I've posed it to ask you to
reconsider your decision in the light of the facts I presented :) I can
tell you I didn't have high hopes too ;D But still wanted to do it.
For the patch in whole, I am certainly not pushing against it! For one,
I find Error::syslog() a clear benefit. And having an exception
hierarchy in place can surely only be a good thing :)
Cheers,
Lukas
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel