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