I cut quite a bit for clarity. What I cut I agree with ;-)

> On 14 Feb 2018, at 14:45, Lukáš Hrázký <lhra...@redhat.com> wrote:
> 
> On Tue, 2018-02-13 at 17:10 +0100, Christophe de Dinechin wrote:
>> Hi Lukas,
>> 
> 
> For your comments which are unrelated to my changes, I commended
> uniformly with "Was in original code." as I didn't know what else to do
> with them :)

I understood that. A lot of what I picked up is review of earlier stuff, some 
of which was written before I ever joined. It’s more to document later 
improvements I’d like to see happen, in case you get there first. And then I 
thought that I’d rather send patches, and I sent a first round.
        

> This way it may be easier to go through them in the future
> and fix them. I can't really go off fixing all that now, I'd never
> finish this and it would be hard to manage the series while trying to
> do separate patches for them.

Agreed (and I knew that sending the comments out)

> 
>> The commit log does not explain what an AgentRunner is, why it’s a necessary 
>> abstraction, and what this is supposed to be helping with. As I see it:
>> - It does not cleanly encapsulate resources nor enforces / provides RAII for 
>> things like streamfd and the like.
> 
> That is correct, and was my concern too when sending it, as mentioned
> in the cover letter :) (in case I could have made something more clear,
> please let me know)
> 
> I will surely try to do better :)
> 
>> - Because we throw hapazardly here and there and because of the previous 
>> comment, it actually makes the situation worse in case of error (at the very 
>> least, by making the flow of control harder to follow, but I suspect by 
>> ending in “terminate()” more often than not).
> 
> I don't think we throw haphazardly and don't think it makes errors
> harder to diagnose, but I may be wrong. If you had a concrete advice
> what to improve, please share it :)

Well, at the very least, we have a call to “agent.LoadPlugins” in the 
constructor, which may throw, because in ConcreteAgent::LoadPlugin we call the 
init function, and if it throws anything but runtime_error, we pass it along. 
If memory serves me right, bad_alloc for example does not derive from 
runtime_error but directly from exception. And the driver is allowed to throw 
42 if it wants to.

So now we have a potential exception from a plugin within the AgentRunner 
constructor. That smells funny. Please Don’t Do That™

> 
>> Also, a number of the changes could have their own individual patch set, 
>> which would make it easier to review and understand later.
> 
> As I said, not really sure I can do that :( I'll try though. Ideas
> welcome.

Since I need to rebase several of my patch series, we might work together on 
getting all this done. But that won’t work with mega patches on either side ;-)

