6:25 a.m.
On 6/19/23 17:02, Daniel P. Berrangé wrote:
On Mon, Jun 19, 2023 at 02:45:57PM +0200, Michal Prívozník wrote:
> On 6/19/23 14:31, Daniel P. Berrangé wrote:
>> On Mon, Jun 19, 2023 at 02:03:48PM +0200, Michal Privoznik wrote:
>>> It's weird that in 2023 there's no reliable and portable way to
>>> parse strings, but okay.
>>>
>>> We started with strtol(). Except, it doesn't work the same on
>>> Linux and Windows. On Windows it behaves a bit different when it
>>> comes to parsing strings with base = 0. So we've switched to
>>> g_ascii_strtoll() which promised to solve this. And it did.
>>> Except, it introduced another problem. I don't really understand
>>> why, but I've seen random test failures and all of them claimed
>>> inability to parse a number (specifically <memory/> from the
>>> hard coded domain XML from test driver, which IMO is just a
>>> coincidence because it's the first number we parse).
>>
>> What platforms are you seeing the failures on ? All platforms or just
>> on Windows, or just some other specific one?
>
> I've tried only Linux so far. Windows is out of question.
snip
>> On the Linux case get_C_locale is
>>
>> static locale_t
>> get_C_locale (void)
>> {
>> static gsize initialized = FALSE;
>> static locale_t C_locale = NULL;
>>
>> if (g_once_init_enter (&initialized))
>> {
>> C_locale = newlocale (LC_ALL_MASK, "C", NULL);
>> g_once_init_leave (&initialized, TRUE);
>> }
>>
>> return C_locale;
>> }
>>
>> and only way that could fail is if newlocale isn't threadsafe, and
>> we have other code in libvirt calling newlocale at exact same time
>> as the *first* call to get_C_locale.
>
> Yeah, that's why I don't understand what's going on. Anyway, try running
> tests repeatedly (you can use oneliner from the commit message) and
> you'll run into the problem.
Yes, I've hit the problem with the error message
error: XML error: Invalid value '8388608' for element or attribute
'./memory[1]'
interestingly, I've also hit a few other error messages, some failures
without error message (I suspect virsh simply crashed with no output),
and most intriguing is that it 'virshtest' simply hangs when I run it
directly.
<snip/>
So the cause of the hang is exceedingly obvious.
Very few glib APIs are safe to uses in between fork & exec, and we are
(yet again) being burnt.
Yep, and switching to plain strtol() fixes this hang. But it does not
solve ...
What I can't explain, however, is why we sometimes get an error message
instead of a hang.
.. this. I bet it has something to do with fork() + exec() because when
I set up logging and run those tests I can see which one is failing
(with that "unable to parse NNN" message), but when I run it manually
with the exact arguments I don't see any hiccups.
If I modify 'virGlobalInit' to call g_ascii_strtoll("0", NULL, 10);
then
neither the hang or error messages occur, but the hang *should* still
occurr as the mutex locking race fundamentally stil exists. I think it
just changes the timing enough to avoid it in our case.
My only thought with the error messages is that somehow the 'locale_t'
is getting its initialization missed somehow.
> I've tried to debug the problem over weekend but was really
> unsuccessful. Firstly, I suspected that glib version of pthread_once()
> was broken. So I've rewritten get_C_locale() to use pthread_once() but
> that didn't help. Which tends to point into direction of glibc,
> supported by the fact that glibc impl of newlocale() does indeed have a
> global RW lock. But I haven't found anything obviously wrong there either.
If glib was actually using pthread_once() we wouldn't have a problem.
Their g_once_init_enter/leave stuff doesn't use pthread_once() and their
impl is not safe due to its mutex usage.
Anyway, I understand why your proposed change here is avoiding problems,
even if I don't understand the full root cause.
Ultimately I think our virExec() impl is verging on broken by design. We
have got too much functionality in it to be safe. Especially since we
switched from gnulib to glib, we're not so concious of what higher level
constructs we're accidentally relying on - eg no one would guess g_ascii_strtol
would acquire locks.
Rather than change our virstring.c impl, I'm inclined to think we need to
be more aggressive and eliminate as much use of glib as possible from the
virExec() and virFork() functions. Limit them to pure POSIX only.
Basically g_new/g_free are the only bits of glib I would entirely
trust in between fork/exec, without analyznig the code of other glib
APIs.
Specifically for this virStrToLong_i problem, I would suggest that we
just directly call strtol from virCommandMassCloseGetFDsLinux(), and
leave the main virStrToLong_i impl unchanged.
I've done this locally and it helped with deadlocks, but not with the
number parsing problem.
Second, we should modify the FreeBSD impl of virCommandMassClose so
that it works on Linux, when close_range() is available. That would
avoid calling the problem code in the first place for modern linux.
Yeah, I remember I wanted to do this when the syscall was introduced to
the Linux kernel, but for some reason didn't. BTW: glibc has closefrom()
which is just a wrapper over close_range(N, ~0U, 0); so we might get the
FreeBSD implementation for nothing.
Third, we should move virExec and virFork and related helpers into a
standalone file, so that it is very clear which bits of code are running
in between fork+exec and thus need careful design to avoid anything
except async safe POSIX.
I've tried to do this, but it's bigger task than I thought. Plenty of
dependencies across the file.
Michal