On Tue, 2018-02-20 at 18:37 +0100, Christophe de Dinechin wrote:
> [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.
No strong opinion on this :) I know some would argue my point, so I
mentioned it for completeness. I can agree with your order of
preference, and I don't care as much for "unsafe" state between commits
in your series, so 3 is fine by me too.
I just mentioned the above so that nobody could get confused and
thought that the class in this patch somehow has those copy ctor and
otor disabled...
> >
> > > >
> > > > > +};
> > > > > +
> > > > > 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