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

[Frediano Ziglio]

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

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