On Sat, Jul 19, 2014 at 03:13:46PM +0200, Max Reitz wrote:
> On 18.07.2014 22:53, Jeff Cody wrote:
> >Use the block layer to create, and write to, the image file in the
> >VDI .bdrv_create() operation.
> >
> >This has a couple of benefits: Images can now be created over protocols,
> >and host raw file optimizations (such as nocow) do not need to be
> >handled in the image format driver.
> >
> >Also some minor cleanup for error handling.
> >
> >Signed-off-by: Jeff Cody <jc...@redhat.com>
> >---
> > block/vdi.c | 68
> > ++++++++++++++++++++++++++-----------------------------------
> > 1 file changed, 29 insertions(+), 39 deletions(-)
>
> With this, I think you could have dropped the #ifdef __linux__ block
> from the top of block/vdi.c, too.
>
> >diff --git a/block/vdi.c b/block/vdi.c
> >index 197bd77..a74ba85 100644
> >--- a/block/vdi.c
> >+++ b/block/vdi.c
> >@@ -681,7 +681,6 @@ static int vdi_co_write(BlockDriverState *bs,
> > static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
> > {
> >- int fd;
> > int result = 0;
> > uint64_t bytes = 0;
> > uint32_t blocks;
> >@@ -690,7 +689,10 @@ static int vdi_create(const char *filename, QemuOpts
> >*opts, Error **errp)
> > VdiHeader header;
> > size_t i;
> > size_t bmap_size;
> >- bool nocow = false;
> >+ int64_t offset = 0;
> >+ Error *local_err = NULL;
> >+ BlockDriverState *bs = NULL;
> >+ uint32_t *bmap = NULL;
> > logout("\n");
> >@@ -707,7 +709,6 @@ static int vdi_create(const char *filename, QemuOpts
> >*opts, Error **errp)
> > image_type = VDI_TYPE_STATIC;
> > }
> > #endif
> >- nocow = qemu_opt_get_bool_del(opts, BLOCK_OPT_NOCOW, false);
> > if (bytes > VDI_DISK_SIZE_MAX) {
> > result = -ENOTSUP;
> >@@ -717,27 +718,16 @@ static int vdi_create(const char *filename, QemuOpts
> >*opts, Error **errp)
> > goto exit;
> > }
> >- fd = qemu_open(filename,
> >- O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> >- 0644);
> >- if (fd < 0) {
> >- result = -errno;
> >+ result = bdrv_create_file(filename, opts, &local_err);
> >+ if (result < 0) {
> >+ error_propagate(errp, local_err);
> > goto exit;
> > }
> >-
> >- if (nocow) {
> >-#ifdef __linux__
> >- /* Set NOCOW flag to solve performance issue on fs like btrfs.
> >- * This is an optimisation. The FS_IOC_SETFLAGS ioctl return value
> >will
> >- * be ignored since any failure of this operation should not block
> >the
> >- * left work.
> >- */
> >- int attr;
> >- if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0) {
> >- attr |= FS_NOCOW_FL;
> >- ioctl(fd, FS_IOC_SETFLAGS, &attr);
> >- }
> >-#endif
> >+ result = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR |
> >BDRV_O_PROTOCOL,
> >+ NULL, &local_err);
> >+ if (result < 0) {
> >+ error_propagate(errp, local_err);
> >+ goto exit;
> > }
> > /* We need enough blocks to store the given disk size,
> >@@ -769,13 +759,15 @@ static int vdi_create(const char *filename, QemuOpts
> >*opts, Error **errp)
> > vdi_header_print(&header);
> > #endif
> > vdi_header_to_le(&header);
> >- if (write(fd, &header, sizeof(header)) < 0) {
> >- result = -errno;
> >- goto close_and_exit;
> >+ result = bdrv_pwrite_sync(bs, offset, &header, sizeof(header));
> >+ if (result < 0) {
> >+ error_setg(errp, "Error writing header to %s", filename);
> >+ goto exit;
> > }
> >+ offset += sizeof(header);
> > if (bmap_size > 0) {
> >- uint32_t *bmap = g_malloc0(bmap_size);
> >+ bmap = g_malloc0(bmap_size);
> > for (i = 0; i < blocks; i++) {
> > if (image_type == VDI_TYPE_STATIC) {
> > bmap[i] = i;
> >@@ -783,27 +775,25 @@ static int vdi_create(const char *filename, QemuOpts
> >*opts, Error **errp)
> > bmap[i] = VDI_UNALLOCATED;
> > }
> > }
> >- if (write(fd, bmap, bmap_size) < 0) {
> >- result = -errno;
> >- g_free(bmap);
> >- goto close_and_exit;
> >+ result = bdrv_pwrite_sync(bs, offset, bmap, bmap_size);
> >+ if (result < 0) {
> >+ error_setg(errp, "Error writing bmap to %s", filename);
> >+ goto exit;
> > }
> >- g_free(bmap);
> >+ offset += bmap_size;
> > }
> > if (image_type == VDI_TYPE_STATIC) {
> >- if (ftruncate(fd, sizeof(header) + bmap_size + blocks *
> >block_size)) {
> >- result = -errno;
> >- goto close_and_exit;
> >+ result = bdrv_truncate(bs->file, offset + blocks * block_size);
>
> bs actually *is* your file, so this should be just bs and not
> bs->file (like this, "qemu-img create -f vdi -o static=on foo.vdi
> 128K" segfaults).
>
Oops, yes that is what I meant. Sending v2.
