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

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

[Richard Henderson]

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.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 63f312bd1f..9de7e43a80 100755
--- a/configure
+++ b/configure
@@ -5878,6 +5878,10 @@ fi
 ##########################################
 # check for slirp
 
+if test "$softmmu" = "no"; then
+    slirp=no
+fi
+
 case "$slirp" in
   "" | yes)
     if $pkg_config slirp; then
-- 
2.17.1

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

[Thomas Huth]

On 10/05/2019 22.34, Richard Henderson 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.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  configure | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure b/configure
> index 63f312bd1f..9de7e43a80 100755
> --- a/configure
> +++ b/configure
> @@ -5878,6 +5878,10 @@ fi
>  ##########################################
>  # check for slirp
>  
> +if test "$softmmu" = "no"; then
> +    slirp=no
> +fi

Maybe also check that the user did not try to run configure with both,
--disable-system and --enable-slirp? I.e. that $slirp != "yes" ?

 Thomas

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

[Aleksandar Markovic]

On May 10, 2019 10:36 PM, "Richard Henderson" <richard.henderson@linaro.org>
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? If no, the patch shoud be amended. If yes, the commit message
should be extended.

Thanks,
Aleksandar

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  configure | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/configure b/configure
> index 63f312bd1f..9de7e43a80 100755
> --- a/configure
> +++ b/configure
> @@ -5878,6 +5878,10 @@ fi
>  ##########################################
>  # check for slirp
>
> +if test "$softmmu" = "no"; then
> +    slirp=no
> +fi
> +
>  case "$slirp" in
>    "" | yes)
>      if $pkg_config slirp; then
> --
> 2.17.1
>
>

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

[Richard Henderson]

On 5/11/19 5:47 AM, Aleksandar Markovic wrote:
> 
> On May 10, 2019 10:36 PM, "Richard Henderson" <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> 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.


r~

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

[Aleksandar Markovic]

On May 13, 2019 11:14 PM, "Richard Henderson" <richard.henderson@linaro.org>
wrote:
>
> On 5/11/19 5:47 AM, Aleksandar Markovic wrote:
> >
> > On May 10, 2019 10:36 PM, "Richard Henderson" <
richard.henderson@linaro.org
> > <mailto:richard.henderson@linaro.org>> 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~

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

[Peter Maydell]

On Tue, 14 May 2019 at 20:16, Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
>
> On May 13, 2019 11:14 PM, "Richard Henderson" <richard.henderson@linaro.org>
> wrote:
> >
> > On 5/11/19 5:47 AM, Aleksandar Markovic wrote:
> > > 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.

Aleksandar: I think this is not really a very productive stance to take.
Richard thinks the commit message is reasonable. If you have something
you would like him to change, I think we will reach a useful endpoint
much more quickly and smoothly if you suggest some new text, rather than
effectively saying "you need to think of something, and I'm going to keep
making you rewrite it until you telepathically figure out what the text
I wanted you to write is".

thanks
-- PMM

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

[Aleksandar Markovic]

On May 15, 2019 12:07 PM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On Tue, 14 May 2019 at 20:16, Aleksandar Markovic
> <aleksandar.m.mail@gmail.com> wrote:
> >
> > On May 13, 2019 11:14 PM, "Richard Henderson" <
richard.henderson@linaro.org>
> > wrote:
> > >
> > > On 5/11/19 5:47 AM, Aleksandar Markovic wrote:
> > > > 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.
>
> Aleksandar: I think this is not really a very productive stance to take.
> Richard thinks the commit message is reasonable. If you have something
> you would like him to change, I think we will reach a useful endpoint
> much more quickly and smoothly if you suggest some new text, rather than
> effectively saying "you need to think of something, and I'm going to keep
> making you rewrite it until you telepathically figure out what the text
> I wanted you to write is".
>

OK, Peter, no problem from my side. I was trying to make Richard think more
about what he writes in his commit messages, and how he organizes his code.
Sorry if this looked unproductive or even perhaps offensive.

Yours,
Aleksadar

> thanks
> -- PMM