Re: [PATCH 2/2] virstring: Prefer strtoll_l() and strtoull_l() whenever possible

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.

> 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.