> 
>>> On 8 Feb 2018, at 17:20, Lukáš Hrázký <lhra...@redhat.com> wrote:
>>> 
>>> Create an AgentRunner (TODO: needs a better name) class to encapsulate
>>> the streaming and communication with the server. The basic setup (cmd
>>> arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
>>> functions is moved to the AgentRunner class and modified as little as
>>> possible:
>>> - The cursor updating code is moved into a functor called CursorThread
>>> - Some initialization and cleanup is moved to AgentRunner's constructor
>>> and destructor
>>> - Some error handling moved over to exceptions, mainly what was in
>>> main() and do_capture()
>>> - A couple of variables gently renamed.
>>> 
>>> Signed-off-by: Lukáš Hrázký <lhra...@redhat.com>
>>> ---
>>> src/Makefile.am               |   2 +
>>> src/main.cpp                  | 127 ++++++++++++
>>> src/spice-streaming-agent.cpp | 455 
>>> +++++++++++++++++-------------------------
>>> src/spice-streaming-agent.hpp |  56 ++++++
>>> 4 files changed, 370 insertions(+), 270 deletions(-)
>>> create mode 100644 src/main.cpp
>>> create mode 100644 src/spice-streaming-agent.hpp
>>> 
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index 8d5c5bd..3a6fee7 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
>>> 
>>> spice_streaming_agent_SOURCES = \
>>>     spice-streaming-agent.cpp \
>>> +   spice-streaming-agent.hpp \
>>>     static-plugin.cpp \
>>>     static-plugin.hpp \
>>>     concrete-agent.cpp \
>>> @@ -56,4 +57,5 @@ spice_streaming_agent_SOURCES = \
>>>     mjpeg-fallback.cpp \
>>>     jpeg.cpp \
>>>     jpeg.hpp \
>>> +   main.cpp \
>>>     $(NULL)
>>> diff --git a/src/main.cpp b/src/main.cpp
>>> new file mode 100644
>>> index 0000000..a309011
>>> --- /dev/null
>>> +++ b/src/main.cpp
>>> @@ -0,0 +1,127 @@
>>> +/* An implementation of a SPICE streaming agent
>>> + *
>>> + * \copyright
>>> + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +
>>> +#include <config.h>
>> 
>> I’d write “config.h”. No reason to ever look config.h in system headers.
> 
> The reason for the <> is described in [1], 4th paragraph. I've
> mentioned it during the previous discussion and didn't get any comment
> on it IIRC. Either is fine by me, I don't plan to introduce another
> file named 'config.h' anywhere in the source tree.
> 
> [1] 
> https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html

That rationale is remarkably inconsistent with the generated makefiles, which 
build for example with:

-I. -I..  -I.. -I.. -I/usr/include/pixman-1  -I/usr/include/cacard 
-I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
-I/usr/include/nspr4   -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
-pthread  -I/usr/include/opus   -DG_LOG_DOMAIN=\"Spice\" 
-I/usr/local/include/spice-1  

or for the streaming agent:

-I. -I..  -DSPICE_STREAMING_AGENT_PROGRAM -I../include 
-DPLUGINSDIR=\"/usr/local/lib/spice-streaming-agent/plugins\" 
-I/usr/local/include/spice-1  

So you -I. first anyway, and you would prefer the local config.h in any case. 
“config.h” just lets compiler find it without looking up at the command-line 
options.

This is all automake-generated, not in our Makefile.am or autoconf.ac AFAICT.

Let me add this to my long list of autotools WTF…

> 
>> 
>>> +    int log_binary = 0;
>> 
>> bool?
> 
> Was in original code.
> 
> Small enough change, can fix it.

Sent a patch.

>> Header reorg would make a nice separate patch
> 
> I was moving some of them elsewhere as integral part of the patch,
> seemed correct to also reorg them.

Understood, but you can probably split that as a separate patch.

git add —patch helps a lot with this kind of refactoring.

> 
>> 
>> Any reason this is in this file?
> 
> You mean I should split it into a separate file? I agree.

Yes. Have the X11 stuff on the side is a bonus.

>>> 
>> 
>> Why make it a functor? (I know the answer, I’m suggesting a comment ;-)
> 
> What kind of comment do you suggest? I'm genuinely not sure what you
> mean.

Because we invoke that operator to start a thread, and it’s the thread 
interface that requires this to look like a function.

An alternative that might be clearer would be to have the thread creation 
within the class.

> 
>>> +            auto fill_cursor = [&](uint32_t *pixels) {
>> 
>> The lambda is fancy. But mixed with decade-old X11 junk… Not really to my 
>> taste. In particular if it’s to implement *one* “customization” of a 
>> *private* function that calls it exactly *once*. Really, just pass the 
>> cursor pointer, you’ll make the job of the compiler much easier. It also 
>> makes not much sense to decouple the “pixel copy” logic from the rest of the 
>> low-level crap like hot spots. If you really want a customization, it would 
>> have to deal with the format of the hot spot, etc. Which, oh, just happen to 
>> be in the cursor…
>> 
>> If you decide to keep the lambda, I think you want [=] here, you really want 
>> the original cursor pointer. Imagine send_cursor becomes some thread, then a 
>> loop could cause your lambda to be evaluated with a cursor fetched at the 
>> next iteration. Or better yet, [cursor].
> 
> Was in original code.

Yes. That probably needs fixing, though, the capture by reference worries me. 
Subtle bugs ahead.

> 
>>> +                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, streamfd, stream_mtx);
>>> +        }
>>> +    }
>>> +
>>> +private:
>>> +    void send_cursor(unsigned width, unsigned height, int hotspot_x, int 
>>> hotspot_y,
>>> +                     std::function<void(uint32_t *)> fill_cursor,
>>> +                     int streamfd, std::mutex &stream_mtx)
>>> +    {
>>> +        if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>>> +            height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
>>> +            return;
>>> +
>>> +        size_t cursor_size =
>>> +            sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
>>> +            width * height * sizeof(uint32_t);
>>> +        std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
>> 
>> Sigh. I’m starting to _really_ regret malloc()…
> 
> Was in original code.
> 
>>> +
>>> +        StreamDevHeader 
>>> &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
>>> +        memset(&dev_hdr, 0, sizeof(dev_hdr));
>> 
>> memset? Super yucky. All that stuff should be three dozen of layers of 
>> encapsulation below what we are doing here.
>> 
>> What about replacing these lines with a placement new:
>> 
>>      StreamDevHeader &dev_hdr = *new(msg.get()) StreamDevHeader {
>>              .protocol_version = STREAM_DEVICE_PROTOCOL,
>>              .type = STREAM_TYPE_CURSOR_SET,
>>              .size = sizeof(StreamDevHeader)
>>      };
> 
> Was in original code.
> 
>>> +        dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>>> +        dev_hdr.type = STREAM_TYPE_CURSOR_SET;
>>> +        dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
>>> +
>>> +        StreamMsgCursorSet &cursor_msg(
>>> +            *reinterpret_cast<StreamMsgCursorSet *>(msg.get() + 
>>> sizeof(StreamDevHeader)));
>> 
>> Combining a reinterpret_cast with low-level uint8_t pointer arithmetic. 
>> That’s a winner.
>> 
>> Same recipe as above, but use array indexing to better express intent:
>> 
>>      StreamMsgCursorSet &cursor_msg = *new(&dev_hdr[1]) StreamMsgCursorSet {
>>              … fields
>>      };
>> 
>> Also avoids a lot of repetition, and makes it easier for the compiler to 
>> initialize in place.
> 
> Was in original code.
> 
>>> +
>>> +        memset(&cursor_msg, 0, sizeof(cursor_msg));
>>> +
>>> +        cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
>>> +        cursor_msg.width = width;
>>> +        cursor_msg.height = height;
>>> +        cursor_msg.hot_spot_x = hotspot_x;
>>> +        cursor_msg.hot_spot_y = hotspot_y;
>>> +
>>> +        uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
>>> +        fill_cursor(pixels);
>> 
>> Just pass cursor and init here. Or abstract everything, including hot spot.
> 
> Was in original code.
> 
>>> +
>>> +        std::lock_guard<std::mutex> stream_guard(stream_mtx);
>>> +        write_all(streamfd, msg.get(), cursor_size);
>> 
>> Why shouldn’t the locking happen within write_all? As is, it’s practically 
>> screaming “please please please make sure someone else calls write_all 
>> without the mutex”
> 
> Was in original code.
> 
> I'll quite surely fix this one though.
> 
>>> +    }
>>> +
>>> +    Display* display;
>> 
>> = NULL, since you init the rest.
> 
> Was in original code.
> 
>>> +    int event_base = 0;
>>> +    int streamfd = -1;
>>> +    std::mutex &stream_mtx;
>> 
>> A global mutex for a global function is one case where I would use a global 
>> variable. At least until we have made further cleanup.
> 
> I'll encapsulate the communication layer properly and this should
> become a non-issue.
>> 
>>> +};
>>> +
>>> +} // namespace
>> 
>> ??? Why are we closing namespaces in the middle?
> 
> It's the unnamed namespace. AFAIK that's how you use it, do suggest a
> better way?
> 
>>> +
>>> +AgentRunner::AgentRunner(const std::string &stream_port,
>>> +                         const std::string &log_filename,
>>> +                         bool log_binary,
>>> +                         bool stdin_ok)
>>> +:
>>> +    stream_port(stream_port),
>>> +    log_binary(log_binary),
>>> +    stdin_ok(stdin_ok)
>>> +{
>>> +    agent.LoadPlugins(PLUGINSDIR);
>> 
>> I personally don’t like having things that are likely to fail in a ctor. 
>> Cleaning up when a dtor throws an exception is messy. Without going into 
>> details, at first sight, I believe it’s not done right here.
> 
> Hey.. don't mix stuff here.. Destructors should never throw, right? :)

Sorry, that was a typo, I was obviously talking about ctor, not dtor.

> Constructors can throw, but then the object is not costructed and the
> destructor is not called. So you have to ensure proper cleanup in the
> constructor if it's needed. Hence the comment below :)
> 
>>> +
>>> +    // TODO proper cleanup on exceptions thrown here - wrap in classes?
>> 
>> Well, obviously, if you go through all this trouble and still don’t have 
>> RAII on the stream, it’s annoying ;-)
> 
> Right :)
> 
>>> +    streamfd = open(stream_port.c_str(), O_RDWR);
>>> +    if (streamfd < 0)
>>> +        throw std::runtime_error("failed to open the streaming device (" +
>>> +                                 stream_port + "): " + strerror(errno));
>>> +
>>> +    if (!log_filename.empty()) {
>>> +        log_file = fopen(log_filename.c_str(), "wb");
>>> +        if (!log_file) {
>>> +            throw std::runtime_error("Failed to open log file '" + 
>>> log_filename +
>>> +                                     "': " + strerror(errno) + "'");
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +AgentRunner::~AgentRunner() {
>>> +    if (streamfd >= 0) {
>> 
>> That if is a clear sign something is wrong in the design.
>> 
>>> +        close(streamfd);
>>> +        streamfd = -1;
>> 
>> Just in case you call the destructor twice. Better safe than sorry. Another 
>> sign something is wrong here.
> 
> Indeed :)
> 
>>> +    }
>>> +
>>> +    if (log_file) {
>>> +        fclose(log_file);
>>> +        log_file = nullptr;
>>> +    }
>>> +}
>>> +
>>> +int AgentRunner::have_something_to_read(int *pfd, int timeout)
>>> {
>>>    int nfds;
>>>    struct pollfd pollfds[2] = {
>>> @@ -82,7 +216,7 @@ static int have_something_to_read(int *pfd, int timeout)
>>>    return *pfd != -1;
>>> }
>>> 
>>> -static int read_command_from_stdin(void)
>>> +int AgentRunner::read_command_from_stdin()
>>> {
>>>    char buffer[64], *p, *save = NULL;
>> 
>> Ugly init and mixed declarators.
> 
> Was in original code.
> 
>>> 
>>> @@ -106,7 +240,7 @@ static int read_command_from_stdin(void)
>>>    return 1;
>>> }
>>> 
>>> -static int read_command_from_device(void)
>>> +int AgentRunner::read_command_from_device()
>>> {
>>>    StreamDevHeader hdr;
>>>    uint8_t msg[64];
>>> @@ -151,7 +285,7 @@ static int read_command_from_device(void)
>>>    return 1;
>>> }
>>> 
>>> -static int read_command(bool blocking)
>>> +int AgentRunner::read_command(bool blocking)
>>> {
>>>    int fd, n=1;
>> 
>> Ugly init.
> 
> Was in original code.
> 
>>>    int timeout = blocking?-1:0;
>>> @@ -173,28 +307,8 @@ static int read_command(bool blocking)
>>>    return n;
>>> }
>>> 
>>> -static size_t
>>> -write_all(int fd, const void *buf, const size_t len)
>>> -{
>>> -    size_t written = 0;
>>> -    while (written < len) {
>>> -        int l = write(fd, (const char *) buf + written, len - written);
>>> -        if (l < 0 && errno == EINTR) {
>>> -            continue;
>>> -        }
>>> -        if (l < 0) {
>>> -            syslog(LOG_ERR, "write failed - %m");
>>> -            return l;
>>> -        }
>>> -        written += l;
>>> -    }
>>> -    syslog(LOG_DEBUG, "write_all -- %u bytes written\n", 
>>> (unsigned)written);
>>> -    return written;
>>> -}
>>> -
>>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>>> +int AgentRunner::spice_stream_send_format(unsigned w, unsigned h, unsigned 
>>> c)
>>> {
>>> -
>>>    SpiceStreamFormatMessage msg;
>>>    const size_t msgsize = sizeof(msg);
>>>    const size_t hdrsize  = sizeof(msg.hdr);
>>> @@ -213,7 +327,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)
>>> +int AgentRunner::spice_stream_send_frame(const void *buf, const unsigned 
>>> size)
>>> {
>>>    SpiceStreamDataMessage msg;
>>>    const size_t msgsize = sizeof(msg);
>>> @@ -244,7 +358,7 @@ static int spice_stream_send_frame(const void *buf, 
>>> const unsigned size)
>>> }
>>> 
>>> /* returns current time in micro-seconds */
>>> -static uint64_t get_time(void)
>>> +uint64_t AgentRunner::get_time(void)
>>> {
>>>    struct timeval now;
>>> 
>>> @@ -254,116 +368,13 @@ static uint64_t get_time(void)
>>> 
>>> }
>>> 
>>> -static void handle_interrupt(int intr)
>>> -{
>>> -    syslog(LOG_INFO, "Got signal %d, exiting", intr);
>>> -    quit = true;
>>> -}
>>> -
>>> -static void register_interrupts(void)
>>> -{
>>> -    struct sigaction sa;
>>> -
>>> -    memset(&sa, 0, sizeof(sa));
>>> -    sa.sa_handler = handle_interrupt;
>>> -    if ((sigaction(SIGINT, &sa, NULL) != 0) &&
>>> -        (sigaction(SIGTERM, &sa, NULL) != 0)) {
>>> -        syslog(LOG_WARNING, "failed to register signal handler %m");
>>> -    }
>>> -}
>>> -
>>> -static void usage(const char *progname)
>>> -{
>>> -    printf("usage: %s <options>\n", progname);
>>> -    printf("options are:\n");
>>> -    printf("\t-p portname  -- virtio-serial port to use\n");
>>> -    printf("\t-i accept commands from stdin\n");
>>> -    printf("\t-l file -- log frames to file\n");
>>> -    printf("\t--log-binary -- log binary frames (following -l)\n");
>>> -    printf("\t-d -- enable debug logs\n");
>>> -    printf("\t-c variable=value -- change settings\n");
>>> -    printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
>>> -    printf("\n");
>>> -    printf("\t-h or --help     -- print this help message\n");
>>> -
>>> -    exit(1);
>>> -}
>>> -
>>> -static void
>>> -send_cursor(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 ||
>>> -        height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
>>> -        return;
>>> -
>>> -    size_t cursor_size =
>>> -        sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
>>> -        width * height * sizeof(uint32_t);
>>> -    std::unique_ptr<uint8_t[]> msg(new uint8_t[cursor_size]);
>>> -
>>> -    StreamDevHeader 
>>> &dev_hdr(*reinterpret_cast<StreamDevHeader*>(msg.get()));
>>> -    memset(&dev_hdr, 0, sizeof(dev_hdr));
>>> -    dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
>>> -    dev_hdr.type = STREAM_TYPE_CURSOR_SET;
>>> -    dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
>>> -
>>> -    StreamMsgCursorSet &cursor_msg(*reinterpret_cast<StreamMsgCursorSet 
>>> *>(msg.get() + sizeof(StreamDevHeader)));
>>> -    memset(&cursor_msg, 0, sizeof(cursor_msg));
>>> -
>>> -    cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
>>> -    cursor_msg.width = width;
>>> -    cursor_msg.height = height;
>>> -    cursor_msg.hot_spot_x = hotspot_x;
>>> -    cursor_msg.hot_spot_y = hotspot_y;
>>> -
>>> -    uint32_t *pixels = reinterpret_cast<uint32_t *>(cursor_msg.data);
>>> -    fill_cursor(pixels);
>>> -
>>> -    std::lock_guard<std::mutex> stream_guard(stream_mtx);
>>> -    write_all(streamfd, msg.get(), cursor_size);
>>> -}
>>> -
>>> -static void cursor_changes(Display *display, int event_base)
>>> +void AgentRunner::do_capture()
>> 
>> That really confuses me. What is AgentRunner? Running the agent? The agent 
>> itself? Why does it have a “do_capture”?
> 
> It's the original method... Further refactor needed.

I understand it is, but I still don’t really get what a “runner” is supposed to 
be.

> 
>>> {
>>> -    unsigned long last_serial = 0;
>>> -
>>> -    while (1) {
>>> -        XEvent event;
>>> -        XNextEvent(display, &event);
>>> -        if (event.type != event_base + 1)
>>> -            continue;
>>> -
>>> -        XFixesCursorImage *cursor = XFixesGetCursorImage(display);
>>> -        if (!cursor)
>>> -            continue;
>>> -
>>> -        if (cursor->cursor_serial == last_serial)
>>> -            continue;
>>> -
>>> -        last_serial = cursor->cursor_serial;
>>> -        auto fill_cursor = [&](uint32_t *pixels) {
>>> -            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);
>>> -    }
>>> -}
>>> -
>>> -static void
>>> -do_capture(const string &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));
>>> -
>>>    unsigned int frame_count = 0;
>>>    while (! quit) {
>>>        while (!quit && !streaming_requested) {
>>>            if (read_command(true) < 0) {
>>> -                syslog(LOG_ERR, "FAILED to read command\n");
>>> -                goto done;
>>> +                throw std::runtime_error("FAILED to read command");
>>>            }
>>>        }
>>> 
>>> @@ -406,12 +417,12 @@ do_capture(const string &streamport, FILE *f_log)
>>>                if (spice_stream_send_format(width, height, codec) == 
>>> EXIT_FAILURE)
>>>                    throw std::runtime_error("FAILED to send format 
>>> message");
>>>            }
>>> -            if (f_log) {
>>> +            if (log_file) {
>>>                if (log_binary) {
>>> -                    fwrite(frame.buffer, frame.buffer_size, 1, f_log);
>>> +                    fwrite(frame.buffer, frame.buffer_size, 1, log_file);
>>>                } else {
>>> -                    fprintf(f_log, "%lu: Frame of %zu bytes:\n", 
>>> get_time(), frame.buffer_size);
>>> -                    hexdump(frame.buffer, frame.buffer_size, f_log);
>>> +                    fprintf(log_file, "%lu: Frame of %zu bytes:\n", 
>>> get_time(), frame.buffer_size);
>>> +                    hexdump(frame.buffer, frame.buffer_size, log_file);
>>>                }
>>>            }
>>>            if (spice_stream_send_frame(frame.buffer, frame.buffer_size) == 
>>> EXIT_FAILURE) {
>>> @@ -420,118 +431,22 @@ do_capture(const string &streamport, FILE *f_log)
>>>            }
>>>            //usleep(1);
>>>            if (read_command(false) < 0) {
>>> -                syslog(LOG_ERR, "FAILED to read command\n");
>>> -                goto done;
>>> +                throw std::runtime_error("FAILED to read command");
>>>            }
>>>        }
>>>    }
>>> +}
>>> 
>>> -done:
>>> -    if (streamfd >= 0) {
>>> -        close(streamfd);
>>> -        streamfd = -1;
>>> +void AgentRunner::add_options(const std::vector<std::pair<std::string, 
>>> std::string>> &options) {
>>> +    for (const auto& option : options) {
>>> +        agent.AddOption(option.first.c_str(), option.second.c_str());
>>>    }
>>> }
>>> 
>>> -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>>> -
>>> -int main(int argc, char* argv[])
>>> +void AgentRunner::run()
>>> {
>>> -    string streamport = "/dev/virtio-ports/com.redhat.stream.0";
>>> -    char opt;
>>> -    const char *log_filename = NULL;
>>> -    int logmask = LOG_UPTO(LOG_WARNING);
>>> -    struct option long_options[] = {
>>> -        { "log-binary", no_argument, &log_binary, 1},
>>> -        { "help", no_argument, NULL, 'h'},
>>> -        { 0, 0, 0, 0}
>>> -    };
>>> -
>>> -    if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
>>> -        stdin_ok = true;
>>> -    }
>>> -
>>> -    openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) : 
>>> LOG_PID, LOG_USER);
>>> -    setlogmask(logmask);
>>> -
>>> -    while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, 
>>> NULL)) != -1) {
>>> -        switch (opt) {
>>> -        case 0:
>>> -            /* Handle long options if needed */
>>> -            break;
>>> -        case 'i':
>>> -            stdin_ok = true;
>>> -            openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
>>> -            break;
>>> -        case 'p':
>>> -            streamport = optarg;
>>> -            break;
>>> -        case 'c': {
>>> -            char *p = strchr(optarg, '=');
>>> -            if (p == NULL) {
>>> -                arg_error("wrong 'c' argument %s\n", optarg);
>>> -                usage(argv[0]);
>>> -            }
>>> -            *p++ = '\0';
>>> -            agent.AddOption(optarg, p);
>>> -            break;
>>> -        }
>>> -        case 'l':
>>> -            log_filename = optarg;
>>> -            break;
>>> -        case 'd':
>>> -            logmask = LOG_UPTO(LOG_DEBUG);
>>> -            setlogmask(logmask);
>>> -            break;
>>> -        case 'h':
>>> -            usage(argv[0]);
>>> -            break;
>>> -        }
>>> -    }
>>> -
>>> -    agent.LoadPlugins(PLUGINSDIR);
>>> -
>>> -    register_interrupts();
>>> -
>>> -    FILE *f_log = NULL;
>>> -    if (log_filename) {
>>> -        f_log = fopen(log_filename, "wb");
>>> -        if (!f_log) {
>>> -            syslog(LOG_ERR, "Failed to open log file '%s': %s\n",
>>> -                   log_filename, strerror(errno));
>>> -            return EXIT_FAILURE;
>>> -        }
>>> -    }
>>> -
>>> -    Display *display = XOpenDisplay(NULL);
>>> -    if (display == NULL) {
>>> -        syslog(LOG_ERR, "failed to open display\n");
>>> -        return EXIT_FAILURE;
>>> -    }
>>> -    int event_base, error_base;
>>> -    if (!XFixesQueryExtension(display, &event_base, &error_base)) {
>>> -        syslog(LOG_ERR, "XFixesQueryExtension failed\n");
>>> -        return EXIT_FAILURE;
>>> -    }
>>> -    Window rootwindow = DefaultRootWindow(display);
>>> -    XFixesSelectCursorInput(display, rootwindow, 
>>> XFixesDisplayCursorNotifyMask);
>>> -
>>> -    std::thread cursor_th(cursor_changes, display, event_base);
>>> +    std::thread cursor_th(CursorThread(streamfd, stream_mtx));
>>>    cursor_th.detach();
>>> 
>>> -    int ret = EXIT_SUCCESS;
>>> -    try {
>>> -        do_capture(streamport, f_log);
>>> -    }
>>> -    catch (std::runtime_error &err) {
>>> -        syslog(LOG_ERR, "%s\n", err.what());
>>> -        ret = EXIT_FAILURE;
>>> -    }
>>> -
>>> -    if (f_log) {
>>> -        fclose(f_log);
>>> -        f_log = NULL;
>>> -    }
>>> -    closelog();
>>> -    return ret;
>>> +    do_capture();
>>> }
>>> diff --git a/src/spice-streaming-agent.hpp b/src/spice-streaming-agent.hpp
>>> new file mode 100644
>>> index 0000000..d6cbfbb
>>> --- /dev/null
>>> +++ b/src/spice-streaming-agent.hpp
>>> @@ -0,0 +1,56 @@
>>> +/* An implementation of a SPICE streaming agent
>>> + *
>>> + * \copyright
>>> + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +
>>> +#ifndef SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
>>> +#define SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
>>> +
>>> +#include "concrete-agent.hpp"
>>> +
>>> +#include <cstdint>
>>> +#include <mutex>
>>> +#include <set>
>>> +
>>> +
>>> +namespace SpiceStreamingAgent {
>>> +
>>> +class AgentRunner
>> 
>> I dislike having the class name differ from the file name. If there is an 
>> “AgentRunner”, I expect it to “run” an “agent”. But there is not agent 
>> anywhere in your “runner”, and it’s not even clear what it’s running.
> 
> Explained already.

??? Not sure where. If it’s in the cover letter, I still don’t get it :-(

> 
>>> +{
>>> +public:
>>> +    AgentRunner(const std::string &stream_port,
>>> +                const std::string &log_filename,
>>> +                bool log_binary,
>>> +                bool stdin_ok);
>> 
>> Same comments as Frediano rergarding the ctor.
>> 
>> As I see it, the logger is a resource, I think it deserves its own class. 
>> The stream is a resource, it deserves its own class. Option management is a 
>> separate thing, also a separate set of classes. And things like the list of 
>> codecs or capture belong to the agent itself, I see no reason to defer them 
>> here.
>> 
>> In other words, the way I would like this to change is:
>> 
>> - Main creates a “Stream”, a “LogFile” and an “Agent” object
> 
> That is more or less what I have in mind now.
> 
>> - The cursor class encapsulation is semi-OK, should be split apart in the 
>> only place that even remotely knows anything about X11. Just get rid of the 
>> overblown lambda, or, if you can prove there will be valid use cases for it, 
>> provide a real encapsulation for what a cursor is that includes everything, 
>> including hot spot.
> 
> Not sure if I'll be doing anything about the Cursor internals right
> now...
> 
>> - ConcreteAgent is really bad, it should really be AgentInterface and Agent, 
>> or IAgent and Agent if you speak Microsoftese).
> 
> Wholeheartedly agree :) I was having some time to see if I can come up
> with a better naming than AgentInterface and Agent, and didn't. No
> Microsoftese please! :D
> 
>> - Once this is done, I don’t know that there will be much room for a 
>> “runner”, although I could be convinced otherwise if it deals with things 
>> like threads
> 
> That is actually kind of my hope...
> 
>> Hope that this helps and that you won’t read that as me being overly picky. 
>> To my discharge, I’m feeling nauseated, which usually does not improve my 
>> mood ;-)
> 
> No worries at all, as I said, I'm grateful for all the relevant
> remarks! :P

Let’s keep pushing on this. Hoping that today’s patches for the nits help 
cleaning up in the spirit of “small patches”/


Thanks
Christophe

> 
> Thanks!
> Lukas
> 
>> Let’s keep working on this!
>> 
>> 
>> Cheers
>> Christophe
>> 
>> 
>>> +    ~AgentRunner();
>>> +
>>> +    void do_capture();
>>> +    void add_options(const std::vector<std::pair<std::string, 
>>> std::string>> &options);
>>> +    void run();
>>> +
>>> +    static bool quit;
>>> +
>>> +private:
>>> +    int have_something_to_read(int *pfd, int timeout);
>>> +    int read_command_from_stdin();
>>> +    int read_command_from_device();
>>> +    int read_command(bool blocking);
>>> +    int spice_stream_send_format(unsigned w, unsigned h, unsigned c);
>>> +    int spice_stream_send_frame(const void *buf, const unsigned size);
>>> +    uint64_t get_time(void);
>>> +
>>> +    std::string stream_port;
>>> +    FILE *log_file = nullptr;
>>> +    ConcreteAgent agent;
>>> +    int streaming_requested = 0;
>>> +    std::set<SpiceVideoCodecType> client_codecs;
>>> +    int streamfd = -1;
>>> +    bool log_binary = false;
>>> +    bool stdin_ok = false;
>>> +    std::mutex stream_mtx;
>>> +};
>>> +
>>> +} // SpiceStreamingAgent
>>> +
>>> +#endif // SPICE_STREAMING_AGENT_SPICE_STREAMING_AGENT_HPP
>>> -- 
>>> 2.16.1
>>> 
>>> _______________________________________________
>>> 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

Reply via email to