* [PATCH v4 0/1] python/machine: Fix AF_UNIX path too long
@ 2023-01-10 8:07 Peter Delevoryas
2023-01-10 8:07 ` [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
0 siblings, 1 reply; 9+ messages in thread
From: Peter Delevoryas @ 2023-01-10 8:07 UTC (permalink / raw)
Cc: jsnow, crosa, bleal, philmd, wainersm, qemu-devel, Peter Delevoryas
v1: https://lore.kernel.org/qemu-devel/20220705214659.73369-1-peter@pjd.dev/
v2: https://lore.kernel.org/qemu-devel/20220716173434.17183-1-peter@pjd.dev/
v3:
- Changed QEMUMachine._name to f"{id(self):x}". Suggestion was to do
f"{id(self):02x}", but the id's look like they are probably just the
object address (8-byte pointer), so the "02" had no effect.
- Changed QMP socket name suffix from "-monitor.sock" to ".qmp".
- Changed console socket name suffix from "-console.sock" to ".con".
v4:
- Just resending v3 after rebasing from a long time ago
I tried to run `make check-avocado` before sending again, but it looks like
there is some other issue. Probably related to the python version I have (I
have like 5 different Python versions installed on my work laptop).
Thanks,
Peter
$ make check-avocado
changing dir to build for /Library/Developer/CommandLineTools/usr/bin/make "check-avocado"...
GIT ui/keycodemapdb tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc
VENV /Users/pdel/qemu/build/tests/venv
VENVPIP install -e /Users/pdel/qemu/python/
VENVPIP install -r /Users/pdel/qemu/tests/requirements.txt
MKDIR /Users/pdel/qemu/build/tests/results
AVOCADO Downloading avocado tests VM image for aarch64
The image was downloaded:
Provider Version Architecture File
fedora 31 aarch64 /Users/pdel/avocado/data/cache/by_location/4f156e531446a679cbfe13caef8b7c9f9f79aafa/Fedora-C
loud-Base-31-1.9.aarch64.qcow2
AVOCADO tests/avocado
Fetching asset from tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf
Fetching asset from tests/avocado/boot_xen.py:BootXen.test_arm64_xen_411_and_dom0
Fetching asset from tests/avocado/boot_xen.py:BootXen.test_arm64_xen_414_and_dom0
Fetching asset from tests/avocado/boot_xen.py:BootXen.test_arm64_xen_415_and_dom0
Fetching asset from tests/avocado/machine_aarch64_virt.py:Aarch64VirtMachine.test_alpine_virt_tcg_gic_max
Fetching asset from tests/avocado/machine_aarch64_virt.py:Aarch64VirtMachine.test_aarch64_virt
Fetching asset from tests/avocado/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt
Fetching asset from tests/avocado/reverse_debugging.py:ReverseDebugging_AArch64.test_aarch64_virt
JOB ID : 18a949ed9150e22d6ecea69b99ede1ded17233f4
JOB LOG : /Users/pdel/qemu/build/tests/results/job-2023-01-10T00.03-18a949e/job.log
Avocado crashed: TypeError: cannot pickle '_thread.RLock' object
Peter Delevoryas (1):
python/machine: Fix AF_UNIX path too long on macOS
python/qemu/machine/machine.py | 6 +++---
tests/avocado/avocado_qemu/__init__.py | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
--
2.39.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-10 8:07 [PATCH v4 0/1] python/machine: Fix AF_UNIX path too long Peter Delevoryas
@ 2023-01-10 8:07 ` Peter Delevoryas
2023-01-10 8:13 ` Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Peter Delevoryas @ 2023-01-10 8:07 UTC (permalink / raw)
Cc: jsnow, crosa, bleal, philmd, wainersm, qemu-devel, Peter Delevoryas
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
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 resolves by reducing the size of the socket directory prefix
and the suffix on the QMP and console socket names.
The result is paths like this:
pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
$ tree qemu*
qemu_df4evjeq
qemu_jbxel3gy
qemu_ml9s_gg7
qemu_oc7h7f3u
qemu_oqb1yf97
├── 10a004050.con
└── 10a004050.qmp
[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 <peter@pjd.dev>
---
python/qemu/machine/machine.py | 6 +++---
tests/avocado/avocado_qemu/__init__.py | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 748a0d807c9d..d70977378305 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"{id(self):x}"
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}.qmp"
)
self._console_log_path = console_log
@@ -192,7 +192,7 @@ def __init__(self,
self._console_set = False
self._console_device_type: Optional[str] = None
self._console_address = os.path.join(
- self.sock_dir, f"{self._name}-console.sock"
+ self.sock_dir, f"{self._name}.con"
)
self._console_socket: Optional[socket.socket] = None
self._remove_files: List[str] = []
diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index 910f3ba1eab8..25a546842fab 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -306,7 +306,7 @@ def require_netdev(self, netdevname):
self.cancel('no support for user networking')
def _new_vm(self, name, *args):
- self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
+ self._sd = tempfile.TemporaryDirectory(prefix="qemu_")
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)
--
2.39.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-10 8:07 ` [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
@ 2023-01-10 8:13 ` Philippe Mathieu-Daudé
2023-01-10 9:36 ` Daniel P. Berrangé
2023-01-10 23:18 ` John Snow
2 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-10 8:13 UTC (permalink / raw)
To: Peter Delevoryas; +Cc: jsnow, crosa, bleal, wainersm, qemu-devel
On 10/1/23 09:07, 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
>
> 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
Ah, I'm using this klugde:
$ TMPDIR=/tmp avocado run ...
> This change resolves by reducing the size of the socket directory prefix
> and the suffix on the QMP and console socket names.
>
> The result is paths like this:
>
> pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> $ tree qemu*
> qemu_df4evjeq
> qemu_jbxel3gy
> qemu_ml9s_gg7
> qemu_oc7h7f3u
> qemu_oqb1yf97
> ├── 10a004050.con
> └── 10a004050.qmp
>
> [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 <peter@pjd.dev>
> ---
> python/qemu/machine/machine.py | 6 +++---
> tests/avocado/avocado_qemu/__init__.py | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-10 8:07 ` [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
2023-01-10 8:13 ` Philippe Mathieu-Daudé
@ 2023-01-10 9:36 ` Daniel P. Berrangé
2023-01-10 23:18 ` John Snow
2 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrangé @ 2023-01-10 9:36 UTC (permalink / raw)
To: Peter Delevoryas; +Cc: jsnow, crosa, bleal, philmd, wainersm, qemu-devel
On Tue, Jan 10, 2023 at 12:07:56AM -0800, 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
>
> 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 resolves by reducing the size of the socket directory prefix
> and the suffix on the QMP and console socket names.
>
> The result is paths like this:
>
> pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> $ tree qemu*
> qemu_df4evjeq
> qemu_jbxel3gy
> qemu_ml9s_gg7
> qemu_oc7h7f3u
> qemu_oqb1yf97
> ├── 10a004050.con
> └── 10a004050.qmp
>
> [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 <peter@pjd.dev>
> ---
> python/qemu/machine/machine.py | 6 +++---
> tests/avocado/avocado_qemu/__init__.py | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-10 8:07 ` [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
2023-01-10 8:13 ` Philippe Mathieu-Daudé
2023-01-10 9:36 ` Daniel P. Berrangé
@ 2023-01-10 23:18 ` John Snow
2023-01-10 23:21 ` Peter Delevoryas
2023-01-11 9:07 ` Daniel P. Berrangé
2 siblings, 2 replies; 9+ messages in thread
From: John Snow @ 2023-01-10 23:18 UTC (permalink / raw)
To: Peter Delevoryas; +Cc: crosa, bleal, philmd, wainersm, qemu-devel
On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> 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
>
> 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 resolves by reducing the size of the socket directory prefix
> and the suffix on the QMP and console socket names.
>
> The result is paths like this:
>
> pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> $ tree qemu*
> qemu_df4evjeq
> qemu_jbxel3gy
> qemu_ml9s_gg7
> qemu_oc7h7f3u
> qemu_oqb1yf97
> ├── 10a004050.con
> └── 10a004050.qmp
>
> [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 <peter@pjd.dev>
I'm tentatively staging this with a benefit-of-the-doubt [1] -- my
tests are still running -- but I do have a question:
> ---
> python/qemu/machine/machine.py | 6 +++---
> tests/avocado/avocado_qemu/__init__.py | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> index 748a0d807c9d..d70977378305 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"{id(self):x}"
Why is it safe to not differentiate based on the process ID?
... I suppose the thinking is: by default, in machine.py, this is a
temp dir created by tempfile.mkdtemp which will be unique per-process.
I suppose there's no protection against a caller supplying the same
tempdir (or sockdir) to multiple instances, but I suppose in those
cases we get to argue that "Well, don't do that, then."
Does that sound about right?
--js
[1] staged @ https://gitlab.com/jsnow/qemu/-/commits/python
> 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}.qmp"
> )
>
> self._console_log_path = console_log
> @@ -192,7 +192,7 @@ def __init__(self,
> self._console_set = False
> self._console_device_type: Optional[str] = None
> self._console_address = os.path.join(
> - self.sock_dir, f"{self._name}-console.sock"
> + self.sock_dir, f"{self._name}.con"
> )
> self._console_socket: Optional[socket.socket] = None
> self._remove_files: List[str] = []
> diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> index 910f3ba1eab8..25a546842fab 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -306,7 +306,7 @@ def require_netdev(self, netdevname):
> self.cancel('no support for user networking')
>
> def _new_vm(self, name, *args):
> - self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> + self._sd = tempfile.TemporaryDirectory(prefix="qemu_")
> 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)
> --
> 2.39.0
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-10 23:18 ` John Snow
@ 2023-01-10 23:21 ` Peter Delevoryas
2023-01-11 9:07 ` Daniel P. Berrangé
1 sibling, 0 replies; 9+ messages in thread
From: Peter Delevoryas @ 2023-01-10 23:21 UTC (permalink / raw)
To: John Snow; +Cc: crosa, bleal, philmd, wainersm, qemu-devel
On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote:
> On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> 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
> >
> > 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 resolves by reducing the size of the socket directory prefix
> > and the suffix on the QMP and console socket names.
> >
> > The result is paths like this:
> >
> > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> > $ tree qemu*
> > qemu_df4evjeq
> > qemu_jbxel3gy
> > qemu_ml9s_gg7
> > qemu_oc7h7f3u
> > qemu_oqb1yf97
> > ├── 10a004050.con
> > └── 10a004050.qmp
> >
> > [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 <peter@pjd.dev>
>
> I'm tentatively staging this with a benefit-of-the-doubt [1] -- my
> tests are still running -- but I do have a question:
>
> > ---
> > python/qemu/machine/machine.py | 6 +++---
> > tests/avocado/avocado_qemu/__init__.py | 2 +-
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index 748a0d807c9d..d70977378305 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"{id(self):x}"
>
> Why is it safe to not differentiate based on the process ID?
>
> ... I suppose the thinking is: by default, in machine.py, this is a
> temp dir created by tempfile.mkdtemp which will be unique per-process.
> I suppose there's no protection against a caller supplying the same
> tempdir (or sockdir) to multiple instances, but I suppose in those
> cases we get to argue that "Well, don't do that, then."
>
> Does that sound about right?
Yeah, I think that's it
>
> --js
>
> [1] staged @ https://gitlab.com/jsnow/qemu/-/commits/python
>
>
> > 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}.qmp"
> > )
> >
> > self._console_log_path = console_log
> > @@ -192,7 +192,7 @@ def __init__(self,
> > self._console_set = False
> > self._console_device_type: Optional[str] = None
> > self._console_address = os.path.join(
> > - self.sock_dir, f"{self._name}-console.sock"
> > + self.sock_dir, f"{self._name}.con"
> > )
> > self._console_socket: Optional[socket.socket] = None
> > self._remove_files: List[str] = []
> > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> > index 910f3ba1eab8..25a546842fab 100644
> > --- a/tests/avocado/avocado_qemu/__init__.py
> > +++ b/tests/avocado/avocado_qemu/__init__.py
> > @@ -306,7 +306,7 @@ def require_netdev(self, netdevname):
> > self.cancel('no support for user networking')
> >
> > def _new_vm(self, name, *args):
> > - self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> > + self._sd = tempfile.TemporaryDirectory(prefix="qemu_")
> > 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)
> > --
> > 2.39.0
> >
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-10 23:18 ` John Snow
2023-01-10 23:21 ` Peter Delevoryas
@ 2023-01-11 9:07 ` Daniel P. Berrangé
2023-01-11 15:06 ` John Snow
1 sibling, 1 reply; 9+ messages in thread
From: Daniel P. Berrangé @ 2023-01-11 9:07 UTC (permalink / raw)
To: John Snow; +Cc: Peter Delevoryas, crosa, bleal, philmd, wainersm, qemu-devel
On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote:
> On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> 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
> >
> > 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 resolves by reducing the size of the socket directory prefix
> > and the suffix on the QMP and console socket names.
> >
> > The result is paths like this:
> >
> > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> > $ tree qemu*
> > qemu_df4evjeq
> > qemu_jbxel3gy
> > qemu_ml9s_gg7
> > qemu_oc7h7f3u
> > qemu_oqb1yf97
> > ├── 10a004050.con
> > └── 10a004050.qmp
> >
> > [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 <peter@pjd.dev>
>
> I'm tentatively staging this with a benefit-of-the-doubt [1] -- my
> tests are still running -- but I do have a question:
>
> > ---
> > python/qemu/machine/machine.py | 6 +++---
> > tests/avocado/avocado_qemu/__init__.py | 2 +-
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > index 748a0d807c9d..d70977378305 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"{id(self):x}"
>
> Why is it safe to not differentiate based on the process ID?
>
> ... I suppose the thinking is: by default, in machine.py, this is a
> temp dir created by tempfile.mkdtemp which will be unique per-process.
> I suppose there's no protection against a caller supplying the same
> tempdir (or sockdir) to multiple instances, but I suppose in those
> cases we get to argue that "Well, don't do that, then."
Every process will have a separate tempdir, and if there are
multiple instances of this class, 'id(self)' will provide
uniqueness within the process.
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 v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-11 9:07 ` Daniel P. Berrangé
@ 2023-01-11 15:06 ` John Snow
2023-01-11 15:23 ` Daniel P. Berrangé
0 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2023-01-11 15:06 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Peter Delevoryas, crosa, bleal, philmd, wainersm, qemu-devel
On Wed, Jan 11, 2023 at 4:07 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote:
> > On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> 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
> > >
> > > 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 resolves by reducing the size of the socket directory prefix
> > > and the suffix on the QMP and console socket names.
> > >
> > > The result is paths like this:
> > >
> > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> > > $ tree qemu*
> > > qemu_df4evjeq
> > > qemu_jbxel3gy
> > > qemu_ml9s_gg7
> > > qemu_oc7h7f3u
> > > qemu_oqb1yf97
> > > ├── 10a004050.con
> > > └── 10a004050.qmp
> > >
> > > [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 <peter@pjd.dev>
> >
> > I'm tentatively staging this with a benefit-of-the-doubt [1] -- my
> > tests are still running -- but I do have a question:
> >
> > > ---
> > > python/qemu/machine/machine.py | 6 +++---
> > > tests/avocado/avocado_qemu/__init__.py | 2 +-
> > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > index 748a0d807c9d..d70977378305 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"{id(self):x}"
> >
> > Why is it safe to not differentiate based on the process ID?
> >
> > ... I suppose the thinking is: by default, in machine.py, this is a
> > temp dir created by tempfile.mkdtemp which will be unique per-process.
> > I suppose there's no protection against a caller supplying the same
> > tempdir (or sockdir) to multiple instances, but I suppose in those
> > cases we get to argue that "Well, don't do that, then."
>
> Every process will have a separate tempdir, and if there are
> multiple instances of this class, 'id(self)' will provide
> uniqueness within the process.
Right. The only small thing is if a caller passes the same directory
to multiple instances across multiple processes, you could
*theoretically* get a collision, and we don't guard against it. It's
not a super likely occurrence so I'm fine with ignoring it, but I
think it's technically possible.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS
2023-01-11 15:06 ` John Snow
@ 2023-01-11 15:23 ` Daniel P. Berrangé
0 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrangé @ 2023-01-11 15:23 UTC (permalink / raw)
To: John Snow; +Cc: Peter Delevoryas, crosa, bleal, philmd, wainersm, qemu-devel
On Wed, Jan 11, 2023 at 10:06:49AM -0500, John Snow wrote:
> On Wed, Jan 11, 2023 at 4:07 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, Jan 10, 2023 at 06:18:29PM -0500, John Snow wrote:
> > > On Tue, Jan 10, 2023 at 3:34 AM Peter Delevoryas <peter@pjd.dev> 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
> > > >
> > > > 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 resolves by reducing the size of the socket directory prefix
> > > > and the suffix on the QMP and console socket names.
> > > >
> > > > The result is paths like this:
> > > >
> > > > pdel@pdel-mbp:/var/folders/d7/rz20f6hd709c1ty8f6_6y_z40000gn/T
> > > > $ tree qemu*
> > > > qemu_df4evjeq
> > > > qemu_jbxel3gy
> > > > qemu_ml9s_gg7
> > > > qemu_oc7h7f3u
> > > > qemu_oqb1yf97
> > > > ├── 10a004050.con
> > > > └── 10a004050.qmp
> > > >
> > > > [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 <peter@pjd.dev>
> > >
> > > I'm tentatively staging this with a benefit-of-the-doubt [1] -- my
> > > tests are still running -- but I do have a question:
> > >
> > > > ---
> > > > python/qemu/machine/machine.py | 6 +++---
> > > > tests/avocado/avocado_qemu/__init__.py | 2 +-
> > > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
> > > > index 748a0d807c9d..d70977378305 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"{id(self):x}"
> > >
> > > Why is it safe to not differentiate based on the process ID?
> > >
> > > ... I suppose the thinking is: by default, in machine.py, this is a
> > > temp dir created by tempfile.mkdtemp which will be unique per-process.
> > > I suppose there's no protection against a caller supplying the same
> > > tempdir (or sockdir) to multiple instances, but I suppose in those
> > > cases we get to argue that "Well, don't do that, then."
> >
> > Every process will have a separate tempdir, and if there are
> > multiple instances of this class, 'id(self)' will provide
> > uniqueness within the process.
>
> Right. The only small thing is if a caller passes the same directory
> to multiple instances across multiple processes, you could
> *theoretically* get a collision, and we don't guard against it. It's
> not a super likely occurrence so I'm fine with ignoring it, but I
> think it's technically possible.
If they want to be insane, then they can also pass a 'name' explicitly
to override the default socket path choice.
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:[~2023-01-11 15:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-10 8:07 [PATCH v4 0/1] python/machine: Fix AF_UNIX path too long Peter Delevoryas
2023-01-10 8:07 ` [PATCH v4 1/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
2023-01-10 8:13 ` Philippe Mathieu-Daudé
2023-01-10 9:36 ` Daniel P. Berrangé
2023-01-10 23:18 ` John Snow
2023-01-10 23:21 ` Peter Delevoryas
2023-01-11 9:07 ` Daniel P. Berrangé
2023-01-11 15:06 ` John Snow
2023-01-11 15:23 ` 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.