Re: [Qemu-devel] [PATCH for-2.10 v3] block: Skip implicit nodes in query-block/blockstats

From: Kevin Wolf <kwolf@redhat.com>
To: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org, eblake@redhat.com, armbru@redhat.com,
	pkrempa@redhat.com, nsoffer@redhat.com, el13635@mail.ntua.gr,
	qemu-devel@nongnu.org, qemu-stable@nongnu.org
Subject: Re: [Qemu-devel] [PATCH for-2.10 v3] block: Skip implicit nodes in query-block/blockstats
Date: Thu, 20 Jul 2017 16:17:13 +0200	[thread overview]
Message-ID: <20170720141713.GB4963@noname.redhat.com> (raw)
In-Reply-To: <b94821ce-476f-42b7-7fc5-fcff1b83fe7d@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

Am 20.07.2017 um 16:06 hat Max Reitz geschrieben:
> On 2017-07-20 15:11, Kevin Wolf wrote:
> > @@ -133,13 +133,21 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> >              qapi_free_BlockDeviceInfo(info);
> >              return NULL;
> >          }
> > +
> >          if (bs0->drv && bs0->backing) {
> > +            info->backing_file_depth++;
> >              bs0 = bs0->backing->bs;
> >              (*p_image_info)->has_backing_image = true;
> >              p_image_info = &((*p_image_info)->backing_image);
> >          } else {
> >              break;
> >          }
> > +
> > +        /* Skip automatically inserted nodes that the user isn't aware of for
> > +         * query-block (blk != NULL), but not for query-named-block-nodes */
> > +        while (blk && bs0 && bs0->drv && bs0->implicit) {
> > +            bs0 = backing_bs(bs0);
> > +        }
> 
> If the bottom-most backing file is implicit, this will leave bs0 to be
> NULL. The surrounding loop does not look like it could cope well with
> that. (And even if it could, we would have to reset has_backing_image to
> false, wouldn't we?)
> 
> Not necessarily an issue here because all of our implicit nodes are
> somewhere in between, but still...
> 
> (And I just saw you're even asserting this in another place...)
> 
> >      }
> >  
> >      return info;
> 
> [...]
> 
> > @@ -446,6 +459,14 @@ static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
> >          return s;
> >      }
> >  
> > +    /* Skip automatically inserted nodes that the user isn't aware of in
> > +     * a BlockBackend-level command. Stay at the exact node for a node-level
> > +     * command. */
> > +    while (blk_level && bs->drv && bs->implicit) {
> > +        bs = backing_bs(bs);
> > +        assert(bs);
> 
> (...namely here.)
> 
> I extremely doubt this assertion will hold in the future, but well, it's
> good enough for now.
> 
> So, I've always wanted "file" to be the default child for block filters.
> This patch very much establishes that "backing" is the default child, right?

Manos is going to address this when he uses an I/O throttling filter
node to implement the legacy options. The plan there is to replace it
with some function that asserts that there is only one child (because
only filter nodes are supposed to be implicit) and then uses that child,
no matter whether it's bs->file, bs->backing or something else.

I just tried to keep things simple for 2.10.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]