>
> > On 15 Feb 2018, at 11:04, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >
> >>
> >> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> >>
> >> This lets us get rid of C-style 'goto done' in do_capture.
> >>
> >> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> >
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel