* [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
@ 2020-04-28 13:26 ` Max Reitz
2020-04-28 13:53 ` Eric Blake
2020-04-28 14:21 ` Kevin Wolf
2020-04-28 13:26 ` [PATCH 2/4] block: Use bdrv_make_empty() where possible Max Reitz
` (9 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-28 13:26 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Right now, all users of bdrv_make_empty() call the BlockDriver method
directly. That is not only bad style, it is also wrong, unless the
caller has a BdrvChild with a WRITE permission.
Introduce bdrv_make_empty() that verifies that it does.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/block.h | 1 +
block.c | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index b05995fe9c..d947fb4080 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
int bdrv_commit(BlockDriverState *bs);
+int bdrv_make_empty(BdrvChild *c, Error **errp);
int bdrv_change_backing_file(BlockDriverState *bs,
const char *backing_file, const char *backing_fmt);
void bdrv_register(BlockDriver *bdrv);
diff --git a/block.c b/block.c
index 2e3905c99e..b0d5b98617 100644
--- a/block.c
+++ b/block.c
@@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
}
+
+int bdrv_make_empty(BdrvChild *c, Error **errp)
+{
+ BlockDriver *drv = c->bs->drv;
+ int ret;
+
+ assert(c->perm & BLK_PERM_WRITE);
+
+ if (!drv->bdrv_make_empty) {
+ error_setg(errp, "%s does not support emptying nodes",
+ drv->format_name);
+ return -ENOTSUP;
+ }
+
+ ret = drv->bdrv_make_empty(c->bs);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to empty %s",
+ c->bs->filename);
+ return ret;
+ }
+
+ return 0;
+}
--
2.25.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 13:26 ` [PATCH 1/4] block: Add bdrv_make_empty() Max Reitz
@ 2020-04-28 13:53 ` Eric Blake
2020-04-28 14:01 ` Kevin Wolf
2020-04-28 14:21 ` Kevin Wolf
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2020-04-28 13:53 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:26 AM, Max Reitz wrote:
> Right now, all users of bdrv_make_empty() call the BlockDriver method
> directly. That is not only bad style, it is also wrong, unless the
> caller has a BdrvChild with a WRITE permission.
>
> Introduce bdrv_make_empty() that verifies that it does.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/block/block.h | 1 +
> block.c | 23 +++++++++++++++++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index b05995fe9c..d947fb4080 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
> int bdrv_commit(BlockDriverState *bs);
> +int bdrv_make_empty(BdrvChild *c, Error **errp);
Can we please fix this to take a flags parameter? I want to make it
easier for callers to request BDRV_REQ_NO_FALLBACK for distinguishing
between callers where the image must be made empty (read as all zeroes)
regardless of time spent, vs. made empty quickly (including if it is
already all zero) but where the caller is prepared for the operation to
fail and will write zeroes itself if fast bulk zeroing was not possible.
> +int bdrv_make_empty(BdrvChild *c, Error **errp)
> +{
> + BlockDriver *drv = c->bs->drv;
> + int ret;
> +
> + assert(c->perm & BLK_PERM_WRITE);
> +
> + if (!drv->bdrv_make_empty) {
> + error_setg(errp, "%s does not support emptying nodes",
> + drv->format_name);
> + return -ENOTSUP;
> + }
And here's where we can add some automatic fallbacks, such as
recognizing if the image already reads as all zeroes. But those
optimizations can come in separate patches; for YOUR patch, just getting
the proper API in place is fine.
> +
> + ret = drv->bdrv_make_empty(c->bs);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to empty %s",
> + c->bs->filename);
> + return ret;
> + }
> +
> + return 0;
> +}
>
Other than a missing flag parameter, this looks fine.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 13:53 ` Eric Blake
@ 2020-04-28 14:01 ` Kevin Wolf
2020-04-28 14:07 ` Eric Blake
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2020-04-28 14:01 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, qemu-block, Max Reitz
Am 28.04.2020 um 15:53 hat Eric Blake geschrieben:
> On 4/28/20 8:26 AM, Max Reitz wrote:
> > Right now, all users of bdrv_make_empty() call the BlockDriver method
> > directly. That is not only bad style, it is also wrong, unless the
> > caller has a BdrvChild with a WRITE permission.
> >
> > Introduce bdrv_make_empty() that verifies that it does.
> >
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> > include/block/block.h | 1 +
> > block.c | 23 +++++++++++++++++++++++
> > 2 files changed, 24 insertions(+)
> >
> > diff --git a/include/block/block.h b/include/block/block.h
> > index b05995fe9c..d947fb4080 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> > void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> > void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
> > int bdrv_commit(BlockDriverState *bs);
> > +int bdrv_make_empty(BdrvChild *c, Error **errp);
>
> Can we please fix this to take a flags parameter? I want to make it easier
> for callers to request BDRV_REQ_NO_FALLBACK for distinguishing between
> callers where the image must be made empty (read as all zeroes) regardless
> of time spent, vs. made empty quickly (including if it is already all zero)
> but where the caller is prepared for the operation to fail and will write
> zeroes itself if fast bulk zeroing was not possible.
bdrv_make_empty() is not for making an image read as all zeroes, but to
make it fully unallocated so that the backing file becomes visible.
Are you confusing it with bdrv_make_zero(), which is just a wrapper
around bdrv_pwrite_zeroes() and does take flags?
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 14:01 ` Kevin Wolf
@ 2020-04-28 14:07 ` Eric Blake
2020-04-28 14:16 ` Kevin Wolf
0 siblings, 1 reply; 31+ messages in thread
From: Eric Blake @ 2020-04-28 14:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On 4/28/20 9:01 AM, Kevin Wolf wrote:
>> Can we please fix this to take a flags parameter? I want to make it easier
>> for callers to request BDRV_REQ_NO_FALLBACK for distinguishing between
>> callers where the image must be made empty (read as all zeroes) regardless
>> of time spent, vs. made empty quickly (including if it is already all zero)
>> but where the caller is prepared for the operation to fail and will write
>> zeroes itself if fast bulk zeroing was not possible.
>
> bdrv_make_empty() is not for making an image read as all zeroes, but to
> make it fully unallocated so that the backing file becomes visible.
>
> Are you confusing it with bdrv_make_zero(), which is just a wrapper
> around bdrv_pwrite_zeroes() and does take flags?
Yes. Although now I'm wondering if the two should remain separate or
should just be a single driver callback where flags can include
BDRV_REQ_ZERO_WRITE to distinguish whether exposing the backing file vs.
reading as all zeroes is intended, or if that is merging too much.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 14:07 ` Eric Blake
@ 2020-04-28 14:16 ` Kevin Wolf
2020-04-28 14:25 ` Eric Blake
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2020-04-28 14:16 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, qemu-block, Max Reitz
Am 28.04.2020 um 16:07 hat Eric Blake geschrieben:
> On 4/28/20 9:01 AM, Kevin Wolf wrote:
>
> > > Can we please fix this to take a flags parameter? I want to make it easier
> > > for callers to request BDRV_REQ_NO_FALLBACK for distinguishing between
> > > callers where the image must be made empty (read as all zeroes) regardless
> > > of time spent, vs. made empty quickly (including if it is already all zero)
> > > but where the caller is prepared for the operation to fail and will write
> > > zeroes itself if fast bulk zeroing was not possible.
> >
> > bdrv_make_empty() is not for making an image read as all zeroes, but to
> > make it fully unallocated so that the backing file becomes visible.
> >
> > Are you confusing it with bdrv_make_zero(), which is just a wrapper
> > around bdrv_pwrite_zeroes() and does take flags?
>
> Yes. Although now I'm wondering if the two should remain separate or should
> just be a single driver callback where flags can include BDRV_REQ_ZERO_WRITE
> to distinguish whether exposing the backing file vs. reading as all zeroes
> is intended, or if that is merging too much.
I don't think the implementations for both things are too similar, so
you might just end up having two if branches and then two separate
implementations in the block drivers.
If anything, bdrv_make_empty() is more related to discard than
write_zeroes. But we use the discard code for it in qcow2 only as a
fallback because in the most common cases, making an image completely
empty means effectively just creating an entirely new L1 and refcount
table, which is much faster than individually discarding all clusters.
For bdrv_make_zero() I don't see an opportunity for such optimisations,
so I don't really see a reason to have a separate callback. Unless you
do know one?
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 14:16 ` Kevin Wolf
@ 2020-04-28 14:25 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2020-04-28 14:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On 4/28/20 9:16 AM, Kevin Wolf wrote:
>>
>> Yes. Although now I'm wondering if the two should remain separate or should
>> just be a single driver callback where flags can include BDRV_REQ_ZERO_WRITE
>> to distinguish whether exposing the backing file vs. reading as all zeroes
>> is intended, or if that is merging too much.
>
> I don't think the implementations for both things are too similar, so
> you might just end up having two if branches and then two separate
> implementations in the block drivers.
>
Yeah, the more I think about it, the more two callbacks still make
sense. .bdrv_make_empty may or may not need a flag, but .bdrv_make_zero
definitely does (because that's where we want a difference between
making the entire image zero no matter the delay, vs. only making it all
zero if it is is fast).
> If anything, bdrv_make_empty() is more related to discard than
> write_zeroes. But we use the discard code for it in qcow2 only as a
> fallback because in the most common cases, making an image completely
> empty means effectively just creating an entirely new L1 and refcount
> table, which is much faster than individually discarding all clusters.
>
> For bdrv_make_zero() I don't see an opportunity for such optimisations,
> so I don't really see a reason to have a separate callback. Unless you
> do know one?
The optimization I have in mind is adding a qcow2 autoclear bit to track
when an image is known to read as all zero - at which point
.bdrv_make_zero instantly returns success. For raw files, a possible
optimization is to truncate to size 0 and then back to the full size,
when it is known that truncation forces read-as-zero. For NBD, I'm
still playing with whether adding new 64-bit transactions for a bulk
zero will be useful, and even if not, maybe special-casing
NBD_CMD_WRITE_ZEROES with a size of 0 to do a bulk zero, if both server
and client negotiated that particular meaning.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 13:26 ` [PATCH 1/4] block: Add bdrv_make_empty() Max Reitz
2020-04-28 13:53 ` Eric Blake
@ 2020-04-28 14:21 ` Kevin Wolf
2020-04-29 7:39 ` Max Reitz
1 sibling, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2020-04-28 14:21 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Right now, all users of bdrv_make_empty() call the BlockDriver method
> directly. That is not only bad style, it is also wrong, unless the
> caller has a BdrvChild with a WRITE permission.
>
> Introduce bdrv_make_empty() that verifies that it does.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/block/block.h | 1 +
> block.c | 23 +++++++++++++++++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index b05995fe9c..d947fb4080 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
> void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
> int bdrv_commit(BlockDriverState *bs);
> +int bdrv_make_empty(BdrvChild *c, Error **errp);
> int bdrv_change_backing_file(BlockDriverState *bs,
> const char *backing_file, const char *backing_fmt);
> void bdrv_register(BlockDriver *bdrv);
> diff --git a/block.c b/block.c
> index 2e3905c99e..b0d5b98617 100644
> --- a/block.c
> +++ b/block.c
> @@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>
> parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
> }
> +
> +int bdrv_make_empty(BdrvChild *c, Error **errp)
> +{
> + BlockDriver *drv = c->bs->drv;
> + int ret;
> +
> + assert(c->perm & BLK_PERM_WRITE);
If I understand correctly, bdrv_make_empty() is called to drop an
overlay whose content is identical to what it would read from its
backing file (in particular after a commit operation). This means that
the caller promises that the visible content doesn't change.
So should we check BLK_PERM_WRITE_UNCHANGED instead?
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/4] block: Add bdrv_make_empty()
2020-04-28 14:21 ` Kevin Wolf
@ 2020-04-29 7:39 ` Max Reitz
0 siblings, 0 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-29 7:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 2125 bytes --]
On 28.04.20 16:21, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Right now, all users of bdrv_make_empty() call the BlockDriver method
>> directly. That is not only bad style, it is also wrong, unless the
>> caller has a BdrvChild with a WRITE permission.
>>
>> Introduce bdrv_make_empty() that verifies that it does.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> include/block/block.h | 1 +
>> block.c | 23 +++++++++++++++++++++++
>> 2 files changed, 24 insertions(+)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index b05995fe9c..d947fb4080 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
>> void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>> void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>> int bdrv_commit(BlockDriverState *bs);
>> +int bdrv_make_empty(BdrvChild *c, Error **errp);
>> int bdrv_change_backing_file(BlockDriverState *bs,
>> const char *backing_file, const char *backing_fmt);
>> void bdrv_register(BlockDriver *bdrv);
>> diff --git a/block.c b/block.c
>> index 2e3905c99e..b0d5b98617 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6791,3 +6791,26 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
>>
>> parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>> }
>> +
>> +int bdrv_make_empty(BdrvChild *c, Error **errp)
>> +{
>> + BlockDriver *drv = c->bs->drv;
>> + int ret;
>> +
>> + assert(c->perm & BLK_PERM_WRITE);
>
> If I understand correctly, bdrv_make_empty() is called to drop an
> overlay whose content is identical to what it would read from its
> backing file (in particular after a commit operation). This means that
> the caller promises that the visible content doesn't change.
>
> So should we check BLK_PERM_WRITE_UNCHANGED instead?
Ah, right. Yes, that would be better. (Or to check both, whether any
of them has been taken.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/4] block: Use bdrv_make_empty() where possible
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
2020-04-28 13:26 ` [PATCH 1/4] block: Add bdrv_make_empty() Max Reitz
@ 2020-04-28 13:26 ` Max Reitz
2020-04-28 13:54 ` Eric Blake
2020-04-28 15:03 ` Kevin Wolf
2020-04-28 13:26 ` [PATCH 3/4] block: Add blk_make_empty() Max Reitz
` (8 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-28 13:26 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/replication.c | 6 ++----
block/vvfat.c | 4 +---
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index da013c2041..cc6a40d577 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -331,9 +331,8 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
return;
}
- ret = s->active_disk->bs->drv->bdrv_make_empty(s->active_disk->bs);
+ ret = bdrv_make_empty(s->active_disk, errp);
if (ret < 0) {
- error_setg(errp, "Cannot make active disk empty");
return;
}
@@ -343,9 +342,8 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
return;
}
- ret = s->hidden_disk->bs->drv->bdrv_make_empty(s->hidden_disk->bs);
+ ret = bdrv_make_empty(s->hidden_disk, errp);
if (ret < 0) {
- error_setg(errp, "Cannot make hidden disk empty");
return;
}
}
diff --git a/block/vvfat.c b/block/vvfat.c
index ab800c4887..e3020b65c8 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2960,9 +2960,7 @@ static int do_commit(BDRVVVFATState* s)
return ret;
}
- if (s->qcow->bs->drv && s->qcow->bs->drv->bdrv_make_empty) {
- s->qcow->bs->drv->bdrv_make_empty(s->qcow->bs);
- }
+ bdrv_make_empty(s->qcow, NULL);
memset(s->used_clusters, 0, sector2cluster(s, s->sector_count));
--
2.25.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] block: Use bdrv_make_empty() where possible
2020-04-28 13:26 ` [PATCH 2/4] block: Use bdrv_make_empty() where possible Max Reitz
@ 2020-04-28 13:54 ` Eric Blake
2020-04-28 15:03 ` Kevin Wolf
1 sibling, 0 replies; 31+ messages in thread
From: Eric Blake @ 2020-04-28 13:54 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:26 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/replication.c | 6 ++----
> block/vvfat.c | 4 +---
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
Yes, definitely nicer :) May have some obvious fallout to add a 0 flag
parameter, per my request on 1/4, but that doesn't stop me from giving:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/4] block: Use bdrv_make_empty() where possible
2020-04-28 13:26 ` [PATCH 2/4] block: Use bdrv_make_empty() where possible Max Reitz
2020-04-28 13:54 ` Eric Blake
@ 2020-04-28 15:03 ` Kevin Wolf
1 sibling, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2020-04-28 15:03 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/4] block: Add blk_make_empty()
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
2020-04-28 13:26 ` [PATCH 1/4] block: Add bdrv_make_empty() Max Reitz
2020-04-28 13:26 ` [PATCH 2/4] block: Use bdrv_make_empty() where possible Max Reitz
@ 2020-04-28 13:26 ` Max Reitz
2020-04-28 13:55 ` Eric Blake
2020-04-28 14:47 ` Kevin Wolf
2020-04-28 13:26 ` [PATCH 4/4] block: Use blk_make_empty() after commits Max Reitz
` (7 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-28 13:26 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
Two callers of BlockDriver.bdrv_make_empty() remain that should not call
this method directly. Both do not have access to a BdrvChild, but they
can use a BlockBackend, so we add this function that lets them use it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/sysemu/block-backend.h | 2 ++
block/block-backend.c | 5 +++++
2 files changed, 7 insertions(+)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index d37c1244dd..14338b76dc 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
const BdrvChild *blk_root(BlockBackend *blk);
+int blk_make_empty(BlockBackend *blk, Error **errp);
+
#endif
diff --git a/block/block-backend.c b/block/block-backend.c
index 3592066b42..5d36efd32f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
{
return blk->root;
}
+
+int blk_make_empty(BlockBackend *blk, Error **errp)
+{
+ return bdrv_make_empty(blk->root, errp);
+}
--
2.25.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] block: Add blk_make_empty()
2020-04-28 13:26 ` [PATCH 3/4] block: Add blk_make_empty() Max Reitz
@ 2020-04-28 13:55 ` Eric Blake
2020-04-28 14:28 ` Eric Blake
2020-04-28 14:47 ` Kevin Wolf
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2020-04-28 13:55 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:26 AM, Max Reitz wrote:
> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
> this method directly. Both do not have access to a BdrvChild, but they
> can use a BlockBackend, so we add this function that lets them use it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/sysemu/block-backend.h | 2 ++
> block/block-backend.c | 5 +++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index d37c1244dd..14338b76dc 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>
> const BdrvChild *blk_root(BlockBackend *blk);
>
> +int blk_make_empty(BlockBackend *blk, Error **errp);
> +
Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice.
> #endif
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3592066b42..5d36efd32f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
> {
> return blk->root;
> }
> +
> +int blk_make_empty(BlockBackend *blk, Error **errp)
> +{
> + return bdrv_make_empty(blk->root, errp);
> +}
>
Otherwise looks fine.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] block: Add blk_make_empty()
2020-04-28 13:55 ` Eric Blake
@ 2020-04-28 14:28 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2020-04-28 14:28 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:55 AM, Eric Blake wrote:
>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend
>> *blk_in, int64_t off_in,
>> const BdrvChild *blk_root(BlockBackend *blk);
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
>
> Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice.
Or maybe not, after reading Kevin's responses. Making an image empty is
not the same as making it read as zero. If we can't come up with a use
for a flag, then deferring the addition of a flag until later is a
perfectly reasonable approach (rather than adding a flag now that will
never get set to anything other than 0). This isn't quite the same as a
public API where we would regret being locked out of a flag down the road.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] block: Add blk_make_empty()
2020-04-28 13:26 ` [PATCH 3/4] block: Add blk_make_empty() Max Reitz
2020-04-28 13:55 ` Eric Blake
@ 2020-04-28 14:47 ` Kevin Wolf
2020-04-29 7:39 ` Max Reitz
1 sibling, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2020-04-28 14:47 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
> this method directly. Both do not have access to a BdrvChild, but they
> can use a BlockBackend, so we add this function that lets them use it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/sysemu/block-backend.h | 2 ++
> block/block-backend.c | 5 +++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index d37c1244dd..14338b76dc 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>
> const BdrvChild *blk_root(BlockBackend *blk);
>
> +int blk_make_empty(BlockBackend *blk, Error **errp);
> +
> #endif
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3592066b42..5d36efd32f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
> {
> return blk->root;
> }
> +
> +int blk_make_empty(BlockBackend *blk, Error **errp)
> +{
> + return bdrv_make_empty(blk->root, errp);
> +}
Should we check that blk->root != NULL? Most other functions do that
through blk_is_available().
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/4] block: Add blk_make_empty()
2020-04-28 14:47 ` Kevin Wolf
@ 2020-04-29 7:39 ` Max Reitz
0 siblings, 0 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-29 7:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 1484 bytes --]
On 28.04.20 16:47, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
>> this method directly. Both do not have access to a BdrvChild, but they
>> can use a BlockBackend, so we add this function that lets them use it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> include/sysemu/block-backend.h | 2 ++
>> block/block-backend.c | 5 +++++
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index d37c1244dd..14338b76dc 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>>
>> const BdrvChild *blk_root(BlockBackend *blk);
>>
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
>> #endif
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 3592066b42..5d36efd32f 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>> {
>> return blk->root;
>> }
>> +
>> +int blk_make_empty(BlockBackend *blk, Error **errp)
>> +{
>> + return bdrv_make_empty(blk->root, errp);
>> +}
>
> Should we check that blk->root != NULL? Most other functions do that
> through blk_is_available().
Why not.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/4] block: Use blk_make_empty() after commits
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (2 preceding siblings ...)
2020-04-28 13:26 ` [PATCH 3/4] block: Add blk_make_empty() Max Reitz
@ 2020-04-28 13:26 ` Max Reitz
2020-04-28 14:07 ` Eric Blake
2020-04-28 15:03 ` Kevin Wolf
2020-04-28 13:38 ` [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly no-reply
` (6 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-28 13:26 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz
bdrv_commit() already has a BlockBackend pointing to the BDS that we
want to empty, it just has the wrong permissions.
qemu-img commit has no BlockBackend pointing to the old backing file
yet, but introducing one is simple.
After this commit, bdrv_make_empty() is the only remaining caller of
BlockDriver.bdrv_make_empty().
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/commit.c | 8 +++++++-
qemu-img.c | 19 ++++++++++++++-----
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 8e672799af..24720ba67d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
}
if (drv->bdrv_make_empty) {
- ret = drv->bdrv_make_empty(bs);
+ ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
if (ret < 0) {
goto ro_cleanup;
}
+
+ ret = blk_make_empty(src, NULL);
+ if (ret < 0) {
+ goto ro_cleanup;
+ }
+
blk_flush(src);
}
diff --git a/qemu-img.c b/qemu-img.c
index 821cbf610e..a5e8659867 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
goto unref_backing;
}
- if (!drop && bs->drv->bdrv_make_empty) {
- ret = bs->drv->bdrv_make_empty(bs);
- if (ret) {
- error_setg_errno(&local_err, -ret, "Could not empty %s",
- filename);
+ if (!drop) {
+ BlockBackend *old_backing_blk;
+
+ old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
+ &local_err);
+ if (!old_backing_blk) {
+ goto unref_backing;
+ }
+ ret = blk_make_empty(old_backing_blk, &local_err);
+ blk_unref(old_backing_blk);
+ if (ret == -ENOTSUP) {
+ error_free(local_err);
+ local_err = NULL;
+ } else if (ret < 0) {
goto unref_backing;
}
}
--
2.25.4
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] block: Use blk_make_empty() after commits
2020-04-28 13:26 ` [PATCH 4/4] block: Use blk_make_empty() after commits Max Reitz
@ 2020-04-28 14:07 ` Eric Blake
2020-04-29 7:58 ` Max Reitz
2020-04-28 15:03 ` Kevin Wolf
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2020-04-28 14:07 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:26 AM, Max Reitz wrote:
> bdrv_commit() already has a BlockBackend pointing to the BDS that we
> want to empty, it just has the wrong permissions.
>
> qemu-img commit has no BlockBackend pointing to the old backing file
> yet, but introducing one is simple.
>
> After this commit, bdrv_make_empty() is the only remaining caller of
> BlockDriver.bdrv_make_empty().
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/commit.c | 8 +++++++-
> qemu-img.c | 19 ++++++++++++++-----
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 8e672799af..24720ba67d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
> }
>
> if (drv->bdrv_make_empty) {
This 'if' is still a bit awkward. Do all filter drivers set this
function, or will bdrv_make_empty() automatically take care of looking
through filters? Should this be a check of a supported_ variable
instead (similar to how Kevin just added supported_truncate_flags)?
> - ret = drv->bdrv_make_empty(bs);
> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
> if (ret < 0) {
> goto ro_cleanup;
> }
> +
> + ret = blk_make_empty(src, NULL);
So, if the driver lacks the callback, you miss calling blk_make_empty()
even if it would have done something.
> + if (ret < 0) {
> + goto ro_cleanup;
> + }
> +
> blk_flush(src);
> }
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e..a5e8659867 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
> goto unref_backing;
> }
>
> - if (!drop && bs->drv->bdrv_make_empty) {
> - ret = bs->drv->bdrv_make_empty(bs);
Old code: if the driver had the callback, use it.
> - if (ret) {
> - error_setg_errno(&local_err, -ret, "Could not empty %s",
> - filename);
> + if (!drop) {
> + BlockBackend *old_backing_blk;
> +
> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
> + &local_err);
> + if (!old_backing_blk) {
> + goto unref_backing;
> + }
> + ret = blk_make_empty(old_backing_blk, &local_err);
New code: Call blk_make_empty() whether or not driver has the callback,
then deal with the fallout.
> + blk_unref(old_backing_blk);
> + if (ret == -ENOTSUP) {
> + error_free(local_err);
> + local_err = NULL;
> + } else if (ret < 0) {
> goto unref_backing;
> }
But this actually looks smarter than the commit case.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] block: Use blk_make_empty() after commits
2020-04-28 14:07 ` Eric Blake
@ 2020-04-29 7:58 ` Max Reitz
0 siblings, 0 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-29 7:58 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 3919 bytes --]
On 28.04.20 16:07, Eric Blake wrote:
> On 4/28/20 8:26 AM, Max Reitz wrote:
>> bdrv_commit() already has a BlockBackend pointing to the BDS that we
>> want to empty, it just has the wrong permissions.
>>
>> qemu-img commit has no BlockBackend pointing to the old backing file
>> yet, but introducing one is simple.
>>
>> After this commit, bdrv_make_empty() is the only remaining caller of
>> BlockDriver.bdrv_make_empty().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/commit.c | 8 +++++++-
>> qemu-img.c | 19 ++++++++++++++-----
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 8e672799af..24720ba67d 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>> }
>> if (drv->bdrv_make_empty) {
>
> This 'if' is still a bit awkward. Do all filter drivers set this
> function, or will bdrv_make_empty() automatically take care of looking
> through filters? Should this be a check of a supported_ variable
> instead (similar to how Kevin just added supported_truncate_flags)?
>
>> - ret = drv->bdrv_make_empty(bs);
>> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>> if (ret < 0) {
>> goto ro_cleanup;
>> }
>> +
>> + ret = blk_make_empty(src, NULL);
>
> So, if the driver lacks the callback, you miss calling blk_make_empty()
> even if it would have done something.
We can’t just call blk_make_empty() and then special case -ENOTSUP here,
though, because the BB doesn’t have a WRITE permission beforehand. So
we have to take that permission before we can call blk_make_empty().
But taking the permission can fail, and then we kind of have to report
the -EPERM, even though the BlockDriver may not support emptying anyway.
I suppose if we just have to take the WRITE_UNCHANGED permission,
though, we can assume that that’s basically always allowed, so it
shouldn’t be that much of a problem there.
Max
>> + if (ret < 0) {
>> + goto ro_cleanup;
>> + }
>> +
>> blk_flush(src);
>> }
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 821cbf610e..a5e8659867 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>> goto unref_backing;
>> }
>> - if (!drop && bs->drv->bdrv_make_empty) {
>> - ret = bs->drv->bdrv_make_empty(bs);
>
> Old code: if the driver had the callback, use it.
>
>> - if (ret) {
>> - error_setg_errno(&local_err, -ret, "Could not empty %s",
>> - filename);
>> + if (!drop) {
>> + BlockBackend *old_backing_blk;
>> +
>> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE,
>> BLK_PERM_ALL,
>> + &local_err);
>> + if (!old_backing_blk) {
>> + goto unref_backing;
>> + }
>> + ret = blk_make_empty(old_backing_blk, &local_err);
>
> New code: Call blk_make_empty() whether or not driver has the callback,
> then deal with the fallout.
>
>> + blk_unref(old_backing_blk);
>> + if (ret == -ENOTSUP) {
>> + error_free(local_err);
>> + local_err = NULL;
>> + } else if (ret < 0) {
>> goto unref_backing;
>> }
>
> But this actually looks smarter than the commit case.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] block: Use blk_make_empty() after commits
2020-04-28 13:26 ` [PATCH 4/4] block: Use blk_make_empty() after commits Max Reitz
2020-04-28 14:07 ` Eric Blake
@ 2020-04-28 15:03 ` Kevin Wolf
2020-04-29 8:01 ` Max Reitz
1 sibling, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2020-04-28 15:03 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> bdrv_commit() already has a BlockBackend pointing to the BDS that we
> want to empty, it just has the wrong permissions.
>
> qemu-img commit has no BlockBackend pointing to the old backing file
> yet, but introducing one is simple.
>
> After this commit, bdrv_make_empty() is the only remaining caller of
> BlockDriver.bdrv_make_empty().
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/commit.c | 8 +++++++-
> qemu-img.c | 19 ++++++++++++++-----
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 8e672799af..24720ba67d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
> }
>
> if (drv->bdrv_make_empty) {
> - ret = drv->bdrv_make_empty(bs);
> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
This is very likely to fail because the common case is that the source
node is attached to a guest device that doesn't share writes.
(qemu-iotests 131 and 274 catch this.)
So I think after my theoretical comment in patch 1, this is the
practical reason why we need WRITE_UNCHANGED rather than WRITE.
Also, why don't you take this permission from the start so that we would
error out right away rather than failing after waiting for the all the
data to be copied?
> if (ret < 0) {
> goto ro_cleanup;
> }
> +
> + ret = blk_make_empty(src, NULL);
> + if (ret < 0) {
> + goto ro_cleanup;
> + }
> +
> blk_flush(src);
> }
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e..a5e8659867 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
> goto unref_backing;
> }
>
> - if (!drop && bs->drv->bdrv_make_empty) {
> - ret = bs->drv->bdrv_make_empty(bs);
> - if (ret) {
> - error_setg_errno(&local_err, -ret, "Could not empty %s",
> - filename);
> + if (!drop) {
> + BlockBackend *old_backing_blk;
> +
> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
> + &local_err);
Oh, you actually depend on another series that you didn't mention in
the cover letter.
> + if (!old_backing_blk) {
> + goto unref_backing;
> + }
> + ret = blk_make_empty(old_backing_blk, &local_err);
> + blk_unref(old_backing_blk);
> + if (ret == -ENOTSUP) {
> + error_free(local_err);
> + local_err = NULL;
> + } else if (ret < 0) {
> goto unref_backing;
> }
> }
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/4] block: Use blk_make_empty() after commits
2020-04-28 15:03 ` Kevin Wolf
@ 2020-04-29 8:01 ` Max Reitz
0 siblings, 0 replies; 31+ messages in thread
From: Max Reitz @ 2020-04-29 8:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 3048 bytes --]
On 28.04.20 17:03, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> bdrv_commit() already has a BlockBackend pointing to the BDS that we
>> want to empty, it just has the wrong permissions.
>>
>> qemu-img commit has no BlockBackend pointing to the old backing file
>> yet, but introducing one is simple.
>>
>> After this commit, bdrv_make_empty() is the only remaining caller of
>> BlockDriver.bdrv_make_empty().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/commit.c | 8 +++++++-
>> qemu-img.c | 19 ++++++++++++++-----
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 8e672799af..24720ba67d 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>> }
>>
>> if (drv->bdrv_make_empty) {
>> - ret = drv->bdrv_make_empty(bs);
>> + ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>
> This is very likely to fail because the common case is that the source
> node is attached to a guest device that doesn't share writes.
> (qemu-iotests 131 and 274 catch this.)
>
> So I think after my theoretical comment in patch 1, this is the
> practical reason why we need WRITE_UNCHANGED rather than WRITE.
>
> Also, why don't you take this permission from the start so that we would
> error out right away rather than failing after waiting for the all the
> data to be copied?
Because we only need to take it when the BlockDriver actually supports
bdrv_make_empty(), so I thought I’d put it here where we have the check
anyway.
However, yes, when we take WRITE_UNCHANGED, we might as well take it
unconditionally from the start. (And then call blk_make_empty()
unconditionally here, too, and let it figure out -ENOTSUP, like Eric noted.)
>> if (ret < 0) {
>> goto ro_cleanup;
>> }
>> +
>> + ret = blk_make_empty(src, NULL);
>> + if (ret < 0) {
>> + goto ro_cleanup;
>> + }
>> +
>> blk_flush(src);
>> }
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 821cbf610e..a5e8659867 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>> goto unref_backing;
>> }
>>
>> - if (!drop && bs->drv->bdrv_make_empty) {
>> - ret = bs->drv->bdrv_make_empty(bs);
>> - if (ret) {
>> - error_setg_errno(&local_err, -ret, "Could not empty %s",
>> - filename);
>> + if (!drop) {
>> + BlockBackend *old_backing_blk;
>> +
>> + old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
>> + &local_err);
>
> Oh, you actually depend on another series that you didn't mention in
> the cover letter.
Well, yes. I didn’t really realize because I just based it on my
block-next...
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (3 preceding siblings ...)
2020-04-28 13:26 ` [PATCH 4/4] block: Use blk_make_empty() after commits Max Reitz
@ 2020-04-28 13:38 ` no-reply
2020-04-28 13:43 ` no-reply
` (5 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-04-28 13:38 UTC (permalink / raw)
To: mreitz; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
SIGN pc-bios/optionrom/kvmvapic.bin
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
/tmp/qemu-test/src/qemu-img.c:1071:27: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: incompatible integer to pointer conversion assigning to 'BlockBackend *' (aka 'struct BlockBackend *') from 'int' [-Werror,-Wint-conversion]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=1896d2b1f1044091b832be313d66ac6e', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-fql9ti5h/src/docker-src.2020-04-28-09.34.00.5332:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=1896d2b1f1044091b832be313d66ac6e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-fql9ti5h/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 4m44.194s
user 0m8.084s
The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (4 preceding siblings ...)
2020-04-28 13:38 ` [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly no-reply
@ 2020-04-28 13:43 ` no-reply
2020-04-28 13:57 ` Eric Blake
2020-04-28 13:48 ` no-reply
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: no-reply @ 2020-04-28 13:43 UTC (permalink / raw)
To: mreitz; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:9: error: implicit declaration of function 'blk_new_with_bs' [-Werror=implicit-function-declaration]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
/tmp/qemu-test/src/qemu-img.c:1071:9: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment makes pointer from integer without a cast [-Werror]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
cc1: all warnings being treated as errors
make: *** [qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2d32da85331c4d51b4632262369586d1', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-mtvq5xk5/src/docker-src.2020-04-28-09.40.27.12753:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=2d32da85331c4d51b4632262369586d1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-mtvq5xk5/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 2m58.028s
user 0m8.393s
The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:43 ` no-reply
@ 2020-04-28 13:57 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2020-04-28 13:57 UTC (permalink / raw)
To: qemu-devel, no-reply, mreitz; +Cc: kwolf, qemu-block
On 4/28/20 8:43 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
>
>
> SIGN pc-bios/optionrom/pvh.bin
> /tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
> /tmp/qemu-test/src/qemu-img.c:1071:9: error: implicit declaration of function 'blk_new_with_bs' [-Werror=implicit-function-declaration]
> old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
Ah, you forgot to tell patchew:
Based-on: <20200424190903.522087-1-eblake@redhat.com>
[PATCH v3 0/3] qcow2: Allow resize of images with internal snapshots
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (5 preceding siblings ...)
2020-04-28 13:43 ` no-reply
@ 2020-04-28 13:48 ` no-reply
2020-04-28 13:49 ` Eric Blake
` (3 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-04-28 13:48 UTC (permalink / raw)
To: mreitz; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
CC aarch64-softmmu/exec-vary.o
CC aarch64-softmmu/tcg/tcg.o
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs'; did you mean 'blk_get_stats'? [-Werror=implicit-function-declaration]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^~~~~~~~~~~~~~~
blk_get_stats
/tmp/qemu-test/src/qemu-img.c:1071:27: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment to 'BlockBackend *' {aka 'struct BlockBackend *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
CC aarch64-softmmu/tcg/tcg-op.o
CC aarch64-softmmu/tcg/tcg-op-vec.o
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7e33797e28514c48a1cce7021d8b04fd', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-dh8ik377/src/docker-src.2020-04-28-09.44.17.22581:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=7e33797e28514c48a1cce7021d8b04fd
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dh8ik377/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 4m4.967s
user 0m8.349s
The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (6 preceding siblings ...)
2020-04-28 13:48 ` no-reply
@ 2020-04-28 13:49 ` Eric Blake
2020-04-28 14:05 ` Eric Blake
2020-04-28 14:53 ` no-reply
` (2 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Eric Blake @ 2020-04-28 13:49 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:26 AM, Max Reitz wrote:
> Branch: https://github.com/XanClic/qemu.git fix-bdrv_make_empty-v1
> Branch: https://git.xanclic.moe/XanClic/qemu.git fix-bdrv_make_empty-v1
>
> Hi,
>
> Right now, there is no centralized bdrv_make_empty() function. Not only
> is it bad style to call BlockDriver methods directly, it is also wrong,
> unless the caller has a BdrvChild with BLK_PERM_WRITE taken.
I'm also in the middle of writing a patch series that adds a
corresponding .bdrv_make_empty driver callback. I'll rebase that work
on top of this, as part of my efforts at fixing more code to rely on
bdrv_make_empty rather than directly querying bdrv_has_zero_init[_truncate].
>
> This series fixes that.
>
> Note that as far as I’m aware this series shouldn’t visibly fix anything
> at this point; but “block: Introduce real BdrvChildRole”
> (https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00737.html)
> makes the iotest break when run with -o data_file=$SOMETHING, without
> this series applied beforehand. (That is because without that series,
> external data files are treated much like metadata children, so the
> format driver always takes the WRITE permission if the file is writable;
> but after that series, it only does so when it itself has a parent
> requestion the WRITE permission.)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:49 ` Eric Blake
@ 2020-04-28 14:05 ` Eric Blake
0 siblings, 0 replies; 31+ messages in thread
From: Eric Blake @ 2020-04-28 14:05 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 4/28/20 8:49 AM, Eric Blake wrote:
> On 4/28/20 8:26 AM, Max Reitz wrote:
>> Branch: https://github.com/XanClic/qemu.git fix-bdrv_make_empty-v1
>> Branch: https://git.xanclic.moe/XanClic/qemu.git fix-bdrv_make_empty-v1
>>
>> Hi,
>>
>> Right now, there is no centralized bdrv_make_empty() function. Not only
>> is it bad style to call BlockDriver methods directly, it is also wrong,
>> unless the caller has a BdrvChild with BLK_PERM_WRITE taken.
>
> I'm also in the middle of writing a patch series that adds a
> corresponding .bdrv_make_empty driver callback. I'll rebase that work
> on top of this, as part of my efforts at fixing more code to rely on
> bdrv_make_empty rather than directly querying
> bdrv_has_zero_init[_truncate].
Correction - I'm working on adding .bdrv_make_zero, not .bdrv_make empty
(which already exists), although maybe it really only needs to be one
callback instead of two if we have decent flags.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (7 preceding siblings ...)
2020-04-28 13:49 ` Eric Blake
@ 2020-04-28 14:53 ` no-reply
2020-04-28 14:57 ` no-reply
2020-04-28 15:02 ` no-reply
10 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-04-28 14:53 UTC (permalink / raw)
To: mreitz; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
BUILD pc-bios/optionrom/pvh.img
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
/tmp/qemu-test/src/qemu-img.c:1071:27: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: incompatible integer to pointer conversion assigning to 'BlockBackend *' (aka 'struct BlockBackend *') from 'int' [-Werror,-Wint-conversion]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=c01c8a7b80794cb2a4458fd17b18ff83', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-7kkbz7sd/src/docker-src.2020-04-28-10.48.39.6384:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=c01c8a7b80794cb2a4458fd17b18ff83
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-7kkbz7sd/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 4m22.457s
user 0m8.829s
The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (8 preceding siblings ...)
2020-04-28 14:53 ` no-reply
@ 2020-04-28 14:57 ` no-reply
2020-04-28 15:02 ` no-reply
10 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-04-28 14:57 UTC (permalink / raw)
To: mreitz; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
SIGN pc-bios/optionrom/linuxboot.bin
SIGN pc-bios/optionrom/kvmvapic.bin
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:9: error: implicit declaration of function 'blk_new_with_bs' [-Werror=implicit-function-declaration]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
/tmp/qemu-test/src/qemu-img.c:1071:9: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment makes pointer from integer without a cast [-Werror]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
cc1: all warnings being treated as errors
make: *** [qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=46a96d39761c47298a2d1eaffd870dde', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-5y1a3roy/src/docker-src.2020-04-28-10.55.01.17538:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=46a96d39761c47298a2d1eaffd870dde
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-5y1a3roy/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 2m49.941s
user 0m8.502s
The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly
2020-04-28 13:26 [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly Max Reitz
` (9 preceding siblings ...)
2020-04-28 14:57 ` no-reply
@ 2020-04-28 15:02 ` no-reply
10 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-04-28 15:02 UTC (permalink / raw)
To: mreitz; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Patchew URL: https://patchew.org/QEMU/20200428132629.796753-1-mreitz@redhat.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
SIGN pc-bios/optionrom/linuxboot.bin
SIGN pc-bios/optionrom/kvmvapic.bin
/tmp/qemu-test/src/qemu-img.c: In function 'img_commit':
/tmp/qemu-test/src/qemu-img.c:1071:27: error: implicit declaration of function 'blk_new_with_bs'; did you mean 'blk_get_stats'? [-Werror=implicit-function-declaration]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^~~~~~~~~~~~~~~
blk_get_stats
/tmp/qemu-test/src/qemu-img.c:1071:27: error: nested extern declaration of 'blk_new_with_bs' [-Werror=nested-externs]
/tmp/qemu-test/src/qemu-img.c:1071:25: error: assignment to 'BlockBackend *' {aka 'struct BlockBackend *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
^
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=ce8305e22d9c45ed90ec2dc43d7f1cc3', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-kzo016wj/src/docker-src.2020-04-28-10.58.45.27385:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=ce8305e22d9c45ed90ec2dc43d7f1cc3
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-kzo016wj/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 3m31.450s
user 0m8.978s
The full log is available at
http://patchew.org/logs/20200428132629.796753-1-mreitz@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread