[Spice-devel] [PATCH spice-streaming-agent 4/9] Introduce a WriteError exception for write_all()
Lukáš Hrázký
lhrazky at redhat.com
Wed May 16 09:13:46 UTC 2018
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), 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.
> > + 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
More information about the Spice-devel
mailing list