* [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create
@ 2017-08-11 8:09 Fam Zheng
2017-08-11 14:42 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Fam Zheng @ 2017-08-11 8:09 UTC (permalink / raw
To: qemu-devel; +Cc: Eric Blake, Kevin Wolf, Max Reitz, qemu-block
People get surprised when, after "qemu-img 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 confusion.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
v2: Use stack allocated buffer. [Eric]
Fix return value.
(Keep qemu_write_full instead of switching to qemu_pwritev because
the former handles short writes.)
Fix typo "qemu-img". [Changlong]
---
block/file-posix.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index f4de022ae0..a63bbf2b90 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2703,6 +2703,16 @@ static int hdev_create(const char *filename, QemuOpts *opts,
ret = -ENOSPC;
}
+ if (total_size) {
+ uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
+ int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
+ if (lseek(fd, 0, SEEK_SET) == -1) {
+ ret = -errno;
+ } else {
+ ret = qemu_write_full(fd, buf, zero_size);
+ ret = ret == zero_size ? 0 : -errno;
+ }
+ }
qemu_close(fd);
return ret;
}
--
2.13.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create
2017-08-11 8:09 [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create Fam Zheng
@ 2017-08-11 14:42 ` Eric Blake
2017-08-12 1:06 ` Fam Zheng
2017-08-31 6:53 ` Fam Zheng
2017-09-07 15:07 ` Kevin Wolf
2 siblings, 1 reply; 6+ messages in thread
From: Eric Blake @ 2017-08-11 14:42 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]
On 08/11/2017 03:09 AM, Fam Zheng wrote:
> People get surprised when, after "qemu-img 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 confusion.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
>
> ---
>
> v2: Use stack allocated buffer. [Eric]
> Fix return value.
> (Keep qemu_write_full instead of switching to qemu_pwritev because
> the former handles short writes.)
> Fix typo "qemu-img". [Changlong]
> ---
> block/file-posix.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f4de022ae0..a63bbf2b90 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2703,6 +2703,16 @@ static int hdev_create(const char *filename, QemuOpts *opts,
> ret = -ENOSPC;
> }
>
> + if (total_size) {
> + uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
> + int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
> + if (lseek(fd, 0, SEEK_SET) == -1) {
> + ret = -errno;
> + } else {
> + ret = qemu_write_full(fd, buf, zero_size);
> + ret = ret == zero_size ? 0 : -errno;
> + }
> + }
Question: are we ever constrained by O_DIRECT where writing only 512
bytes would be too small for a block device that mandates 4k alignment?
If so, then we need MAX(minimum write size, MIN(BDRV_SECTOR_SIZE,
total_size)) - it would also mean we can't stack-allocate any more, but
that we have to do an aligned buffer allocation (where g_malloc is not
necessarily suitably aligned).
If O_DIRECT is not a problem, then this is okay:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create
2017-08-11 14:42 ` Eric Blake
@ 2017-08-12 1:06 ` Fam Zheng
0 siblings, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2017-08-12 1:06 UTC (permalink / raw
To: Eric Blake; +Cc: qemu-devel, Kevin Wolf, Max Reitz, qemu-block
On Fri, 08/11 09:42, Eric Blake wrote:
> On 08/11/2017 03:09 AM, Fam Zheng wrote:
> > People get surprised when, after "qemu-img 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 confusion.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> >
> > ---
> >
> > v2: Use stack allocated buffer. [Eric]
> > Fix return value.
> > (Keep qemu_write_full instead of switching to qemu_pwritev because
> > the former handles short writes.)
> > Fix typo "qemu-img". [Changlong]
> > ---
> > block/file-posix.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
>
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index f4de022ae0..a63bbf2b90 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2703,6 +2703,16 @@ static int hdev_create(const char *filename, QemuOpts *opts,
> > ret = -ENOSPC;
> > }
> >
> > + if (total_size) {
> > + uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
> > + int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
> > + if (lseek(fd, 0, SEEK_SET) == -1) {
> > + ret = -errno;
> > + } else {
> > + ret = qemu_write_full(fd, buf, zero_size);
> > + ret = ret == zero_size ? 0 : -errno;
> > + }
> > + }
>
> Question: are we ever constrained by O_DIRECT where writing only 512
> bytes would be too small for a block device that mandates 4k alignment?
> If so, then we need MAX(minimum write size, MIN(BDRV_SECTOR_SIZE,
> total_size)) - it would also mean we can't stack-allocate any more, but
> that we have to do an aligned buffer allocation (where g_malloc is not
> necessarily suitably aligned).
>
> If O_DIRECT is not a problem, then this is okay:
A few lines above:
fd = qemu_open(filename, O_WRONLY | O_BINARY);
so there is no O_DIRECT issue.
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create
2017-08-11 8:09 [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create Fam Zheng
2017-08-11 14:42 ` Eric Blake
@ 2017-08-31 6:53 ` Fam Zheng
2017-09-07 15:07 ` Kevin Wolf
2 siblings, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2017-08-31 6:53 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On Fri, 08/11 16:09, Fam Zheng wrote:
> People get surprised when, after "qemu-img 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 confusion.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
Gentle ping as a reminder for 2.11 as we have now released 2.10.
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create
2017-08-11 8:09 [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create Fam Zheng
2017-08-11 14:42 ` Eric Blake
2017-08-31 6:53 ` Fam Zheng
@ 2017-09-07 15:07 ` Kevin Wolf
2017-09-08 9:44 ` Fam Zheng
2 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2017-09-07 15:07 UTC (permalink / raw
To: Fam Zheng; +Cc: qemu-devel, Eric Blake, Max Reitz, qemu-block
Am 11.08.2017 um 10:09 hat Fam Zheng geschrieben:
> People get surprised when, after "qemu-img 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 confusion.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
>
> ---
>
> v2: Use stack allocated buffer. [Eric]
> Fix return value.
> (Keep qemu_write_full instead of switching to qemu_pwritev because
> the former handles short writes.)
> Fix typo "qemu-img". [Changlong]
> ---
> block/file-posix.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f4de022ae0..a63bbf2b90 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2703,6 +2703,16 @@ static int hdev_create(const char *filename, QemuOpts *opts,
> ret = -ENOSPC;
> }
So the error paths above only set ret, but don't actually return or jump
to the end of the function.
> + if (total_size) {
> + uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
> + int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
> + if (lseek(fd, 0, SEEK_SET) == -1) {
> + ret = -errno;
> + } else {
> + ret = qemu_write_full(fd, buf, zero_size);
> + ret = ret == zero_size ? 0 : -errno;
Which means that an error above (like a too small block device or using
a regular file) can be overwritten with a success value if clearing the
first sector works. That's probably not quite right.
> + }
> + }
> qemu_close(fd);
> return ret;
> }
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create
2017-09-07 15:07 ` Kevin Wolf
@ 2017-09-08 9:44 ` Fam Zheng
0 siblings, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2017-09-08 9:44 UTC (permalink / raw
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On Thu, 09/07 17:07, Kevin Wolf wrote:
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index f4de022ae0..a63bbf2b90 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2703,6 +2703,16 @@ static int hdev_create(const char *filename, QemuOpts *opts,
> > ret = -ENOSPC;
> > }
>
> So the error paths above only set ret, but don't actually return or jump
> to the end of the function.
>
> > + if (total_size) {
> > + uint8_t buf[BDRV_SECTOR_SIZE] = { 0 };
> > + int64_t zero_size = MIN(BDRV_SECTOR_SIZE, total_size);
> > + if (lseek(fd, 0, SEEK_SET) == -1) {
> > + ret = -errno;
> > + } else {
> > + ret = qemu_write_full(fd, buf, zero_size);
> > + ret = ret == zero_size ? 0 : -errno;
>
> Which means that an error above (like a too small block device or using
> a regular file) can be overwritten with a success value if clearing the
> first sector works. That's probably not quite right.
You're right, will fix.
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-08 9:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-11 8:09 [Qemu-devel] [PATCH for-2.11 v2] file-posix: Clear out first sector in hdev_create Fam Zheng
2017-08-11 14:42 ` Eric Blake
2017-08-12 1:06 ` Fam Zheng
2017-08-31 6:53 ` Fam Zheng
2017-09-07 15:07 ` Kevin Wolf
2017-09-08 9:44 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.