On Wed, Jun 21, 2023 at 04:40:10PM +0100, Daniel P. Berrangé wrote: > On Wed, Jun 21, 2023 at 04:09:10PM +0200, Michal Privoznik wrote: > > As of commit v5.9-rc1~160^2~3 the Linux kernel has close_range() > > syscall, which closes not just one FD but whole range. In glibc > > this is exposed by automatically generated wrapper of the same > > name. In musl, this is not exposed, yet, but we can call the > > syscall() directly. In either case, we have to deal with a > > situation, when the kernel we're running under does not have the > > syscall as glibc deliberately does not implement fallback. > > > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > > --- > > meson.build | 1 + > > src/util/vircommand.c | 117 +++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 117 insertions(+), 1 deletion(-) > > > > diff --git a/meson.build b/meson.build > > index a4b52b6156..ecfc1b6bdf 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -573,6 +573,7 @@ libvirt_export_dynamic = cc.first_supported_link_argument([ > > # check availability of various common functions (non-fatal if missing) > > > > functions = [ > > + 'close_range', > > 'closefrom', > > 'elf_aux_info', > > 'explicit_bzero', > > diff --git a/src/util/vircommand.c b/src/util/vircommand.c > > index b8b8d48f92..e826f5f348 100644 > > --- a/src/util/vircommand.c > > +++ b/src/util/vircommand.c > > @@ -479,13 +479,128 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups) > > return 0; > > } > > > > -# ifdef WITH_CLOSEFROM > > +# if defined(WITH_CLOSE_RANGE) || \ > > + (defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range)) > > +# define USE_CLOSE_RANGE > > +# elif defined(WITH_CLOSEFROM) > > # define USE_CLOSEFROM > > # else > > # define USE_GENERIC > > # endif > > Although you've optimized to take advantage of using close_range > to deal with gaps, in the interests of code brevity, I wonder if > we shouldn't simplify this patch down to merely: > > #ifndef WITH_CLOSEFROM > # ifdef WITH_CLOSE_RANGE > # define closefrom(n) closerange(n, ~0U, 0) > # define WITH_CLOSEFROM > # else if defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range) > # define closefrom(n) syscall(SYS_close_range, n, ~0U, 0) > # define WITH_CLOSEFROM > # endif > #endif > > thus just using the existing closefrom() impl (assuming you adjust the > previous patch to check return status for ENOSYS and do legacy fallback. > > Not convinced it is worth worrying about the FD gaps ? Changing my mind on this. I learnt that FreeBSD added support for close_range() to match Linux, since it was considered more flexible than their previous closefrom(). This got into FreeBSD in Apr 2020, and IIUC they backported it to 12.2, though worst case it is in 12.3 Given our supported platforms matrix I think we can assume any FreeBSD we target has close_range. IOW, we don't need to support closefrom() on FreeBSD any more. Thus I'd suggest we exclusively use close_range in livirt for both Linux and FreeBSD in the fast path, henceforth. With 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 :|
