[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.
- [PATCH RESEND v6 31/36] multi-process/mon: choose HMP commands based on target, (continued)
- [PATCH RESEND v6 31/36] multi-process/mon: choose HMP commands based on target, elena . ufimtseva, 2020/04/23
- [PATCH RESEND v6 34/36] multi-process/mon: Initialize QMP module for remote processes, elena . ufimtseva, 2020/04/23
- [PATCH RESEND v6 32/36] multi-process/mon: stub functions to enable QMP module for remote process, elena . ufimtseva, 2020/04/23
- [PATCH RESEND v6 35/36] multi-process: add the concept description to docs/devel/qemu-multiprocess, elena . ufimtseva, 2020/04/23
- [PATCH RESEND v6 02/36] multi-process: Refactor machine_init and exit notifiers, elena . ufimtseva, 2020/04/23
- [PATCH RESEND v6 08/36] multi-process: Add stub functions to facilitate build of multi-process, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 10/36] multi-process: build system for remote device process, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 11/36] multi-process: define mpqemu-link object, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 12/36] multi-process: add functions to synchronize proxy and remote endpoints, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 14/36] multi-process: setup a machine object for remote device process, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 17/36] multi-process: introduce proxy object, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 24/36] multi-process: Retrieve PCI info from remote process, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 26/36] multi-process: add parse_cmdline in remote process, elena . ufimtseva, 2020/04/23
[PATCH RESEND v6 28/36] multi-process: send heartbeat messages to remote, elena . ufimtseva, 2020/04/23