qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jag Raman
Subject: Re: [PATCH RESEND v6 08/36] multi-process: Add stub functions to facilitate build of multi-process
Date: Tue, 28 Apr 2020 14:58:21 -0400


> On Apr 28, 2020, at 12:29 PM, Stefan Hajnoczi <address@hidden> wrote:
> 
> 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.

Hi Stefan,

We grouped all the stubs into a separate patch for ease of review. If you’re 
finding
it hard to review this way, we’ll modify to ensure that the Makefile changes go 
along
with the stubs.

--
Jag

> 
>>> 
>>> 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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]