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

[Jonathon Jongsma]

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.
> 
> Do you have many other pending patches that introduce new messages?
> 
> 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.
> 
> 
> > +{
> > +    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.

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

> > +}
> > +
> > static int read_command(bool blocking)
> > {
> >     int timeout = blocking?-1:0;
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel