Am 21.07.2014 um 14:58 hat Jeff Cody geschrieben:
> 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.
If qemu-iotests doesn't catch this yet, can you add a case?
Kevin
