[Spice-devel] [PATCH win-vdagent] Don't exit when receiving unknown messages

[Jonathon Jongsma]

On Thu, 2019-02-28 at 03:31 -0500, Frediano Ziglio wrote:
> > 
> > In 8251fa25, a check on the minimum size of a message was
> > introduced.
> > For unsupported messages, the vdagent simply exited. This makes it
> > difficult to extend the vdagent protocol without breaking old
> 
> The protocol is using capabilities, there should not be
> unsupported messages.
> 
> > installations. Instead, just print a warning indicating that an
> > unsupported message was received and ignore it.
> > 
> 
> If you want to print a warning it means that it's not correct,
> this is another indication that it's wrong.
> 
> > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > ---
> >  vdagent/vdagent.cpp | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> > index 89019bb..177e663 100644
> > --- a/vdagent/vdagent.cpp
> > +++ b/vdagent/vdagent.cpp
> > @@ -1288,8 +1288,7 @@ void
> > VDAgent::dispatch_message(VDAgentMessage* msg,
> > uint32_t port)
> >          break;
> >      }
> >      if (min_size < 0) {
> > -        vd_printf("Unsupported message type %u size %u", msg-
> > >type,
> > msg->size);
> > -        _running = false;
> > +        vd_printf("Unsupported message type %u size %u, ignoring",
> > msg->type, msg->size);
> >          return;
> >      }
> >      if (msg->size < (unsigned) min_size) {
> 
> RHEL 8 has 8251fa25 patch, should the users be forced to update
> the guests?
> If we want to change the protocol to allow unsupported message we
> should first change the protocol specification, this is not a
> fix, it's a workaround for a wrong/incomplete implementation on
> the server side.
> I prefer a proper fix than a workaround and this is not
> fixing already installed guests.
> 
> Frediano


Well, I didn't necessarily mean that this is a full solution, but I
think it is still an improvement. In fact, this was the behavior that
vdagent-win had before commit 8251fa25. For example, later on in this
function when we're handling the messages, there is the following code:

    default:
        vd_printf("Unsupported message type %u size %u", msg->type, msg->size);
    }

The agent does not exit due to an unsupported message. It just prints a
warning.

In addition, this is the same behavior as the linux vdagent. In linux
src/vdagentd/vdagentd.c function virtio_port_read_complete():

    default:
        g_warn_if_reached();
    }

    return 0;

So: Linux vdagent prints a warning, but the vdagent does not exit. 

You say above that my change requires a change to the protocol, but in
fact this is how both vdagents worked for years until commit 8251fa25
made the windows agent exit when it encounters unsupported messages. So
I would argue that patch 8251fa25 actually introduced a regression in
RHEL 8.0.

Jonathon