Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
On Fri, Jun 24, 2011 at 2:31 PM, Marc-Antoine Perennou wrote: > On 10 June 2011 09:14, Markus Armbruster wrote: >> Peter Maydell writes: >> >>> On 9 June 2011 18:44, Andreas Färber 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 >> >> 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
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
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 > --- > 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
On 10 June 2011 09:14, Markus Armbruster wrote: > Peter Maydell writes: > >> On 9 June 2011 18:44, Andreas Färber 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 > > 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
Peter Maydell writes: > On 9 June 2011 18:44, Andreas Färber 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 I agree with your reasoning.
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
On 9 June 2011 18:44, Andreas Färber 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 -- PMM
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
Aurelien Jarno 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 , not . 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). > is > included from which is included from . 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 >> --- >> 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
Am 09.06.2011 um 17:52 schrieb Marc-Antoine Perennou:
On 4 June 2011 00:54, malc 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
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
On 4 June 2011 00:54, malc 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 , not . is
>> > > included from which is included from .
>> > >
>> > > Do you have more information about the issue.
>> > >
>> > > > Signed-off-by: Marc-Antoine Perennou
>> > > > ---
>> > > > 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 , 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:av1...@comtv.ru
It's kinda weird actually, just wrote a small program to test:
#include
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
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 , not . is > > > included from which is included from . > > > > > > Do you have more information about the issue. > > > > > > > Signed-off-by: Marc-Antoine Perennou > > > > --- > > > > 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 , 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:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
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 , not . is > > included from which is included from . > > > > Do you have more information about the issue. > > > > > Signed-off-by: Marc-Antoine Perennou > > > --- > > > 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 , 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 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
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 , not . is > included from which is included from . > > Do you have more information about the issue. > > > Signed-off-by: Marc-Antoine Perennou > > --- > > 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:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
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 , not . is included from which is included from . Do you have more information about the issue. > Signed-off-by: Marc-Antoine Perennou > --- > 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 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] qemu-kvm: fix pulseaudio detection in configure
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 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 > --- > 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 >
