Hi
On Wed, May 5, 2021 at 4:38 PM Dr. David Alan Gilbert <dgilb...@redhat.com>
wrote:
> (Resend, remembering to add list)
> Hi,
> I'm trying to understand what restrictions there are on the
> payload that's part of VhostUserMsg; and am confused by
> inconsistencies.
>
> Lets start with this version:
>
> subprojects/libvhost-user/libvhost-user.h :
> typedef struct VhostUserMsg {
> int request;
>
> #define VHOST_USER_VERSION_MASK (0x3)
> #define VHOST_USER_REPLY_MASK (0x1 << 2)
> #define VHOST_USER_NEED_REPLY_MASK (0x1 << 3)
> uint32_t flags;
> uint32_t size; /* the following payload size */
>
> union {
> #define VHOST_USER_VRING_IDX_MASK (0xff)
> #define VHOST_USER_VRING_NOFD_MASK (0x1 << 8)
> uint64_t u64;
> struct vhost_vring_state state;
> struct vhost_vring_addr addr;
> VhostUserMemory memory;
> VhostUserMemRegMsg memreg;
> VhostUserLog log;
> VhostUserConfig config;
> VhostUserVringArea area;
> VhostUserInflight inflight;
> } payload;
>
> int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> int fd_num;
> uint8_t *data;
> } VU_PACKED VhostUserMsg;
>
> note the 'fds' array after the payload but before
> the end of the structure.
>
> But then there's the version in:
> hw/virtio/vhost-user.c
> typedef union {
> #define VHOST_USER_VRING_IDX_MASK (0xff)
> #define VHOST_USER_VRING_NOFD_MASK (0x1<<8)
> uint64_t u64;
> struct vhost_vring_state state;
> struct vhost_vring_addr addr;
> VhostUserMemory memory;
> VhostUserMemRegMsg mem_reg;
> VhostUserLog log;
> struct vhost_iotlb_msg iotlb;
> VhostUserConfig config;
> VhostUserCryptoSession session;
> VhostUserVringArea area;
> VhostUserInflight inflight;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
> VhostUserHeader hdr;
> VhostUserPayload payload;
> } QEMU_PACKED VhostUserMsg;
>
> which hasn't got the 'fds' section.
> Yet they're both marked as 'packed'.
>
They are packed, because both are used to serialize/deserialize the stream.
> That's a bit unfortunate for two structures with the same name.
>
>
Yes, maybe it's time to have a canonical system header used by both?
> Am I right in thinking that the vhost-user.c version is sent over
> the wire, while the libvhost-user.h one is really just an interface?
>
>
I believe the extra fields are not used for serializing the message, but
just a convenient way to group related data.
> Is it safe for me to add a new, larger entry in the payload union
> without breaking existing clients?
>
It should be.
> I ended up at this question after trying to add a variable length
> entry to the union:
>
> typedef struct {
> VhostUserFSSlaveMsg fs;
> VhostUserFSSlaveMsgEntry entries[VHOST_USER_FS_SLAVE_MAX_ENTRIES];
> } QEMU_PACKED VhostUserFSSlaveMsgMax;
>
> ...
> union ....
> VhostUserFSSlaveMsg fs;
> VhostUserFSSlaveMsgMax fs_max; /* Never actually used */
> } VhostUserPayload;
>
> and in the .h I had:
> typedef struct {
> /* Generic flags for the overall message */
> uint32_t flags;
> /* Number of entries */
> uint16_t count;
> /* Spare */
> uint16_t align;
>
> VhostUserFSSlaveMsgEntry entries[];
> } VhostUserFSSlaveMsg;
>
> union {
> ...
> VhostUserInflight inflight;
> VhostUserFSSlaveMsg fs;
> } payload;
>
> which is apparently OK in the .c version, and gcc is happy with the same
> in the libvhost-user.h version; but clang gets upset by the .h
> version because it doesn't like the variable length structure
> before the end of the struct - which I have sympathy for.
>
>
Indeed, we probably want to allocate the message separately then.
thanks
--
Marc-André Lureau
