Re: [PATCH 3/4] vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, Aug 28, 2024 at 02:59:09PM +0200, Michal Prívozník wrote:
> On 8/28/24 14:39, Daniel P. Berrangé wrote:
> > On Wed, Aug 28, 2024 at 02:16:16PM +0200, Michal Privoznik wrote:
> > > The point of calling sysconf(_SC_OPEN_MAX) is to allocate big
> > > enough bitmap so that subsequent call to
> > > virCommandMassCloseGetFDsDir() can just set the bit instead of
> > > expanding memory (this code runs in a forked off child and thus
> > > using async-signal-unsafe functions like malloc() is a bit
> > > tricky).
> > >
> > > But on some systems the limit for opened FDs is virtually
> > > non-existent (typically macOS Ventura started reporting EINVAL).
> > >
> > > But with both glibc and musl using malloc() after fork() is safe.
> > > And with sufficiently new glib too, as it's using malloc() with
> > > newer releases instead of their own allocator.

I guess I'm out of the loop lately, could you point the possible readers
to the proof of the above?  I'm hesitant to say this is fine due to the
virBitmapSetBitExpand() call.

> > > Therefore, pick a sufficiently large value (glibc falls back to
> > > 256, [1], so 1024 should be good enough) to fall back to and make
> > > the error non-fatal.
> > >
> > > 1: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getdtsz.c;h=4c5a6208067d2f9eaaac6dba652702fb4af9b7e3;hb=HEAD
> > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > > ---
> > >  src/util/vircommand.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> > > index c0aab85c53..e48f361114 100644
> > > --- a/src/util/vircommand.c
> > > +++ b/src/util/vircommand.c
> > > @@ -493,7 +493,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds,
> > >              return -1;
> > >          }
> > >
> > > -        ignore_value(virBitmapSetBit(fds, fd));
> > > +        virBitmapSetBitExpand(fds, fd);
> > >      }
> > >
> > >      if (rc < 0)
> > > @@ -537,10 +537,8 @@ virCommandMassCloseFrom(virCommand *cmd,
> > >       * Therefore we can safely allocate memory here (and transitively call
> > >       * opendir/readdir) without a deadlock. */
> > >
> > > t -    if (openmax < 0) {
> > > -        virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed"));
> > > -        return -1;
> > > -    }
> > > +    if (openmax <= 0)
> > > +        openmax = 1024;
> >
> > Darwin has a OPEN_MAX constant that is x10 bigger than this:
> >
> >   https://github.com/apple/darwin-xnu/blob/main/bsd/sys/syslimits.h#L104
> >
> > IMHO we should be using that instead
>
> Fair enough. virBitmapSetBitExpand() would cause realloc of the bitmap,
> but we can start with a generous value. Consider the following squashed
> in:

And don't forget to adjust the commit message to go with the below
change as well.

> diff --git i/src/util/vircommand.c w/src/util/vircommand.c
> index e48f361114..0f2f87c80c 100644
> --- i/src/util/vircommand.c
> +++ w/src/util/vircommand.c
> @@ -537,8 +537,12 @@ virCommandMassCloseFrom(virCommand *cmd,
>      * Therefore we can safely allocate memory here (and transitively call
>      * opendir/readdir) without a deadlock. */
>
> -    if (openmax <= 0)
> -        openmax = 1024;
> +    if (openmax <= 0) {
> +        /* Darwin defaults to 10240. Start with a generous value.
> +         * virCommandMassCloseGetFDsDir() uses virBitmapSetBitExpand() anyways.
> +         */
> +        openmax = 10240;
> +    }
>
>     fds = virBitmapNew(openmax);
>
>
> Michal

Attachment: signature.asc
Description: PGP signature