Re: [Qemu-devel] [RFC] libvhost-user: implement VHOST_USER_PROTOCOL_F_KICK_CALL_MSGS

[Johannes Berg]

On Mon, 2019-09-09 at 10:59 -0400, Michael S. Tsirkin wrote:

> > > Next command is GET_FEATURES. Return an error response from that
> > > and device init will fail.
> > 
> > Hmm, what's an error here though? You can only return a value, no?
> 
> Either returning id that does not match GET_FEATURES or
> returning size != 8 bytes will work.

Hmm, right.

> Using 0 size payload has precedent in VHOST_USER_GET_CONFIG:
>       vhost-user slave uses zero length of
>         payload to indicate an error to vhost-user master.

OK, I think zero-len makes sense, that's certainly something that must
be checked by the receiver already.

> It's annoying that we don't get an error indicator in that case though.
> Worth returning e.g. a 4 byte code?

That also would need to be checked by the receiver, but

1) Then we have to start defining an error code enum, as there's no
   natural space. Is that worth it for what's basically a corner case?
2) It'd also be annoying that we now have a 4-byte error code, but with
   F_REPLY_ACK / need_reply you get an 8-byte status code, which would
   be incompatible in a way.

> And let's generalize for all other messages with response?

We could theoretically have a message in the future that wants a 4-byte
response, though by convention basically everything uses u64 anyway ...

OTOH, 0-byte as error doesn't generalize, VHOST_USER_POSTCOPY_ADVISE has
only a file descriptor as slave payload AFAICT... But then possibly in
those cases the master also wouldn't check the response size at all,
since it never uses it. Qemu does though, and is probably the currently
relevant implementation? Our UML implementation doesn't use this.


Maybe instead we should just add a "VHOST_USER_REPLY_ERROR" bit (e.g.
bit 4 after NEED_REPLY). Qemu in vhost_user_read_header() validates that
it received REPLY_MASK | VERSION, so it would reject the message at that
point.

Another possibility would be to define the highest bit of the 'request'
field to indicate an error, so for GET_FEATURES we'd return the value
0x80000000 | GET_FEATURES.

For even more safety we could make a 4-byte response which is never
valid for anything right now, but it feels overly cautious given that we
currently do check both the request and flag fields. If you think a
response status is needed and want to define an enum with possible
values, then I'd probably prefer an 8-byte status since that's the way
everything works now, but I don't really care much either way.

johannes