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

[Frediano Ziglio]

> 
> 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.
> 

Not strong about, I'll revert.

> 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
>