Re: [Qemu-devel] [PATCH 1/1] util/path: Do not cache all filenames at startup

[Daniel P . Berrangé]

On Tue, Apr 23, 2019 at 08:30:55PM +0200, David Hildenbrand wrote:
> On 23.04.19 12:01, Daniel P. Berrangé wrote:
> > On Tue, Apr 23, 2019 at 11:54:53AM +0200, Philippe Mathieu-Daudé wrote:
> >> Hi Richard, Daniel,
> >>
> >> On 4/17/19 7:32 AM, Richard Henderson wrote:
> >>> If one uses -L $PATH to point to a full chroot, the startup time
> >>> is significant.  In addition, the existing probing algorithm fails
> >>> to handle symlink loops.
> >>>
> >>> Instead, probe individual paths on demand.  Cache both positive
> >>> and negative results within $PATH, so that any one filename is
> >>> probed only once.
> >>>
> >>> Use glib filename functions for clarity.
> >>>
> >>> Signed-off-by: Richard Henderson <address@hidden>
> >>> ---
> >>>  util/path.c | 211 ++++++++++++++--------------------------------------
> >>>  1 file changed, 57 insertions(+), 154 deletions(-)
> > 
> > 
> >>> +#if GLIB_CHECK_VERSION(2, 58, 0)
> >>
> >> Should we raise GLIB_VERSION_MAX_ALLOWED in "glib-compat.h"?
> >>
> >> Currently it is:
> >>
> >>   /* Ask for warnings if code tries to use function that did not
> >>    * exist in the defined version. These risk breaking builds
> >>    */
> >>   #define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_40
> > 
> > Use of 2.40 is set in accordance with our targetted platform support
> > policy. If Peter has stopped using the Ubuntu 14.04 build host, we
> > could bump it to 2.42. Once our dev tree opens up we could in fact
> > drop Jessie since we'll be supporting Buster by the time next QEMU
> > is released. That would still only take us upto perhaps Glib 2.48
> > 
> > Glib 2.58 is waaaay to new to rely on.
> > 
> > commit e7b3af81597db1a6b55f2c15d030d703c6b2c6ac
> > Author: Daniel P. Berrangé <address@hidden>
> > Date:   Fri May 4 15:34:46 2018 +0100
> > 
> >     glib: bump min required glib library version to 2.40
> >     
> >     Per supported platforms doc[1], the various min glib on relevant 
> > distros is:
> >     
> >       RHEL-7: 2.50.3
> >       Debian (Stretch): 2.50.3
> >       Debian (Jessie): 2.42.1
> >       OpenBSD (Ports): 2.54.3
> >       FreeBSD (Ports): 2.50.3
> >       OpenSUSE Leap 15: 2.54.3
> >       SLE12-SP2: 2.48.2
> >       Ubuntu (Xenial): 2.48.0
> >       macOS (Homebrew): 2.56.0
> >     
> >     This suggests that a minimum glib of 2.42 is a reasonable target.
> >     
> >     The GLibC compile farm, however, uses Ubuntu 14.04 (Trusty) which only
> >     has glib 2.40.0, and this is needed for testing during merge. Thus an
> >     exception is made to the documented platform support policy to allow for
> >     all three current LTS releases to be supported.
> >     
> >     Docker jobs that not longer satisfy this new min version are removed.
> >     
> >     [1] https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms
> >     
> >     Reviewed-by: Thomas Huth <address@hidden>
> >     Signed-off-by: Daniel P. Berrangé <address@hidden>
> > 
> > 
> >>
> >> From commit e71e8cc035558eabd6b3e19f6d3254c754c027ef:
> >>
> >>  glib: enforce the minimum required version and warn about old APIs
> >>
> >>  There are two useful macros that can be defined before including
> >>  glib.h that are related to the min required glib version
> >>
> >>   - GLIB_VERSION_MIN_REQUIRED
> >>
> >>     When this is defined, if code uses an API that was deprecated
> >>     in this version, or older, a compiler warning will be emitted.
> >>     This alerts maintainers to update their code to whatever new
> >>     replacement API is now recommended best practice.
> >>
> >>   - GLIB_VERSION_MAX_ALLOWED
> >>
> >>     When this is defined, if code uses an API that was introduced
> >>     in a version that is newer than the declared version, a compiler
> >>     warning will be emitted. This alerts maintainers if new code
> >>     accidentally uses functionality that won't be available on some
> >>     supported platforms.
> >>
> >>  The GLIB_VERSION_MAX_ALLOWED constant makes it a bit harder to opt
> >>  in to using specific new APIs with a GLIB_CHECK_VERSION conditional.
> >>  To workaround this Pragmas can be used to temporarily turn off the
> >>  -Wdeprecated-declarations compiler warning, while a static inline
> >>  compat function is implemented. This workaround is illustrated with the
> >>  implementation of the g_strv_contains method to satisfy the test suite.
> >>
> >>> +    base = g_canonicalize_filename(prefix, NULL);
> >>> +#else
> >>> +    if (prefix[0] != '/') {
> >>> +        char *cwd = g_get_current_dir();
> >>> +        base = g_build_filename(cwd, prefix, NULL);
> >>> +        g_free(cwd);
> >>> +    } else {
> >>> +        base = g_strdup(prefix);
> >>> +    }
> >>> +#endif
> > 
> > To use functionality from newer glib releases we can't put the #ifdef
> > conditionals inline in the main source files.
> > 
> > They have to be put into glib-compat.h instead. There's a detailed
> > comment in that file illustrating how todo this without triggering
> > the compile warnings about deprecations.
> > 
> > 
> > Regards,
> > Daniel
> > 
> 
> Trying to build (Richard's tcg-vec-next branch) on Fedora29 I get
> 
> util/path.c: In function 'init_paths':
> util/path.c:24:5: error: 'g_canonicalize_filename' is deprecated: Not
> available before 2.58 [-Werror=deprecated-declarations]
>      base = g_canonicalize_filename(prefix, NULL);
>      ^~~~

[snip]

This is exactly why the compat code needs to go into glib-compat.h.
There is a special mechanism in that file for avoiding triggering
the deprecation warnings from glib.

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 :|