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 <diet...@proxmox.com> > ---
> +++ 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
