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

[Chris Venteicher]

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.

I tried pretty hard to create / retain as many distinct patches as I
could but those rules, as I understand them, didn't seem to leave me
many options.

Should I be creating unit tests or something for what would otherwise
be unused functions to get patches with working compiles?  Am I
misunderstanding the rules?  Some other trick I am missing that would
enable me to get to smaller patches?

Thanks for any feedback you can give on this.

Chris

> 
> Jirka