Am 27.03.2014 19:52, schrieb Jeff Cody: [...] > I looked around, and I could not find a definitive source for a VDI > specification. Do you know if there is a specified max size for a VDI > image?
I used the reference which I also mentioned in the header comment of block/vdi.c: http://forums.virtualbox.org/viewtopic.php?t=8046. It does not say anything about a specific maximum size. The image size is limited by some entries in VdiHeader: disk_size is 64 bit, so UINT64_MAX is a hard limit. blocks_in_image is 32 bit, so there is another hard limit of UINT32_MAX * block_size where the default block size is 1 MiB. As you write below, the current implementation in block/vdi.c imposes additional limits which are lower than the hard limits above: the block cache is allocated and filled by functions which take a size_t argument. > > If we assume support for 64 bit size_t values, that may or may not be > within the VDI spec (I don't know). But I think there are practical > limits we need to set in place with our current driver implementation, > as we currently dynamically allocate the block cache based on > blocks_in_image * 4 * SECTOR_SIZE. > > Of course, this needs to be tempered with the notion of not breaking > existing support for VDI images that are in-spec. > >>> +#define VDI_DISK_SIZE_MAX ((uint64_t)VDI_BLOCKS_IN_IMAGE_MAX * \ >>> + (uint64_t)VDI_BLOCK_SIZE) >>> + >> >> This macro cannot be used because the block size might have a non >> default value. >> > > The VDI driver does not currently support non-default block sizes. > There is partial support for vdi image creation for variable block > sizes, but it is commented out (with a note saying it is untested). > But for image open, the vdi_open() function currently has a hard check > for header.block_size != 1MB. I fixed this in the patch which I have sent this week. See http://patchwork.ozlabs.org/patch/334116/. > > So, the above macro is in line with what the VDI driver supports, I > believe. > > This was meant to be a bug-fix only, so adding in new support for > non-default block sizes wasn't the original intent. > Agreed, but it's also good to make the necessary changes so that they go into the right direction. > >>> #if !defined(CONFIG_UUID) >>> static inline void uuid_generate(uuid_t out) >>> { >>> @@ -385,6 +391,11 @@ static int vdi_open(BlockDriverState *bs, QDict >>> *options, int flags, >>> vdi_header_print(&header); >>> #endif >>> >>> + if (header.disk_size > VDI_DISK_SIZE_MAX) { >> >> Here error_setg is missing. The style does not match the other checks. >> More changes are needed because VDI_DISK_SIZE_MAX cannot be used. >> > > I agree error_setg could be useful here. > >>> + ret = -EINVAL; >>> + goto fail; >>> + } >>> + >>> if (header.disk_size % SECTOR_SIZE != 0) { >>> /* 'VBoxManage convertfromraw' can create images with odd disk >>> sizes. >>> We accept them but round the disk size to the next multiple of >>> @@ -420,9 +431,9 @@ static int vdi_open(BlockDriverState *bs, QDict >>> *options, int flags, >>> header.sector_size, SECTOR_SIZE); >>> ret = -ENOTSUP; >>> goto fail; >>> - } else if (header.block_size != 1 * MiB) { >>> + } else if (header.block_size != VDI_BLOCK_SIZE) { >>> error_setg(errp, "unsupported VDI image (sector size %u is not >>> %u)", >> >> Here is a copy+paste bug which was recently introduced. >> > > Yes, the error message should be modified: s/sector/block > > >>> - header.block_size, 1 * MiB); >>> + header.block_size, VDI_BLOCK_SIZE); >> >> Replace VDI_BLOCK_SIZE by the existing macro here. >> > > OK > >>> ret = -ENOTSUP; >>> goto fail; >>> } else if (header.disk_size > >>> @@ -441,6 +452,10 @@ static int vdi_open(BlockDriverState *bs, QDict >>> *options, int flags, >>> error_setg(errp, "unsupported VDI image (non-NULL parent UUID)"); >>> ret = -ENOTSUP; >>> goto fail; >>> + } else if (header.blocks_in_image > VDI_BLOCKS_IN_IMAGE_MAX) { >>> + error_setg(errp, "unsupported VDI image (too many blocks)"); >> >> Fix test and improve error message (show limit) here. >> >>> + ret = -ENOTSUP; >>> + goto fail; >>> } > > > > bs->total_sectors = header.disk_size / SECTOR_SIZE; >>> @@ -689,11 +704,17 @@ static int vdi_create(const char *filename, >>> QEMUOptionParameter *options, >>> options++; >>> } >>> >>> + if (bytes > VDI_DISK_SIZE_MAX) { >> >> Dto. >> >>> + result = -EINVAL; >>> + goto exit; >>> + } >>> + >>> fd = qemu_open(filename, >>> O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, >>> 0644); >>> if (fd < 0) { >>> - return -errno; >>> + result = -errno; >>> + goto exit; >>> } >>> >>> /* We need enough blocks to store the given disk size, >>> @@ -754,6 +775,7 @@ static int vdi_create(const char *filename, >>> QEMUOptionParameter *options, >>> result = -errno; >>> } >>> >>> +exit: >> >> Is goto+label better than a simple return statement? >> > > In this case, maybe not. > >>> return result; >>> } >>> >>> >> >> Do we need this patch for QEMU 2.0? For 32 bit systems, the image size >> limit is 1000 TB, and that image would need 4 GB for the block cache in >> memory. Are such image sizes used anywhere? For 64 bit systems, the >> limit will be much higher. >> > > I don't know what systems are in use in the wild. But since we > allocate block cache to fit the entire cache in RAM currently, that > does open us up to potentially allocating a lot of memory, based on > what the image file header specifies. > > That is a reason to keep the maximum blocks_in_image size bounded to > the size of 0x3fffffff. With an unbound blocks_in_image size (except > to UINT32_MAX), the driver would potentially attempt to allocate 16GB > of RAM, if qemu attempts to open a VDI image file with such a header. Either we crash because of the 0x3fffffff limit, or we might crash because a memory allocation fails (but it might also be successful). I prefer this 2nd variant. > > Of course, if the VDI spec allows for image sizes > 1000TB, then maybe > you are right and we should allow it, even if it means a lot of RAM > allocation. I don't know. > > I think allowing blocks_in_images and block_size to be more variable > is probably a good idea, but I think that if we are going to allow > that, we should probably modify how we handle the block cache. And to > add that support in would seem more in line with a feature addition. > I implemented most of the variable block handling. The only reason why I did not activate it by default was that I had no real test cases. Regards Stefan
