[libvirt] [PATCH] qemu: domain: validate memory access during validate domain config

Wed Aug 29 15:08:44 UTC 2018

On 08/27/2018 11:51 PM, lhuang wrote:
> 
> 
> On 08/28/2018 06:01 AM, John Ferlan wrote:
>>
>> On 08/20/2018 05:48 AM, Luyao Huang wrote:
>>> commit 6534b3c4 try to raise an error when there is no numa nodes but
>>> set access='shared' in domain config. In that commit, we add a memory
>>> access
>>> validate function for memory device, but this check is not related to
>>> memory
>>> device and won't work if there is no memory device in guest xml.
>>>
>>> Move the memory access related check in the domain config validate
>>> function,
>>> and simplify the unit test xml to avoid other error.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1448149#c14
>>>
>>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>>> ---
>>>   src/qemu/qemu_domain.c                          | 55
>>> +++++++++++-----------
>>>   tests/qemuxml2argvdata/hugepages-memaccess3.xml | 62
>>> +------------------------
>>>   tests/qemuxml2argvtest.c                        |  6 +--
>>>   3 files changed, 32 insertions(+), 91 deletions(-)
>>>
>> Hmm... I have a recollection of reviewing this... Let's see, yes:
>>
>> https://www.redhat.com/archives/libvir-list/2017-December/msg00387.html
>>
>> and follow-ups...  I think I agree with you the validation should be
>> done in qemuDomainDefValidate instead of qemuDomainDeviceDefValidate;
>> however, you're making multiple changes at one time - so I went to
>> dissect...  This response got lengthy and I'm kind of at a hmm? wtf
>> moment that hopefully Michal can look at since he was the original
>> author.  Maybe he can recall 8 months ago and whether he got the
>> expected error message or not on output.
>>
>> My concern/point was changing hugepages-memaccess3.xml and
>> qemuxml2argvtest.c to remove some things while also changing from
>> DO_TEST_FAILURE to DO_TEST_PARSE_ERROR is doing two different things in
>> one patch, which we generally try to avoid, but in this case it may just
>> be necessary.
> 
> yes, i was trying to fix the hugepages-memaccess3.xml since my changes
> will break the unit test.
> Actually the code changes for the hugepages-memaccess3.xml and
> qemuxml2argvtest.c included 2 different fix, one for fixing the exist
> unit test and another for fixing the failure introduced in this patch.
> And i merged them in one patch, maybe i should keep them split in next
> time ?
> 
>>
>> When I tried to separate the changes in such a way that the unnecessary
>> lines were removed from hugepages-memaccess3.xml first - the test ended
>> up succeeding! So that was strange, but I guess not totally unexpected
>> since the device mems don't exist and the wrong check was being done.
>>
>> So I went back to whence we started and note that qemuxml2argvtest fails
>> the hugepages-memaccess3 without any of these changes with (as seen with
>> VIR_TEST_DEBUG=1 VIR_TEST_RANGE=169 tests/qemuxml2argvtest):
>>
>> 169) QEMU XML-2-ARGV hugepages-memaccess3
>> ... Got expected error:
>> 2018-08-27 21:24:04.934+0000: 5816: info : libvirt version: 4.7.0
>> 2018-08-27 21:24:04.934+0000: 5816: info : hostname:
>> localhost.localdomain
>> 2018-08-27 21:24:04.934+0000: 5816: error :
>> qemuProcessStartValidateVideo:5033 : unsupported configuration: this
>> QEMU does not support 'virtio' video device
>>
>> So, going a bit slower... and removing things to see if I can get that
>> "memory access mode '%s' not supported..." error message...
>>
>> 1. Remove <video> and <graphics> entries, but get:
>>
>> 169) QEMU XML-2-ARGV hugepages-memaccess3
>> ... Got expected error:
>> 2018-08-27 21:26:28.754+0000: 5907: info : libvirt version: 4.7.0
>> 2018-08-27 21:26:28.754+0000: 5907: info : hostname:
>> localhost.localdomain
>> 2018-08-27 21:26:28.754+0000: 5907: error : qemuBuildPMCommandLine:6523
>> : unsupported configuration: setting ACPI S3 not supported
>>
>> 2. Remove <pm> and get:
>>
>> 169) QEMU XML-2-ARGV hugepages-memaccess3
>> ... Got expected error:
>> 2018-08-27 21:27:48.000+0000: 5972: info : libvirt version: 4.7.0
>> 2018-08-27 21:27:48.000+0000: 5972: info : hostname:
>> localhost.localdomain
>> 2018-08-27 21:27:48.000+0000: 5972: error :
>> qemuBuildUSBControllerDevStr:2681 : unsupported configuration:
>> piix3-usb-uhci not supported in this QEMU binary
>>
>> 3. Remove "model='piix3-uhci'" from the <controller type='usb' and get:
>>
>> 169) QEMU XML-2-ARGV hugepages-memaccess3
>> ... Got expected error:
>> 2018-08-27 21:29:01.554+0000: 6179: info : libvirt version: 4.7.0
>> 2018-08-27 21:29:01.554+0000: 6179: info : hostname:
>> localhost.localdomain
>> 2018-08-27 21:29:01.554+0000: 6179: error : qemuCheckDiskConfig:1297 :
>> unsupported configuration: discard is not supported by this QEMU binary
>>
>> 4. Remove "discard='unmap' from the <disk> and get:
>>
>> 169) QEMU XML-2-ARGV hugepages-memaccess3
>> ... Got expected error:
>> 2018-08-27 21:30:21.989+0000: 6360: info : libvirt version: 4.7.0
>> 2018-08-27 21:30:21.989+0000: 6360: info : hostname:
>> localhost.localdomain
>> 2018-08-27 21:30:21.989+0000: 6360: error :
>> qemuBuildSerialChrDeviceStr:10576 : unsupported configuration:
>> 'isa-serial' is not supported in this QEMU binary
>>
>> 5. Remove <serial>, <console>, and <channel> and get passed instead of
>> failure.
> 
> You really have a good patience ;) i deleted all the unrelated elements
> when i saw the error come from qemuProcessStartValidateVideo() function.
> 
> BTW: I think maybe we can add some function like
> DO_TEST_FAILURE_CHECK_ERROR which use regex to check if the error is
> expected.
> 
>>
>> So at first I was a bit stumped as to what happened... Could there be
>> "conflict" or no error message now due to changes Pavel Hrdina made
>> dealing with hugepages and parsing. So, in any case, I'd like to wait
>> for Michal's input before proceeding further on this one.
> 
> Sure, and i have checked that there is no conflict with the Pavel
> Hrdina's "Fix and improve hugepage code" patch set.
> 
>> BTW: Your changes look correct and fine to me as well as eliciting the
>> proper error message, which is good/proper, but before pushing let's see
>> if there's feedback from Michal.
>>
>> John
>>
> 
> Thanks a lot for your review !
> 
> Have a nice day !
> Luyao
> 

I got word from Michal in private chat that he agrees with the patch...
So, I adjusted the commit message a bit and pushed (using bugfix during
freeze) with my R-By tag,

John