Re: [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

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

 




> On 16 Feb 2018, at 17:40, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>> 
>> 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@xxxxxxxxxx>
>> ---
>> 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;
>> +};
>> +
> 
> This is weird in this patch

Yes. I may have merged two patches. Will check.

> 
>> +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);
> 
> You probably what to check for -1 to avoid calling close(-1)

I think this cannot happen, since in that case you’d have thrown from the ctor.

> 
>> +    }
>> +    operator int() { return fd; }
> 
> Sure you want this? I think is safer to avoid implicit cast
> also considering stuff like
> 
>  Stream s;
>  if (s) …

It is removed in a later patch once it’s no longer needed. The objective here is to minimize noisy changes at each step.

> 
>> +
>> +private:
>> +    int fd = -1;
> 
> So with a default constructor you want a class with
> an invalid state?
> Or just disable default constructor.

It’s disabled, there’s a ctor with args.

> I would disable also copy.

Also implicitly disabled.

> 
>> +};
>> +
>> 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)
> 
> maybe would be better to use the "fd" name here, is no more
> bound to stream.

Kept to minimize changes

>> {
>>     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)
> 
> Have you considered Stream::read_command?

For that specific case, I liked the “from_device”. But…

> Ok, mostly in a following patch


> 
>> {
>>     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);
> 
> Not a fun of this implicit conversion

Well, we can make it explicitl. But this is temporary, and is suppressed by the end of the series.

> .
> 
>>     }
>>     catch (std::runtime_error &err) {
>>         syslog(LOG_ERR, "%s\n", err.what());
> 
> Frediano

_______________________________________________
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]