> On 22 Feb 2018, at 10:42, Lukáš Hrázký <lhra...@redhat.com> wrote:
> 
> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinec...@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.

I realize that I only shared the benefits in a private message, not on-list. So 
here:

The benefit I see in this approach are

1. there is no alloc at the throw point,
2. the catch point can extract a specific exception type and its args (e.g. for 
unit testing)
3. formatting is in the class, rather than ad-hoc for every single throw
4. We can distinguish errors we “expect” (e.g. Error and derived) from those we 
expect “less” (e.g. runtime_error).

Which ones do you disagree with?


> How exactly makes it easier to format messages?

Consider for example if you wanted to use a printf-style interface for 
formatting the message.

Two use cases:
1. Localization (printf is better than appending strings, allows for reordering)
2. Flight recorder


> 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).

You don’t “need to”, you “want to”. It’s important to be able to discriminate 
between classes of error. For exceptions, the exception class plays the same 
role as errno values. If you would return the same errno, you can throw the 
same class. Otherwise, you shouldn’t.


> 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).

There are cases were we can reuse the same exception class, but using 
runtime_error is not that class.

> 
>> Signed-off-by: Christophe de Dinechin <dinec...@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.

I don’t like introducing a class without its use case, you can’t test it.

> Also, it could go in it's own file straight away.

Yes. 

> 
>> 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.

No. It’s for a write error as the name indicates. I could have a ReadError with 
the same signature, and if I really have both, have an IOError base with two 
trivial derived classes.

If at some point we want to deal with such errors, how we reconnect is likely 
to differ for read and write errors. Same for unit testing.


> 
>> +            : 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?

It is, which is, as I pointed out, the reason the standard stuck with const 
char *.

But in our application, at the point I’m causing the potential problem, cleanup 
already happened. In other words, I’m moving allocations at the place where 
they are most likely to succeed.

BTW, if we are so out of memory that this will cause a bad_alloc, then iostream 
itself may also throw it, so short of going the long way and using ‘write’, we 
can’t do much better.

> You construct the error string during the exception handling, thus if this 
> throws an exception (as it can), you get a terminate?

The (only safe) alternative is to format the message in a pre-allocated static 
buffer using snprintf, and to print it with ::write to avoid anything that can 
allocate buffers. A bit overkill IMO.

> 
> 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;
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to