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

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(a)redhat.com&gt; >> --- >>   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