Re: [Qemu-devel] [PATCH v5 4/6] introduce new vma archive format

[Dietmar Maurer]

> > +        return -1;
> > +    }
> > +
> > +    VmaHeader *h = (VmaHeader *)vmar->head_data;
> > +
> > +    if (h->magic != VMA_MAGIC) {
> > +        error_setg(errp, "not a vma file - wrong magic number");
> > +        return -1;
> > +    }
> 
> Doesn't seem like this is endian-safe.  h->magic is host-endian, but VMA_MAGIC
> is big-endian.

h->magic is read from the file, so it should be also big-endian.

> > +
> > +/* File Format Definitions */
> > +
> > +#define VMA_MAGIC (GUINT32_TO_BE(('V'<<24)|('M'<<16)|('A'<<8)|0x00))
> > +#define VMA_EXTENT_MAGIC
> > +(GUINT32_TO_BE(('V'<<24)|('M'<<16)|('A'<<8)|'E'))
> 
> Do we care about EBCDIC, in which case you should be using raw hex values
> instead of relying on ASCII conversion of your magic number?
> That is, I'd much rather see:
> 
> #define VMA_MAGIC 0x564D4100 /* ascii "VMA\0" */

The other code in qemu (for example block/qcow2.h) also use this style:

#define QCOW_MAGIC (('Q' << 24) | ('F' << 16) | ('I' << 8) | 0xfb)

> > +
> > +typedef struct VmaDeviceInfoHeader {
> > +    uint32_t devname_ptr; /* offset into blob_buffer table */
> > +    uint32_t reserved0;
> > +    uint64_t size; /* device size in bytes */
> > +    uint64_t reserved1;
> > +    uint64_t reserved2;
> > +} VmaDeviceInfoHeader;
> > +
> > +typedef struct VmaHeader {
> > +    uint32_t magic;
> > +    uint32_t version;
> > +    unsigned char uuid[16];
> > +    int64_t ctime;
> > +    unsigned char md5sum[16];
> > +
> > +    uint32_t blob_buffer_offset;
> > +    uint32_t blob_buffer_size;
> > +    uint32_t header_size;
> > +
> > +    unsigned char reserved[1984];
> > +
> > +    uint32_t config_names[VMA_MAX_CONFIGS]; /* offset into blob_buffer
> table */
> > +    uint32_t config_data[VMA_MAX_CONFIGS];  /* offset into
> > + blob_buffer table */
> > +
> > +    VmaDeviceInfoHeader dev_info[256]; } VmaHeader;
> 
> Is it worth a compile-time assertion that this header is a fixed size, to 
> make sure
> that future edits evenly reduce the size of reserved when carving out new 
> fields?

I tried to do that, but it seems that gcc does not support sizeof in 
preprocessor conditionals,
so I added a runtime check in vma_writer_create().

Do you have an example how I can add a compile-time assertion that this header 
is a fixed size?