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@xxxxxxxxxx> >>> --- >>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
