On Sat, Jul 16, 2022 at 10:34:34AM -0700, Peter Delevoryas wrote:
> On macOS, private $TMPDIR's are the default. These $TMPDIR's are
> generated from a user's unix UID and UUID [1], which can create a
> relatively long path:
>
> /var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T/
>
> QEMU's avocado tests create a temporary directory prefixed by
> "avo_qemu_sock_", and create QMP sockets within _that_ as well.
> The QMP socket is unnecessarily long, because a temporary directory
> is created for every QEMUMachine object.
>
> /avo_qemu_sock_uh3w_dgc/qemu-37331-10bacf110-monitor.sock
Looking at this again, I realize my suggestion for dealing with the
second part of the path was mistaken.
The "qemu-37331-10bacf110-monitor.sock" part is combining two
pieces.
First the result of
f"qemu-{os.getpid()}-{id(self):02x}"
is
qemu-37331-10bacf110
and the code later than appends '-monitor.sock'
So...
>
> The path limit for unix sockets on macOS is 104: [2]
>
> /*
> * [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) */
> };
>
> This results in avocado tests failing on macOS because the QMP unix
> socket can't be created, because the path is too long:
>
> ERROR| Failed to establish connection: OSError: AF_UNIX path too long
>
> This change reduces the size of both paths, and removes the unique
> identification information from the socket name, since it seems to be
> unnecessary.
>
> This commit produces paths like the following:
>
> pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> $ tree qemu*
> qemu_oc7h7f3u
> ├── qmp-console.sock
> └── qmp-monitor.sock
>
> [1]
> https://apple.stackexchange.com/questions/353832/why-is-mac-osx-temp-directory-in-weird-path
> [2]
> /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk/usr/include/sys/un.h
>
> Signed-off-by: Peter Delevoryas <pe...@pjd.dev>
> ---
> python/qemu/machine/machine.py | 2 +-
> tests/avocado/avocado_qemu/__init__.py | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 37191f433b..b1823966b3 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 "qmp"
...my suggestion here was wrong.
We don't need the os.getpid() unoiqueness because the tmpdir already
ensures that is safe, but keeping 'id(self)' is a good idea, if the
test case creates multiple machines concurrently. Bearing in mind we
later append '-monitor.sock' we don't need 'qmp' in the self._name.
So on reflection I think I should have suggested using:
self._name = name or f"{id(self):02x}"
And *in addition*, a few lines later change:
self._monitor_address = os.path.join(
self.sock_dir, f"{self._name}-monitor.sock"
)
To
self._monitor_address = os.path.join(
self.sock_dir, f"{self._name}.qmp"
)
> diff --git a/tests/avocado/avocado_qemu/__init__.py
> b/tests/avocado/avocado_qemu/__init__.py
> index ed4853c805..43b8c8848c 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -296,7 +296,7 @@ def require_accelerator(self, accelerator):
> "available" % accelerator)
>
> def _new_vm(self, name, *args):
> - self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> + self._sd = tempfile.TemporaryDirectory(prefix="qemu_")
This bit is fine.
> vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
> sock_dir=self._sd.name, log_dir=self.logdir)
> self.log.debug('QEMUMachine "%s" created', name)
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 :|
