| Message ID | ce5921730e8a99aedc1e5e64571f812e2bd50d11.1405715876.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jcody@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). Other than that, the patch looks good. Max
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 <jcody@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.
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 <jcody@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
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); + if (result < 0) { + error_setg(errp, "Failed to statically allocate %s", filename); + goto exit; } } -close_and_exit: - if ((close(fd) < 0) && !result) { - result = -errno; - } - exit: + bdrv_unref(bs); + g_free(bmap); return result; }
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 <jcody@redhat.com> --- block/vdi.c | 68 ++++++++++++++++++++++++++----------------------------------- 1 file changed, 29 insertions(+), 39 deletions(-)