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

[Frediano Ziglio]

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

Hi,
   I'm not that strong on code but I'm pretty strong on the rationale.
Coherence between Linux and Windows agent is fine, a more graceful
handling is fine.

The wrong part is: "This makes it difficult to extend the vdagent
protocol without breaking old installations.", this seems to indicate
that to extend the protocol is good to have unsupported messages.
IMO it would have been better if also the Linux agent would exit,
it would help us detecting the issue easily and avoid the regression
on the server.
What would happen for new messages that needs a reply? It would create
two set of messages, some that should be coded paying attention to
capabilities and some not. And the warnings would be useless, hard
to understand for a customer if the logs are a problem or not.

About patch 8251fa25 introducing a regression then if the current
behaviour for a message handling is to crash the server than
fixing the crash is a regression too. The behaviour on undefined
cases should not invalidate the protocol, you are confusing
protocol definition with protocol implementation.

Frediano