* [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS
@ 2022-07-05 21:46 Peter Delevoryas
2022-07-06 8:02 ` Daniel P. Berrangé
0 siblings, 1 reply; 9+ messages in thread
From: Peter Delevoryas @ 2022-07-05 21:46 UTC (permalink / raw)
Cc: Peter Delevoryas, John Snow, Cleber Rosa, Beraldo Leal, qemu-devel
I noticed that I can't run any avocado tests on macOS because the QMP
unix socket path is too long:
$ ./configure --target-list=arm-softmmu
$ make
$ make check-avocado AVOCADO_TESTS=tests/avocado/boot_linux_console.py
changing dir to build for /Library/Developer/CommandLineTools/usr/bin/make "check-avocado"...
GIT ui/keycodemapdb meson tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc slirp
AVOCADO tests/avocado
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_emcraft_sf2
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_emcraft_sf2
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_raspi2_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_exynos4210_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_exynos4210_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_sata
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_cubieboard_sata
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_quanta_gsj_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_initrd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_ast2600_debian
JOB ID : 84bd26125345c6f12c92d118e98acde6aacfea57
JOB LOG : /Users/pdel/qemu/build/tests/results/job-2022-07-02T11.24-84bd261/job.log
(01/16) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_virt: ERROR: ConnectError: Failed to establish connection: AF_UNIX path too lo
ng\n Command: \n Output: None\n (0.11 s)
Interrupting job (failfast).
RESULTS : PASS 0 | ERROR 1 | FAIL 0 | SKIP 15 | WARN 0 | INTERRUPT 0 | CANCEL 0
The job log shows a backtrace that points to the QMP monitor's unix socket path:
2022-07-02 11:24:05,906 protocol L0554 DEBUG| Awaiting connection on /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock ...
2022-07-02 11:24:05,907 protocol L0426 ERROR| Failed to establish connection: OSError: AF_UNIX path too long
I think the path limit for unix sockets on macOS might be 104 [1]
/*
* [XSI] Definitions for UNIX IPC domain.
*/
struct sockaddr_un {
unsigned char sun_len; /* sockaddr len including null */
sa_family_t sun_family; /* [XSI] AF_UNIX */
char sun_path[104]; /* [XSI] path name (gag) */
};
The path we're using is exactly 105 characters:
$ python
Python 2.7.10 (default, Jan 19 2016, 22:24:01)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock')
105
In this change I removed some of the unnecessary elements of the path, to keep
us under the limit. This seemed like the simplest thing to do, and I imagine
the extra human-readable elements were just added to make it easier to identify
left-over sockets or something else related to debugging.
[1]: /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h
Signed-off-by: Peter Delevoryas <peter@pjd.dev>
---
python/qemu/machine/machine.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 37191f433b..93451774e3 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -157,7 +157,7 @@ def __init__(self,
self._wrapper = wrapper
self._qmp_timer = qmp_timer
- self._name = name or f"qemu-{os.getpid()}-{id(self):02x}"
+ self._name = name or f"{os.getpid()}{id(self):02x}"
self._temp_dir: Optional[str] = None
self._base_temp_dir = base_temp_dir
self._sock_dir = sock_dir
@@ -167,7 +167,7 @@ def __init__(self,
self._monitor_address = monitor_address
else:
self._monitor_address = os.path.join(
- self.sock_dir, f"{self._name}-monitor.sock"
+ self.sock_dir, f"{self._name}.sock"
)
self._console_log_path = console_log
--
2.37.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-05 21:46 [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas @ 2022-07-06 8:02 ` Daniel P. Berrangé 2022-07-06 16:46 ` Peter Delevoryas 0 siblings, 1 reply; 9+ messages in thread From: Daniel P. Berrangé @ 2022-07-06 8:02 UTC (permalink / raw) To: Peter Delevoryas; +Cc: John Snow, Cleber Rosa, Beraldo Leal, qemu-devel On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > I noticed that I can't run any avocado tests on macOS because the QMP > unix socket path is too long: > I think the path limit for unix sockets on macOS might be 104 [1] All platforms have a very limited path limit, so it isn't really a macOS specific problem, rather.... > > /* > * [XSI] Definitions for UNIX IPC domain. > */ > struct sockaddr_un { > unsigned char sun_len; /* sockaddr len including null */ > sa_family_t sun_family; /* [XSI] AF_UNIX */ > char sun_path[104]; /* [XSI] path name (gag) */ > }; > > The path we're using is exactly 105 characters: > > $ python > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > Type "help", "copyright", "credits" or "license" for more information. > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') It is a problem related to where the test suite is creating the paths. /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ is way too deep a directory location. It seems we just create this location using 'tempfile.TemporyDirectory' to get a standard tmp dir. Do you know why python is choosing /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ as the temp dir ? Is that a standard location on macOS or is it from some env variable you have set ? > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > index 37191f433b..93451774e3 100644 > --- a/python/qemu/machine/machine.py > +++ b/python/qemu/machine/machine.py > @@ -157,7 +157,7 @@ def __init__(self, > self._wrapper = wrapper > self._qmp_timer = qmp_timer > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > + self._name = name or f"{os.getpid()}{id(self):02x}" I don't think this is the right fix really, because IMHO the problem is the hugely long path, rather than the final socket name. That said, there is redundancy in the path - avocado is passing in a dierctory created using 'tempfile.TemporyDirectory' so there is no reason why we need to add more entropy via the POD and the 'id(self)' hex string. IMHO avocado should pass in the 'name' parameter explicitly, using a plain name and thus get a shorter string. > self._temp_dir: Optional[str] = None > self._base_temp_dir = base_temp_dir > self._sock_dir = sock_dir > @@ -167,7 +167,7 @@ def __init__(self, > self._monitor_address = monitor_address > else: > self._monitor_address = os.path.join( > - self.sock_dir, f"{self._name}-monitor.sock" > + self.sock_dir, f"{self._name}.sock" > ) > > self._console_log_path = console_log > -- > 2.37.0 > > 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 :| ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-06 8:02 ` Daniel P. Berrangé @ 2022-07-06 16:46 ` Peter Delevoryas 2022-07-07 0:52 ` Peter Delevoryas 0 siblings, 1 reply; 9+ messages in thread From: Peter Delevoryas @ 2022-07-06 16:46 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: John Snow, Cleber Rosa, Beraldo Leal, qemu-devel On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > I noticed that I can't run any avocado tests on macOS because the QMP > > unix socket path is too long: > > > > I think the path limit for unix sockets on macOS might be 104 [1] > > All platforms have a very limited path limit, so it isn't really > a macOS specific problem, rather.... > > > > > /* > > * [XSI] Definitions for UNIX IPC domain. > > */ > > struct sockaddr_un { > > unsigned char sun_len; /* sockaddr len including null */ > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > char sun_path[104]; /* [XSI] path name (gag) */ > > }; > > > > The path we're using is exactly 105 characters: > > > > $ python > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > > Type "help", "copyright", "credits" or "license" for more information. > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > It is a problem related to where the test suite is creating the > paths. > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > is way too deep a directory location. That's a good point. > > It seems we just create this location using 'tempfile.TemporyDirectory' > to get a standard tmp dir. > > Do you know why python is choosing > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > as the temp dir ? Is that a standard location on macOS or is it > from some env variable you have set ? Hmmm yeah it is odd, I'm not really sure why it's created there or if standard glibc tmpfile creation goes there too, I'll go experiment and report back. And yeah, maybe I'll double check any environment variables or other things. The macOS system I use happens to be a Facebook work laptop, which could also be related now that I think about it. > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > index 37191f433b..93451774e3 100644 > > --- a/python/qemu/machine/machine.py > > +++ b/python/qemu/machine/machine.py > > @@ -157,7 +157,7 @@ def __init__(self, > > self._wrapper = wrapper > > self._qmp_timer = qmp_timer > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > I don't think this is the right fix really, because IMHO the problem > is the hugely long path, rather than the final socket name. True, yeah let me try to investigate the directory placement. > > That said, there is redundancy in the path - avocado is passing in > a dierctory created using 'tempfile.TemporyDirectory' so there is no > reason why we need to add more entropy via the POD and the 'id(self)' > hex string. Oh good point, I hadn't thought about that. > > IMHO avocado should pass in the 'name' parameter explicitly, using a > plain name and thus get a shorter string. I see, yeah that makes sense. Maybe I can include a couple patches for this, one fixing the directory location, and one refactoring the temporary file name template (If I'm understanding your suggestion correctly). Thanks for the review! I really appreciate it. Peter > > > self._temp_dir: Optional[str] = None > > self._base_temp_dir = base_temp_dir > > self._sock_dir = sock_dir > > @@ -167,7 +167,7 @@ def __init__(self, > > self._monitor_address = monitor_address > > else: > > self._monitor_address = os.path.join( > > - self.sock_dir, f"{self._name}-monitor.sock" > > + self.sock_dir, f"{self._name}.sock" > > ) > > > > self._console_log_path = console_log > > -- > > 2.37.0 > > > > > > 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 :| > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-06 16:46 ` Peter Delevoryas @ 2022-07-07 0:52 ` Peter Delevoryas 2022-07-07 18:46 ` Peter Delevoryas 0 siblings, 1 reply; 9+ messages in thread From: Peter Delevoryas @ 2022-07-07 0:52 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: John Snow, Cleber Rosa, Beraldo Leal, qemu-devel On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > I noticed that I can't run any avocado tests on macOS because the QMP > > > unix socket path is too long: > > > > > > > I think the path limit for unix sockets on macOS might be 104 [1] > > > > All platforms have a very limited path limit, so it isn't really > > a macOS specific problem, rather.... > > > > > > > > /* > > > * [XSI] Definitions for UNIX IPC domain. > > > */ > > > struct sockaddr_un { > > > unsigned char sun_len; /* sockaddr len including null */ > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > }; > > > > > > The path we're using is exactly 105 characters: > > > > > > $ python > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > > > Type "help", "copyright", "credits" or "license" for more information. > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > It is a problem related to where the test suite is creating the > > paths. > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > is way too deep a directory location. > > That's a good point. > > > > > It seems we just create this location using 'tempfile.TemporyDirectory' > > to get a standard tmp dir. > > > > Do you know why python is choosing > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > as the temp dir ? Is that a standard location on macOS or is it > > from some env variable you have set ? > > Hmmm yeah it is odd, I'm not really sure why it's created there or if > standard glibc tmpfile creation goes there too, I'll go experiment and > report back. And yeah, maybe I'll double check any environment variables or > other things. > > The macOS system I use happens to be a Facebook work laptop, which could > also be related now that I think about it. Hmmm yeah looks like this is because my TMPDIR is weird. $ echo $TMPDIR /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably something wrong with the Facebook development environment. Peter > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > index 37191f433b..93451774e3 100644 > > > --- a/python/qemu/machine/machine.py > > > +++ b/python/qemu/machine/machine.py > > > @@ -157,7 +157,7 @@ def __init__(self, > > > self._wrapper = wrapper > > > self._qmp_timer = qmp_timer > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > I don't think this is the right fix really, because IMHO the problem > > is the hugely long path, rather than the final socket name. > > True, yeah let me try to investigate the directory placement. > > > > > That said, there is redundancy in the path - avocado is passing in > > a dierctory created using 'tempfile.TemporyDirectory' so there is no > > reason why we need to add more entropy via the POD and the 'id(self)' > > hex string. > > Oh good point, I hadn't thought about that. > > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a > > plain name and thus get a shorter string. > > I see, yeah that makes sense. Maybe I can include a couple patches for this, > one fixing the directory location, and one refactoring the temporary file > name template (If I'm understanding your suggestion correctly). > > Thanks for the review! I really appreciate it. > Peter > > > > > > self._temp_dir: Optional[str] = None > > > self._base_temp_dir = base_temp_dir > > > self._sock_dir = sock_dir > > > @@ -167,7 +167,7 @@ def __init__(self, > > > self._monitor_address = monitor_address > > > else: > > > self._monitor_address = os.path.join( > > > - self.sock_dir, f"{self._name}-monitor.sock" > > > + self.sock_dir, f"{self._name}.sock" > > > ) > > > > > > self._console_log_path = console_log > > > -- > > > 2.37.0 > > > > > > > > > > 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 :| > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-07 0:52 ` Peter Delevoryas @ 2022-07-07 18:46 ` Peter Delevoryas 2022-07-11 20:56 ` John Snow 0 siblings, 1 reply; 9+ messages in thread From: Peter Delevoryas @ 2022-07-07 18:46 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: John Snow, Cleber Rosa, Beraldo Leal, qemu-devel On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote: > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > > I noticed that I can't run any avocado tests on macOS because the QMP > > > > unix socket path is too long: > > > > > > > > > > I think the path limit for unix sockets on macOS might be 104 [1] > > > > > > All platforms have a very limited path limit, so it isn't really > > > a macOS specific problem, rather.... > > > > > > > > > > > /* > > > > * [XSI] Definitions for UNIX IPC domain. > > > > */ > > > > struct sockaddr_un { > > > > unsigned char sun_len; /* sockaddr len including null */ > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > }; > > > > > > > > The path we're using is exactly 105 characters: > > > > > > > > $ python > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > > > > Type "help", "copyright", "credits" or "license" for more information. > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > > > It is a problem related to where the test suite is creating the > > > paths. > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > > > is way too deep a directory location. > > > > That's a good point. > > > > > > > > It seems we just create this location using 'tempfile.TemporyDirectory' > > > to get a standard tmp dir. > > > > > > Do you know why python is choosing > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > as the temp dir ? Is that a standard location on macOS or is it > > > from some env variable you have set ? > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or if > > standard glibc tmpfile creation goes there too, I'll go experiment and > > report back. And yeah, maybe I'll double check any environment variables or > > other things. > > > > The macOS system I use happens to be a Facebook work laptop, which could > > also be related now that I think about it. > > Hmmm yeah looks like this is because my TMPDIR is weird. > > $ echo $TMPDIR > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably > something wrong with the Facebook development environment. > > Peter Update: Actually, this might not be a Facebook-work-laptop specific thing. I asked my non-engineer friend to print out $TMPDIR on his macbook and he got the same thing. https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path I guess this person suggests it's just to separate the permissions for each user's /tmp directory, for better isolation. I'll resubmit this patch with the suggestions you had, because perhaps this is actually affecting other macOS users too. > > > > > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > > index 37191f433b..93451774e3 100644 > > > > --- a/python/qemu/machine/machine.py > > > > +++ b/python/qemu/machine/machine.py > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > self._wrapper = wrapper > > > > self._qmp_timer = qmp_timer > > > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > > > I don't think this is the right fix really, because IMHO the problem > > > is the hugely long path, rather than the final socket name. > > > > True, yeah let me try to investigate the directory placement. > > > > > > > > That said, there is redundancy in the path - avocado is passing in > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no > > > reason why we need to add more entropy via the POD and the 'id(self)' > > > hex string. > > > > Oh good point, I hadn't thought about that. > > > > > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a > > > plain name and thus get a shorter string. > > > > I see, yeah that makes sense. Maybe I can include a couple patches for this, > > one fixing the directory location, and one refactoring the temporary file > > name template (If I'm understanding your suggestion correctly). > > > > Thanks for the review! I really appreciate it. > > Peter > > > > > > > > > self._temp_dir: Optional[str] = None > > > > self._base_temp_dir = base_temp_dir > > > > self._sock_dir = sock_dir > > > > @@ -167,7 +167,7 @@ def __init__(self, > > > > self._monitor_address = monitor_address > > > > else: > > > > self._monitor_address = os.path.join( > > > > - self.sock_dir, f"{self._name}-monitor.sock" > > > > + self.sock_dir, f"{self._name}.sock" > > > > ) > > > > > > > > self._console_log_path = console_log > > > > -- > > > > 2.37.0 > > > > > > > > > > > > > > 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 :| > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-07 18:46 ` Peter Delevoryas @ 2022-07-11 20:56 ` John Snow 2022-07-12 1:46 ` Peter Delevoryas 2022-07-12 8:16 ` Daniel P. Berrangé 0 siblings, 2 replies; 9+ messages in thread From: John Snow @ 2022-07-11 20:56 UTC (permalink / raw) To: peter; +Cc: Daniel P. Berrangé, Cleber Rosa, Beraldo Leal, qemu-devel On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote: > > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote: > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > > > I noticed that I can't run any avocado tests on macOS because the QMP > > > > > unix socket path is too long: > > > > > > > > > > > > > I think the path limit for unix sockets on macOS might be 104 [1] > > > > > > > > All platforms have a very limited path limit, so it isn't really > > > > a macOS specific problem, rather.... > > > > > > > > > > > > > > /* > > > > > * [XSI] Definitions for UNIX IPC domain. > > > > > */ > > > > > struct sockaddr_un { > > > > > unsigned char sun_len; /* sockaddr len including null */ > > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > > }; > > > > > > > > > > The path we're using is exactly 105 characters: > > > > > > > > > > $ python > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > > > > > Type "help", "copyright", "credits" or "license" for more information. > > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > > > > > It is a problem related to where the test suite is creating the > > > > paths. > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > > > > > is way too deep a directory location. > > > > > > That's a good point. > > > > > > > > > > > It seems we just create this location using 'tempfile.TemporyDirectory' > > > > to get a standard tmp dir. > > > > > > > > Do you know why python is choosing > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > as the temp dir ? Is that a standard location on macOS or is it > > > > from some env variable you have set ? > > > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or if > > > standard glibc tmpfile creation goes there too, I'll go experiment and > > > report back. And yeah, maybe I'll double check any environment variables or > > > other things. > > > > > > The macOS system I use happens to be a Facebook work laptop, which could > > > also be related now that I think about it. > > > > Hmmm yeah looks like this is because my TMPDIR is weird. > > > > $ echo $TMPDIR > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 > > > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably > > something wrong with the Facebook development environment. > > > > Peter > > Update: Actually, this might not be a Facebook-work-laptop specific > thing. I asked my non-engineer friend to print out $TMPDIR on his > macbook and he got the same thing. > > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > I guess this person suggests it's just to separate the permissions for > each user's /tmp directory, for better isolation. > > I'll resubmit this patch with the suggestions you had, because perhaps > this is actually affecting other macOS users too. > > > > > > > > > > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > > > index 37191f433b..93451774e3 100644 > > > > > --- a/python/qemu/machine/machine.py > > > > > +++ b/python/qemu/machine/machine.py > > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > > self._wrapper = wrapper > > > > > self._qmp_timer = qmp_timer > > > > > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > > > > > I don't think this is the right fix really, because IMHO the problem > > > > is the hugely long path, rather than the final socket name. > > > > > > True, yeah let me try to investigate the directory placement. > > > > > > > > > > > That said, there is redundancy in the path - avocado is passing in > > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no > > > > reason why we need to add more entropy via the POD and the 'id(self)' > > > > hex string. > > > > > > Oh good point, I hadn't thought about that. > > > > > > > > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a > > > > plain name and thus get a shorter string. > > > > > > I see, yeah that makes sense. Maybe I can include a couple patches for this, > > > one fixing the directory location, and one refactoring the temporary file > > > name template (If I'm understanding your suggestion correctly). > > > > > > Thanks for the review! I really appreciate it. > > > Peter I agree with Dan: I believe the correct solution here is for Avocado to provide its own less redundant name; but the default name that machine.py provides is not *that* long and provides adequate protection against collisions with multiple instances of the VM utility within a single python process. If Avocado is creating its own directories that guard against that redundancy, Avocado should provide a shortened name for the VM. Note that the QEMUMachine process also provides a sock_dir parameter that was introduced for precisely this reason; it should be possible to instruct the avocado tests to use a shorter path for sock_dir. I'm not clear on what the best "just works" solution will be when certain operating environments choose a tmp dir that's quite long to begin with; maybe we need a different default sockfile naming strategy that avoids the instance collision problem in machine.py, too. Ideas? --js > > > > > > > > > > > > self._temp_dir: Optional[str] = None > > > > > self._base_temp_dir = base_temp_dir > > > > > self._sock_dir = sock_dir > > > > > @@ -167,7 +167,7 @@ def __init__(self, > > > > > self._monitor_address = monitor_address > > > > > else: > > > > > self._monitor_address = os.path.join( > > > > > - self.sock_dir, f"{self._name}-monitor.sock" > > > > > + self.sock_dir, f"{self._name}.sock" > > > > > ) > > > > > > > > > > self._console_log_path = console_log > > > > > -- > > > > > 2.37.0 > > > > > > > > > > > > > > > > > > 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 :| > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-11 20:56 ` John Snow @ 2022-07-12 1:46 ` Peter Delevoryas 2022-07-12 15:14 ` John Snow 2022-07-12 8:16 ` Daniel P. Berrangé 1 sibling, 1 reply; 9+ messages in thread From: Peter Delevoryas @ 2022-07-12 1:46 UTC (permalink / raw) To: John Snow; +Cc: Daniel P. Berrangé, Cleber Rosa, Beraldo Leal, qemu-devel On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote: > On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote: > > > > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote: > > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > > > > I noticed that I can't run any avocado tests on macOS because the QMP > > > > > > unix socket path is too long: > > > > > > > > > > > > > > > > I think the path limit for unix sockets on macOS might be 104 [1] > > > > > > > > > > All platforms have a very limited path limit, so it isn't really > > > > > a macOS specific problem, rather.... > > > > > > > > > > > > > > > > > /* > > > > > > * [XSI] Definitions for UNIX IPC domain. > > > > > > */ > > > > > > struct sockaddr_un { > > > > > > unsigned char sun_len; /* sockaddr len including null */ > > > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > > > }; > > > > > > > > > > > > The path we're using is exactly 105 characters: > > > > > > > > > > > > $ python > > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > > > > > > Type "help", "copyright", "credits" or "license" for more information. > > > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > > > > > > > It is a problem related to where the test suite is creating the > > > > > paths. > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > > > > > > > is way too deep a directory location. > > > > > > > > That's a good point. > > > > > > > > > > > > > > It seems we just create this location using 'tempfile.TemporyDirectory' > > > > > to get a standard tmp dir. > > > > > > > > > > Do you know why python is choosing > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > > > as the temp dir ? Is that a standard location on macOS or is it > > > > > from some env variable you have set ? > > > > > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or if > > > > standard glibc tmpfile creation goes there too, I'll go experiment and > > > > report back. And yeah, maybe I'll double check any environment variables or > > > > other things. > > > > > > > > The macOS system I use happens to be a Facebook work laptop, which could > > > > also be related now that I think about it. > > > > > > Hmmm yeah looks like this is because my TMPDIR is weird. > > > > > > $ echo $TMPDIR > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 > > > > > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably > > > something wrong with the Facebook development environment. > > > > > > Peter > > > > Update: Actually, this might not be a Facebook-work-laptop specific > > thing. I asked my non-engineer friend to print out $TMPDIR on his > > macbook and he got the same thing. > > > > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > > > I guess this person suggests it's just to separate the permissions for > > each user's /tmp directory, for better isolation. > > > > I'll resubmit this patch with the suggestions you had, because perhaps > > this is actually affecting other macOS users too. > > > > > > > > > > > > > > > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > > > > index 37191f433b..93451774e3 100644 > > > > > > --- a/python/qemu/machine/machine.py > > > > > > +++ b/python/qemu/machine/machine.py > > > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > > > self._wrapper = wrapper > > > > > > self._qmp_timer = qmp_timer > > > > > > > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > > > > > > > I don't think this is the right fix really, because IMHO the problem > > > > > is the hugely long path, rather than the final socket name. > > > > > > > > True, yeah let me try to investigate the directory placement. > > > > > > > > > > > > > > That said, there is redundancy in the path - avocado is passing in > > > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no > > > > > reason why we need to add more entropy via the POD and the 'id(self)' > > > > > hex string. > > > > > > > > Oh good point, I hadn't thought about that. > > > > > > > > > > > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a > > > > > plain name and thus get a shorter string. > > > > > > > > I see, yeah that makes sense. Maybe I can include a couple patches for this, > > > > one fixing the directory location, and one refactoring the temporary file > > > > name template (If I'm understanding your suggestion correctly). > > > > > > > > Thanks for the review! I really appreciate it. > > > > Peter > > I agree with Dan: I believe the correct solution here is for Avocado > to provide its own less redundant name; but the default name that > machine.py provides is not *that* long and provides adequate > protection against collisions with multiple instances of the VM > utility within a single python process. If Avocado is creating its own > directories that guard against that redundancy, Avocado should provide > a shortened name for the VM. Hmmm, I see. > > Note that the QEMUMachine process also provides a sock_dir parameter > that was introduced for precisely this reason; it should be possible > to instruct the avocado tests to use a shorter path for sock_dir. > > I'm not clear on what the best "just works" solution will be when > certain operating environments choose a tmp dir that's quite long to > begin with; maybe we need a different default sockfile naming strategy > that avoids the instance collision problem in machine.py, too. Ideas? I guess I don't really understand why QEMU is attempting to create a unique path instead of just using mktemp(), right? This seems like something that the OS can provide. If we want some semblance of meaning to the filename, we can provide a short template, right? -- Peter > > --js > > > > > > > > > > > > > > > > self._temp_dir: Optional[str] = None > > > > > > self._base_temp_dir = base_temp_dir > > > > > > self._sock_dir = sock_dir > > > > > > @@ -167,7 +167,7 @@ def __init__(self, > > > > > > self._monitor_address = monitor_address > > > > > > else: > > > > > > self._monitor_address = os.path.join( > > > > > > - self.sock_dir, f"{self._name}-monitor.sock" > > > > > > + self.sock_dir, f"{self._name}.sock" > > > > > > ) > > > > > > > > > > > > self._console_log_path = console_log > > > > > > -- > > > > > > 2.37.0 > > > > > > > > > > > > > > > > > > > > > > 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 :| > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-12 1:46 ` Peter Delevoryas @ 2022-07-12 15:14 ` John Snow 0 siblings, 0 replies; 9+ messages in thread From: John Snow @ 2022-07-12 15:14 UTC (permalink / raw) To: Peter Delevoryas Cc: Daniel P. Berrangé, Cleber Rosa, Beraldo Leal, qemu-devel [-- Attachment #1: Type: text/plain, Size: 8812 bytes --] On Mon, Jul 11, 2022, 9:46 PM Peter Delevoryas <peter@pjd.dev> wrote: > On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote: > > On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote: > > > > > > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote: > > > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > > > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > > > > > I noticed that I can't run any avocado tests on macOS because > the QMP > > > > > > > unix socket path is too long: > > > > > > > > > > > > > > > > > > > I think the path limit for unix sockets on macOS might be 104 > [1] > > > > > > > > > > > > All platforms have a very limited path limit, so it isn't really > > > > > > a macOS specific problem, rather.... > > > > > > > > > > > > > > > > > > > > /* > > > > > > > * [XSI] Definitions for UNIX IPC domain. > > > > > > > */ > > > > > > > struct sockaddr_un { > > > > > > > unsigned char sun_len; /* sockaddr len including > null */ > > > > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > > > > }; > > > > > > > > > > > > > > The path we're using is exactly 105 characters: > > > > > > > > > > > > > > $ python > > > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on > darwin > > > > > > > Type "help", "copyright", "credits" or "license" for more > information. > > > > > > > >>> > len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > > > > > > > > > It is a problem related to where the test suite is creating the > > > > > > paths. > > > > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > > > > > > > > > is way too deep a directory location. > > > > > > > > > > That's a good point. > > > > > > > > > > > > > > > > > It seems we just create this location using > 'tempfile.TemporyDirectory' > > > > > > to get a standard tmp dir. > > > > > > > > > > > > Do you know why python is choosing > > > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > > > > > as the temp dir ? Is that a standard location on macOS or is it > > > > > > from some env variable you have set ? > > > > > > > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or > if > > > > > standard glibc tmpfile creation goes there too, I'll go experiment > and > > > > > report back. And yeah, maybe I'll double check any environment > variables or > > > > > other things. > > > > > > > > > > The macOS system I use happens to be a Facebook work laptop, which > could > > > > > also be related now that I think about it. > > > > > > > > Hmmm yeah looks like this is because my TMPDIR is weird. > > > > > > > > $ echo $TMPDIR > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 > > > > > > > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's > probably > > > > something wrong with the Facebook development environment. > > > > > > > > Peter > > > > > > Update: Actually, this might not be a Facebook-work-laptop specific > > > thing. I asked my non-engineer friend to print out $TMPDIR on his > > > macbook and he got the same thing. > > > > > > > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > > > > > I guess this person suggests it's just to separate the permissions for > > > each user's /tmp directory, for better isolation. > > > > > > I'll resubmit this patch with the suggestions you had, because perhaps > > > this is actually affecting other macOS users too. > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/python/qemu/machine/machine.py > b/python/qemu/machine/machine.py > > > > > > > index 37191f433b..93451774e3 100644 > > > > > > > --- a/python/qemu/machine/machine.py > > > > > > > +++ b/python/qemu/machine/machine.py > > > > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > > > > self._wrapper = wrapper > > > > > > > self._qmp_timer = qmp_timer > > > > > > > > > > > > > > - self._name = name or > f"qemu-{os.getpid()}-{id(self):02x}" > > > > > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > > > > > > > > > I don't think this is the right fix really, because IMHO the > problem > > > > > > is the hugely long path, rather than the final socket name. > > > > > > > > > > True, yeah let me try to investigate the directory placement. > > > > > > > > > > > > > > > > > That said, there is redundancy in the path - avocado is passing > in > > > > > > a dierctory created using 'tempfile.TemporyDirectory' so there > is no > > > > > > reason why we need to add more entropy via the POD and the > 'id(self)' > > > > > > hex string. > > > > > > > > > > Oh good point, I hadn't thought about that. > > > > > > > > > > > > > > > > > IMHO avocado should pass in the 'name' parameter explicitly, > using a > > > > > > plain name and thus get a shorter string. > > > > > > > > > > I see, yeah that makes sense. Maybe I can include a couple patches > for this, > > > > > one fixing the directory location, and one refactoring the > temporary file > > > > > name template (If I'm understanding your suggestion correctly). > > > > > > > > > > Thanks for the review! I really appreciate it. > > > > > Peter > > > > I agree with Dan: I believe the correct solution here is for Avocado > > to provide its own less redundant name; but the default name that > > machine.py provides is not *that* long and provides adequate > > protection against collisions with multiple instances of the VM > > utility within a single python process. If Avocado is creating its own > > directories that guard against that redundancy, Avocado should provide > > a shortened name for the VM. > > Hmmm, I see. > > > > > Note that the QEMUMachine process also provides a sock_dir parameter > > that was introduced for precisely this reason; it should be possible > > to instruct the avocado tests to use a shorter path for sock_dir. > > > > I'm not clear on what the best "just works" solution will be when > > certain operating environments choose a tmp dir that's quite long to > > begin with; maybe we need a different default sockfile naming strategy > > that avoids the instance collision problem in machine.py, too. Ideas? > > I guess I don't really understand why QEMU is attempting to create a > unique path instead of just using mktemp(), right? This seems like > something that the OS can provide. If we want some semblance of meaning > to the filename, we can provide a short template, right? > Yeah, should be good. The "name" of the VM should be left unique for logging purposes, so the name should remain as-is, IMO. However, if you want to change how we create the sockfile by using a new, temporary name (and not the vm name), that works for me as far as I can remember. I think there was some desire to have the temp file obviously correlate to the instance, but that's not a hard requirement, so feel free to change that part. Just make sure that come hell or high water the sockfile is cleaned up on exit. > -- Peter > > > > > > --js > > > > > > > > > > > > > > > > > > > > self._temp_dir: Optional[str] = None > > > > > > > self._base_temp_dir = base_temp_dir > > > > > > > self._sock_dir = sock_dir > > > > > > > @@ -167,7 +167,7 @@ def __init__(self, > > > > > > > self._monitor_address = monitor_address > > > > > > > else: > > > > > > > self._monitor_address = os.path.join( > > > > > > > - self.sock_dir, f"{self._name}-monitor.sock" > > > > > > > + self.sock_dir, f"{self._name}.sock" > > > > > > > ) > > > > > > > > > > > > > > self._console_log_path = console_log > > > > > > > -- > > > > > > > 2.37.0 > > > > > > > > > > > > > > > > > > > > > > > > > > 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 :| > > > > > > > > > > > > > > > > > > > > > > [-- Attachment #2: Type: text/html, Size: 13039 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS 2022-07-11 20:56 ` John Snow 2022-07-12 1:46 ` Peter Delevoryas @ 2022-07-12 8:16 ` Daniel P. Berrangé 1 sibling, 0 replies; 9+ messages in thread From: Daniel P. Berrangé @ 2022-07-12 8:16 UTC (permalink / raw) To: John Snow; +Cc: peter, Cleber Rosa, Beraldo Leal, qemu-devel On Mon, Jul 11, 2022 at 04:56:28PM -0400, John Snow wrote: > On Thu, Jul 7, 2022 at 2:46 PM Peter Delevoryas <peter@pjd.dev> wrote: > > > > On Wed, Jul 06, 2022 at 05:52:48PM -0700, Peter Delevoryas wrote: > > > On Wed, Jul 06, 2022 at 09:46:48AM -0700, Peter Delevoryas wrote: > > > > On Wed, Jul 06, 2022 at 09:02:14AM +0100, Daniel P. Berrangé wrote: > > > > > On Tue, Jul 05, 2022 at 02:46:59PM -0700, Peter Delevoryas wrote: > > > > > > I noticed that I can't run any avocado tests on macOS because the QMP > > > > > > unix socket path is too long: > > > > > > > > > > > > > > > > I think the path limit for unix sockets on macOS might be 104 [1] > > > > > > > > > > All platforms have a very limited path limit, so it isn't really > > > > > a macOS specific problem, rather.... > > > > > > > > > > > > > > > > > /* > > > > > > * [XSI] Definitions for UNIX IPC domain. > > > > > > */ > > > > > > struct sockaddr_un { > > > > > > unsigned char sun_len; /* sockaddr len including null */ > > > > > > sa_family_t sun_family; /* [XSI] AF_UNIX */ > > > > > > char sun_path[104]; /* [XSI] path name (gag) */ > > > > > > }; > > > > > > > > > > > > The path we're using is exactly 105 characters: > > > > > > > > > > > > $ python > > > > > > Python 2.7.10 (default, Jan 19 2016, 22:24:01) > > > > > > [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin > > > > > > Type "help", "copyright", "credits" or "license" for more information. > > > > > > >>> len('/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock') > > > > > > > > > > It is a problem related to where the test suite is creating the > > > > > paths. > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/avo_qemu_sock_uh3w_dgc/ > > > > > > > > > > is way too deep a directory location. > > > > > > > > That's a good point. > > > > > > > > > > > > > > It seems we just create this location using 'tempfile.TemporyDirectory' > > > > > to get a standard tmp dir. > > > > > > > > > > Do you know why python is choosing > > > > > > > > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > > > > > as the temp dir ? Is that a standard location on macOS or is it > > > > > from some env variable you have set ? > > > > > > > > Hmmm yeah it is odd, I'm not really sure why it's created there or if > > > > standard glibc tmpfile creation goes there too, I'll go experiment and > > > > report back. And yeah, maybe I'll double check any environment variables or > > > > other things. > > > > > > > > The macOS system I use happens to be a Facebook work laptop, which could > > > > also be related now that I think about it. > > > > > > Hmmm yeah looks like this is because my TMPDIR is weird. > > > > > > $ echo $TMPDIR > > > /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/ > > > > > > I didn't think to check this cause I wasn't familiar with TMPDIR. 🤷 > > > > > > Thanks for responding, I'll just use TMPDIR=/tmp for now. It's probably > > > something wrong with the Facebook development environment. > > > > > > Peter > > > > Update: Actually, this might not be a Facebook-work-laptop specific > > thing. I asked my non-engineer friend to print out $TMPDIR on his > > macbook and he got the same thing. > > > > https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path > > > > I guess this person suggests it's just to separate the permissions for > > each user's /tmp directory, for better isolation. > > > > I'll resubmit this patch with the suggestions you had, because perhaps > > this is actually affecting other macOS users too. > > > > > > > > > > > > > > > > > > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py > > > > > > index 37191f433b..93451774e3 100644 > > > > > > --- a/python/qemu/machine/machine.py > > > > > > +++ b/python/qemu/machine/machine.py > > > > > > @@ -157,7 +157,7 @@ def __init__(self, > > > > > > self._wrapper = wrapper > > > > > > self._qmp_timer = qmp_timer > > > > > > > > > > > > - self._name = name or f"qemu-{os.getpid()}-{id(self):02x}" > > > > > > + self._name = name or f"{os.getpid()}{id(self):02x}" > > > > > > > > > > I don't think this is the right fix really, because IMHO the problem > > > > > is the hugely long path, rather than the final socket name. > > > > > > > > True, yeah let me try to investigate the directory placement. > > > > > > > > > > > > > > That said, there is redundancy in the path - avocado is passing in > > > > > a dierctory created using 'tempfile.TemporyDirectory' so there is no > > > > > reason why we need to add more entropy via the POD and the 'id(self)' > > > > > hex string. > > > > > > > > Oh good point, I hadn't thought about that. > > > > > > > > > > > > > > IMHO avocado should pass in the 'name' parameter explicitly, using a > > > > > plain name and thus get a shorter string. > > > > > > > > I see, yeah that makes sense. Maybe I can include a couple patches for this, > > > > one fixing the directory location, and one refactoring the temporary file > > > > name template (If I'm understanding your suggestion correctly). > > > > > > > > Thanks for the review! I really appreciate it. > > > > Peter > > I agree with Dan: I believe the correct solution here is for Avocado > to provide its own less redundant name; but the default name that > machine.py provides is not *that* long and provides adequate > protection against collisions with multiple instances of the VM > utility within a single python process. If Avocado is creating its own > directories that guard against that redundancy, Avocado should provide > a shortened name for the VM. > > Note that the QEMUMachine process also provides a sock_dir parameter > that was introduced for precisely this reason; it should be possible > to instruct the avocado tests to use a shorter path for sock_dir. > > I'm not clear on what the best "just works" solution will be when > certain operating environments choose a tmp dir that's quite long to > begin with; maybe we need a different default sockfile naming strategy > that avoids the instance collision problem in machine.py, too. Ideas? Ultimately there's no solution that is guaranteed to work. If $HOME or $TMPDIR point to paths that are >= 108 chars long, we're doomed no matter what. To maximise the liklihood of working though, we need to be minimal in paths we append. Right now, Peter's TMPDIR is /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T Onto which QEMU appends /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock IOW, QEMU is more than 1/2 of the problem here. QEMU could cut down the size of the temporary directory using a more concise pattern, changing tempfile.TemporaryDirectory(prefix="avo_qemu_sock_") To tempfile.TemporaryDirectory(prefix="qemu_") And then use a fixed socket name 'qmp.sock', giving us: /qemu_uh3w_dgc/qmp.sock to append to TMPDIR 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 :| ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-12 15:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-05 21:46 [PATCH RESEND] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas 2022-07-06 8:02 ` Daniel P. Berrangé 2022-07-06 16:46 ` Peter Delevoryas 2022-07-07 0:52 ` Peter Delevoryas 2022-07-07 18:46 ` Peter Delevoryas 2022-07-11 20:56 ` John Snow 2022-07-12 1:46 ` Peter Delevoryas 2022-07-12 15:14 ` John Snow 2022-07-12 8:16 ` Daniel P. Berrangé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.