> On 15 Feb 2018, at 15:35, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>>
>>> 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.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel