The commit and mirror block nodes are filters, so they should be marked
as such.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c | 2 ++
 block/mirror.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 2b876bf6e9..87d0b208ea 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -254,6 +254,8 @@ static BlockDriver bdrv_commit_top = {
     .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
     .bdrv_child_perm            = bdrv_commit_top_child_perm,
+
+    .is_filter                  = true,
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
diff --git a/block/mirror.c b/block/mirror.c
index 726d3c27fb..c7ca8078d1 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1469,6 +1469,8 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
+
+    .is_filter                  = true,
 };
 
 static void mirror_start_job(const char *job_id, BlockDriverState *bs,
-- 
2.20.1

On 2/13/19 4:53 PM, Max Reitz wrote:
> The commit and mirror block nodes are filters, so they should be marked
> as such.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/commit.c | 2 ++
>  block/mirror.c | 2 ++
>  2 files changed, 4 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

I know there are patches pending review for a backup driver (for image
fleecing and otherwise), have you checked if that series would need
similar treatment?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 14.02.19 00:24, Eric Blake wrote:
> On 2/13/19 4:53 PM, Max Reitz wrote:
>> The commit and mirror block nodes are filters, so they should be marked
>> as such.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/commit.c | 2 ++
>>  block/mirror.c | 2 ++
>>  2 files changed, 4 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I know there are patches pending review for a backup driver (for image
> fleecing and otherwise), have you checked if that series would need
> similar treatment?

Good point; it is indeed marked as a filter driver from the start, so
we're good there.

Max

Am 13.02.2019 um 23:53 hat Max Reitz geschrieben:
> The commit and mirror block nodes are filters, so they should be marked
> as such.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>

Then we need to update the definition of a filter:

    /* set to true if the BlockDriver is a block filter. Block filters pass
     * certain callbacks that refer to data (see block.c) to their bs->file if
     * the driver doesn't implement them. Drivers that do not wish to forward
     * must implement them and return -ENOTSUP.
     */
    bool is_filter;

This talks about bs->file specifically, and commit and mirror nodes use
bs->backing instead. Most places that check bs->in_filter do so to check
whether they can forward stuff to bs->file, and they don't consider
bs->backing.

I think the latter will be fixed after patch 3, but the definition in
the comment is still wrong at the end of the series.

Kevin

On 19.03.19 14:16, Kevin Wolf wrote:
> Am 13.02.2019 um 23:53 hat Max Reitz geschrieben:
>> The commit and mirror block nodes are filters, so they should be marked
>> as such.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Then we need to update the definition of a filter:
> 
>     /* set to true if the BlockDriver is a block filter. Block filters pass
>      * certain callbacks that refer to data (see block.c) to their bs->file if
>      * the driver doesn't implement them. Drivers that do not wish to forward
>      * must implement them and return -ENOTSUP.
>      */
>     bool is_filter;
> 
> This talks about bs->file specifically, and commit and mirror nodes use
> bs->backing instead. Most places that check bs->in_filter do so to check
> whether they can forward stuff to bs->file, and they don't consider
> bs->backing.
> 
> I think the latter will be fixed after patch 3, but the definition in
> the comment is still wrong at the end of the series.

Right, I forgot this comment.

(Interesting how Quorum has already ignored this definition for a long
time.) O:-)


Max