On older Solaris releases, we didn't get a protype for madvise, and so
util/osdep.c provides its own prototype. Some time between the public
Solaris 11.4 release and Solaris 11.4.42 CBE, we started getting an
madvise prototype that looks like this:
extern int madvise(void *, size_t, int);
Which conflicts with the prototype in util/osdeps.c. Instead of always
declaring this prototype, check if madvise() is already declared, so
we don't need to declare it ourselves.
Signed-off-by: Andrew Deason <adeason@sinenomine.net>
---
I'm not sure if a test is needed for this at all; that is, how much qemu
cares about earlier Solaris. The madvise prototype exists earlier in
Solaris 11 (I'm not sure when it started appearing usefully), but in
11.4 and earlier it was compatible with the char* prototype.
meson.build | 3 +++
util/osdep.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/meson.build b/meson.build
index 2d6601467f..c29d49c7a1 100644
--- a/meson.build
+++ b/meson.build
@@ -322,20 +322,23 @@ if targetos == 'windows'
include_directories: include_directories('.'))
host_dsosuf = '.dll'
elif targetos == 'darwin'
coref = dependency('appleframeworks', modules: 'CoreFoundation')
iokit = dependency('appleframeworks', modules: 'IOKit', required: false)
host_dsosuf = '.dylib'
elif targetos == 'sunos'
socket = [cc.find_library('socket'),
cc.find_library('nsl'),
cc.find_library('resolv')]
+ config_host_data.set('HAVE_MADVISE_PROTO',
+ cc.has_function('madvise',
+ prefix: '#include <sys/mman.h>'))
elif targetos == 'haiku'
socket = [cc.find_library('posix_error_mapper'),
cc.find_library('network'),
cc.find_library('bsd')]
elif targetos == 'openbsd'
if get_option('tcg').allowed() and target_dirs.length() > 0
# Disable OpenBSD W^X if available
emulator_link_args = cc.get_supported_link_arguments('-Wl,-z,wxneeded')
endif
endif
diff --git a/util/osdep.c b/util/osdep.c
index 7c4deda6fe..c99083372b 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -21,24 +21,26 @@
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
#include "qemu/osdep.h"
#include "qapi/error.h"
/* Needed early for CONFIG_BSD etc. */
#ifdef CONFIG_SOLARIS
#include <sys/statvfs.h>
+#ifndef HAVE_MADVISE_PROTO
/* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
discussion about Solaris header problems */
extern int madvise(char *, size_t, int);
#endif
+#endif
#include "qemu-common.h"
#include "qemu/cutils.h"
#include "qemu/sockets.h"
#include "qemu/error-report.h"
#include "qemu/madvise.h"
#include "qemu/mprotect.h"
#include "qemu/hw-version.h"
#include "monitor/monitor.h"
--
2.11.0On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
>
> On older Solaris releases, we didn't get a protype for madvise, and so
> util/osdep.c provides its own prototype. Some time between the public
> Solaris 11.4 release and Solaris 11.4.42 CBE, we started getting an
> madvise prototype that looks like this:
>
> extern int madvise(void *, size_t, int);
>
> Which conflicts with the prototype in util/osdeps.c. Instead of always
> declaring this prototype, check if madvise() is already declared, so
> we don't need to declare it ourselves.
>
> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> ---
> I'm not sure if a test is needed for this at all; that is, how much qemu
> cares about earlier Solaris. The madvise prototype exists earlier in
> Solaris 11 (I'm not sure when it started appearing usefully), but in
> 11.4 and earlier it was compatible with the char* prototype.
> #ifdef CONFIG_SOLARIS
> #include <sys/statvfs.h>
> +#ifndef HAVE_MADVISE_PROTO
> /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> discussion about Solaris header problems */
> extern int madvise(char *, size_t, int);
> #endif
> +#endif
Rather than keeping this inside a CONFIG_SOLARIS and only doing
the meson.build test if targetos == sunos, I would prefer it if we
unconditionally determined two things in meson.build:
(1) do we have madvise in the usual way? (this is what we would
want CONFIG_MADVISE to be, and might even be what it actually is)
(2) do we have madvise but only if we provide a prototype for it
ourselves? (maybe CONFIG_MADVISE_NO_PROTO)
and then in osdep.h provide the prototype if CONFIG_MADVISE_NO_PROTO.
(osdep.h is where we provide "this is a fixup to the system headers"
portability workarounds, which this seems to be.)
This isn't the only .c file that directly calls madvise() :
softmmu/physmem.c does also. That looks like maybe a bug though:
perhaps it should be calling qemu_madvise()...
Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ?
Is that unrelated to madvise() ?
thanks
-- PMM
On Mon, 14 Mar 2022 16:36:00 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote: > > #ifdef CONFIG_SOLARIS > > #include <sys/statvfs.h> > > +#ifndef HAVE_MADVISE_PROTO > > /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for > > discussion about Solaris header problems */ > > extern int madvise(char *, size_t, int); > > #endif > > +#endif > > Rather than keeping this inside a CONFIG_SOLARIS and only doing > the meson.build test if targetos == sunos, I would prefer it if we > unconditionally determined two things in meson.build: > (1) do we have madvise in the usual way? (this is what we would > want CONFIG_MADVISE to be, and might even be what it actually is) > (2) do we have madvise but only if we provide a prototype for it > ourselves? (maybe CONFIG_MADVISE_NO_PROTO) CONFIG_MADVISE is set if we can cc.links() something that calls madvise(). If we're missing the prototype, that will fail with -Werror, but I expect succeeds otherwise. If cc.links() just uses the cflags for the build, then it seems like it might succeed or fail depending on --enable-werror. I see some other tests give -Werror as an explicit extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should this be doing the same to make sure it fails for a missing prototype? Also just to mention, if we don't care about older Solaris, the prototype can just be unconditionally removed. It's pretty annoying to even try to build qemu from git on Solaris 11.4 and earlier, because many of the build requirements need to be installed/compiled manually (notably python 3.6+, but iirc also ninja, meson, etc). So I haven't really tried; there may be many other build issues there. > Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ? > Is that unrelated to madvise() ? I think so, it was added in commit 605686cd7ac, which predates madvise() in that file. It does look like it could be removed from a quick glance. Would you want me to add a commit to remove it? (Assuming it still compiles okay.) Or just do that in the same commit as changing the madvise prototype logic? -- Andrew Deason adeason@sinenomine.net
On Mon, 14 Mar 2022 at 18:18, Andrew Deason <adeason@sinenomine.net> wrote:
>
> On Mon, 14 Mar 2022 16:36:00 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
> > > #ifdef CONFIG_SOLARIS
> > > #include <sys/statvfs.h>
> > > +#ifndef HAVE_MADVISE_PROTO
> > > /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> > > discussion about Solaris header problems */
> > > extern int madvise(char *, size_t, int);
> > > #endif
> > > +#endif
> >
> > Rather than keeping this inside a CONFIG_SOLARIS and only doing
> > the meson.build test if targetos == sunos, I would prefer it if we
> > unconditionally determined two things in meson.build:
> > (1) do we have madvise in the usual way? (this is what we would
> > want CONFIG_MADVISE to be, and might even be what it actually is)
> > (2) do we have madvise but only if we provide a prototype for it
> > ourselves? (maybe CONFIG_MADVISE_NO_PROTO)
>
> CONFIG_MADVISE is set if we can cc.links() something that calls
> madvise(). If we're missing the prototype, that will fail with -Werror,
> but I expect succeeds otherwise. If cc.links() just uses the cflags for
> the build, then it seems like it might succeed or fail depending on
> --enable-werror.
Mmm. I think that we wrote it that way because it was a straight
translation to meson of the previous configure-script madvise
detection code. I think it is equivalent to
config_host_data.set('CONFIG_MEMALIGN', cc.has_function('memalign'))
which also effectively does a pure "does this link?" test.
So maybe we want to keep CONFIG_MEMALIGN the way it is and add
a CONFIG_MISSING_MEMALIGN_PROTOTYPE or something. I think that
this is rapidly getting out of my depth as far as meson is concerned,
though.
> I see some other tests give -Werror as an explicit
> extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should
> this be doing the same to make sure it fails for a missing prototype?
>
> Also just to mention, if we don't care about older Solaris, the
> prototype can just be unconditionally removed. It's pretty annoying to
> even try to build qemu from git on Solaris 11.4 and earlier, because
> many of the build requirements need to be installed/compiled manually
> (notably python 3.6+, but iirc also ninja, meson, etc). So I haven't
> really tried; there may be many other build issues there.
Hard to say whether we do or don't care. We have had some
mailing list threads in the past. I think we might also care
a bit about Illumos, which might still have some issues that
Solaris proper has since fixed. Sometimes it's easier to hang on
to the portability workaround than to try to find out :-)
> > Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ?
> > Is that unrelated to madvise() ?
>
> I think so, it was added in commit 605686cd7ac, which predates madvise()
> in that file. It does look like it could be removed from a quick glance.
Yeah, I think in commit 4a1418e07bdcfaa31 in 2009 we removed kqemu
support, which was the only thing using statvfs() in that file,
but forgot to remove the #include. Since that's an entirely separate
thing from madvise, feel free to either ignore it or send a separate patch.
-- PMM
On Mon, Mar 14, 2022 at 01:18:00PM -0500, Andrew Deason wrote: > On Mon, 14 Mar 2022 16:36:00 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > > > On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote: > > > #ifdef CONFIG_SOLARIS > > > #include <sys/statvfs.h> > > > +#ifndef HAVE_MADVISE_PROTO > > > /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for > > > discussion about Solaris header problems */ > > > extern int madvise(char *, size_t, int); > > > #endif > > > +#endif > > > > Rather than keeping this inside a CONFIG_SOLARIS and only doing > > the meson.build test if targetos == sunos, I would prefer it if we > > unconditionally determined two things in meson.build: > > (1) do we have madvise in the usual way? (this is what we would > > want CONFIG_MADVISE to be, and might even be what it actually is) > > (2) do we have madvise but only if we provide a prototype for it > > ourselves? (maybe CONFIG_MADVISE_NO_PROTO) > > CONFIG_MADVISE is set if we can cc.links() something that calls > madvise(). If we're missing the prototype, that will fail with -Werror, > but I expect succeeds otherwise. If cc.links() just uses the cflags for > the build, then it seems like it might succeed or fail depending on > --enable-werror. I see some other tests give -Werror as an explicit > extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should > this be doing the same to make sure it fails for a missing prototype? > > Also just to mention, if we don't care about older Solaris, the > prototype can just be unconditionally removed. It's pretty annoying to > even try to build qemu from git on Solaris 11.4 and earlier, because > many of the build requirements need to be installed/compiled manually > (notably python 3.6+, but iirc also ninja, meson, etc). So I haven't > really tried; there may be many other build issues there. If we had a tiered support status, Solaris would be tier 3 right now, as we have no testing of it at all. If it compiles at any given point in time it is luck. We have a general purpose platform support policy https://www.qemu.org/docs/master/about/build-platforms.html where the common rule ends up being "the current major release, and the previous major release for 2 years overlap". The question is what counts as a major release from a Solaris POV ? In terms of long life distros, our policy gives about 4-5 years of supportable life in the best case. I wouldn't want to go beyond that ballpark for Solaris. Can we come up with an interpration of our policy to map to Solaris that doesn't tie our hands for longer than 4-5 years worst case. IOW, we certainly do NOT need to support arbitrarily old Solaris. If madvise has done what we need for 4-5 years, then we can likely not need to test for it, and just assume its existance. This just requires someone to specify how we interpret our build platform policy to exclude older Solaris. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, 14 Mar 2022 19:01:06 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote: > We have a general purpose platform support policy > > https://www.qemu.org/docs/master/about/build-platforms.html > > where the common rule ends up being "the current major release, > and the previous major release for 2 years overlap". > > The question is what counts as a major release from a Solaris > POV ? In terms of long life distros, our policy gives about > 4-5 years of supportable life in the best case. I wouldn't > want to go beyond that ballpark for Solaris. Can we come up > with an interpration of our policy to map to Solaris that > doesn't tie our hands for longer than 4-5 years worst case. FWIW, some relevant Solaris version numbers, as I understand it: 11.4.42 CBE: public release March 2022. (Non-production use only, rolling release schedule.) 11.4: public release August 2018. 11.3: public release October 2015. Going by the minor version number (11.3 vs 11.4) sounds similar to Linux distros; they've come out every few years in the past. But I don't know how this is going to look with the CBE stuff in the future, or if anyone knows (it's very new). > IOW, we certainly do NOT need to support arbitrarily old > Solaris. If madvise has done what we need for 4-5 years, > then we can likely not need to test for it, and just assume > its existance. This just requires someone to specify how > we interpret our build platform policy to exclude older > Solaris. Specifically for the madvise() prototype workaround, I looked a little bit into what version changes actually matter. I think all of Solaris 11 is probably okay without the workaround (I can check back to Solaris 11.1, but just by looking at the mman.h header, not actually testing a build), because we specify -D__EXTENSIONS__. Illumos and Solaris 10 look like they would need the workaround. So practically speaking for this patchset, it seems more like a question of Illumos support. -- Andrew Deason adeason@sinenomine.net
© 2016 - 2026 Red Hat, Inc.