Re: [PATCH spice-streaming-agent 4/9] Introduce a WriteError exception for write_all()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 16 May 2018, at 11:13, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> 
> On Tue, 2018-05-15 at 16:42 -0400, Frediano Ziglio wrote:
>>> 
>>> Update the interface to not return the size written, as it is not needed
>>> anymore.
>>> 
>>> Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
>>> ---
>>> src/error.cpp                 |  3 ++-
>>> src/error.hpp                 | 14 +++++++++++++
>>> src/spice-streaming-agent.cpp | 48
>>> +++++++++++++++----------------------------
>>> src/stream-port.cpp           |  7 ++-----
>>> src/stream-port.hpp           |  2 +-
>>> 5 files changed, 35 insertions(+), 39 deletions(-)
>>> 
>>> diff --git a/src/error.cpp b/src/error.cpp
>>> index 1b76ea4..4ef275c 100644
>>> --- a/src/error.cpp
>>> +++ b/src/error.cpp
>>> @@ -7,7 +7,6 @@
>>> #include "error.hpp"
>>> 
>>> #include <string.h>
>>> -#include <syslog.h>
>>> 
>>> 
>>> namespace spice {
>>> @@ -26,4 +25,6 @@ IOError::IOError(const std::string &msg, int errno_) :
>>> 
>>> ReadError::ReadError(const std::string &msg, int errno_) : IOError(msg,
>>> errno_) {}
>>> 
>>> +WriteError::WriteError(const std::string &msg, int errno_) : IOError(msg,
>>> errno_) {}
>>> +
>> 
>> Some comment on errno_ of previous patch.
>> 
>>> }} // namespace spice::streaming_agent
>>> diff --git a/src/error.hpp b/src/error.hpp
>>> index de1cb83..d69942c 100644
>>> --- a/src/error.hpp
>>> +++ b/src/error.hpp
>>> @@ -9,6 +9,7 @@
>>> 
>>> #include <exception>
>>> #include <string>
>>> +#include <syslog.h>
>>> 
>>> 
>>> namespace spice {
>>> @@ -39,6 +40,19 @@ public:
>>>     ReadError(const std::string &msg, int errno_);
>>> };
>>> 
>>> +class WriteError : public IOError
>>> +{
>>> +public:
>>> +    WriteError(const std::string &msg, int errno_);
>>> +};
>>> +
>>> +template<class T>
>>> +const T &syslog(const T &e) noexcept
>> 
>> Why not accepting a const std::exception &e instead of the template?
> 
> Because then it wouldn't automatically deduce the T type and you'd have
> to write it like:
> 
> throw syslog<MyException>(MyException());
> 
> Note that the T is there because you need to return the actual type of
> the exception, not a predecessor (which was the version in c3d's patch,
> where syslog() was a method of the Error class),

Wow, good catch. You are correct.

