Re: [Qemu-devel] [PATCH] configure: Disable slirp if --disable-system

[Aleksandar Markovic]

On May 13, 2019 11:14 PM, "Richard Henderson" <address@hidden>
wrote:
>
> On 5/11/19 5:47 AM, Aleksandar Markovic wrote:
> >
> > On May 10, 2019 10:36 PM, "Richard Henderson" <
address@hidden
> > <mailto:address@hidden>> wrote:
> >>
> >> For linux-user, there is no need to add slirp to the set of
> >> git modules checked out, nor build it.
> >>
> >> This also avoids a makefile bug wrt insufficient dependencies
> >> on subdir-slirp.  If slirp/ is not initially present, the
> >> dependencies that check it out are associated with softmmu,
> >> which then generates a build error on slirp/ not present.
> >>
> >
> > Hi,
> >
> > Does this work if only user mode targets are specified via
ˊ--target-listˊ
> > switch?
>
> Yes.  There is a bit of code that converts such a target list to the same
> result as --disable-system, which is $softmmu = no.
>
> > If no, the patch shoud be amended. If yes, the commit message should be
> > extended.
>
> Like what?  I think it's pretty clear as is.
>

Richard, no. In this case, there is a glaring discrepancy between the title
and the functionality that the change provides. Much better title would be
“configure: Disable slirp if no system mode target is selected”.

I leave it to you to find out what can be improved in the commit message.

How well did you test your change? Did you try some corner cases?

I don't have concerns about the wording of the commit message only. I agree
with Thomas that combination of “no system mode target is selected” and
“--enable-slirp is used” must have some special handing. We can't just
leave the rest of the script to do whatever the current code happens to do.
The patch code should be completed.

Thanks,
Aleksandar

>
> r~