[Spice-devel] [PATCH spice-streaming-agent v2 4/5] Handle capabilities

[Frediano Ziglio]

> On Tue, 2018-02-20 at 18:33 +0100, Christophe de Dinechin wrote:
> > > On 20 Feb 2018, at 15:30, Frediano Ziglio <fziglio at redhat.com>
> > > wrote:
> > > 
> > > Do not bail if the server is attempting to communicate some
> > > extensions
> > > but just ignore as at the moment we support none.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > > src/spice-streaming-agent.cpp | 31 +++++++++++++++++++++++++++++++
> > > 1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > > agent.cpp
> > > index d72c30b..9547e49 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -40,6 +40,8 @@
> > > 
> > > using namespace spice::streaming_agent;
> > > 
> > > +static size_t write_all(int fd, const void *buf, const size_t
> > > len);
> > > +
> > > static ConcreteAgent agent;
> > > 
> > > struct SpiceStreamFormatMessage
> > > @@ -77,6 +79,7 @@ static int have_something_to_read(int timeout)
> > >     return 0;
> > > }
> > > 
> > > +static int read_stream_capabilities(uint32_t len);
> > > static int read_stream_start_stop(uint32_t len);
> > > 
> > > static int read_command_from_device(void)
> > > @@ -96,6 +99,8 @@ static int read_command_from_device(void)
> > >     }
> > > 
> > >     switch (hdr.type) {
> > > +    case STREAM_TYPE_CAPABILITIES:
> > > +        return read_stream_capabilities(hdr.size);
> > >     case STREAM_TYPE_START_STOP:
> > >         return read_stream_start_stop(hdr.size);
> > >     }
> > > @@ -124,6 +129,32 @@ static int read_stream_start_stop(uint32_t
> > > len)
> > >     return 1;
> > > }
> > > 
> > > +static int read_stream_capabilities(uint32_t len)
> > 
> > It says “read” but reads and writes.
> > 

Yes, maybe better name them "handle_XXX".

> > Do you have many other pending patches that introduce new messages?
> > 

No, with these all possible current messages are supported.

> > This messes with the outgoing message refactoring I have in flight,
> > but also with the incoming refactoring that Lukas is looking at.
> > 
> > So my guess is this one should go first (it’s relatively small). Or
> > if it’s OK to wait a little, be rewritten on top of my series, but
> > I’m not too sure how to do that in the spirit of what Lukas wants to
> > do for input refactoring.
> > 

Depends on how much stuff this blocks (testing or other) and how
long is the "little", I would say end of week.

> > 
> > > +{
> > > +    uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > > +
> > > +    if (len > sizeof(caps)) {
> > > +        throw std::runtime_error("capability message too long");
> > > +    }
> > > +    int n = read(streamfd, caps, len);
> > > +    if (n != len) {
> > > +        throw std::runtime_error("read command from device FAILED
> > > -- read " + std::to_string(n) +
> > > +                                 " expected " +
> > > std::to_string(len));
> > > +    }
> 
> In most other places in the stream device, we assume that we may get
> partial reads (including partial headers, which are small). Here you
> report an error if you don't read the full message in one read. That
> seems inconsistent.
> 

I think the inconsistency is not having a read_all and having a
write_all.
Note that on server code is async so we need to support partial
read, here not much (calls are blocking). The current code works
as we don't handle much interrupts beside for terminating so
handling EINTR as an error is fine but would be better to
handle EINTR even here.
If we move to socket without a read_all we'll have problems
as sockets are more willing to have partial read.

> > > +
> > > +    // we currently do not suppor extensions so just reply so
> 
> "suppor" -> "support"
> 
> > > +    StreamDevHeader hdr = {
> > > +        STREAM_DEVICE_PROTOCOL,
> > > +        0,
> > > +        STREAM_TYPE_CAPABILITIES,
> > > +        0
> > > +    };
> > > +    if (write_all(streamfd, &hdr, sizeof(hdr)) != sizeof(hdr)) {
> > > +        throw std::runtime_error("error writing capabilities");
> > > +    }
> > > +    return 1;
> 
> It seems that the return value of these functions are becoming
> pointless after we have started introducing more exceptions.
> 

Yes, I think would be better to return a void instead, at least on
the "handle" functions.

> > > +}
> > > +
> > > static int read_command(bool blocking)
> > > {
> > >     int timeout = blocking?-1:0;

Frediano