Re: [Qemu-devel] [PATCH 3/9] block jobs: Use BdrvChild callbacks for iostatus operations

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3207 bytes --]

On 22.03.2016 20:36, Kevin Wolf wrote:
> The block jobs currently modify the target BB's error handling options
> and require that the source BB's iostatus is enabled in order to
> implement the per-job error options. It's obvious that this is something
> between ugly, adventurous and plain wrong, so we should ideally fix this
> instead of thinking about how to handle multiple BBs in this case.
> 
> Unfortunately we have a chicken-and-egg problem here: In order to fix
> the problem, the block jobs need their own BBs. This requires multiple
> BBs per BDS, which in turn means that BDS.blk must be removed. Removing
> BDS.blk means that the jobs already need their own BB. The only other
> options is that we lift the iostatus operations to the BdrvChild level
> and deal with multiple BBs now.
> 
> So even though we don't want to have these callbacks in the end (because
> they indicate broken logic), this patch converts the iostatus operations
> for block jobs targets to BdrvChild callbacks; if more than one parent
> implements iostatus operations, they are called for each of them.
> 
> Once the conversion is completed, block jobs will get their own internal
> BB and the callbacks can be removed again.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c            | 20 +++++------
>  block/block-backend.c     | 42 +++++++++++++++++++++++
>  block/commit.c            |  2 +-
>  block/mirror.c            | 21 ++++++------
>  block/stream.c            |  2 +-
>  blockjob.c                | 85 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/block/block_int.h | 10 ++++++
>  include/block/blockjob.h  |  9 +++++
>  8 files changed, 165 insertions(+), 26 deletions(-)

[...]

> diff --git a/block/block-backend.c b/block/block-backend.c
> index c4c0dc0..03e68a7 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -100,6 +100,39 @@ static const char *blk_root_get_name(BdrvChild *child)
>      return blk_name(child->opaque);
>  }
>  
> +static void blk_root_iostatus_reset(BdrvChild *child)
> +{
> +    return blk_iostatus_reset(child->opaque);

The C11 standard (draft) says in 6.8.6.4 paragraph 1 that:
> A return statement with an expression shall not appear in a function
> whose return type is void.

Even if it is a void expression, it still is an expression. Also, the
semantics of a return statement with an expression is as follows
(paragraph 3):
> If a return statement with an expression is executed, the value of
> the expression is returned to the caller as the value of the function
> call expression.

However, 6.3.2.2 ("void") says:
> The (nonexistent) value of a void expression (an expression that has
> type void) shall not be used in any way

Therefore, the value of a void expression cannot be returned through a
return instruction because such a value does not exist.

I looked it up because I remember you had quite a bit of knowledge
regarding corner cases of the void types. The patch looks good overall,
so if you can point me to why this is allowed or that it's something we
already do somewhere else in qemu, then I'll give my R-b.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]