[libvirt] [PATCH] Block info query: Add flag to allow failure if not active

[Daniel P. Berrange]

On Wed, Dec 18, 2013 at 09:49:46AM -0500, John Ferlan wrote:
> 
> 
> On 12/18/2013 09:35 AM, Daniel P. Berrange wrote:
> > On Wed, Dec 18, 2013 at 06:58:10AM -0500, John Ferlan wrote:
> 
> > 
> > I'm not convinced this flag is desirable. Can't apps just
> > check whether the guest is running themselves, or indeed
> > something like RHEV surely knows what its own VM is doing
> > already.
> 
> I do agree with you and that's been my argument in the referenced BZ;
> however, as I understand it - they have a thread that continually polls
> for blockInfo information and a separate thread that handles the
> migration. The blockInfo thread doesn't have the state information.

But it must have a virDomainPtr instance, so it can use
virDomainGetState() or virDomainIsActive() if it cares
about this.

> I contend it's just as simple to add a check about the domain state and
> to get/check the reason value as well. That is - I think the blockInfo
> thread should be more aware of state. Of course, the return argument is
> that libvirt shouldn't return different answers on consecutive fetches
> where the first fetch is when the guest is "active" and the second is
> when it's not.

Sure, I agree that libvirt isn't ideal here - our hands are tied by
the fact that QEMU doesn't make this data available to us offline.
If we changed anything on the libvirt side, then I'd want to see us
do a proper fix to get the data but that'd require qemu help;

> 
> To be honest this feels a bit hackish and it requires a deliberate
> change to their code to not only provide the flag, but handle the error.
> Since they'd already be changing their code to handle this, then why not
> change to check state. There is a downside to their checking state
> though and that's a timing condition where their check could indicate
> the guest was active, but the "next clock tick" it becomes inactive.
> Thus, their code would be in the same position of getting what is felt
> is the wrong answer.

If they check the state after querying the data, then yes they could
get bogus data.  If they check the state before querying the info,
then the race condition scenario does not lead to bad behaviour - they
would be simply be discarding the "allocation" value unncessarily
which is exactly the same situation they'd be in if the libvirt API
itself checked this too, and the guest shutoff after libvirt checked
its state. So I don't see a need for this libvirt change here.

> I figure opening it up to community discussion will help "decide" one
> way or another the best approach.
> 
> I'm all for adjusting the documentation and leaving the code the way it
> is now as well.

Updating docs would be my recommendation.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|