* [Qemu-devel] [PATCH 1/4] block: allow bdrv_unref() to be passed NULL pointers
2014-07-18 20:53 [Qemu-devel] [PATCH 0/4] Allow VPC and VDI to be created over protocols Jeff Cody
@ 2014-07-18 20:53 ` Jeff Cody
2014-07-19 12:59 ` Max Reitz
2014-07-18 20:53 ` [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls Jeff Cody
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Jeff Cody @ 2014-07-18 20:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, sw, stefanha
If bdrv_unref() is passed a NULL BDS pointer, it is safe to
exit with no operation. This will allow cleanup code to blindly
call bdrv_unref() on a BDS that has been initialized to NULL.
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block.c b/block.c
index 23f366d..f79efc8 100644
--- a/block.c
+++ b/block.c
@@ -5385,6 +5385,9 @@ void bdrv_ref(BlockDriverState *bs)
* deleted. */
void bdrv_unref(BlockDriverState *bs)
{
+ if (!bs) {
+ return;
+ }
assert(bs->refcnt > 0);
if (--bs->refcnt == 0) {
bdrv_delete(bs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls
2014-07-18 20:53 [Qemu-devel] [PATCH 0/4] Allow VPC and VDI to be created over protocols Jeff Cody
2014-07-18 20:53 ` [Qemu-devel] [PATCH 1/4] block: allow bdrv_unref() to be passed NULL pointers Jeff Cody
@ 2014-07-18 20:53 ` Jeff Cody
2014-07-19 13:13 ` Max Reitz
2014-07-18 20:54 ` [Qemu-devel] [PATCH 3/4] block: use the standard 'ret' instead of 'result' Jeff Cody
2014-07-18 20:54 ` [Qemu-devel] [PATCH 4/4] block: vpc - use block layer ops in vpc_create, instead of posix calls Jeff Cody
3 siblings, 1 reply; 11+ messages in thread
From: Jeff Cody @ 2014-07-18 20:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, sw, stefanha
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(-)
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;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls
2014-07-18 20:53 ` [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls Jeff Cody
@ 2014-07-19 13:13 ` Max Reitz
2014-07-21 12:58 ` Jeff Cody
0 siblings, 1 reply; 11+ messages in thread
From: Max Reitz @ 2014-07-19 13:13 UTC (permalink / raw)
To: Jeff Cody, qemu-devel; +Cc: kwolf, sw, stefanha
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls
2014-07-19 13:13 ` Max Reitz
@ 2014-07-21 12:58 ` Jeff Cody
2014-07-21 13:02 ` Kevin Wolf
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Cody @ 2014-07-21 12:58 UTC (permalink / raw)
To: Max Reitz; +Cc: kwolf, sw, qemu-devel, stefanha
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls
2014-07-21 12:58 ` Jeff Cody
@ 2014-07-21 13:02 ` Kevin Wolf
0 siblings, 0 replies; 11+ messages in thread
From: Kevin Wolf @ 2014-07-21 13:02 UTC (permalink / raw)
To: Jeff Cody; +Cc: sw, qemu-devel, stefanha, Max Reitz
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/4] block: use the standard 'ret' instead of 'result'
2014-07-18 20:53 [Qemu-devel] [PATCH 0/4] Allow VPC and VDI to be created over protocols Jeff Cody
2014-07-18 20:53 ` [Qemu-devel] [PATCH 1/4] block: allow bdrv_unref() to be passed NULL pointers Jeff Cody
2014-07-18 20:53 ` [Qemu-devel] [PATCH 2/4] block: vdi - use block layer ops in vdi_create, instead of posix calls Jeff Cody
@ 2014-07-18 20:54 ` Jeff Cody
2014-07-19 13:23 ` Max Reitz
2014-07-18 20:54 ` [Qemu-devel] [PATCH 4/4] block: vpc - use block layer ops in vpc_create, instead of posix calls Jeff Cody
3 siblings, 1 reply; 11+ messages in thread
From: Jeff Cody @ 2014-07-18 20:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, sw, stefanha
Most QEMU code uses 'ret' for function return values. The VDI driver
uses a mix of 'result' and 'ret'. This cleans that up, switching over
to the standard 'ret' usage.
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block/vdi.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index a74ba85..880aeea 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -357,23 +357,23 @@ static int vdi_make_empty(BlockDriverState *bs)
static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
{
const VdiHeader *header = (const VdiHeader *)buf;
- int result = 0;
+ int ret = 0;
logout("\n");
if (buf_size < sizeof(*header)) {
/* Header too small, no VDI. */
} else if (le32_to_cpu(header->signature) == VDI_SIGNATURE) {
- result = 100;
+ ret = 100;
}
- if (result == 0) {
+ if (ret == 0) {
logout("no vdi image\n");
} else {
logout("%s", header->text);
}
- return result;
+ return ret;
}
static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
@@ -681,7 +681,7 @@ static int vdi_co_write(BlockDriverState *bs,
static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
{
- int result = 0;
+ int ret = 0;
uint64_t bytes = 0;
uint32_t blocks;
size_t block_size = DEFAULT_CLUSTER_SIZE;
@@ -711,21 +711,21 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
#endif
if (bytes > VDI_DISK_SIZE_MAX) {
- result = -ENOTSUP;
+ ret = -ENOTSUP;
error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
", max supported is 0x%" PRIx64 ")",
bytes, VDI_DISK_SIZE_MAX);
goto exit;
}
- result = bdrv_create_file(filename, opts, &local_err);
- if (result < 0) {
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
error_propagate(errp, local_err);
goto exit;
}
- result = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
- NULL, &local_err);
- if (result < 0) {
+ ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ NULL, &local_err);
+ if (ret < 0) {
error_propagate(errp, local_err);
goto exit;
}
@@ -759,8 +759,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
vdi_header_print(&header);
#endif
vdi_header_to_le(&header);
- result = bdrv_pwrite_sync(bs, offset, &header, sizeof(header));
- if (result < 0) {
+ ret = bdrv_pwrite_sync(bs, offset, &header, sizeof(header));
+ if (ret < 0) {
error_setg(errp, "Error writing header to %s", filename);
goto exit;
}
@@ -775,8 +775,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
bmap[i] = VDI_UNALLOCATED;
}
}
- result = bdrv_pwrite_sync(bs, offset, bmap, bmap_size);
- if (result < 0) {
+ ret = bdrv_pwrite_sync(bs, offset, bmap, bmap_size);
+ if (ret < 0) {
error_setg(errp, "Error writing bmap to %s", filename);
goto exit;
}
@@ -784,8 +784,8 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
}
if (image_type == VDI_TYPE_STATIC) {
- result = bdrv_truncate(bs->file, offset + blocks * block_size);
- if (result < 0) {
+ ret = bdrv_truncate(bs->file, offset + blocks * block_size);
+ if (ret < 0) {
error_setg(errp, "Failed to statically allocate %s", filename);
goto exit;
}
@@ -794,7 +794,7 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
exit:
bdrv_unref(bs);
g_free(bmap);
- return result;
+ return ret;
}
static void vdi_close(BlockDriverState *bs)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 4/4] block: vpc - use block layer ops in vpc_create, instead of posix calls
2014-07-18 20:53 [Qemu-devel] [PATCH 0/4] Allow VPC and VDI to be created over protocols Jeff Cody
` (2 preceding siblings ...)
2014-07-18 20:54 ` [Qemu-devel] [PATCH 3/4] block: use the standard 'ret' instead of 'result' Jeff Cody
@ 2014-07-18 20:54 ` Jeff Cody
2014-07-19 13:33 ` Max Reitz
3 siblings, 1 reply; 11+ messages in thread
From: Jeff Cody @ 2014-07-18 20:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, sw, stefanha
Use the block layer to create, and write to, the image file in the VPC
.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.
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block/vpc.c | 99 +++++++++++++++++++++++++++----------------------------------
1 file changed, 43 insertions(+), 56 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 8b376a4..c418c23 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -656,39 +656,41 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
return 0;
}
-static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
+static int create_dynamic_disk(BlockDriverState *bs, uint8_t *buf,
+ int64_t total_sectors)
{
VHDDynDiskHeader *dyndisk_header =
(VHDDynDiskHeader *) buf;
size_t block_size, num_bat_entries;
int i;
- int ret = -EIO;
+ int ret;
+ int64_t offset = 0;
// Write the footer (twice: at the beginning and at the end)
block_size = 0x200000;
num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
- if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+ ret = bdrv_pwrite_sync(bs, offset, buf, HEADER_SIZE);
+ if (ret) {
goto fail;
}
- if (lseek(fd, 1536 + ((num_bat_entries * 4 + 511) & ~511), SEEK_SET) < 0) {
- goto fail;
- }
- if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+ offset = 1536 + ((num_bat_entries * 4 + 511) & ~511);
+ ret = bdrv_pwrite_sync(bs, offset, buf, HEADER_SIZE);
+ if (ret < 0) {
goto fail;
}
// Write the initial BAT
- if (lseek(fd, 3 * 512, SEEK_SET) < 0) {
- goto fail;
- }
+ offset = 3 * 512;
memset(buf, 0xFF, 512);
for (i = 0; i < (num_bat_entries * 4 + 511) / 512; i++) {
- if (write(fd, buf, 512) != 512) {
+ ret = bdrv_pwrite_sync(bs, offset, buf, 512);
+ if (ret < 0) {
goto fail;
}
+ offset += 512;
}
// Prepare the Dynamic Disk Header
@@ -709,39 +711,35 @@ static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
dyndisk_header->checksum = be32_to_cpu(vpc_checksum(buf, 1024));
// Write the header
- if (lseek(fd, 512, SEEK_SET) < 0) {
- goto fail;
- }
+ offset = 512;
- if (write(fd, buf, 1024) != 1024) {
+ ret = bdrv_pwrite_sync(bs, offset, buf, 1024);
+ if (ret < 0) {
goto fail;
}
- ret = 0;
fail:
return ret;
}
-static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
+static int create_fixed_disk(BlockDriverState *bs, uint8_t *buf,
+ int64_t total_size)
{
- int ret = -EIO;
+ int ret;
/* Add footer to total size */
- total_size += 512;
- if (ftruncate(fd, total_size) != 0) {
- ret = -errno;
- goto fail;
- }
- if (lseek(fd, -512, SEEK_END) < 0) {
- goto fail;
- }
- if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
- goto fail;
+ total_size += HEADER_SIZE;
+
+ ret = bdrv_truncate(bs, total_size);
+ if (ret < 0) {
+ return ret;
}
- ret = 0;
+ ret = bdrv_pwrite_sync(bs, total_size - HEADER_SIZE, buf, HEADER_SIZE);
+ if (ret < 0) {
+ return ret;
+ }
- fail:
return ret;
}
@@ -750,7 +748,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
uint8_t buf[1024];
VHDFooter *footer = (VHDFooter *) buf;
char *disk_type_param;
- int fd, i;
+ int i;
uint16_t cyls = 0;
uint8_t heads = 0;
uint8_t secs_per_cyl = 0;
@@ -758,7 +756,8 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
int64_t total_size;
int disk_type;
int ret = -EIO;
- bool nocow = false;
+ Error *local_err = NULL;
+ BlockDriverState *bs = NULL;
/* Read out options */
total_size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
@@ -775,28 +774,17 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
} else {
disk_type = VHD_DYNAMIC;
}
- nocow = qemu_opt_get_bool_del(opts, BLOCK_OPT_NOCOW, false);
- /* Create the file */
- fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
- if (fd < 0) {
- ret = -EIO;
+ ret = bdrv_create_file(filename, opts, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
goto out;
}
-
- 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
+ ret = bdrv_open(&bs, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ NULL, &local_err);
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ goto out;
}
/*
@@ -810,7 +798,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
&secs_per_cyl))
{
ret = -EFBIG;
- goto fail;
+ goto out;
}
}
@@ -856,14 +844,13 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
if (disk_type == VHD_DYNAMIC) {
- ret = create_dynamic_disk(fd, buf, total_sectors);
+ ret = create_dynamic_disk(bs, buf, total_sectors);
} else {
- ret = create_fixed_disk(fd, buf, total_size);
+ ret = create_fixed_disk(bs, buf, total_size);
}
-fail:
- qemu_close(fd);
out:
+ bdrv_unref(bs);
g_free(disk_type_param);
return ret;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] block: vpc - use block layer ops in vpc_create, instead of posix calls
2014-07-18 20:54 ` [Qemu-devel] [PATCH 4/4] block: vpc - use block layer ops in vpc_create, instead of posix calls Jeff Cody
@ 2014-07-19 13:33 ` Max Reitz
0 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2014-07-19 13:33 UTC (permalink / raw)
To: Jeff Cody, qemu-devel; +Cc: kwolf, sw, stefanha
On 18.07.2014 22:54, Jeff Cody wrote:
> Use the block layer to create, and write to, the image file in the VPC
> .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.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
> block/vpc.c | 99 +++++++++++++++++++++++++++----------------------------------
> 1 file changed, 43 insertions(+), 56 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread