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 <[email protected]>
>> ---
>> 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
