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

[Christophe de Dinechin]

> On 15 Feb 2018, at 15:35, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> 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.
>> 
> 
> RAII is supposed to encapsulate a resource, your patch is encapsulating
> the allocation and disposal of an external resource, exactly as the defer
> generated code/data are doing.
> The difference is that your code handle only FILE* while defer supports
> different cases.
> In a perfect world everything would be already encapsulated in RAII
> objects but dealing with C objects the defer style provides much more
> reuse.
> 
>> 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…
>> 
> 
> I don't agree with the reuse.
> The __LINE__ is to avoid duplications, you could pass a unique name if you prefer.
> 
>>> 
>>> 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.
>> 
> 
> Then propose a correct series. Has been pending for more or less 3 months,
> we can't wait ages for a perfect solution and between this patch and the
> defer one I prefer the last.

I believe the last series I sent is closer to a “correct series”. My desire to proceed
in that direction is one of the reasons I had not updated my previous attempt.
The fact that the code was moving fact and causing a lot of churn another.
And the work load related to conferences the final straw on the camel’s back.


Thanks
Christophe

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