* [PATCH v2 0/1] python/machine: Fix AF_UNIX path too long on macOS
@ 2022-07-16 17:34 Peter Delevoryas
2022-07-16 17:34 ` [PATCH v2 1/1] " Peter Delevoryas
0 siblings, 1 reply; 7+ messages in thread
From: Peter Delevoryas @ 2022-07-16 17:34 UTC (permalink / raw)
Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel, berrange, peter
v2:
- Reduced QEMUMachine socket directory prefix length.
- Replaced QEMUMachine default name with "qmp".
- Rewrote commit description.
Thanks,
Peter Delevoryas
Peter Delevoryas (1):
python/machine: Fix AF_UNIX path too long on macOS
python/qemu/machine/machine.py | 2 +-
tests/avocado/avocado_qemu/__init__.py | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.37.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS
2022-07-16 17:34 [PATCH v2 0/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
@ 2022-07-16 17:34 ` Peter Delevoryas
2022-07-18 8:56 ` Daniel P. Berrangé
0 siblings, 1 reply; 7+ messages in thread
From: Peter Delevoryas @ 2022-07-16 17:34 UTC (permalink / raw)
Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel, berrange, peter
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 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 <peter@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"
self._temp_dir: Optional[str] = None
self._base_temp_dir = base_temp_dir
self._sock_dir = sock_dir
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_")
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.37.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS
2022-07-16 17:34 ` [PATCH v2 1/1] " Peter Delevoryas
@ 2022-07-18 8:56 ` Daniel P. Berrangé
2022-07-18 18:22 ` Peter Delevoryas
2022-07-22 2:44 ` Peter Delevoryas
0 siblings, 2 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2022-07-18 8:56 UTC (permalink / raw)
To: Peter Delevoryas; +Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel
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 <peter@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 :|
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS
2022-07-18 8:56 ` Daniel P. Berrangé
@ 2022-07-18 18:22 ` Peter Delevoryas
2022-07-22 2:44 ` Peter Delevoryas
1 sibling, 0 replies; 7+ messages in thread
From: Peter Delevoryas @ 2022-07-18 18:22 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel
On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> 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 <peter@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.
Yeah you're right, it's a good idea to make the QEMUMachine name unique.
When I made v2, I looked around to see if we were creating 2
QEMUMachine's concurrently. I didn't see anything, but looking again, I
see this in tests/migration/guestperf/engine.py:
if self._dst_host != "localhost":
dstmonaddr = ("localhost", 9001)
else:
dstmonaddr = "/var/tmp/qemu-dst-%d-monitor.sock" % os.getpid()
srcmonaddr = "/var/tmp/qemu-src-%d-monitor.sock" % os.getpid()
src = QEMUMachine(self._binary,
args=self._get_src_args(hardware),
wrapper=self._get_src_wrapper(hardware),
name="qemu-src-%d" % os.getpid(),
monitor_address=srcmonaddr)
dst = QEMUMachine(self._binary,
args=self._get_dst_args(hardware, uri),
wrapper=self._get_dst_wrapper(hardware),
name="qemu-dst-%d" % os.getpid(),
monitor_address=dstmonaddr)
This test case would probably still pass with this change, because the
monitor_address is specified, but I think you're right, it's a good idea
to make the default name different for each QEMUMachine instance to
avoid breaking anything.
>
> So on reflection I think I should have suggested using:
>
> self._name = name or f"{id(self):02x}"
+1, yeah I'll change it to that
>
> 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"
> )
Oh ok, yeah that looks good too. Thanks for the detailed review and
suggestions!
Peter
>
>
> > 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.
+1
>
> > 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 :|
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS
2022-07-18 8:56 ` Daniel P. Berrangé
2022-07-18 18:22 ` Peter Delevoryas
@ 2022-07-22 2:44 ` Peter Delevoryas
2022-07-22 7:20 ` Daniel P. Berrangé
1 sibling, 1 reply; 7+ messages in thread
From: Peter Delevoryas @ 2022-07-22 2:44 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel
On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> 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 <peter@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"
> )
>
Finally getting back to this (sorry, been working on other stuff), and I noticed
the console socket is just below this:
self._console_address = os.path.join(
self.sock_dir, f"{self._name}-console.sock"
)
So I probably shouldn't do the "-monitor.sock" change right?
That's ok though, the other changes are sufficient to fix the problem. I'll give
you a chance to reply to this email, or else I'll just send a v3 that doesn't
include the -monitor.sock change.
Thanks,
Peter
>
>
> > 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 :|
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS
2022-07-22 2:44 ` Peter Delevoryas
@ 2022-07-22 7:20 ` Daniel P. Berrangé
2022-07-22 18:06 ` Peter Delevoryas
0 siblings, 1 reply; 7+ messages in thread
From: Daniel P. Berrangé @ 2022-07-22 7:20 UTC (permalink / raw)
To: Peter Delevoryas; +Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel
On Thu, Jul 21, 2022 at 07:44:21PM -0700, Peter Delevoryas wrote:
> On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> > 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 <peter@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"
> > )
> >
>
> Finally getting back to this (sorry, been working on other stuff), and I noticed
> the console socket is just below this:
>
> self._console_address = os.path.join(
> self.sock_dir, f"{self._name}-console.sock"
> )
>
> So I probably shouldn't do the "-monitor.sock" change right?
I'd suggest changing this one to f"{self._name}.con" at the
same time.
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] 7+ messages in thread
* Re: [PATCH v2 1/1] python/machine: Fix AF_UNIX path too long on macOS
2022-07-22 7:20 ` Daniel P. Berrangé
@ 2022-07-22 18:06 ` Peter Delevoryas
0 siblings, 0 replies; 7+ messages in thread
From: Peter Delevoryas @ 2022-07-22 18:06 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: jsnow, crosa, bleal, f4bug, wainersm, qemu-devel
On Fri, Jul 22, 2022 at 08:20:11AM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 21, 2022 at 07:44:21PM -0700, Peter Delevoryas wrote:
> > On Mon, Jul 18, 2022 at 09:56:17AM +0100, Daniel P. Berrangé wrote:
> > > 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 <peter@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"
> > > )
> > >
> >
> > Finally getting back to this (sorry, been working on other stuff), and I noticed
> > the console socket is just below this:
> >
> > self._console_address = os.path.join(
> > self.sock_dir, f"{self._name}-console.sock"
> > )
> >
> > So I probably shouldn't do the "-monitor.sock" change right?
>
> I'd suggest changing this one to f"{self._name}.con" at the
> same time.
Ohh ok, yeah that's nice and short. Ok I'll include the socket name suffix
changes then.
>
>
> 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] 7+ messages in thread
end of thread, other threads:[~2022-07-22 18:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-16 17:34 [PATCH v2 0/1] python/machine: Fix AF_UNIX path too long on macOS Peter Delevoryas
2022-07-16 17:34 ` [PATCH v2 1/1] " Peter Delevoryas
2022-07-18 8:56 ` Daniel P. Berrangé
2022-07-18 18:22 ` Peter Delevoryas
2022-07-22 2:44 ` Peter Delevoryas
2022-07-22 7:20 ` Daniel P. Berrangé
2022-07-22 18:06 ` Peter Delevoryas
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.