Re: [libvirt] [PATCH v3 0/6] BaselineHypervisorCPU using QEMU QMP exchanges

Quoting Jiri Denemark (2018-10-03 09:36:34) > Hmm, I guess I could have checked the cover letter before looking at the > individual patches. That would safe me some time and thinking :-) > > On Thu, Sep 27, 2018 at 16:26:39 -0500, Chris Venteicher wrote: > > Some architectures (S390) depend on QEMU to compute baseline CPU model and > > expand a models feature set. > > > > Interacting with QEMU requires starting the QEMU process and completing one or > > more query-cpu-model-baseline QMP exchanges with QEMU in addition to a > > query-cpu-model-expansion QMP exchange to expand all features in the model. > > > > See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion > > of QEMU aspects. > > > > This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 > > > > Version 3 attempts to address all issues from V1 and V2 including making the > > QEMU process activation for QMP Queries generic (not specific to capabilities) > > and moving that code from qemu_capabilities to qemu_process. > > So far so good. > > > I attempted to make the new qemu_process functions consistent with the functions > > but mostly ended up reusing the guts of the previous functions from qemu_capabilities. > > I guess you wanted to say "... consistent with the existing functions in > qemu_process.c" or something similar, right? It's fine to just move and > generalize the functions from qemu_capabilities (and perhaps make the > original functions just wrappers around the new ones if needed). > > > Of interest may be the choice to reuse a domain structure to hold the Qmp Query > > process state and connection information. > > This sounds like a bad idea to me. > > > Also, pay attention to the use of a large random number to uniquely > > identify decoupled processes in terms of sockets and file system > > footprint. If you believe it's worth the effort I think there are > > some pre-existing library functions to establish files with unique > > identifiers in a thread safe way. > > We already have src/util/virrandom.c for random numbers, but it doesn't > really matter anyway since we don't need or either want to use them > here. Just use mkdtemp() and store the socket, pid, whatever you need in > there. > > > The last patch may also be interesting and is based on past discussion of QEMU cpu > > expansion only returning migratable features except for one x86 case where > > non-migratable features are explicitly requested. The patch records that features > > are migratable based on QEMU only returning migratable features. The main result > > is the CPU xml for S390 CPU's contains the same migratability info the X86 currently > > does. The testcases were updated to reflect the change. Is this ok? > > Yeah, looks OK, although I didn't look too closely at it. > > > Unlike the previous versions every patch should compile independently if applied > > in sequence. > > Good, each series have to make sure the code can be compiled after every > single patch in the series (so that we can easily use git bisect). But > please, don't achieve the goal by squashing patches until the result can > be compiled. Hi Jiri, At first look I am getting what your saying on everything but am concerned I am not understanding something about the rules on splitting out patches. Thanks for pointing out that I could split out virQEMUCapsMigratablePropsDiff. I missed that one. I am fully on board with making the patches as small and easy to review as possible. But I think I got pretty strong feedback in the last review that there should be no floating (unused) functions in a patch and each patch should compile and pass test cases and function without crashing.