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.
> >
> > > +
> > > +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.
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.
> >
> > > +};
> > > +
> > > 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.
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