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