> because if you don't
> throw it as the actual type, you then can't catch it as the actual
> type.
> 
>>> +{
>>> +    ::syslog(LOG_ERR, "%s\n", e.what());
>>> +    return e;
>>> +}
>>> +
>>> }} // namespace spice::streaming_agent
>>> 
>>> #endif // SPICE_STREAMING_AGENT_ERROR_HPP
>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>> index 2c0340d..26258d0 100644
>>> --- a/src/spice-streaming-agent.cpp
>>> +++ b/src/spice-streaming-agent.cpp
>>> @@ -8,6 +8,7 @@
>>> #include "hexdump.h"
>>> #include "mjpeg-fallback.hpp"
>>> #include "stream-port.hpp"
>>> +#include "error.hpp"
>>> 
>>> #include <spice/stream-device.h>
>>> #include <spice/enums.h>
>>> @@ -113,9 +114,7 @@ static void handle_stream_capabilities(uint32_t len)
>>>         STREAM_TYPE_CAPABILITIES,
>>>         0
>>>     };
>>> -    if (write_all(streamfd, &hdr, sizeof(hdr)) != sizeof(hdr)) {
>>> -        throw std::runtime_error("error writing capabilities");
>>> -    }
>>> +    write_all(streamfd, &hdr, sizeof(hdr));
>>> }
>>> 
>>> static void handle_stream_error(size_t len)
>>> @@ -186,7 +185,7 @@ static int read_command(bool blocking)
>>>     return 1;
>>> }
>>> 
>>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>>> +static void spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>>> {
>>> 
>>>     SpiceStreamFormatMessage msg;
>>> @@ -201,40 +200,23 @@ static int spice_stream_send_format(unsigned w,
>>> unsigned h, unsigned c)
>>>     msg.msg.codec = c;
>>>     syslog(LOG_DEBUG, "writing format\n");
>>>     std::lock_guard<std::mutex> stream_guard(stream_mtx);
>>> -    if (write_all(streamfd, &msg, msgsize) != msgsize) {
>>> -        return EXIT_FAILURE;
>>> -    }
>>> -    return EXIT_SUCCESS;
>>> +    write_all(streamfd, &msg, msgsize);
>>> }
>>> 
>>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>>> +static void spice_stream_send_frame(const void *buf, const unsigned size)
>>> {
>>>     SpiceStreamDataMessage msg;
>>>     const size_t msgsize = sizeof(msg);
>>> -    ssize_t n;
>>> 
>>>     memset(&msg, 0, msgsize);
>>>     msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>>>     msg.hdr.type = STREAM_TYPE_DATA;
>>>     msg.hdr.size = size; /* includes only the body? */
>>>     std::lock_guard<std::mutex> stream_guard(stream_mtx);
>>> -    n = write_all(streamfd, &msg, msgsize);
>>> -    syslog(LOG_DEBUG,
>>> -           "wrote %ld bytes of header of data msg with frame of size %u
>>> bytes\n",
>>> -           n, msg.hdr.size);
>>> -    if (n != msgsize) {
>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %lu\n",
>>> -               n, msgsize);
>>> -        return EXIT_FAILURE;
>>> -    }
>>> -    n = write_all(streamfd, buf, size);
>>> -    syslog(LOG_DEBUG, "wrote data msg body of size %ld\n", n);
>>> -    if (n != size) {
>>> -        syslog(LOG_WARNING, "write_all header: wrote %ld expected %u\n",
>>> -               n, size);
>>> -        return EXIT_FAILURE;
>>> -    }
>>> -    return EXIT_SUCCESS;
>>> +    write_all(streamfd, &msg, msgsize);
>>> +    write_all(streamfd, buf, size);
>>> +
>>> +    syslog(LOG_DEBUG, "Sent a frame of size %u\n", size);
>>> }
>>> 
>>> /* returns current time in micro-seconds */
>>> @@ -403,9 +385,7 @@ do_capture(const char *streamport, FILE *f_log)
>>> 
>>>                 syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>>>                 codec);
>>> 
>>> -                if (spice_stream_send_format(width, height, codec) ==
>>> EXIT_FAILURE) {
>>> -                    throw std::runtime_error("FAILED to send format
>>> message");
>>> -                }
>>> +                spice_stream_send_format(width, height, codec);
>>>             }
>>>             if (f_log) {
>>>                 if (log_binary) {
>>> @@ -416,10 +396,14 @@ do_capture(const char *streamport, FILE *f_log)
>>>                     hexdump(frame.buffer, frame.buffer_size, f_log);
>>>                 }
>>>             }
>>> -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
>>> EXIT_FAILURE) {
>>> -                syslog(LOG_ERR, "FAILED to send a frame\n");
>>> +
>>> +            try {
>>> +                spice_stream_send_frame(frame.buffer, frame.buffer_size);
>>> +            } catch (const WriteError& e) {
>> 
>> Why not catching IOError, you never know how spice_stream_send_frame can
>> evolve.
> 
> Well, yeah, but since you never know how the function will evolve, you
> can't really predict what kind of class should be in the catch
> clause... I somewhat prefer to keep it narrowed down, but it doesn't
> matter much IMO. If the exceptions that can be thrown from the functio
> never change, this catch block will most probably need revising.

Agree, would rather catch as a function of current code rather than future code.

> 
>>> +                syslog(e);
>>>                 break;
>> 
>> OT: what is this break suppose to do? Or better what the caller can do with
>> it ?
> 
> Not sure what you call the caller here. It seems the break breaks the
> inner while loop, causing, apart from unimportant side-effects, the
> FrameCapture to be reinitialized, but that shouldn't really affect any
> errors while sending the frame...
> 
>>>             }
>>> +
>>>             //usleep(1);
>>>             if (read_command(false) < 0) {
>>>                 syslog(LOG_ERR, "FAILED to read command\n");
>>> diff --git a/src/stream-port.cpp b/src/stream-port.cpp
>>> index f256698..526c564 100644
>>> --- a/src/stream-port.cpp
>>> +++ b/src/stream-port.cpp
>>> @@ -34,7 +34,7 @@ void read_all(int fd, void *msg, size_t len)
>>>     }
>>> }
>>> 
>>> -size_t write_all(int fd, const void *buf, const size_t len)
>>> +void write_all(int fd, const void *buf, const size_t len)
>>> {
>>>     size_t written = 0;
>>>     while (written < len) {
>>> @@ -43,13 +43,10 @@ size_t write_all(int fd, const void *buf, const size_t
>>> len)
>>>             if (errno == EINTR) {
>>>                 continue;
>>>             }
>>> -            syslog(LOG_ERR, "write failed - %m");
>>> -            return l;
>>> +            throw WriteError("Writing message to device failed", errno);
>>>         }
>>>         written += l;
>>>     }
>>> -    syslog(LOG_DEBUG, "write_all -- %u bytes written\n", (unsigned)written);
>>> -    return written;
>>> }
>>> 
>>> }} // namespace spice::streaming_agent
>>> diff --git a/src/stream-port.hpp b/src/stream-port.hpp
>>> index a296a5c..7780a37 100644
>>> --- a/src/stream-port.hpp
>>> +++ b/src/stream-port.hpp
>>> @@ -14,7 +14,7 @@ namespace spice {
>>> namespace streaming_agent {
>>> 
>>> void read_all(int fd, void *msg, size_t len);
>>> -size_t write_all(int fd, const void *buf, const size_t len);
>>> +void write_all(int fd, const void *buf, const size_t len);
>>> 
>>> }} // namespace spice::streaming_agent
>>> 
>> 
>> Frediano
> _______________________________________________
> 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]