[Spice-devel] [PATCH spice-streaming-agent 4/9] Introduce a WriteError exception for write_all()
Christophe de Dinechin
cdupontd at redhat.com
Wed May 16 09:57:12 UTC 2018
> On 16 May 2018, at 11:13, Lukáš Hrázký <lhrazky at redhat.com> 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 at redhat.com>
>>> ---
>>> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list