[Spice-devel] [PATCH spice-common] proto: Add agent features message

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi,
> 
> On Thu, Sep 15, 2016 at 09:01:30AM -0400, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> > > Hi,
> > >
> > > On Thu, Sep 15, 2016 at 08:18:38AM -0400, Marc-André Lureau wrote:
> > > > > > Actually, there is agent capabilities, I think that's what the
> > > > > > server should be overriding instead.
> > > > >
> > > > > I know that is possible but imo it is hack. It would be needed to
> > > > > filter VD_AGENT_ANNOUNCE_CAPABILITIES from the agent, insert
> > > > > something
> > > > > like VD_AGENT_CAP_FILE_XFER_DISABLED,
> > > > >  VD_AGENT_CAP_COPY_PASTE_DISABLED
> > > > > and also in the case that the filter is changed on the fly, it would
> > > > > be needed to generate complete VD_AGENT_ANNOUNCE_CAPABILITIES (or
> > > > > request agent to send them). I think it would be more complicated...
> > > >
> > > > I don't think it is so complicated, but I might be wrong. The server
> > > > already parses some agents messages for filtering.
> > > >
> > > > At least I think it would be cleaner from the protocol POV. I don't
> > > > see much benefit for the client to know that the server disabled
> > > > something explicitely vs the agent not having the capability.
> > >
> > > I think we could do some distinction about agent capabilities and
> > > features that are disabled on host and for that reason I think this
> > > message makes sense from protocol POV.
> >
> > What differences does that make from a client? Do you think it helps
> > much for the client to say "the feature foo has been disabled by the
> > server" vs "the feature foo is not available"?
> 
> Well, with "feature is disabled" user can request a sysadmin to enable
> it while with "feature not available" only, user will spam us saying
> that vdagent is running fine, why it does not work? :)

Features could also be disabled from the agent itself (although we lack configuration for that), or by the client. Ie, there are many reasons why a feature may not be available. It would be endless to extend the protocol to teach all the ways :)

> 
> > The are already many caps: common caps, channel caps, vdagent caps..
> > Does the protocol need to grow yet another "agent_features"? That
> > really seems redundant with vdagent caps.
> 
> As you said, it *can* be implemented with capabilities but that changes
> the meaning of it. Agent is capable of doing file-transfer but the fact
> that it is disabled by QEMU should be handled differently, IMHO.

To me it doesn't change the meaning, it simply states it's not available, not the reason why.

> AFAICT, what happens today is that client tries to file-transfer even if
> file-transfer is disabled on server. Server ignores it. We could reply
> with some server message saying that feature is disabled? We would still
> need to improve protocol for that, I think.

I think that would be an interesting extension (perhaps we already have such error replies?)

But it's still better to state upfront the the feature is unavailable.