Hi Peter, On Wed, Feb 12, 2025 at 10:05:08AM +0100, Peter Krempa wrote: > On Wed, Feb 12, 2025 at 07:26:09 +0100, Andrea Righi via Devel wrote: > > Allow to define new acpi-generic-initiator objects to link a PCI device > > with multiple NUMA nodes. > > > > Link: https://mail.gnu.org/archive/html/qemu-arm/2024-03/msg00358.html > > Signed-off-by: Andrea Righi <arighi@xxxxxxxxxx> > > --- > > Note this is not a full review and I don't plan to review any further > aspects of this series. > > > src/ch/ch_domain.c | 1 + > > src/conf/domain_conf.c | 153 ++++++++++++++++++++++++++++++ > > src/conf/domain_conf.h | 14 +++ > > src/conf/domain_postparse.c | 1 + > > src/conf/domain_validate.c | 40 ++++++++ > > src/conf/schemas/domaincommon.rng | 19 ++++ > > We usually tend to separate generic parser infrastructure ... > > > src/conf/virconftypes.h | 2 + > > src/libxl/libxl_driver.c | 6 ++ > > src/lxc/lxc_driver.c | 6 ++ > > src/qemu/qemu_command.c | 18 ++++ > > src/qemu/qemu_domain.c | 2 + > > src/qemu/qemu_domain_address.c | 4 + > > src/qemu/qemu_driver.c | 3 + > > src/qemu/qemu_hotplug.c | 5 + > > src/qemu/qemu_postparse.c | 1 + > > src/qemu/qemu_validate.c | 1 + > > ... from any driver implementation into a separate patch. Ok, I'll split the patch. > > > src/test/test_driver.c | 4 + > > 17 files changed, 280 insertions(+) > > So that's easier to review. > > You are also adding new XML but are completely missing test files which > show how it's used and excercise the parser and formatter as well as the > qemu driver implementation. > > The simplest way is to use qemuxmlconftest to add these. Ack, I'll add a test in qemuxmlconftest. > > You're also referencing 'acpi-generic-initiator' object which is not > supported by all versions so you'll need to add a capability flag for > it. See qemu_capabilities.c/h Ok. > > > > > +static int > > +qemuBuildAcpiInitiatorCommandLine(virCommand *cmd, > > + const virDomainAcpiInitiatorDef *acpiinitiator) > > +{ > > + g_autofree char *obj = NULL; > > + > > + obj = g_strdup_printf("acpi-generic-initiator,id=%s,pci-dev=%s,node=%d", > > + acpiinitiator->name, acpiinitiator->pciDev, acpiinitiator->numaNode); > > + virCommandAddArgList(cmd, "-object", obj, NULL); > > All other code which uses defines -object uses JSON props. You'll have > to convert this to use the same approach and use > > qemuBuildObjectCommandlineFromJSON() > > to convert the JSON definition to the commandline. > > Apart from symmetry, this'll provide validation of the generated > properties against qemu's internal schemas. > Ok, makes sense. Thanks for the hints! -Andrea
