[Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Marc-Antoine Perennou]

pulse/simple.h does not include stdlib.h
We cannot use NULL since it may not be defined
Use 0 instead

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
---
 configure |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index ea8b676..d67c3ce 100755
--- a/configure
+++ b/configure
@@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do

     pa)
     audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
-        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
+        "pa_simple *s = 0; pa_simple_free(s); return 0;"
     libs_softmmu="-lpulse -lpulse-simple $libs_softmmu"
     audio_pt_int="yes"
     ;;
-- 
1.7.5.52.ge839f.dirty

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Marc-Antoine Perennou]

Any chance to get this reviewed/applied any time soon ? It currently
does not build without it with gcc 4.6.0

On 29 April 2011 17:59, Marc-Antoine Perennou <Marc-Antoine@perennou.com> wrote:
> pulse/simple.h does not include stdlib.h
> We cannot use NULL since it may not be defined
> Use 0 instead
>
> Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
> ---
>  configure |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/configure b/configure
> index ea8b676..d67c3ce 100755
> --- a/configure
> +++ b/configure
> @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
>
>     pa)
>     audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
> -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
> +        "pa_simple *s = 0; pa_simple_free(s); return 0;"
>     libs_softmmu="-lpulse -lpulse-simple $libs_softmmu"
>     audio_pt_int="yes"
>     ;;
> --
> 1.7.5.52.ge839f.dirty
>

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Aurelien Jarno]

On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
> pulse/simple.h does not include stdlib.h
> We cannot use NULL since it may not be defined
> Use 0 instead

I am unable to reproduce this issue, even with gcc-4.6. Also please note
that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is 
included from <sys/types.h> which is included from <pulse/simple.h>.

Do you have more information about the issue.

> Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
> ---
>  configure |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/configure b/configure
> index ea8b676..d67c3ce 100755
> --- a/configure
> +++ b/configure
> @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
> 
>      pa)
>      audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
> -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
> +        "pa_simple *s = 0; pa_simple_free(s); return 0;"

It should be ((void*)0) instead of simply 0.

>      libs_softmmu="-lpulse -lpulse-simple $libs_softmmu"
>      audio_pt_int="yes"
>      ;;
> -- 
> 1.7.5.52.ge839f.dirty
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[malc]

On Fri, 3 Jun 2011, Aurelien Jarno wrote:

> On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
> > pulse/simple.h does not include stdlib.h
> > We cannot use NULL since it may not be defined
> > Use 0 instead
> 
> I am unable to reproduce this issue, even with gcc-4.6. Also please note
> that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is 
> included from <sys/types.h> which is included from <pulse/simple.h>.
> 
> Do you have more information about the issue.
> 
> > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
> > ---
> >  configure |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/configure b/configure
> > index ea8b676..d67c3ce 100755
> > --- a/configure
> > +++ b/configure
> > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
> > 
> >      pa)
> >      audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
> > -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
> > +        "pa_simple *s = 0; pa_simple_free(s); return 0;"
> 
> It should be ((void*)0) instead of simply 0.

No it shouldn't.

> 
> >      libs_softmmu="-lpulse -lpulse-simple $libs_softmmu"
> >      audio_pt_int="yes"
> >      ;;
> > -- 
> > 1.7.5.52.ge839f.dirty
> > 
> > 
> 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Aurelien Jarno]

On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote:
> On Fri, 3 Jun 2011, Aurelien Jarno wrote:
> 
> > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
> > > pulse/simple.h does not include stdlib.h
> > > We cannot use NULL since it may not be defined
> > > Use 0 instead
> > 
> > I am unable to reproduce this issue, even with gcc-4.6. Also please note
> > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is 
> > included from <sys/types.h> which is included from <pulse/simple.h>.
> > 
> > Do you have more information about the issue.
> > 
> > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
> > > ---
> > >  configure |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/configure b/configure
> > > index ea8b676..d67c3ce 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
> > > 
> > >      pa)
> > >      audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
> > > -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
> > > +        "pa_simple *s = 0; pa_simple_free(s); return 0;"
> > 
> > It should be ((void*)0) instead of simply 0.
> 
> No it shouldn't.
> 

