[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] fix kvm_pv_eoi with kvmclock



On 10/08/2012 07:52 PM, Peter Krempa wrote:
> On 10/08/12 19:32, Martin Kletzander wrote:
>> When both kvmclock and kvm_pv_eoi are configured (either disabled or
>> enabled) libvirt will generate invalid CPU specification due to the
>> fact that even though kvmclock causes the CPU to be specified, it
>> doesn't set have_cpu flag to true (and the new kvm_pv_eoi as well).
>> This patch fixes the issue and adds a test exactly for that to show
>> that it is fixed correctly (and also to keep it that way in the future
>> of course).
>> ---
>>   src/qemu/qemu_command.c                            |  2 ++
>>   .../qemuxml2argv-kvmclock+eoi-disabled.args        |  4 ++++
>>   .../qemuxml2argv-kvmclock+eoi-disabled.xml         | 27
>> ++++++++++++++++++++++
>>   tests/qemuxml2argvtest.c                           |  1 +
>>   4 files changed, 34 insertions(+)
>>   create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-kvmclock+eoi-disabled.args
>>   create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-kvmclock+eoi-disabled.xml
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 20730a9..09f412e 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4210,6 +4210,7 @@ qemuBuildCpuArgStr(const struct qemud_driver
>> *driver,
>>               virBufferAsprintf(&buf, "%s,%ckvmclock",
>>                                 have_cpu ? "" : default_model,
>>                                 sign);
>> +            have_cpu = true;
>>               break;
>>           }
>>       }
>> @@ -4224,6 +4225,7 @@ qemuBuildCpuArgStr(const struct qemud_driver
>> *driver,
>>           virBufferAsprintf(&buf, "%s,%ckvm_pv_eoi",
>>                             have_cpu ? "" : default_model,
>>                             sign);
>> +        have_cpu = true;
> 
> This assignment has no effect, the value isn't used elsewhere. Although
> it probably would make sense to keep it if somebody else tries to
> improve this function (probably this was the source of the bug fixed by
> this patch).
> 

That's exactly why I've put it there. The last CPU-adding statement
missed this and I've forgot to put it there.  This way it'll work next
time somebody adds something new :)

>>       }
>>
>>       if (virBufferError(&buf))
> 
> ACK regardless of the fate of the last assignment.
> 
> Peter
> 

Thanks, pushed.

Martin


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]