Re: [Qemu-block] [PATCH v2 1/6] block: skip implicit nodes in snapshots, blockjobs

[Manos Pitsidianakis]

On Tue, Aug 15, 2017 at 03:24:38PM +0200, Alberto Garcia wrote:
> On Wed 09 Aug 2017 04:02:51 PM CEST, Manos Pitsidianakis wrote:
> > @@ -2988,6 +3004,9 @@ void qmp_block_stream(bool has_job_id, const char 
> > *job_id, const char *device,
> >          return;
> >      }
> >
> > +    /* Skip implicit filter nodes */
> > +    bs = bdrv_get_first_explicit(bs);
> > +
> >      aio_context = bdrv_get_aio_context(bs);
> >      aio_context_acquire(aio_context);
>
> This change works here because right now the only implicit node that we
> could have in practice is the throttle node, but I wonder if this is
> good enough for any kind of implicit node in general.

It does feel a bit sloppy but block jobs should work the same with
implicit nodes and without, so all we can do is ignore them.

> > +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> > +{
> > +    BdrvChild *child = QLIST_FIRST(&bs->children);
> > +    assert(child && !QLIST_NEXT(child, next));
> > +    return child->bs;
> > +}
>
> This aborts if the bs has a number of children != 1. That's not
> something that I would expect from a function named like that.
>
> Considering that you're only using it in bdrv_get_first_explicit(), why
> don't you simply move the code there?

It felt useful to have a function that also returns file->bs (we have 
backing_bs() already) instead of doing backing_bs(bs) ? : file_bs(bs)
> The other question is of course whether we can rely for the future on
> the assumption that implicit nodes only have one children.

This is only to get either bs->backing or bs->file (we can't have both
anyway). I wanted to document it with something like "Used for filter 
drivers with only one child" which fits with what implicit nodes we have 
so far (mirror, commit, throttle and in the future backup)

signature.asc
Description: PGP signature