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.
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@redhat.com>
---
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. */
- if (openmax < 0) {
- virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed"));
- return -1;
- }
+ if (openmax <= 0)
+ openmax = 1024;
fds = virBitmapNew(openmax);
--
2.44.2On 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.
>
> 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@redhat.com>
> ---
> 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
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 :|
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.
>>
>> 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@redhat.com>
>> ---
>> 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:
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
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@redhat.com>
>>> ---
>>> 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
>
On Thu, Sep 12, 2024 at 03:31:38PM +0200, Martin Kletzander wrote: > 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. Libvirt has relied on malloc-after-fork being safe, essentially, forever. For a while this caused us to have problems on musl, but musl maintainers eventually relented and made it safe too. glib is hardcoded to use system malloc since before our minimum glib version, and again we've relied on that ever since we adopted use of glib. So there's nothing new in what Michal writes above. 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 :|
On Thu, Sep 12, 2024 at 02:36:24PM +0100, Daniel P. Berrangé wrote: >On Thu, Sep 12, 2024 at 03:31:38PM +0200, Martin Kletzander wrote: >> 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. > >Libvirt has relied on malloc-after-fork being safe, essentially, >forever. For a while this caused us to have problems on musl, >but musl maintainers eventually relented and made it safe too. >glib is hardcoded to use system malloc since before our minimum >glib version, and again we've relied on that ever since we adopted >use of glib. > >So there's nothing new in what Michal writes above. > OK, I thought it might not be that obvious, but clearly I'm just out of as I said. So in that case, the fix incorporated and the commit message adjusted it's Reviewed-by: Martin Kletzander <mkletzan@redhat.com> > > >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 :| >
© 2016 - 2024 Red Hat, Inc.