Re: [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Forgot to send, sorry for delay]

> On 19 Feb 2018, at 19:03, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> 
> On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
>>> On 16 Feb 2018, at 17:40, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>> 
>>>> 
>>>> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>> 
>>>> Get rid of C-style 'goto done' in do_capture.
>>>> Get rid of global streamfd, pass it around (cleaned up in later patch)
>>>> Fixes a race condition, make sure we only use stream after opening
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>>>> ---
>>>> src/spice-streaming-agent.cpp | 79
>>>> +++++++++++++++++++++++++------------------
>>>> 1 file changed, 47 insertions(+), 32 deletions(-)
>>>> 
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index 646d15b..9b8fd45 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>>>>    StreamMsgData msg;
>>>> };
>>>> 
>>>> +struct SpiceStreamCursorMessage
>>>> +{
>>>> +    StreamDevHeader hdr;
>>>> +    StreamMsgCursorSet msg;
>>>> +};
>>>> +
>>> 
>>> This is weird in this patch
>> 
>> Yes. I may have merged two patches. Will check.
>> 
>>> 
>>>> +class Stream
>>>> +{
>>>> +public:
>>>> +    Stream(const char *name)
>>>> +    {
>>>> +        fd = open(name, O_RDWR);
>>>> +        if (fd < 0)
>>>> +            throw std::runtime_error("failed to open streaming device");
> 
> Braces around if body (according to the coding style). Repeated a few
> times below.
> 
>>>> +    }
>>>> +    ~Stream()
>>>> +    {
>>>> +        close(fd);
>>> 
>>> You probably what to check for -1 to avoid calling close(-1)
>> 
>> I think this cannot happen, since in that case you’d have thrown from the ctor.
>> 
>>> 
>>>> +    }
>>>> +    operator int() { return fd; }
>>> 
>>> Sure you want this? I think is safer to avoid implicit cast
>>> also considering stuff like
>>> 
>>> Stream s;
>>> if (s) …
>> 
>> It is removed in a later patch once it’s no longer needed. The objective here is to minimize noisy changes at each step.
> 
> I'd also rather not see it, but if it's removed later, no big deal.

So I feel the general agreement is that it’s better to add an explicit getter and use that rather than implicitly convert.

> 
>>> 
>>>> +
>>>> +private:
>>>> +    int fd = -1;
>>> 
>>> So with a default constructor you want a class with
>>> an invalid state?
>>> Or just disable default constructor.
>> 
>> It’s disabled, there’s a ctor with args.
>> 
>>> I would disable also copy.
>> 
>> Also implicitly disabled.
> 
> It is not, actually, disabled here. It gets disabled later when you add
> the mutex member, as that is a non-copyable type.

You are right, of course. But like for the above, the idea is to minimize “noise” in the series.

Of course, I could explicitly delete constructors and assignment. I feel, but I may be wrong, that having a mutex field “says that”, both in terms of language safety and in terms of “what it means” (i.e. you are really creating a class for the purpose of RAII and thread safety).

If you think it’s better to add a few lines, I’m OK with it, but I don’t like it much as I read it more as noise than anything else. There are a few choices here, roughly in my personal order of preference:
1. Introduce = delete in this patch, remove it when mutex is added
2. Introduce = delete in this patch and leave it there even when it becomes “useless"
3 Leave a temporary “unsafe” state for a couple of commits
4. Merge the two and make sure there’s the mutex at the beginning

As a side note, this reminds me of a talk I highly recommend, https://www.youtube.com/watch?v=4AfRAVcThyA in case you have not seen it. Among other things, Herb Sutter proposes a $class mechanism that makes it possible to define a class of classes, e.g. “interface”, so that you explain with $class interface { … } what an interface is, and then you can write “interface X { .. }” and have an interface class (e.g. only pure virtual methods, etc).

Here, I see the mutex as indicative of a “locked resource” class, which I understand as having only explicit ctors for initialization and nothing else to consider, no copy, no assignment, etc. But it could also be made explicit with this mechanism. Post C++ 20, so not right away…

> 
> It might be a good idea to explicitly delete the copy ctr/ao anyway...
> In case someone ever removes the mutex and doesn't realize this.

That’s my choice #2, it could easily become #1.

> 
>>> 
>>>> +};
>>>> +
>>>> static bool streaming_requested = false;
>>>> static bool quit_requested = false;
>>>> static bool log_binary = false;
>>>> static std::set<SpiceVideoCodecType> client_codecs;
>>>> -static int streamfd = -1;
>>>> static std::mutex stream_mtx;
>>>> 
>>>> -static int have_something_to_read(int timeout)
>>>> +static int have_something_to_read(int streamfd, int timeout)
>>> 
>>> maybe would be better to use the "fd" name here, is no more
>>> bound to stream.
>> 
>> Kept to minimize changes
>> 
>>>> {
>>>>    struct pollfd pollfd = {streamfd, POLLIN, 0};
>>>> 
>>>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>>>>    return 0;
>>>> }
>>>> 
>>>> -static int read_command_from_device(void)
>>>> +static int read_command_from_device(int streamfd)
>>> 
>>> Have you considered Stream::read_command?
>> 
>> For that specific case, I liked the “from_device”. But…
>> 
>>> Ok, mostly in a following patch
>> 
>> 
>>> 
>>>> {
>>>>    StreamDevHeader hdr;
>>>>    uint8_t msg[64];
>>>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>>>>    return 1;
>>>> }
>>>> 
>>>> -static int read_command(bool blocking)
>>>> +static int read_command(int streamfd, bool blocking)
>>>> {
>>>>    int timeout = blocking?-1:0;
>>>>    while (!quit_requested) {
>>>> -        if (!have_something_to_read(timeout)) {
>>>> +        if (!have_something_to_read(streamfd, timeout)) {
>>>>            if (!blocking) {
>>>>                return 0;
>>>>            }
>>>>            sleep(1);
>>>>            continue;
>>>>        }
>>>> -        return read_command_from_device();
>>>> +        return read_command_from_device(streamfd);
>>>>    }
>>>> 
>>>>    return 1;
>>>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>>>>    return written;
>>>> }
>>>> 
>>>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>>>> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
>>>> unsigned c)
>>>> {
>>>> 
>>>>    SpiceStreamFormatMessage msg;
>>>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
>>>> h, unsigned c)
>>>>    return EXIT_SUCCESS;
>>>> }
>>>> 
>>>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>>>> +static int spice_stream_send_frame(int streamfd, const void *buf, const
>>>> unsigned size)
>>>> {
>>>>    SpiceStreamDataMessage msg;
>>>>    const size_t msgsize = sizeof(msg);
>>>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>>>> }
>>>> 
>>>> static void
>>>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>>>> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
>>>> int hotspot_y,
>>>>            std::function<void(uint32_t *)> fill_cursor)
>>>> {
>>>>    if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>>>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>>>> hotspot_x, int hotspot_y,
>>>>    write_all(streamfd, msg.get(), cursor_size);
>>>> }
>>>> 
>>>> -static void cursor_changes(Display *display, int event_base)
>>>> +static void cursor_changes(int streamfd, Display *display, int event_base)
>>>> {
>>>>    unsigned long last_serial = 0;
>>>> 
>>>> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
>>>> event_base)
>>>>            for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>>>>                pixels[i] = cursor->pixels[i];
>>>>        };
>>>> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
>>>> cursor->yhot, fill_cursor);
>>>> +        send_cursor(streamfd,
>>>> +                    cursor->width, cursor->height, cursor->xhot,
>>>> cursor->yhot, fill_cursor);
>>>>    }
>>>> }
>>>> 
>>>> static void
>>>> -do_capture(const char *streamport, FILE *f_log)
>>>> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>>>> {
>>>> -    streamfd = open(streamport, O_RDWR);
>>>> -    if (streamfd < 0)
>>>> -        throw std::runtime_error("failed to open the streaming device (" +
>>>> -                                 std::string(streamport) + "): "
>>>> -                                 + strerror(errno));
>>>> -
>>>>    unsigned int frame_count = 0;
>>>>    while (!quit_requested) {
>>>>        while (!quit_requested && !streaming_requested) {
>>>> -            if (read_command(true) < 0) {
>>>> +            if (read_command(streamfd, true) < 0) {
>>>>                syslog(LOG_ERR, "FAILED to read command\n");
>>>> -                goto done;
>>>> +                return;
>>>>            }
>>>>        }
>>>> 
>>>> @@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
>>>> 
>>>>                syslog(LOG_DEBUG, "wXh %uX%u  codec=%u\n", width, height,
>>>>                codec);
>>>> 
>>>> -                if (spice_stream_send_format(width, height, codec) ==
>>>> EXIT_FAILURE)
>>>> +                if (spice_stream_send_format(streamfd, width, height, codec)
>>>> == EXIT_FAILURE)
>>>>                    throw std::runtime_error("FAILED to send format
>>>>                    message");
>>>>            }
>>>>            if (f_log) {
>>>> @@ -382,23 +401,18 @@ do_capture(const char *streamport, FILE *f_log)
>>>>                    hexdump(frame.buffer, frame.buffer_size, f_log);
>>>>                }
>>>>            }
>>>> -            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
>>>> EXIT_FAILURE) {
>>>> +            if (spice_stream_send_frame(streamfd,
>>>> +                                        frame.buffer, frame.buffer_size) ==
>>>> EXIT_FAILURE) {
>>>>                syslog(LOG_ERR, "FAILED to send a frame\n");
>>>>                break;
>>>>            }
>>>>            //usleep(1);
>>>> -            if (read_command(false) < 0) {
>>>> +            if (read_command(streamfd, 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__);
>>>> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>>>>    Window rootwindow = DefaultRootWindow(display);
>>>>    XFixesSelectCursorInput(display, rootwindow,
>>>>    XFixesDisplayCursorNotifyMask);
>>>> 
>>>> -    std::thread cursor_th(cursor_changes, display, event_base);
>>>> +    Stream streamfd(streamport);
> 
> Better call it 'stream', it's not just a fd anymore. Along with the
> implicit conversion, this is quite confusing.

OK. Will be done in next round.

> 
> Lukas
> 
>>>> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
>>>> event_base);
>>>>    cursor_th.detach();
>>>> 
>>>>    int ret = EXIT_SUCCESS;
>>>>    try {
>>>> -        do_capture(streamport, f_log);
>>>> +        do_capture(streamfd, streamport, f_log);
>>> 
>>> Not a fun of this implicit conversion
>> 
>> Well, we can make it explicitl. But this is temporary, and is suppressed by the end of the series.
>> 
>>> .
>>> 
>>>>    }
>>>>    catch (std::runtime_error &err) {
>>>>        syslog(LOG_ERR, "%s\n", err.what());
>>> 
>>> Frediano
>> 
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]