On Wed, Aug 16, 2017 at 11:11:44PM +0300, Manos Pitsidianakis wrote:
> On Wed, Aug 16, 2017 at 02:25:44PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Aug 15, 2017 at 11:18:53AM +0300, Manos Pitsidianakis wrote:
> > > @@ -3142,7 +3174,7 @@ static bool should_update_child(BdrvChild *c,
> > > BlockDriverState *to)
> > > return false;
> > > }
> > >
> > > - if (c->role == &child_backing) {
> > > + if (c->role == &child_backing || c->role == &child_file) {
> > > /* If @from is a backing file of @to, ignore the child to avoid
> > > * creating a loop. We only want to change the pointer of other
> > > * parents. */
> >
> > This may have unwanted side-effects. I think you're using is so that
> > bdrv_set_file() + bdrv_replace_node() does not create a loop in the
> > graph. That is okay if there is only one parent with child_file. If
> > there are multiple parents with that role then it's not clear to me that
> > they should all be skipped.
>
> I am afraid I don't understand what you're saying. What is the difference
> with the child_backing scenario here? In both cases we should update all
> from->parents children unless they also happen to be a child of `to`. If
> there are multiple parents with child_file, they are not skipped except for
> the ones where `to` is the parent.
Okay, I'll have to look at this again. Thanks!
> > > +static BlockDriver backup_top = {
> > > + .format_name = "backup-top",
> > > + .instance_size = sizeof(BackupBlockJob *),
> > > +
> > > + .bdrv_open = backup_top_open,
> >
> > .bdrv_open may be NULL. There's no need to define this function.
> >
> > > + .bdrv_close = backup_top_close,
> > > +
> > > + .bdrv_co_flush = backup_top_co_flush,
> > > + .bdrv_co_preadv = backup_top_co_preadv,
> > > + .bdrv_co_pwritev = backup_top_co_pwritev,
> > > + .bdrv_co_pwrite_zeroes = backup_top_co_pwrite_zeroes,
> > > + .bdrv_co_pdiscard = backup_top_co_pdiscard,
> > > +
> > > + .bdrv_getlength = backup_top_getlength,
> > > + .bdrv_child_perm = bdrv_filter_default_perms,
> > > + .bdrv_recurse_is_first_non_filter =
> > > backup_recurse_is_first_non_filter,
> >
> > I think this is dead code since .is_filter = true. It will not be
> > called.
>
> bdrv_recurse_is_first_non_filter() is only called if is_filter is true and
> the driver implements it to allow recursion to its children.
You are right, I misread the code.
Stefan
signature.asc
Description: PGP signature
