From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MTgMD-0002Xn-4K
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 14:16:01 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MTgM8-0002VX-6e
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 14:16:00 -0400
Received: from [199.232.76.173] (port=54220 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MTgM8-0002VM-1m
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 14:15:56 -0400
Received: from qw-out-1920.google.com ([74.125.92.149]:38137)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <anthony@codemonkey.ws>) id 1MTgM7-0004cV-J1
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 14:15:55 -0400
Received: by qw-out-1920.google.com with SMTP id 5so186387qwc.4
	for <qemu-devel@nongnu.org>; Wed, 22 Jul 2009 11:15:55 -0700 (PDT)
Message-ID: <4A675758.2080302@codemonkey.ws>
Date: Wed, 22 Jul 2009 13:15:52 -0500
From: Anthony Liguori <anthony@codemonkey.ws>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] Re: [COMMIT bd83677] Fake dirty loggin when it's
	not there
References: <200907221626.n6MGQGH0022104@d03av02.boulder.ibm.com>
	<4A6745C0.5060201@siemens.com>
In-Reply-To: <4A6745C0.5060201@siemens.com>
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>, qemu-devel <qemu-devel@nongnu.org>, Alexander Graf <agraf@suse.de>

Jan Kiszka wrote:
>> Signed-off-by: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>
>> Signed-off-by: Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 961fa32..2032949 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -300,6 +300,7 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>      KVMDirtyLog d;
>>      KVMSlot *mem;
>>      int ret = 0;
>> +    int r;
>>  
>>      d.dirty_bitmap = NULL;
>>      while (start_addr < end_addr) {
>> @@ -308,6 +309,11 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>              break;
>>          }
>>  
>> +        /* We didn't activate dirty logging? Don't care then. */
>> +        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>> +            continue;
>> +        }
>> +
>>     
>
> According to Alex' reply [1], I think this is probably better expressed
> as an assert() than a silent skip. It indicates an error at higher
> level, no?
>   

I don't have a strong opinion either way.

>>          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>>          if (!d.dirty_bitmap) {
>>              d.dirty_bitmap = qemu_malloc(size);
>> @@ -319,7 +325,8 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>  
>>          d.slot = mem->slot;
>>  
>> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
>> +        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
>> +        if (r == -EINVAL) {
>>              dprintf("ioctl failed %d\n", errno);
>>              ret = -1;
>>              break;
>>     
>
> My remark still stands: This is an "optimistic" assumption that only
> EINVAL is a "real" error. If KVM is buggy /wrt a specific invalid return
> value, work around that particular bug (and we should also fix the
> kernel, but that's a different topic).
>   

The behavior is identical to what it was before.  It's just a stylistic 
change.

I agree that this needs to be properly fixed in KVM and then in QEMU but 
I also think that that is best done as a separate set of patches.

Regards,

Anthony Liguori