Re: [Spice-devel] [PATCH 08/14] Use RAII to cleanup stream in case of exception or return
> On 15 Feb 2018, at 15:35, Frediano Ziglio wrote:
>
>>>
>>> On 15 Feb 2018, at 11:04, Frediano Ziglio wrote:
>>>
From: Christophe de Dinechin
This lets us get rid of C-style 'goto done' in do_capture.
Signed-off-by: Christophe de Dinechin
>>>
>>> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 08/14] Use RAII to cleanup stream in case of exception or return
>
> > On 15 Feb 2018, at 11:04, Frediano Ziglio wrote:
> >
> >>
> >> From: Christophe de Dinechin
> >>
> >> This lets us get rid of C-style 'goto done' in do_capture.
> >>
> >> Signed-off-by: Christophe de Dinechin
> >
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 08/14] Use RAII to cleanup stream in case of exception or return
> On 15 Feb 2018, at 11:04, Frediano Ziglio wrote:
>
>>
>> From: Christophe de Dinechin
>>
>> This lets us get rid of C-style 'goto done' in do_capture.
>>
>> Signed-off-by: Christophe de Dinechin
>
> 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 08/14] Use RAII to cleanup stream in case of exception or return
>
> From: Christophe de Dinechin
>
> This lets us get rid of C-style 'goto done' in do_capture.
>
> Signed-off-by: Christophe de Dinechin
I honestly prefer the "defer" style way.
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.
> ---
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 08/14] Use RAII to cleanup stream in case of exception or return
From: Christophe de Dinechin
This lets us get rid of C-style 'goto done' in do_capture.
Signed-off-by: Christophe de Dinechin
---
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__);
--
2.13.5 (Apple Git-94)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
