[Spice-devel] [PATCH 08/14] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

> On 15 Feb 2018, at 11:04, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin at redhat.com>
>> 
>> This lets us get rid of C-style 'goto done' in do_capture.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> 
> I honestly prefer the "defer" style way.

The standard C++ practice for resource management is RAII, not defer. RAII is a composable abstraction, i.e. I can pile structs in structs and it still works.

I don’t know how you can prefer ‘defer’. It is, in all honestly, a clever but vile and ugly hack. Frankly, a C macro? With a lambda? Using a variable named after the __LINE__ in the code? It’s not reusable, it stays very low level, it’s brittle, hard to write (forget the ; and die). Smart people like Uri had to ask on IRC how it worked and what it was supposed to do…

> 
> Beside that you correctly pointed out that there's a race condition
> on cursor thread which could lead the cursor thread to attempt writing
> the cursor shape before the device is open and you proposed some fixes
> in a different series.
> I think would be a better fix for this.

But it follows this patch and requires it.

> 
>> ---
>> src/spice-streaming-agent.cpp | 32 ++++++++++++++++++++------------
>> 1 file changed, 20 insertions(+), 12 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 25a38a6..e9ef310 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,6 +53,23 @@ struct SpiceStreamDataMessage
>>     StreamMsgData msg;
>> };
>> 
>> +struct Stream
>> +{
>> +    Stream(const char *name, int &fd): fd(fd)
>> +    {
>> +        fd = open(name, O_RDWR);
>> +        if (fd < 0)
>> +            throw std::runtime_error("failed to open streaming device");
>> +    }
>> +    ~Stream()
>> +    {
>> +        if (fd >= 0)
>> +            close(fd);
>> +        fd = -1;
>> +    }
>> +    int &fd;
>> +};
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> @@ -354,17 +371,14 @@ static void cursor_changes(Display *display, int
>> event_base)
>> static void
>> do_capture(const char *streamport, FILE *f_log)
>> {
>> -    streamfd = open(streamport.c_str(), O_RDWR);
>> -    if (streamfd < 0)
>> -        throw std::runtime_error("failed to open the streaming device (" +
>> -                                 streamport + "): " + strerror(errno));
>> +    Stream stream(streamport, streamfd);
>> 
>>     unsigned int frame_count = 0;
>>     while (!quit_requested) {
>>         while (!quit_requested && !streaming_requested) {
>>             if (read_command(true) < 0) {
>>                 syslog(LOG_ERR, "FAILED to read command\n");
>> -                goto done;
>> +                return;
>>             }
>>         }
>> 
>> @@ -422,16 +436,10 @@ do_capture(const char *streamport, FILE *f_log)
>>             //usleep(1);
>>             if (read_command(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__);
> 
> Frediano