Re: Prevent compiler warning on block.c

[Vladimir Sementsov-Ogievskiy]

05.05.2021 13:03, Paolo Bonzini wrote:
> On 05/05/21 10:05, Vladimir Sementsov-Ogievskiy wrote:
> > > diff --git a/block.c b/block.c
> > > index 874c22c43e..3ca27bd2d9 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -4851,7 +4851,7 @@ static int bdrv_replace_node_common(BlockDriverState 
> > > *from,
> > >       Transaction *tran = tran_new();
> > >       g_autoptr(GHashTable) found = NULL;
> > >       g_autoptr(GSList) refresh_list = NULL;
> > > -    BlockDriverState *to_cow_parent;
> > > +    BlockDriverState *to_cow_parent = NULL;
> >
> > May be
> >
> > +    BlockDriverState *to_cow_parent = NULL; /* Silence compiler warning */
>
>
> We can also do something like this where the only caller with
> to_detach==true takes care of passing the right CoW-parent, and the
> for loop goes away completely if I'm not mistaken:
>
> diff --git a/block.c b/block.c
> index ae1a7e25aa..3f6fa8475c 100644
> --- a/block.c
> +++ b/block.c
> @@ -4839,31 +4839,19 @@ static int bdrv_replace_node_noperm(BlockDriverState 
> *from,
>   * With auto_skip=false the error is returned if from has a parent which should
>   * not be updated.
>   *
> - * With @detach_subchain=true @to must be in a backing chain of @from. In this
> - * case backing link of the cow-parent of @to is removed.
> + * With @to_detach is not #NULL its link to @to is removed.
>   */
> static int bdrv_replace_node_common(BlockDriverState *from,
>                                      BlockDriverState *to,
> -                                    bool auto_skip, bool detach_subchain,
> +                                    bool auto_skip, BlockDriverState 
> *to_detach,
>                                      Error **errp)
> {
>      Transaction *tran = tran_new();
>      g_autoptr(GHashTable) found = NULL;
>      g_autoptr(GSList) refresh_list = NULL;
> -    BlockDriverState *to_cow_parent;
> +    BlockDriverState *to_detach;
>      int ret;
>
> -    if (detach_subchain) {
> -        assert(bdrv_chain_contains(from, to));
> -        assert(from != to);
> -        for (to_cow_parent = from;
> -             bdrv_filter_or_cow_bs(to_cow_parent) != to;
> -             to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent))
> -        {
> -            ;
> -        }
> -    }
> -
>      /* Make sure that @from doesn't go away until we have successfully attached
>       * all of its parents to @to. */
>      bdrv_ref(from);
> @@ -4883,8 +4871,8 @@ static int bdrv_replace_node_common(BlockDriverState 
> *from,
>          goto out;
>      }
>
> -    if (detach_subchain) {
> -        bdrv_remove_filter_or_cow_child(to_cow_parent, tran);
> +    if (to_detach) {
> +        bdrv_remove_filter_or_cow_child(to_detach, tran);
>      }
>
>      found = g_hash_table_new(NULL, NULL);
> @@ -4911,13 +4899,21 @@ out:
> int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>                        Error **errp)
> {
> -    return bdrv_replace_node_common(from, to, true, false, errp);
> +    return bdrv_replace_node_common(from, to, true, NULL, errp);
> }
>
> int bdrv_drop_filter(BlockDriverState *bs, Error **errp)
> {
> -    return bdrv_replace_node_common(bs, bdrv_filter_or_cow_bs(bs), true, true,
> -                                    errp);
> +    BlockDriverState *to = bdrv_filter_or_cow_bs(bs);
> +
> +    assert(bdrv_chain_contains(bs, to));
> +    assert(bs != to);
> +    return bdrv_replace_node_common(bs, to, true, bs, errp);
> }
>
> /*
> @@ -5262,7 +5262,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
> BlockDriverState *base,
>       * test-bdrv-drain. test_drop_intermediate_poll() test-case will crash.
>       * That's a FIXME.
>       */

I'd prefer first fix this FIXME. Then, one more caller with 
detach_subchain=true will appear, and we'll see, how to refactor the interface 
in the best way. Otherwise we'll just have to refactor it twice.


> -    bdrv_replace_node_common(top, base, false, false, &local_err);
> +    bdrv_replace_node_common(top, base, false, NULL, &local_err);
>      if (local_err) {
>          error_report_err(local_err);
>          goto exit;
>
> Even nicer would be to move the bdrv_remove_filter_or_cow_child() call to
> bdrv_drop_filter, and pass in a Transaction to bdrv_replace_node_common, but
> I'm not sure if bdrv_replace_node_noperm and bdrv_remove_filter_or_cow_child
> can commute.


--
Best regards,
Vladimir