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

[Stefan Hajnoczi]

On Tue, Apr 28, 2020 at 02:58:21PM -0400, Jag Raman wrote:
> > 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.

Grouping all the stubs into a single patch isn't a problem.

The issue is that some of the new .c files are not referenced by any
makefile rules.  They aren't being compiled and may contain syntax
errors.

There is also no justification for these stubs so reviewers don't know
why exactly they are needed.  I would have liked to know what required
the creation of each stub.  If you don't remember the details anymore
then let's not worry about it, but sometimes there are cleaner ways of
resolving dependencies than adding stubs.

Stefan

signature.asc
Description: PGP signature