Re: [PATCH RESEND v6 08/36] multi-process: Add stub functions to facilitate build of multi-process

[Stefan Hajnoczi]

On Fri, Apr 24, 2020 at 09:47:56AM -0400, Jag Raman wrote:
> > On Apr 24, 2020, at 9:12 AM, Stefan Hajnoczi <address@hidden> wrote:
> > On Wed, Apr 22, 2020 at 09:13:43PM -0700, address@hidden wrote:
> >> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> >> index f884bb6180..f74c7e927b 100644
> >> --- a/stubs/Makefile.objs
> >> +++ b/stubs/Makefile.objs
> >> @@ -20,6 +20,7 @@ stub-obj-y += migr-blocker.o
> >> stub-obj-y += change-state-handler.o
> >> stub-obj-y += monitor.o
> >> stub-obj-y += monitor-core.o
> >> +stub-obj-y += get-fd.o
> >> stub-obj-y += notify-event.o
> >> stub-obj-y += qtest.o
> >> stub-obj-y += replay.o
> > 
> > audio.c, vl-stub.c, and xen-mapcache.c are added by this patch but not
> > added to Makefile.objs?  Can they be removed?
> 
> Hey Stefan,
> 
> Sorry it’s not clear. but these files are referenced in Makefile.target.

Why is the Makefile.target change not in this patch?

Please structure patch series as logical changes that can be reviewed
sequentially.  Not only is it hard for reviewers to understand what is
going on but it probably also breaks bisectability if patches contain
incomplete changes.

> > 
> > This entire patch requires justification.  Stubs exist so that common
> > code can be linked without optional features.
> > 
> > For example, common code may call into kvm but that callback isn't
> > relevant when building with kvm accelerator support (e.g. say qemu-nbd).
> > That's where the stub function comes in.  It fulfills the dependency
> > without dragging in the actual kvm accelerator code.
> > 
> > Adding lots of stubs suggests you are building QEMU in a new way that
> > wasn't done before (this is true and expected for this patch series).  I
> > would like to understand the reason for these stubs though.  For
> > example, why do you need to stub audio?
> 
> These stub functions are only used by the remote process, and not by
> QEMU itself.
> 
> Our goal is to ensure that the remote process is building the smallest
> set of files necessary and these stub functions are necessary to meet
> that goal.
> 
> For example, the remote process needs to build some of the functions
> defined in “hw/core/qdev-properties-system.c”. However, this file
> depends on audio.c (references audio_state_by_name()), which is not
> needed for the remote process. The alternative to stub functions would
> be to compile audio.c into the remote process, but that was not necessary
> in our judgement. When the project started out, we spent a lot of time
> figuring out which functions/files are necessary for the remote process, and
> we stubbed out the ones which are needed to resolve dependency during
> compilation, but not needed for functionality.
> 
> audio.c is just an example of tens of other places where we needed to
> make similar judgements.
> 
> Would you prefer if we moved these stub functions into a separate
> library (instead of stub-obj-y) which is only linked by the remote process?

It's too bad that none of these judgements were documented.  As a
reviewer I have no idea what the justification for each individual stub
was.

Some stubs are unavoidable but they also indicate that the code is
tightly coupled where maybe it can be split up.  The
qdev-properties-system.c example you mentioned sounds like something
that should be broken up into multiple files.  Then stubs wouldn't be
necessary.

That said, adding stubs doesn't place a great burden on anyone and I
think they can be merged.

signature.asc
Description: PGP signature