> On Apr 28, 2020, at 12:29 PM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Fri, Apr 24, 2020 at 09:47:56AM -0400, Jag Raman wrote:
>>> On Apr 24, 2020, at 9:12 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>>> On Wed, Apr 22, 2020 at 09:13:43PM -0700, elena.ufimts...@oracle.com 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.