If we want to replace #include <stddef.h>, which should use the same
code, that is:

| #ifndef __cplusplus
| #define NULL ((void *)0)
| #else   /* C++ */
| #define NULL 0
| #endif  /* C++ */

Given we are writing C code and not C++ code, NULL is defined as 
((void *)0).

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[malc]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1843 bytes --]

On Sat, 4 Jun 2011, Aurelien Jarno wrote:

> On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote:
> > On Fri, 3 Jun 2011, Aurelien Jarno wrote:
> > 
> > > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
> > > > pulse/simple.h does not include stdlib.h
> > > > We cannot use NULL since it may not be defined
> > > > Use 0 instead
> > > 
> > > I am unable to reproduce this issue, even with gcc-4.6. Also please note
> > > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is 
> > > included from <sys/types.h> which is included from <pulse/simple.h>.
> > > 
> > > Do you have more information about the issue.
> > > 
> > > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
> > > > ---
> > > >  configure |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/configure b/configure
> > > > index ea8b676..d67c3ce 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
> > > > 
> > > >      pa)
> > > >      audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
> > > > -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
> > > > +        "pa_simple *s = 0; pa_simple_free(s); return 0;"
> > > 
> > > It should be ((void*)0) instead of simply 0.
> > 
> > No it shouldn't.
> > 
> 
> If we want to replace #include <stddef.h>, which should use the same
> code, that is:
> 
> | #ifndef __cplusplus
> | #define NULL ((void *)0)
> | #else   /* C++ */
> | #define NULL 0
> | #endif  /* C++ */
> 
> Given we are writing C code and not C++ code, NULL is defined as 
> ((void *)0).

Doesn't make a dent of a differnce how particular implementation defines
NULL, 6.2.3.3#3 covers it, what i'm trying to get across is that your
strong "should" is wrong.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Marc-Antoine Perennou]

On 4 June 2011 00:54, malc <av1474@comtv.ru> wrote:
> On Sat, 4 Jun 2011, Aurelien Jarno wrote:
>
>> On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote:
>> > On Fri, 3 Jun 2011, Aurelien Jarno wrote:
>> >
>> > > On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
>> > > > pulse/simple.h does not include stdlib.h
>> > > > We cannot use NULL since it may not be defined
>> > > > Use 0 instead
>> > >
>> > > I am unable to reproduce this issue, even with gcc-4.6. Also please note
>> > > that NULL is defined in <stddef.h>, not <stdlib.h>. <stddef.h> is
>> > > included from <sys/types.h> which is included from <pulse/simple.h>.
>> > >
>> > > Do you have more information about the issue.
>> > >
>> > > > Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
>> > > > ---
>> > > >  configure |    2 +-
>> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
>> > > >
>> > > > diff --git a/configure b/configure
>> > > > index ea8b676..d67c3ce 100755
>> > > > --- a/configure
>> > > > +++ b/configure
>> > > > @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
>> > > >
>> > > >      pa)
>> > > >      audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
>> > > > -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
>> > > > +        "pa_simple *s = 0; pa_simple_free(s); return 0;"
>> > >
>> > > It should be ((void*)0) instead of simply 0.
>> >
>> > No it shouldn't.
>> >
>>
>> If we want to replace #include <stddef.h>, which should use the same
>> code, that is:
>>
>> | #ifndef __cplusplus
>> | #define NULL ((void *)0)
>> | #else   /* C++ */
>> | #define NULL 0
>> | #endif  /* C++ */
>>
>> Given we are writing C code and not C++ code, NULL is defined as
>> ((void *)0).
>
> Doesn't make a dent of a differnce how particular implementation defines
> NULL, 6.2.3.3#3 covers it, what i'm trying to get across is that your
> strong "should" is wrong.
>
> --
> mailto:av1474@comtv.ru

