[Spice-devel] [PATCH spice-streaming-agent v3 2/4] Separate handling start/stop message from server

[Lukáš Hrázký]

On Wed, 2018-02-21 at 05:45 -0500, Frediano Ziglio wrote:
> > 
> > On Tue, 2018-02-20 at 20:48 +0000, Frediano Ziglio wrote:
> > > Prepare to add support for other messages.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  src/spice-streaming-agent.cpp | 26 ++++++++++++++++++--------
> > >  1 file changed, 18 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 5613934..8d91f2d 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -77,10 +77,11 @@ static int have_something_to_read(int timeout)
> > >      return 0;
> > >  }
> > >  
> > > +static void handle_stream_start_stop(uint32_t len);
> > > +
> > 
> > Why not put the method here instead of a forward declaration (and
> > adding more in the future)?
> > 
> 
> Easier to read if you read the file from top to bottom.

I don't think so, because since functions always need to be declared
before use, the natural structure of a source file always is from
bottom to top. People are used to it. This seems to me like an
exception that actually breaks the flow. It appears there should be
another reason to reverse the order and do a forward declaration, while
in fact there isn't any.

It is bound to be refactored anyway, but still...

> > >  static void read_command_from_device(void)
> > >  {
> > >      StreamDevHeader hdr;
> > > -    uint8_t msg[64];
> > >      int n;
> > >  
> > >      std::lock_guard<std::mutex> stream_guard(stream_mtx);
> > > @@ -93,17 +94,26 @@ static void read_command_from_device(void)
> > >          throw std::runtime_error("BAD VERSION " +
> > >          std::to_string(hdr.protocol_version) +
> > >                                   " (expected is " +
> > >                                   std::to_string(STREAM_DEVICE_PROTOCOL) +
> > >                                   ")");
> > >      }
> > > -    if (hdr.type != STREAM_TYPE_START_STOP) {
> > > -        throw std::runtime_error("UNKNOWN msg of type " +
> > > std::to_string(hdr.type));
> > > +
> > > +    switch (hdr.type) {
> > > +    case STREAM_TYPE_START_STOP:
> > > +        return handle_stream_start_stop(hdr.size);
> > >      }
> > > -    if (hdr.size >= sizeof(msg)) {
> > > -        throw std::runtime_error("msg size (" + std::to_string(hdr.size) +
> > > ") is too long "
> > > +    throw std::runtime_error("UNKNOWN msg of type " +
> > > std::to_string(hdr.type));
> > > +}
> > > +
> > > +static void handle_stream_start_stop(uint32_t len)
> > > +{
> > > +    uint8_t msg[256];
> > > +
> > > +    if (len >= sizeof(msg)) {
> > > +        throw std::runtime_error("msg size (" + std::to_string(len) + ")
> > > is too long "
> > >                                   "(longer than " +
> > >                                   std::to_string(sizeof(msg)) + ")");
> > >      }
> > > -    n = read(streamfd, &msg, hdr.size);
> > > -    if (n != hdr.size) {
> > > +    int n = read(streamfd, &msg, len);
> > > +    if (n != len) {
> > >          throw std::runtime_error("read command from device FAILED -- read
> > >          " + std::to_string(n) +
> > > -                                 " expected " + std::to_string(hdr.size));
> > > +                                 " expected " + std::to_string(len));
> > >      }
> > >      streaming_requested = (msg[0] != 0); /* num_codecs */
> > >      syslog(LOG_INFO, "GOT START_STOP message -- request to %s
> > >      streaming\n",
> > 
> > Apart from the forward declaration,
> > 
> > Acked-by: Lukáš Hrázký <lhrazky at redhat.com>
> > 
> 
> Frediano