[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.10?] file-posix: Clear out first sector in
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH for-2.10?] file-posix: Clear out first sector in hdev_create |
Date: |
Fri, 11 Aug 2017 15:28:06 +0800 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Thu, 08/10 08:58, Eric Blake wrote:
> On 08/10/2017 03:01 AM, Fam Zheng wrote:
> > People get surprised when, after "qemu-imc create -f raw /dev/sdX", they
> > still see qcow2 with "qemu-img info", if previously the bdev had a qcow2
> > header. While this is natural because raw doesn't need to write any
> > magic bytes during creation, hdev_create is free to clear out the first
> > sector to make sure the stale qcow2 header doesn't cause such a
> > confusion.
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> > block/file-posix.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index f4de022ae0..1d8ef6f873 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2703,6 +2703,17 @@ static int hdev_create(const char *filename,
> > QemuOpts *opts,
> > ret = -ENOSPC;
> > }
> >
> > + if (total_size) {
> > + int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
> > + uint8_t *buf;
>
> Since BDRV_SECTOR_SIZE is small enough to stack-allocate, you could skip
> the malloc by doing:
>
> uint8_t buf[BDRV_SECTOR_SIZE] = "";
>
> > + if (lseek(fd, 0, SEEK_SET) == -1) {
> > + ret = -errno;
> > + } else {
> > + buf = g_malloc0(zero_size);
> > + ret = qemu_write_full(fd, buf, zero_size);
>
> Instead of doing lseek + qemu_write_full, can we just use
> qemu_pwritev(fd, &iov, 1, 0) with an iov set up to point to the
> appropriate amount of buf?
Neat, will update.
>
> At any rate, my ideas are micro-optimizations, so I can also live with
> how you wrote it.
>
> Reviewed-by: Eric Blake <address@hidden>
>
> Are you arguing that this is a bug-fix worthy of inclusion in 2.10,
> because it helps avoid user confusion? Or are you delaying it to 2.11,
> because we've had the existing behavior for longer than one release, so
> one release more won't hurt?
IMO no need to rush for 2.10, it can wait for 2.11.
Thanks!
Fam