It's kinda weird actually, just wrote a small program to test:

#include <stddef.h>
int main() {
    int * i = NULL;
    return 0;
}

It builds fine
Now if I replace "stddef.h" by "sys/types.h"
‘NULL’ undeclared

I'm running Gentoo GNU/Linux, and everyone using it in my friends get
the same problem, and there is no problem at all with the whole system
which is around.
(Only gcc 4.6 is installed fyi)

Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it work.

Hope I provided enough informations to justify this thread

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Andreas Färber]

Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou:

> On 4 June 2011 00:54, malc <av1474@comtv.ru> wrote:
>> On Sat, 4 Jun 2011, Aurelien Jarno wrote:
>>
>>> On Sat, Jun 04, 2011 at 01:57:23AM +0400, malc wrote:
>>>> On Fri, 3 Jun 2011, Aurelien Jarno wrote:
>>>>
>>>>> On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou  
>>>>> wrote:
>>>>>> diff --git a/configure b/configure
>>>>>> index ea8b676..d67c3ce 100755
>>>>>> --- a/configure
>>>>>> +++ b/configure
>>>>>> @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
>>>>>>
>>>>>>      pa)
>>>>>>      audio_drv_probe $drv pulse/simple.h "-lpulse-simple - 
>>>>>> lpulse" \
>>>>>> -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
>>>>>> +        "pa_simple *s = 0; pa_simple_free(s); return 0;"

> It's kinda weird actually, just wrote a small program to test:
>
> #include <stddef.h>
> int main() {
>    int * i = NULL;
>    return 0;
> }
>
> It builds fine
> Now if I replace "stddef.h" by "sys/types.h"
> ‘NULL’ undeclared
>
> I'm running Gentoo GNU/Linux [...]
>
> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes  
> it work.

Then please submit a new patch that explicit includes that header with  
a comment explaining why, so that it doesn't get removed by accident.

Andreas

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Markus Armbruster]

Aurelien Jarno <aurelien@aurel32.net> writes:

> On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
>> pulse/simple.h does not include stdlib.h
>> We cannot use NULL since it may not be defined
>> Use 0 instead
>
> I am unable to reproduce this issue, even with gcc-4.6. Also please note
> that NULL is defined in <stddef.h>, not <stdlib.h>.

NULL is defined in stddef.h (ISO/IEC 9899:1999 7.17), locale.h (ibid
7.11), stdio.h (ibid 7.19.1), stdlib.h (ibid 7.20), string.h (ibid
7.21.1), time.h (ibid 7.23.1), wchar.h (ibid 7.24.1).

>                                                     <stddef.h> is 
> included from <sys/types.h> which is included from <pulse/simple.h>.

I don't think POSIX specifies sys/types.h defines NULL, let alone
includes stddef.h.

> Do you have more information about the issue.
>
>> Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
>> ---
>>  configure |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/configure b/configure
>> index ea8b676..d67c3ce 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1567,7 +1567,7 @@ for drv in $audio_drv_list; do
>> 
>>      pa)
>>      audio_drv_probe $drv pulse/simple.h "-lpulse-simple -lpulse" \
>> -        "pa_simple *s = NULL; pa_simple_free(s); return 0;"
>> +        "pa_simple *s = 0; pa_simple_free(s); return 0;"
>
> It should be ((void*)0) instead of simply 0.

Matter of taste.

>>      libs_softmmu="-lpulse -lpulse-simple $libs_softmmu"
>>      audio_pt_int="yes"
>>      ;;

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Peter Maydell]

On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou:
>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it
>> work.
>
> Then please submit a new patch that explicit includes that header with a
> comment explaining why, so that it doesn't get removed by accident.

