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

[Eric Blake]

On 02/21/2013 04:27 AM, Dietmar Maurer wrote:
> This is a very simple archive format, see docs/specs/vma_spec.txt
> 
> Signed-off-by: Dietmar Maurer <address@hidden>
> ---

> +++ b/docs/specs/vma_spec.txt
> @@ -0,0 +1,24 @@
> +=Virtual Machine Archive format (VMA)=
> +
> +This format contains a header which includes the VM configuration as
> +binary blobs, and a list of devices (dev_id, name).

I still argue that forcing the reader to look at source code to learn
the magic number is not helpful.

> +static int vma_reader_read_head(VmaReader *vmar, Error **errp)
> +{
> +    assert(vmar);
> +    assert(errp);
> +    assert(*errp == NULL);
> +
> +    unsigned char md5sum[16];
> +    int i;
> +    int ret = 0;
> +
> +    vmar->head_data = g_malloc(sizeof(VmaHeader));
> +
> +    if (full_read(vmar->fd, vmar->head_data, sizeof(VmaHeader)) !=
> +        sizeof(VmaHeader)) {
> +        error_setg(errp, "can't read vma header - %s",
> +                   errno ? strerror(errno) : "got EOF");

You're not the first user, but strerror() isn't thread-safe.  strerror_r
is not necessarily portable (glibc vs. POSIX), and strerror_l isn't yet
widely implemented.  Should qemu be providing a better interface
qemu_strerror() that guarantees thread-safety when converting errno to a
string?

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

> +
> +/* 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" */

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature