Re: [PATCH v2 1/3] util/osdep: Avoid madvise proto on modern Solaris

[Peter Maydell]

On Tue, 15 Mar 2022 at 02:20, 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 we're missing the madvise()
> prototype, and only declare it ourselves if the prototype is missing.
>
> The 'missing_madvise_proto' meson check contains an obviously wrong
> prototype for madvise. So if that code compiles and links, we must be
> missing the actual prototype for madvise.
>
> Signed-off-by: Andrew Deason <adeason@sinenomine.net>
> ---
> To be clear, I'm okay with removing the prototype workaround
> unconditionally; I'm just not sure if there's enough consensus on doing
> that.
>
> The "missing prototype" check is based on getting a compiler error on a
> conflicting prototype, since this just seems more precise and certain
> than getting an error from a missing prototype (needs
> -Werror=missing-prototypes or -Werror). But I can do it the other way
> around if needed.

Seems a reasonable approach to me.

> Changes since v1:
> - madvise prototype check changed to not be platforms-specific, and turned 
> into
>   CONFIG_MADVISE_MISSING_PROTOTYPE.
>
>  meson.build  | 17 +++++++++++++++--
>  util/osdep.c |  3 +++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 2d6601467f..ff5fce693e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1705,25 +1705,38 @@ config_host_data.set('CONFIG_EVENTFD', cc.links('''
>    int main(void) { return eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); }'''))
>  config_host_data.set('CONFIG_FDATASYNC', cc.links(gnu_source_prefix + '''
>    #include <unistd.h>
>    int main(void) {
>    #if defined(_POSIX_SYNCHRONIZED_IO) && _POSIX_SYNCHRONIZED_IO > 0
>    return fdatasync(0);
>    #else
>    #error Not supported
>    #endif
>    }'''))
> -config_host_data.set('CONFIG_MADVISE', cc.links(gnu_source_prefix + '''
> +
> +has_madvise = cc.links(gnu_source_prefix + '''
>    #include <sys/types.h>
>    #include <sys/mman.h>
>    #include <stddef.h>
> -  int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }'''))
> +  int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }''')

Since this is a little bit tricky, I think a comment here will help
future readers:

# Older Solaris/Illumos provide madvise() but forget to prototype it.
# In this case has_madvise will be true (the test program links despite
# a compile warning). To detect the missing-prototype case, we try
# again with a definitely-bogus prototype. This will only compile
# if the system headers don't provide the prototype; otherwise the
# conflicting prototypes will cause a compiler error.

> +missing_madvise_proto = false
> +if has_madvise
> +  missing_madvise_proto = cc.links(gnu_source_prefix + '''
> +    #include <sys/types.h>
> +    #include <sys/mman.h>
> +    #include <stddef.h>
> +    extern int madvise(int);
> +    int main(void) { return madvise(0); }''')
> +endif
> +config_host_data.set('CONFIG_MADVISE', has_madvise)
> +config_host_data.set('CONFIG_MADVISE_MISSING_PROTOTYPE', 
> missing_madvise_proto)

> +#ifdef HAVE_MADVISE_MISSING_PROTOTYPE
>  /* 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

As you note, this doesn't match the name we picked in meson.build.
I don't feel very strongly about the name (we certainly don't manage
consistency across the project about CONFIG_ vs HAVE_ !), but my suggestion
is HAVE_MADVISE_WITHOUT_PROTOTYPE.

Can you put the prototype in include/qemu/osdep.h, please?
(Exact location not very important as long as it's inside
the extern-C block, but I suggest just under the bit where we
define SIGIO for __HAIKU__.)

This means moving the comment, which will then want fixing up to
our coding style, which these days is
 /*
  * line 1
  * line 2
  */

for multi-line comments.

thanks
-- PMM