I think the original patch is fine. This configure test isn't
trying to test whether we correctly found some definition of
NULL, it's testing pulseaudio presence. The test code should
be the simplest and most stand-alone code that achieves that.
There's no need to go dragging in extra headers when we don't
even need to be using NULL here anyhow.

We already have configure tests which use unadorned 0 for
pointers (eg the pthread test), it's legal C, and it works.
I think we should call this a trivial patch and just apply it.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote:
>> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou:
>>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it
>>> work.
>>
>> Then please submit a new patch that explicit includes that header with a
>> comment explaining why, so that it doesn't get removed by accident.
>
> I think the original patch is fine. This configure test isn't
> trying to test whether we correctly found some definition of
> NULL, it's testing pulseaudio presence. The test code should
> be the simplest and most stand-alone code that achieves that.
> There's no need to go dragging in extra headers when we don't
> even need to be using NULL here anyhow.
>
> We already have configure tests which use unadorned 0 for
> pointers (eg the pthread test), it's legal C, and it works.
> I think we should call this a trivial patch and just apply it.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I agree with your reasoning.

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Marc-Antoine Perennou]

On 10 June 2011 09:14, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote:
>>> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou:
>>>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it
>>>> work.
>>>
>>> Then please submit a new patch that explicit includes that header with a
>>> comment explaining why, so that it doesn't get removed by accident.
>>
>> I think the original patch is fine. This configure test isn't
>> trying to test whether we correctly found some definition of
>> NULL, it's testing pulseaudio presence. The test code should
>> be the simplest and most stand-alone code that achieves that.
>> There's no need to go dragging in extra headers when we don't
>> even need to be using NULL here anyhow.
>>
>> We already have configure tests which use unadorned 0 for
>> pointers (eg the pthread test), it's legal C, and it works.
>> I think we should call this a trivial patch and just apply it.
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I agree with your reasoning.
>

If you want me to provide a patch with the extra include instead, I
can, otherwise, could this trivial one get merged so qemu-kvm can be
built on several systems on which it actually can't ?

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Stefan Hajnoczi]

On Fri, Apr 29, 2011 at 05:59:19PM +0200, Marc-Antoine Perennou wrote:
> pulse/simple.h does not include stdlib.h
> We cannot use NULL since it may not be defined
> Use 0 instead
> 
> Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
> ---
>  configure |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

There has been discussion but I hope we've arrived at consensus based on
the fact that 0 is already used for pthreads and it's valid C.

Thanks, applied to the trivial patches tree:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

Stefan

Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure

[Stefan Hajnoczi]

On Fri, Jun 24, 2011 at 2:31 PM, Marc-Antoine Perennou
<Marc-Antoine@perennou.com> wrote:
> On 10 June 2011 09:14, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 9 June 2011 18:44, Andreas Färber <andreas.faerber@web.de> wrote:
>>>> Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou:
>>>>> Manually including stddef.h or replacing NULL by 0 or (void*)0 makes it
>>>>> work.
>>>>
>>>> Then please submit a new patch that explicit includes that header with a
>>>> comment explaining why, so that it doesn't get removed by accident.
>>>
>>> I think the original patch is fine. This configure test isn't
>>> trying to test whether we correctly found some definition of
>>> NULL, it's testing pulseaudio presence. The test code should
>>> be the simplest and most stand-alone code that achieves that.
>>> There's no need to go dragging in extra headers when we don't
>>> even need to be using NULL here anyhow.
>>>
>>> We already have configure tests which use unadorned 0 for
>>> pointers (eg the pthread test), it's legal C, and it works.
>>> I think we should call this a trivial patch and just apply it.
>>>
>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> I agree with your reasoning.
>>
>
> If you want me to provide a patch with the extra include instead, I
> can, otherwise, could this trivial one get merged so qemu-kvm can be
> built on several systems on which it actually can't ?

It is in the trivial-patches tree now.  qemu.git will pick it up next
and then qemu-kvm.git will pull it in when it syncs up with qemu.git.

Stefan