[Forgot to send, sorry for delay] > On 19 Feb 2018, at 19:03, Lukáš Hrázký <lhra...@redhat.com> wrote: > > On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote: >>> On 16 Feb 2018, at 17:40, Frediano Ziglio <fzig...@redhat.com> wrote: >>> >>>> >>>> From: Christophe de Dinechin <dinec...@redhat.com> >>>> >>>> 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 <dinec...@redhat.com> >>>> --- >>>> 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel