Re: [PATCH RFC 5/9] KVM: Provide helper to get kvm dirty log

[Peter Xu <peterx@redhat.com>]

On Wed, Mar 25, 2020 at 05:52:20PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Provide a helper kvm_slot_get_dirty_log() to make the function
> > kvm_physical_sync_dirty_bitmap() clearer.  We can even cache the as_id
> > into KVMSlot when it is created, so that we don't even need to pass it
> > down every time.
> > 
> > Since at it, remove return value of kvm_physical_sync_dirty_bitmap()
> > because it should never fail.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  accel/kvm/kvm-all.c      | 39 +++++++++++++++++++--------------------
> >  include/sysemu/kvm_int.h |  2 ++
> >  2 files changed, 21 insertions(+), 20 deletions(-)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index bb635c775f..608216fd53 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -553,6 +553,18 @@ static void kvm_slot_init_dirty_bitmap(KVMSlot *mem)
> >      mem->dirty_bmap = g_malloc0(bitmap_size);
> >  }
> >  
> > +/* Sync dirty bitmap from kernel to KVMSlot.dirty_bmap */
> > +static void kvm_slot_get_dirty_log(KVMState *s, KVMSlot *slot)
> > +{
> > +    struct kvm_dirty_log d = {};
> > +    int ret;
> > +
> > +    d.dirty_bitmap = slot->dirty_bmap;
> > +    d.slot = slot->slot | (slot->as_id << 16);
> > +    ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> > +    assert(ret != -1);
> 
> As discussed on irc, that -1 check seems odd given your previous check
> but there seems to be some history as to why it was still there.  Hmm.
> 
> It also seems very trusting that it can never possibly fail!

Yeah I agree.  I've asked the question under patch 1.

Before we know the answer, I'll remove the assertion to print an error
if it triggers; check against -1 (-EPERM) explicitly does look odd...

Thanks,

-- 
Peter Xu