[Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return
Christophe de Dinechin
christophe.de.dinechin at gmail.com
Tue Feb 20 22:25:32 UTC 2018
> On 20 Feb 2018, at 22:38, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin at redhat.com>
>>
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later
>> patch)
>> Fixes a race condition, make sure we only use stream after opening
>>
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 79 +++++++++++++++++++++++++------
>> ------------
>> 1 file changed, 47 insertions(+), 32 deletions(-)
>>
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
>> agent.cpp
>> index 646d15b..9b8fd45 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>> StreamMsgData msg;
>> };
>>
>> +struct SpiceStreamCursorMessage
>> +{
>> + StreamDevHeader hdr;
>> + StreamMsgCursorSet msg;
>> +};
>> +
>> +class Stream
>> +{
>> +public:
>> + Stream(const char *name)
>> + {
>> + fd = open(name, O_RDWR);
>> + if (fd < 0)
>> + throw std::runtime_error("failed to open streaming
>> device");
>> + }
>> + ~Stream()
>> + {
>> + close(fd);
>> + }
>> + operator int() { return fd; }
>> +
>> +private:
>> + int fd = -1;
>> +};
>
>
> What about making the 'Stream' constructor take an fd so that the same
> class can be used to encapsulate e.g. socket fds as well?
I guess you are thinking about the vsock case?
Well, the whole idea of RAII is that one class has both open and close, so I don’t think passing an fd as input would work.
If we need to deal with a socket fd, we could have an additional ctor doing a ‘socket’ call (the dtor closing the socket), and another one looking like ‘accept’, something like:
Stream(int domain, int type, int protocol); // socket()-like
Stream(int sockfd, struct sockaddr *addr, socklen_t *len); // accept-like
If we do that, then each ‘Stream’ would own one fd and only one, and be responsible for closing it. But then, we’d probably want to split the Stream class between the fd-management aspect (deals with the fd only, offers read and write), and the message packaging aspect (holds a mutex, responsible for writing or receiving messages, the function I called ‘send’, …)
That being said, my objective was not to create some sort of general purpose class, KISS, just what we need. If what we need is for vsocks, that can be done in a later step as I outlined above without too much disruption.
Christophe
>
>
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> static std::set<SpiceVideoCodecType> client_codecs;
>> -static int streamfd = -1;
>> static std::mutex stream_mtx;
>>
>> -static int have_something_to_read(int timeout)
>> +static int have_something_to_read(int streamfd, int timeout)
>> {
>> struct pollfd pollfd = {streamfd, POLLIN, 0};
>>
>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>> return 0;
>> }
>>
>> -static int read_command_from_device(void)
>> +static int read_command_from_device(int streamfd)
>> {
>> StreamDevHeader hdr;
>> uint8_t msg[64];
>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>> return 1;
>> }
>>
>> -static int read_command(bool blocking)
>> +static int read_command(int streamfd, bool blocking)
>> {
>> int timeout = blocking?-1:0;
>> while (!quit_requested) {
>> - if (!have_something_to_read(timeout)) {
>> + if (!have_something_to_read(streamfd, timeout)) {
>> if (!blocking) {
>> return 0;
>> }
>> sleep(1);
>> continue;
>> }
>> - return read_command_from_device();
>> + return read_command_from_device(streamfd);
>> }
>>
>> return 1;
>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t
>> len)
>> return written;
>> }
>>
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned
>> c)
>> +static int spice_stream_send_format(int streamfd, unsigned w,
>> unsigned h, unsigned c)
>> {
>>
>> SpiceStreamFormatMessage msg;
>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w,
>> unsigned h, unsigned c)
>> return EXIT_SUCCESS;
>> }
>>
>> -static int spice_stream_send_frame(const void *buf, const unsigned
>> size)
>> +static int spice_stream_send_frame(int streamfd, const void *buf,
>> const unsigned size)
>> {
>> SpiceStreamDataMessage msg;
>> const size_t msgsize = sizeof(msg);
>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>> }
>>
>> static void
>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int
>> hotspot_y,
>> +send_cursor(int streamfd, unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>> std::function<void(uint32_t *)> fill_cursor)
>> {
>> if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>> write_all(streamfd, msg.get(), cursor_size);
>> }
>>
>> -static void cursor_changes(Display *display, int event_base)
>> +static void cursor_changes(int streamfd, Display *display, int
>> event_base)
>> {
>> unsigned long last_serial = 0;
>>
>> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display,
>> int event_base)
>> for (unsigned i = 0; i < cursor->width * cursor->height;
>> ++i)
>> pixels[i] = cursor->pixels[i];
>> };
>> - send_cursor(cursor->width, cursor->height, cursor->xhot,
>> cursor->yhot, fill_cursor);
>> + send_cursor(streamfd,
>> + cursor->width, cursor->height, cursor->xhot,
>> cursor->yhot, fill_cursor);
>> }
>> }
>>
>> static void
>> -do_capture(const char *streamport, FILE *f_log)
>> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>> {
>> - streamfd = open(streamport, O_RDWR);
>> - if (streamfd < 0)
>> - throw std::runtime_error("failed to open the streaming
>> device (" +
>> - std::string(streamport) + "): "
>> - + strerror(errno));
>> -
>> unsigned int frame_count = 0;
>> while (!quit_requested) {
>> while (!quit_requested && !streaming_requested) {
>> - if (read_command(true) < 0) {
>> + if (read_command(streamfd, true) < 0) {
>> syslog(LOG_ERR, "FAILED to read command\n");
>> - goto done;
>> + return;
>> }
>> }
>>
>> @@ -370,7 +389,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)
>> + if (spice_stream_send_format(streamfd, width,
>> height, codec) == EXIT_FAILURE)
>> throw std::runtime_error("FAILED to send format
>> message");
>> }
>> if (f_log) {
>> @@ -382,23 +401,18 @@ 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) {
>> + if (spice_stream_send_frame(streamfd,
>> + frame.buffer,
>> frame.buffer_size) == EXIT_FAILURE) {
>> syslog(LOG_ERR, "FAILED to send a frame\n");
>> break;
>> }
>> //usleep(1);
>> - if (read_command(false) < 0) {
>> + if (read_command(streamfd, false) < 0) {
>> syslog(LOG_ERR, "FAILED to read command\n");
>> - goto done;
>> + return;
>> }
>> }
>> }
>> -
>> -done:
>> - if (streamfd >= 0) {
>> - close(streamfd);
>> - streamfd = -1;
>> - }
>> }
>>
>> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>> Window rootwindow = DefaultRootWindow(display);
>> XFixesSelectCursorInput(display, rootwindow,
>> XFixesDisplayCursorNotifyMask);
>>
>> - std::thread cursor_th(cursor_changes, display, event_base);
>> + Stream streamfd(streamport);
>> + std::thread cursor_th(cursor_changes, (int) streamfd, display,
>> event_base);
>> cursor_th.detach();
>>
>> int ret = EXIT_SUCCESS;
>> try {
>> - do_capture(streamport, f_log);
>> + do_capture(streamfd, streamport, f_log);
>> }
>> catch (std::runtime_error &err) {
>> syslog(LOG_ERR, "%s\n", err.what());
> _______________________________________________
> 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